Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2251993004:
media/.../{android_,gpu_}video_encode_accelerator{,_host} cleanup (Closed)
|
Created:
4 years, 4 months ago by mcasas Modified:
4 years, 4 months ago Reviewers:
watk CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, avayvod+watch_chromium.org, mlamouri+watch-media_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia/.../{android_,gpu_}video_encode_accelerator{,_host} cleanup
Just found a couple of things that needed cleanup during
the investigation of the bug below. So, this CL:
- Removes unnecessary CHECK(env); after
JNIEnv* env = AttachCurrentThread();
(see https://crrev.com/2231923002)
- s/__PRETTY_FUNCTION__/__FUNCTION__/ because the former
is illegible in adb logcat or any other log, really.
- s/NULL/nullptr/
And concretely in GpuVideoEncodeAccelerator this CL:
- moves the static methods
std::unique_ptr<VideoEncodeAccelerator> Create{platform}VEA
out of the class and into anonymous namespace of the .cc
file, since they don't need to be in the class at all.
- nukes (*CreateVEAFp)() in that class and uses instead a
base::Callback(), bound to the static method mentioned above.
- uses for-range loops.
BUG=638664
TEST=all unittests. content_browsertests, browser_tests etc
working, and tested by hand in N7.
Committed: https://crrev.com/0b1178c9eb68681f7a305f400d02351074a2e8fa
Cr-Commit-Position: refs/heads/master@{#413057}
Patch Set 1 #Patch Set 2 : #
Total comments: 17
Patch Set 3 : watk@ comments #
Messages
Total messages: 26 (15 generated)
Description was changed from
==========
{android_,gpu_}video_encode_accelerator{,_host} cleanup
BUG=638664
==========
to
==========
{android_,gpu_}video_encode_accelerator{,_host} cleanup
Just found a couple of things that needed cleanup during
the investigation of the bug below. So, this CL:
- Removes unnecessary CHECK(env); after
JNIEnv* env = AttachCurrentThread();
(see https://crrev.com/2231923002)
- s/__PRETTY_FUNCTION__/__FUNCTION__/ because the former
is illegible in adb logcat or any other log, really.
- s/NULL/nullptr/
And concretely in GpuVideoEncodeAccelerator this CL:
- moves the static methods
std::unique_ptr<VideoEncodeAccelerator> Create{platform}VEA
out of the class and into anonymous namespace of the .cc
file, since they don't need to be in the class at all.
- nukes (*CreateVEAFp)() in that class and uses instead a
base::Callback(), bound to the static method mentioned above.
- uses for-range loops.
BUG=638664
TEST=all unittests. content_browsertests, browser_tests etc
working, and tested by hand in N7.
==========
Description was changed from
==========
{android_,gpu_}video_encode_accelerator{,_host} cleanup
Just found a couple of things that needed cleanup during
the investigation of the bug below. So, this CL:
- Removes unnecessary CHECK(env); after
JNIEnv* env = AttachCurrentThread();
(see https://crrev.com/2231923002)
- s/__PRETTY_FUNCTION__/__FUNCTION__/ because the former
is illegible in adb logcat or any other log, really.
- s/NULL/nullptr/
And concretely in GpuVideoEncodeAccelerator this CL:
- moves the static methods
std::unique_ptr<VideoEncodeAccelerator> Create{platform}VEA
out of the class and into anonymous namespace of the .cc
file, since they don't need to be in the class at all.
- nukes (*CreateVEAFp)() in that class and uses instead a
base::Callback(), bound to the static method mentioned above.
- uses for-range loops.
BUG=638664
TEST=all unittests. content_browsertests, browser_tests etc
working, and tested by hand in N7.
==========
to
==========
media/.../{android_,gpu_}video_encode_accelerator{,_host} cleanup
Just found a couple of things that needed cleanup during
the investigation of the bug below. So, this CL:
- Removes unnecessary CHECK(env); after
JNIEnv* env = AttachCurrentThread();
(see https://crrev.com/2231923002)
- s/__PRETTY_FUNCTION__/__FUNCTION__/ because the former
is illegible in adb logcat or any other log, really.
- s/NULL/nullptr/
And concretely in GpuVideoEncodeAccelerator this CL:
- moves the static methods
std::unique_ptr<VideoEncodeAccelerator> Create{platform}VEA
out of the class and into anonymous namespace of the .cc
file, since they don't need to be in the class at all.
- nukes (*CreateVEAFp)() in that class and uses instead a
base::Callback(), bound to the static method mentioned above.
- uses for-range loops.
BUG=638664
TEST=all unittests. content_browsertests, browser_tests etc
working, and tested by hand in N7.
==========
mcasas@chromium.org changed reviewers: + watk@chromium.org
watk@ presubmit recommended you, PTAL
lgtm https://codereview.chromium.org/2251993004/diff/20001/media/gpu/android_video... File media/gpu/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/2251993004/diff/20001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.cc:322: uint8_t* buffer = NULL; s/NULL/nullptr https://codereview.chromium.org/2251993004/diff/20001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.cc:351: status = media_codec_->QueueInputBuffer(input_buf_index, NULL, queued_size, s/NULL/nullptr https://codereview.chromium.org/2251993004/diff/20001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.cc:374: NoWaitTimeOut(), &buf_index, &offset, &size, NULL, NULL, &key_frame); s/NULL/nullptr https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... File media/gpu/ipc/service/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:46: namespace { I don't think it's in the style guide but we usually put a newline after opening a namespace (unless it's just forward declarations). https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:47: static bool MakeDecoderContextCurrent( Remove static on this one and all below. https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:67: encoder.reset(new V4L2VideoEncodeAccelerator(device)); If you feel like doing some extra cleanup :) You could change these to all use MakeUnique instead of wrapunique. https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:272: DLOG(ERROR) << __FUNCTION__ << " invalidc " s/invalidc/invalid https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... File media/gpu/ipc/service/gpu_video_encode_accelerator.h (right): https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... media/gpu/ipc/service/gpu_video_encode_accelerator.h:80: static std::vector<VEAFactoryMethod> GetVEAFactoryMethods( nit: calling these methods is slightly misleading IMO, since you're binding nonmember functions. I'd just s/method/function.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2251993004/diff/20001/media/gpu/android_video... File media/gpu/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/2251993004/diff/20001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.cc:322: uint8_t* buffer = NULL; On 2016/08/18 18:12:15, watk wrote: > s/NULL/nullptr Done. https://codereview.chromium.org/2251993004/diff/20001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.cc:351: status = media_codec_->QueueInputBuffer(input_buf_index, NULL, queued_size, On 2016/08/18 18:12:15, watk wrote: > s/NULL/nullptr Done. https://codereview.chromium.org/2251993004/diff/20001/media/gpu/android_video... media/gpu/android_video_encode_accelerator.cc:374: NoWaitTimeOut(), &buf_index, &offset, &size, NULL, NULL, &key_frame); On 2016/08/18 18:12:15, watk wrote: > s/NULL/nullptr Done. https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... File media/gpu/ipc/service/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:46: namespace { On 2016/08/18 18:12:15, watk wrote: > I don't think it's in the style guide but we usually put a newline after opening > a namespace (unless it's just forward declarations). oops, done https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:47: static bool MakeDecoderContextCurrent( On 2016/08/18 18:12:15, watk wrote: > Remove static on this one and all below. Done. https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:67: encoder.reset(new V4L2VideoEncodeAccelerator(device)); On 2016/08/18 18:12:15, watk wrote: > If you feel like doing some extra cleanup :) You could change these to all use > MakeUnique instead of wrapunique. Tried but it wouldn't work: ../../media/gpu/ipc/service/gpu_video_encode_accelerator.cc:80:10: error: no viable conversion from returned value of type 'unique_ptr<media::AndroidVideoEncodeAccelerator>' to function return type 'unique_ptr<media::VideoEncodeAccelerator>' prob because we're outside of the class. https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:272: DLOG(ERROR) << __FUNCTION__ << " invalidc " On 2016/08/18 18:12:15, watk wrote: > s/invalidc/invalid Done. https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... File media/gpu/ipc/service/gpu_video_encode_accelerator.h (right): https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... media/gpu/ipc/service/gpu_video_encode_accelerator.h:80: static std::vector<VEAFactoryMethod> GetVEAFactoryMethods( On 2016/08/18 18:12:15, watk wrote: > nit: calling these methods is slightly misleading IMO, since you're binding > nonmember functions. I'd just s/method/function. Done.
https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... File media/gpu/ipc/service/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/2251993004/diff/20001/media/gpu/ipc/service/g... media/gpu/ipc/service/gpu_video_encode_accelerator.cc:67: encoder.reset(new V4L2VideoEncodeAccelerator(device)); On 2016/08/18 19:24:40, mcasas wrote: > On 2016/08/18 18:12:15, watk wrote: > > If you feel like doing some extra cleanup :) You could change these to all use > > MakeUnique instead of wrapunique. > > Tried but it wouldn't work: > ../../media/gpu/ipc/service/gpu_video_encode_accelerator.cc:80:10: error: no > viable conversion from returned value of type > 'unique_ptr<media::AndroidVideoEncodeAccelerator>' to function return type > 'unique_ptr<media::VideoEncodeAccelerator>' > > prob because we're outside of the class. Ah my bad
The CQ bit was unchecked by mcasas@chromium.org
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org Link to the patchset: https://codereview.chromium.org/2251993004/#ps60001 (title: "watk@ comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
media/.../{android_,gpu_}video_encode_accelerator{,_host} cleanup
Just found a couple of things that needed cleanup during
the investigation of the bug below. So, this CL:
- Removes unnecessary CHECK(env); after
JNIEnv* env = AttachCurrentThread();
(see https://crrev.com/2231923002)
- s/__PRETTY_FUNCTION__/__FUNCTION__/ because the former
is illegible in adb logcat or any other log, really.
- s/NULL/nullptr/
And concretely in GpuVideoEncodeAccelerator this CL:
- moves the static methods
std::unique_ptr<VideoEncodeAccelerator> Create{platform}VEA
out of the class and into anonymous namespace of the .cc
file, since they don't need to be in the class at all.
- nukes (*CreateVEAFp)() in that class and uses instead a
base::Callback(), bound to the static method mentioned above.
- uses for-range loops.
BUG=638664
TEST=all unittests. content_browsertests, browser_tests etc
working, and tested by hand in N7.
==========
to
==========
media/.../{android_,gpu_}video_encode_accelerator{,_host} cleanup
Just found a couple of things that needed cleanup during
the investigation of the bug below. So, this CL:
- Removes unnecessary CHECK(env); after
JNIEnv* env = AttachCurrentThread();
(see https://crrev.com/2231923002)
- s/__PRETTY_FUNCTION__/__FUNCTION__/ because the former
is illegible in adb logcat or any other log, really.
- s/NULL/nullptr/
And concretely in GpuVideoEncodeAccelerator this CL:
- moves the static methods
std::unique_ptr<VideoEncodeAccelerator> Create{platform}VEA
out of the class and into anonymous namespace of the .cc
file, since they don't need to be in the class at all.
- nukes (*CreateVEAFp)() in that class and uses instead a
base::Callback(), bound to the static method mentioned above.
- uses for-range loops.
BUG=638664
TEST=all unittests. content_browsertests, browser_tests etc
working, and tested by hand in N7.
==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
media/.../{android_,gpu_}video_encode_accelerator{,_host} cleanup
Just found a couple of things that needed cleanup during
the investigation of the bug below. So, this CL:
- Removes unnecessary CHECK(env); after
JNIEnv* env = AttachCurrentThread();
(see https://crrev.com/2231923002)
- s/__PRETTY_FUNCTION__/__FUNCTION__/ because the former
is illegible in adb logcat or any other log, really.
- s/NULL/nullptr/
And concretely in GpuVideoEncodeAccelerator this CL:
- moves the static methods
std::unique_ptr<VideoEncodeAccelerator> Create{platform}VEA
out of the class and into anonymous namespace of the .cc
file, since they don't need to be in the class at all.
- nukes (*CreateVEAFp)() in that class and uses instead a
base::Callback(), bound to the static method mentioned above.
- uses for-range loops.
BUG=638664
TEST=all unittests. content_browsertests, browser_tests etc
working, and tested by hand in N7.
==========
to
==========
media/.../{android_,gpu_}video_encode_accelerator{,_host} cleanup
Just found a couple of things that needed cleanup during
the investigation of the bug below. So, this CL:
- Removes unnecessary CHECK(env); after
JNIEnv* env = AttachCurrentThread();
(see https://crrev.com/2231923002)
- s/__PRETTY_FUNCTION__/__FUNCTION__/ because the former
is illegible in adb logcat or any other log, really.
- s/NULL/nullptr/
And concretely in GpuVideoEncodeAccelerator this CL:
- moves the static methods
std::unique_ptr<VideoEncodeAccelerator> Create{platform}VEA
out of the class and into anonymous namespace of the .cc
file, since they don't need to be in the class at all.
- nukes (*CreateVEAFp)() in that class and uses instead a
base::Callback(), bound to the static method mentioned above.
- uses for-range loops.
BUG=638664
TEST=all unittests. content_browsertests, browser_tests etc
working, and tested by hand in N7.
Committed: https://crrev.com/0b1178c9eb68681f7a305f400d02351074a2e8fa
Cr-Commit-Position: refs/heads/master@{#413057}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0b1178c9eb68681f7a305f400d02351074a2e8fa Cr-Commit-Position: refs/heads/master@{#413057} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
