Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801803002:
Android: enable H264&VP8 HW accelerator for MediaRecorder (Closed)
|
Created:
3 years, 8 months ago by braveyao Modified:
3 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, emircan+watch+mediarecorder_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+mediarecorder_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid: enable H264&VP8 HW accelerator for MediaRecorder
The HW encoding codes has been revised a bit recently and works pretty well now.
This cl is to enable H264&VP8 HW accelerator for MediaRecorder on Android.
BUG=638664
Review-Url: https://codereview.chromium.org/2801803002
Cr-Commit-Position: refs/heads/master@{#466414}
Committed: https://chromium.googlesource.com/chromium/src/+/feabf3776925e38aab3e7f5eb20e90860cc49b71
Patch Set 1 #
Total comments: 18
Patch Set 2 : rebase to Apr7 #Patch Set 3 : address comments on PS#1 #Patch Set 4 : revision according to the fact that min resolution requirement varies #Patch Set 5 : correct alpha setting in webm_muxer instead of demuxer #
Total comments: 15
Patch Set 6 : address comments on PS#5 #Patch Set 7 : rebase to Apr18 #Patch Set 8 : remove unnecessary comments #
Total comments: 6
Patch Set 9 : address comments on PS#8 #Patch Set 10 : exclude H264 from alpha channel recording test cases. #
Total comments: 1
Patch Set 11 : address nits #
Total comments: 3
Patch Set 12 : remove change in ffmpeg_demuxer.cc #
Messages
Total messages: 133 (101 generated)
The CQ bit was checked by braveyao@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by braveyao@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by braveyao@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by braveyao@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
braveyao@chromium.org changed reviewers: + emircan@chromium.org
Hi emircan@, please take a look.
https://codereview.chromium.org/2801803002/diff/1/content/browser/webrtc/webr... File content/browser/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/2801803002/diff/1/content/browser/webrtc/webr... content/browser/webrtc/webrtc_media_recorder_browsertest.cc:62: // There is no SW H264 on Android. // OpenH264 encoder is not supported on Android. https://codereview.chromium.org/2801803002/diff/1/content/browser/webrtc/webr... content/browser/webrtc/webrtc_media_recorder_browsertest.cc:68: if (disable_flag) How was this test running and passing on android before? This method would result in running the same test case twice by ignoring the |disable_accelerator| param. Why about removing l.25 "if !defined(OS_ANDROID)" instead? https://codereview.chromium.org/2801803002/diff/1/content/browser/webrtc/webr... content/browser/webrtc/webrtc_media_recorder_browsertest.cc:140: // channel. And this test doesn't work on some old platforms. This test only inputs alpha channel video for recording. For Android VEA case, alpha channel would be dropped and rest would be recorded, see the line below. We know that only VPX software encoder returns true for CanEncodeAlphaChannel(). I don't see any reason why this tests should be disabled. https://cs.chromium.org/chromium/src/content/renderer/media_recorder/video_tr... https://codereview.chromium.org/2801803002/diff/1/content/renderer/media_reco... File content/renderer/media_recorder/media_recorder_handler.cc (right): https://codereview.chromium.org/2801803002/diff/1/content/renderer/media_reco... content/renderer/media_recorder/media_recorder_handler.cc:43: #if BUILDFLAG(RTC_USE_H264) || defined(OS_ANDROID) Can you add these checks to video_track_recorder_unittest, media_recorder_handler_unittest as well so that tests run for android too? https://codereview.chromium.org/2801803002/diff/1/content/renderer/media_reco... File content/renderer/media_recorder/video_track_recorder.h (right): https://codereview.chromium.org/2801803002/diff/1/content/renderer/media_reco... content/renderer/media_recorder/video_track_recorder.h:27: const int kVEAEncoderMinResolutionWidthAndroid = 96; Can you put these macros in "#if defined(OS_ANDROID)"? https://codereview.chromium.org/2801803002/diff/1/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2801803002/diff/1/media/ffmpeg/ffmpeg_common.... media/ffmpeg/ffmpeg_common.cc:510: if (codec != kCodecH264) { Why is this necessary? Is there problem with playing alpha?
The CQ bit was checked by braveyao@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by braveyao@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...
The CQ bit was checked by braveyao@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...
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #3 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #3 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the comments. All addressed or replied. PTAL. One remain issue is the Transparency test on older platforms, i.e. N5, which doesn't work with smaller frame size on N5 while works well on N5X. This may need another cl to tweak it for both RTC and mediaRecorder later. https://codereview.chromium.org/2801803002/diff/1/content/browser/webrtc/webr... File content/browser/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/2801803002/diff/1/content/browser/webrtc/webr... content/browser/webrtc/webrtc_media_recorder_browsertest.cc:62: // There is no SW H264 on Android. On 2017/04/07 17:33:04, emircan wrote: > // OpenH264 encoder is not supported on Android. Done. https://codereview.chromium.org/2801803002/diff/1/content/browser/webrtc/webr... content/browser/webrtc/webrtc_media_recorder_browsertest.cc:68: if (disable_flag) On 2017/04/07 17:33:04, emircan wrote: > How was this test running and passing on android before? > > This method would result in running the same test case twice by ignoring the > |disable_accelerator| param. Why about removing l.25 "if !defined(OS_ANDROID)" > instead? Done. https://codereview.chromium.org/2801803002/diff/1/content/browser/webrtc/webr... content/browser/webrtc/webrtc_media_recorder_browsertest.cc:140: // channel. And this test doesn't work on some old platforms. On 2017/04/07 17:33:04, emircan wrote: > This test only inputs alpha channel video for recording. For Android VEA case, > alpha channel would be dropped and rest would be recorded, see the line below. > We know that only VPX software encoder returns true for CanEncodeAlphaChannel(). > I don't see any reason why this tests should be disabled. > > https://cs.chromium.org/chromium/src/content/renderer/media_recorder/video_tr... Emmm you are right. The failure of this case on N5 is due to the request video format. It seems that the minimum supported video format for H264 is different on older platforms(e.g. same case works on N5X but fail on N5). Unfortunately I didn't find any doc for it yet. I'll take deep look later, since this may affect RTC call too and deserve another cl. https://codereview.chromium.org/2801803002/diff/1/content/renderer/media_reco... File content/renderer/media_recorder/media_recorder_handler.cc (right): https://codereview.chromium.org/2801803002/diff/1/content/renderer/media_reco... content/renderer/media_recorder/media_recorder_handler.cc:43: #if BUILDFLAG(RTC_USE_H264) || defined(OS_ANDROID) On 2017/04/07 17:33:04, emircan wrote: > Can you add these checks to video_track_recorder_unittest, > media_recorder_handler_unittest as well so that tests run for android too? Done. https://codereview.chromium.org/2801803002/diff/1/content/renderer/media_reco... File content/renderer/media_recorder/video_track_recorder.h (right): https://codereview.chromium.org/2801803002/diff/1/content/renderer/media_reco... content/renderer/media_recorder/video_track_recorder.h:27: const int kVEAEncoderMinResolutionWidthAndroid = 96; On 2017/04/07 17:33:04, emircan wrote: > Can you put these macros in "#if defined(OS_ANDROID)"? Done. https://codereview.chromium.org/2801803002/diff/1/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2801803002/diff/1/media/ffmpeg/ffmpeg_common.... media/ffmpeg/ffmpeg_common.cc:510: if (codec != kCodecH264) { On 2017/04/07 17:33:04, emircan wrote: > Why is this necessary? Is there problem with playing alpha? Yes there is problem with playing back H264 recording since HW H264 doesn't support alpha and there is no SW H264 to fallback. And according to the developer, alpha mode here is only for WebM/VPx.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Sorry, forgot to run the unittests locally on N5 as well. Will tweak them next week. BTW: RTC call on N5(Android M) has no problem with smaller frame size, e.g. 96x64. Will check it on N5(Android K) again. And do you realize if there is any difference in the frame format between camera and canvas?
https://codereview.chromium.org/2801803002/diff/1/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2801803002/diff/1/media/ffmpeg/ffmpeg_common.... media/ffmpeg/ffmpeg_common.cc:510: if (codec != kCodecH264) { On 2017/04/08 00:56:26, braveyao wrote: > On 2017/04/07 17:33:04, emircan wrote: > > Why is this necessary? Is there problem with playing alpha? > > Yes there is problem with playing back H264 recording since HW H264 doesn't > support alpha and there is no SW H264 to fallback. > And according to the developer, alpha mode here is only for WebM/VPx. Alpha recording would only happen for WebM/VPx as well, see VPX recorder changes. There is no alpha recording for H264 right now, it will be dropped. What is the exact problem with H264 playback here?
The CQ bit was checked by braveyao@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...
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #4 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by braveyao@chromium.org to run a CQ dry run
Patchset #4 (id:160001) has been deleted
Dry run: 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
Dry run: 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 braveyao@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...
Patchset #4 (id:180001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 braveyao@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
emircan@, revised the cl a little bit, based on the recent refactoring and the fact that minimum resolution varies with each Android version. Other comments are responded. Please take another review. Thanks! https://codereview.chromium.org/2801803002/diff/1/content/browser/webrtc/webr... File content/browser/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/2801803002/diff/1/content/browser/webrtc/webr... content/browser/webrtc/webrtc_media_recorder_browsertest.cc:140: // channel. And this test doesn't work on some old platforms. On 2017/04/08 00:56:26, braveyao wrote: > On 2017/04/07 17:33:04, emircan wrote: > > This test only inputs alpha channel video for recording. For Android VEA case, > > alpha channel would be dropped and rest would be recorded, see the line below. > > We know that only VPX software encoder returns true for > CanEncodeAlphaChannel(). > > I don't see any reason why this tests should be disabled. > > > > > https://cs.chromium.org/chromium/src/content/renderer/media_recorder/video_tr... > > Emmm you are right. The failure of this case on N5 is due to the request video > format. It seems that the minimum supported video format for H264 is different > on older platforms(e.g. same case works on N5X but fail on N5). Unfortunately I > didn't find any doc for it yet. I'll take deep look later, since this may affect > RTC call too and deserve another cl. Done. https://codereview.chromium.org/2801803002/diff/1/content/renderer/media_reco... File content/renderer/media_recorder/media_recorder_handler.cc (right): https://codereview.chromium.org/2801803002/diff/1/content/renderer/media_reco... content/renderer/media_recorder/media_recorder_handler.cc:43: #if BUILDFLAG(RTC_USE_H264) || defined(OS_ANDROID) On 2017/04/07 17:33:04, emircan wrote: > Can you add these checks to video_track_recorder_unittest, > media_recorder_handler_unittest as well so that tests run for android too? Can't add the checks to video_track_recorder_unittest, which won't enumerate HW codecs. See the comments added there. Anyway, it's tested in media_recorder_browsertest. https://codereview.chromium.org/2801803002/diff/1/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2801803002/diff/1/media/ffmpeg/ffmpeg_common.... media/ffmpeg/ffmpeg_common.cc:510: if (codec != kCodecH264) { On 2017/04/10 21:18:09, emircan wrote: > On 2017/04/08 00:56:26, braveyao wrote: > > On 2017/04/07 17:33:04, emircan wrote: > > > Why is this necessary? Is there problem with playing alpha? > > > > Yes there is problem with playing back H264 recording since HW H264 doesn't > > support alpha and there is no SW H264 to fallback. > > And according to the developer, alpha mode here is only for WebM/VPx. > > Alpha recording would only happen for WebM/VPx as well, see VPX recorder > changes. There is no alpha recording for H264 right now, it will be dropped. > What is the exact problem with H264 playback here? I don't understand your concern here. Since HW H264 doesn't support alpha, it will fail to decode when ffmpeg_demuxer asks for PIXEL_FORMAT_YV12A during playing back H264 recording. Also, gpu_video_decoder([1]) will reject the decoding request because of it. [1]: https://cs.chromium.org/chromium/src/media/filters/gpu_video_decoder.cc?dr=CS...
The CQ bit was checked by braveyao@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 braveyao@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...
emircan@chromium.org changed reviewers: + mcasas@chromium.org
Looking good % test comment below. I am also adding mcasas@ for review in case there is any other requirement that I might have missed. https://codereview.chromium.org/2801803002/diff/1/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2801803002/diff/1/media/ffmpeg/ffmpeg_common.... media/ffmpeg/ffmpeg_common.cc:510: if (codec != kCodecH264) { On 2017/04/14 22:28:28, braveyao wrote: > On 2017/04/10 21:18:09, emircan wrote: > > On 2017/04/08 00:56:26, braveyao wrote: > > > On 2017/04/07 17:33:04, emircan wrote: > > > > Why is this necessary? Is there problem with playing alpha? > > > > > > Yes there is problem with playing back H264 recording since HW H264 doesn't > > > support alpha and there is no SW H264 to fallback. > > > And according to the developer, alpha mode here is only for WebM/VPx. > > > > Alpha recording would only happen for WebM/VPx as well, see VPX recorder > > changes. There is no alpha recording for H264 right now, it will be dropped. > > What is the exact problem with H264 playback here? > > I don't understand your concern here. > Since HW H264 doesn't support alpha, it will fail to decode when ffmpeg_demuxer > asks for PIXEL_FORMAT_YV12A during playing back H264 recording. > Also, gpu_video_decoder([1]) will reject the decoding request because of it. > [1]: > https://cs.chromium.org/chromium/src/media/filters/gpu_video_decoder.cc?dr=CS... Note: This issue was resolved on https://codereview.chromium.org/2815303005/. https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:51: // So H264 can't be included on Android because OpenH264 is not supported. We create a ChildProcess for test on l.134 which should act like render thread here for testing. What is the exact error you get here?
Please check the response and suggest. https://codereview.chromium.org/2801803002/diff/1/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2801803002/diff/1/media/ffmpeg/ffmpeg_common.... media/ffmpeg/ffmpeg_common.cc:510: if (codec != kCodecH264) { On 2017/04/17 17:24:06, emircan wrote: > On 2017/04/14 22:28:28, braveyao wrote: > > On 2017/04/10 21:18:09, emircan wrote: > > > On 2017/04/08 00:56:26, braveyao wrote: > > > > On 2017/04/07 17:33:04, emircan wrote: > > > > > Why is this necessary? Is there problem with playing alpha? > > > > > > > > Yes there is problem with playing back H264 recording since HW H264 > doesn't > > > > support alpha and there is no SW H264 to fallback. > > > > And according to the developer, alpha mode here is only for WebM/VPx. > > > > > > Alpha recording would only happen for WebM/VPx as well, see VPX recorder > > > changes. There is no alpha recording for H264 right now, it will be dropped. > > > What is the exact problem with H264 playback here? > > > > I don't understand your concern here. > > Since HW H264 doesn't support alpha, it will fail to decode when > ffmpeg_demuxer > > asks for PIXEL_FORMAT_YV12A during playing back H264 recording. > > Also, gpu_video_decoder([1]) will reject the decoding request because of it. > > [1]: > > > https://cs.chromium.org/chromium/src/media/filters/gpu_video_decoder.cc?dr=CS... > > Note: This issue was resolved on https://codereview.chromium.org/2815303005/. Acknowledged. https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:51: // So H264 can't be included on Android because OpenH264 is not supported. On 2017/04/17 17:24:06, emircan wrote: > We create a ChildProcess for test on l.134 which should act like render thread > here for testing. What is the exact error you get here? It fails to enumerate VEA at https://cs.chromium.org/chromium/src/content/renderer/media_recorder/video_tr.... I found the |child_process_| in this file, https://cs.chromium.org/chromium/src/content/renderer/media_recorder/video_tr.... But it seems no one used it. Anything else we can do?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:51: // So H264 can't be included on Android because OpenH264 is not supported. On 2017/04/17 18:09:27, braveyao wrote: > On 2017/04/17 17:24:06, emircan wrote: > > We create a ChildProcess for test on l.134 which should act like render thread > > here for testing. What is the exact error you get here? > > It fails to enumerate VEA at > https://cs.chromium.org/chromium/src/content/renderer/media_recorder/video_tr.... > > I found the |child_process_| in this file, > https://cs.chromium.org/chromium/src/content/renderer/media_recorder/video_tr.... > But it seems no one used it. > > Anything else we can do? I see. This test only exercises the SW encode path then, which doesn't exist on Android for H264.
Also lgtm. But please wait for mcasas@ review.
https://codereview.chromium.org/2801803002/diff/220001/content/browser/webrtc... File content/browser/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/2801803002/diff/220001/content/browser/webrtc... content/browser/webrtc/webrtc_media_recorder_browsertest.cc:25: #if !defined(OS_ANDROID) Don't use negative conditions: they are hard to read. Instead, do: #if defined(RTC_USE_H264) {true, "video/x-matroska;codecs=AVC1"}, #endif This way, if and when H264 sw is enabled for Android, we'll pick it up automatically. https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... File content/renderer/media_recorder/media_recorder_handler.cc (right): https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/media_recorder_handler.cc:43: #if BUILDFLAG(RTC_USE_H264) || defined(OS_ANDROID) This check #if BUILDFLAG(RTC_USE_H264) || defined(OS_ANDROID) (in reality: if-h264-encoding-is-enabled) is duplicated it all over: bad. Find a common place, e.g. video_track_recorder.h, and add a new preprocessor definition: #if BUILDFLAG(RTC_USE_H264) || defined(OS_ANDROID) // H264 encoding is supported on Android using the platform encode // accelerator, and elsewhere using OpenH264. #define IS_H264_SUPPORTED #endif which you can use in places where now you test #if BUILDFLAG(RTC_USE_H264) || defined(OS_ANDROID) doing instead: #if defined(IS_H264_SUPPORTED) https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.h (right): https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.h:35: #if !defined(OS_ANDROID) Don't use negative conditionals: #if defined(OS_ANDROID) // blabla #else // foo foo #endif https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.h:43: // Set 176x144 for now and revise it as the target API level changes. If there's an action for the future, we need a bug and the usual line TODO(developer):... https://crbug.com/xyzuvw Also, you say here: 176x144 but the value in l.44-45 is 172x144. My suggestion here is to make it a conservative estimate, remove all these superfluous data from the comment, and make the minimum dimensions QVGA: 320x240. https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:56: #if !defined(OS_ANDROID) Use positive ifdefs. https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:58: kVEAEncoderMinResolutionHeight / 2), Why do you do this? I think it's so that you can force the Sw encoder to be used. But that's not detailed anywhere, which will puzzle and confuse future developers. Find another way that is more maintainable.
The CQ bit was checked by braveyao@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by braveyao@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...
Patchset #6 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the comments, mcasas@. All addressed. PTAL. https://codereview.chromium.org/2801803002/diff/220001/content/browser/webrtc... File content/browser/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/2801803002/diff/220001/content/browser/webrtc... content/browser/webrtc/webrtc_media_recorder_browsertest.cc:25: #if !defined(OS_ANDROID) On 2017/04/17 20:10:38, mcasas wrote: > Don't use negative conditions: they are hard to read. > Instead, do: > > #if defined(RTC_USE_H264) > {true, "video/x-matroska;codecs=AVC1"}, > #endif > > This way, if and when H264 sw is enabled for Android, > we'll pick it up automatically. Done. https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... File content/renderer/media_recorder/media_recorder_handler.cc (right): https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/media_recorder_handler.cc:43: #if BUILDFLAG(RTC_USE_H264) || defined(OS_ANDROID) On 2017/04/17 20:10:38, mcasas wrote: > This check > #if BUILDFLAG(RTC_USE_H264) || defined(OS_ANDROID) > (in reality: if-h264-encoding-is-enabled) > is duplicated it all over: bad. > > Find a common place, e.g. video_track_recorder.h, > and add a new preprocessor definition: > > #if BUILDFLAG(RTC_USE_H264) || defined(OS_ANDROID) > // H264 encoding is supported on Android using the platform encode > // accelerator, and elsewhere using OpenH264. > #define IS_H264_SUPPORTED > #endif > > which you can use in places where now you test > #if BUILDFLAG(RTC_USE_H264) || defined(OS_ANDROID) > doing instead: > #if defined(IS_H264_SUPPORTED) Done. https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.h (right): https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.h:35: #if !defined(OS_ANDROID) On 2017/04/17 20:10:39, mcasas wrote: > Don't use negative conditionals: > > #if defined(OS_ANDROID) > // blabla > #else > // foo foo > #endif Done. https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.h:43: // Set 176x144 for now and revise it as the target API level changes. On 2017/04/17 20:10:39, mcasas wrote: > If there's an action for the future, we need a bug > and the usual line TODO(developer):... https://crbug.com/xyzuvw > > Also, you say here: 176x144 but the value in l.44-45 is > 172x144. > > My suggestion here is to make it a conservative estimate, remove > all these superfluous data from the comment, and make the > minimum dimensions QVGA: 320x240. Done. Keeping minimum resolution to QCIF: 176x144 https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:56: #if !defined(OS_ANDROID) On 2017/04/17 20:10:39, mcasas wrote: > Use positive ifdefs. Done. https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:58: kVEAEncoderMinResolutionHeight / 2), On 2017/04/17 20:10:39, mcasas wrote: > Why do you do this? I think it's so that you can force > the Sw encoder to be used. But that's not detailed > anywhere, which will puzzle and confuse future > developers. Find another way that is more maintainable. Done. You're right. Forgot to remove it as H264 is excluded lately.
The CQ bit was checked by braveyao@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...
Patchset #7 (id:280001) has been deleted
The CQ bit was checked by braveyao@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2801803002/diff/320001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2801803002/diff/320001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:51: // So H264 can't be included on Android because OpenH264 is not supported. This comment is hard to parse, because you're referring to something that can't happen because something else can't (doesn't?) happen either. Rephrase it in a logical way, e.g. // This test case can't be enabled in Android because there is no software // OpenH264 fallback nor render thread to enumerate the hardware encoders. https://codereview.chromium.org/2801803002/diff/320001/content/test/data/medi... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/2801803002/diff/320001/content/test/data/medi... content/test/data/media/mediarecorder_test.html:548: canvas.height = 240; IIUC you're changing these numbers to avoid using the hardware encoder in Android. Magic numbers like this are brittle, because it'll be forgotten that they are related to video_track_recorder.h:l.41-47. Find another way.
The CQ bit was checked by braveyao@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...
mcasas@ PTAL. https://codereview.chromium.org/2801803002/diff/320001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2801803002/diff/320001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:51: // So H264 can't be included on Android because OpenH264 is not supported. On 2017/04/19 22:32:46, mcasas wrote: > This comment is hard to parse, because you're referring to > something that can't happen because something else can't > (doesn't?) happen either. Rephrase it in a logical way, e.g. > > // This test case can't be enabled in Android because there is no software > // OpenH264 fallback nor render thread to enumerate the hardware encoders. Done. https://codereview.chromium.org/2801803002/diff/320001/content/test/data/medi... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/2801803002/diff/320001/content/test/data/medi... content/test/data/media/mediarecorder_test.html:548: canvas.height = 240; On 2017/04/19 22:32:46, mcasas wrote: > IIUC you're changing these numbers to avoid using the > hardware encoder in Android. Magic numbers like this are > brittle, because it'll be forgotten that they are related to > video_track_recorder.h:l.41-47. > > Find another way. Can't figure out another way for this HTML file, expect adding some comments. If you have any better idea, please suggest.
https://codereview.chromium.org/2801803002/diff/320001/content/test/data/medi... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/2801803002/diff/320001/content/test/data/medi... content/test/data/media/mediarecorder_test.html:548: canvas.height = 240; On 2017/04/20 18:17:29, braveyao wrote: > On 2017/04/19 22:32:46, mcasas wrote: > > IIUC you're changing these numbers to avoid using the > > hardware encoder in Android. Magic numbers like this are > > brittle, because it'll be forgotten that they are related to > > video_track_recorder.h:l.41-47. > > > > Find another way. > > Can't figure out another way for this HTML file, expect adding some comments. > If you have any better idea, please suggest. If the concern is H264 and transparency, that combination should not be supported IIRC, not in hardware nor software. The alpha channel will be dropped, so why bother? I would: - remove the comments you added, - leave the sizes as before (which make the test fast) - go to [1] and call only testRecordWithTransparency when GetParam().mime_type.c_str() does start with "video/webm" [2], then write a note in that location saying that alpha channel recording is _only_ supported in "video/webm;codecs={vp8/vp9}" case(s). [1] https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_media_reco... [2] https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_media_reco...
With the proposed method, one case will be missed: for H264, since it doesn't support alpha channel, then we should drop alpha channel and continue encoding(with either HW or SW codec). At first I suppose this is something need to keep. If this is not the concern of this test case. I can exclude H264 out of transparency test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/20 20:52:20, braveyao wrote: > With the proposed method, one case will be missed: for H264, since it doesn't > support alpha channel, then we should drop alpha channel and continue > encoding(with either HW or SW codec). > At first I suppose this is something need to keep. If this is not the concern of > this test case. I can exclude H264 out of transparency test. This test only counts the number of recorded events. It is just there to make sure that there is still output from all codecs when input has alpha. As a result, either alpha encoding code or the dropping alpha channel code is covered. Since dropping alpha channel is common codepath for all HW codecs and there is added complexity of resolutions, I would say lets just skip that case in Android. I would recommend just removing comments on mediarecorder_test.html and disabling this particular test case in webrtc_media_recorder_browsertest.cc.
The CQ bit was checked by braveyao@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Thanks for the clarification! PTAL. https://codereview.chromium.org/2801803002/diff/320001/content/test/data/medi... File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/2801803002/diff/320001/content/test/data/medi... content/test/data/media/mediarecorder_test.html:548: canvas.height = 240; On 2017/04/20 19:18:37, mcasas wrote: > On 2017/04/20 18:17:29, braveyao wrote: > > On 2017/04/19 22:32:46, mcasas wrote: > > > IIUC you're changing these numbers to avoid using the > > > hardware encoder in Android. Magic numbers like this are > > > brittle, because it'll be forgotten that they are related to > > > video_track_recorder.h:l.41-47. > > > > > > Find another way. > > > > Can't figure out another way for this HTML file, expect adding some comments. > > If you have any better idea, please suggest. > > If the concern is H264 and transparency, that combination > should not be supported IIRC, not in hardware nor software. > The alpha channel will be dropped, so why bother? > > I would: > - remove the comments you added, > - leave the sizes as before (which make the test fast) > - go to [1] and call only testRecordWithTransparency when > GetParam().mime_type.c_str() does start with "video/webm" > [2], then write a note in that location saying that alpha > channel recording is _only_ supported in > "video/webm;codecs={vp8/vp9}" case(s). > > [1] > https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_media_reco... > [2] > https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_media_reco... Done.
lgtm w/ nit below. https://codereview.chromium.org/2801803002/diff/360001/content/browser/webrtc... File content/browser/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/2801803002/diff/360001/content/browser/webrtc... content/browser/webrtc/webrtc_media_recorder_browsertest.cc:132: if (GetParam().mime_type.find("video/webm") != std::string::npos) { if (GetParam().mime_type.find("video/webm") == std::string::npos) return; // The rest.
The CQ bit was checked by braveyao@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
braveyao@chromium.org changed reviewers: + chcunningham@chromium.org
+ chcunningham@ for OWNER's review to media/filters/ffmpeg_demuxer.cc
chcunningham@chromium.org changed reviewers: + dalecurtis@chromium.org
On 2017/04/20 23:12:30, braveyao wrote: > + chcunningham@ for OWNER's review to > media/filters/ffmpeg_demuxer.cc Where is FFmpegDemuxer used in media recording? Dale, does converting 264 bitstream qualify require a license? Not sure if want to enable for non-chrome android.
On 2017/04/21 01:11:03, chcunningham wrote: > On 2017/04/20 23:12:30, braveyao wrote: > > + chcunningham@ for OWNER's review to > > media/filters/ffmpeg_demuxer.cc > > Where is FFmpegDemuxer used in media recording? > > Dale, does converting 264 bitstream qualify require a license? Not sure if want > to enable for non-chrome android. FFmpegDemuxer is used for playing back the recorded media, not in media recording IIUC.
https://codereview.chromium.org/2801803002/diff/380001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2801803002/diff/380001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:665: #if BUILDFLAG(USE_PROPRIETARY_CODECS) || defined(OS_ANDROID) I'm pretty sure this is wrong, why would you need this?
https://codereview.chromium.org/2801803002/diff/380001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2801803002/diff/380001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:665: #if BUILDFLAG(USE_PROPRIETARY_CODECS) || defined(OS_ANDROID) On 2017/04/21 16:43:03, DaleCurtis wrote: > I'm pretty sure this is wrong, why would you need this? But I do hit the "NOTREACHED()" check below. A test did moment ago gets the log as: chromium: [FATAL:ffmpeg_demuxer.cc(668)] Check failed: false. Proprietary codecs not enabled. The proprietary codecs, e.g. OpenH264, is not available on android. We need work around it to utilize MediaCodec to play back H264 recording.
Sounds like you just need to add a gn args to enable proprietary codecs in your local build? proprietary_codecs = true ffmpeg_branding = "ChromeOS"
On 2017/04/21 17:27:38, chcunningham wrote: > Sounds like you just need to add a gn args to enable proprietary codecs in your > local build? > > proprietary_codecs = true > ffmpeg_branding = "ChromeOS" No, we can't enable proprietary codecs on Android because OpenH264 is not available on Android. See https://cs.chromium.org/chromium/src/third_party/webrtc/webrtc.gni?l=142
On 2017/04/21 at 17:31:12, braveyao wrote: > On 2017/04/21 17:27:38, chcunningham wrote: > > Sounds like you just need to add a gn args to enable proprietary codecs in your > > local build? > > > > proprietary_codecs = true > > ffmpeg_branding = "ChromeOS" > > No, we can't enable proprietary codecs on Android because OpenH264 is not available on Android. > > See https://cs.chromium.org/chromium/src/third_party/webrtc/webrtc.gni?l=142 That may be true, but we still set those buildflags for Android, seems your local build just needs them.
The CQ bit was checked by braveyao@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...
Oh, I have no idea that we will set those buildflags for Android too. Good to know. Thanks for the clarification! https://codereview.chromium.org/2801803002/diff/380001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2801803002/diff/380001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:665: #if BUILDFLAG(USE_PROPRIETARY_CODECS) || defined(OS_ANDROID) On 2017/04/21 16:58:53, braveyao wrote: > On 2017/04/21 16:43:03, DaleCurtis wrote: > > I'm pretty sure this is wrong, why would you need this? > > But I do hit the "NOTREACHED()" check below. A test did moment ago gets the log > as: > chromium: [FATAL:ffmpeg_demuxer.cc(668)] Check failed: false. Proprietary codecs > not enabled. > > The proprietary codecs, e.g. OpenH264, is not available on android. We need work > around it to utilize MediaCodec to play back H264 recording. Done.
media (or lack of ;)) lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by braveyao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2801803002/#ps400001 (title: "remove change in ffmpeg_demuxer.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 400001, "attempt_start_ts": 1492803086417810,
"parent_rev": "1cda587abd16e09e3e25768c98e4c656d3f816be", "commit_rev":
"feabf3776925e38aab3e7f5eb20e90860cc49b71"}
Message was sent while issue was closed.
Description was changed from ========== Android: enable H264&VP8 HW accelerator for MediaRecorder The HW encoding codes has been revised a bit recently and works pretty well now. This cl is to enable H264&VP8 HW accelerator for MediaRecorder on Android. BUG=638664 ========== to ========== Android: enable H264&VP8 HW accelerator for MediaRecorder The HW encoding codes has been revised a bit recently and works pretty well now. This cl is to enable H264&VP8 HW accelerator for MediaRecorder on Android. BUG=638664 Review-Url: https://codereview.chromium.org/2801803002 Cr-Commit-Position: refs/heads/master@{#466414} Committed: https://chromium.googlesource.com/chromium/src/+/feabf3776925e38aab3e7f5eb20e... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:400001) as https://chromium.googlesource.com/chromium/src/+/feabf3776925e38aab3e7f5eb20e...
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:400001) has been created in https://codereview.chromium.org/2832263003/ by yolandyan@google.com. The reason for reverting is: The CL seem to be responsible for this failure: https://build.chromium.org/p/chromium.android/builders/Marshmallow%2064%20bit.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
