Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(22)

Issue 2801803002: Android: enable H264&VP8 HW accelerator for MediaRecorder (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 2 weeks ago by braveyao
Modified:
2 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.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -11 lines) Patch
M content/browser/webrtc/webrtc_media_recorder_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +8 lines, -1 line 0 comments Download
M content/renderer/media_recorder/media_recorder_handler.cc View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M content/renderer/media_recorder/video_track_recorder.h View 1 2 3 4 5 6 7 3 chunks +12 lines, -1 line 0 comments Download
M content/renderer/media_recorder/video_track_recorder.cc View 1 2 3 4 5 6 2 chunks +1 line, -6 lines 0 comments Download
M content/renderer/media_recorder/video_track_recorder_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 1 chunk +4 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 133 (101 generated)
braveyao
Hi emircan@, please take a look.
2 months, 2 weeks ago (2017-04-05 22:05:51 UTC) #18
emircan
https://codereview.chromium.org/2801803002/diff/1/content/browser/webrtc/webrtc_media_recorder_browsertest.cc File content/browser/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/2801803002/diff/1/content/browser/webrtc/webrtc_media_recorder_browsertest.cc#newcode62 content/browser/webrtc/webrtc_media_recorder_browsertest.cc:62: // There is no SW H264 on Android. // ...
2 months, 2 weeks ago (2017-04-07 17:33:04 UTC) #19
braveyao
Thanks for the comments. All addressed or replied. PTAL. One remain issue is the Transparency ...
2 months, 2 weeks ago (2017-04-08 00:56:26 UTC) #36
braveyao
Sorry, forgot to run the unittests locally on N5 as well. Will tweak them next ...
2 months, 2 weeks ago (2017-04-08 02:46:04 UTC) #39
emircan
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.cc#newcode510 media/ffmpeg/ffmpeg_common.cc:510: if (codec != kCodecH264) { On 2017/04/08 00:56:26, braveyao ...
2 months, 2 weeks ago (2017-04-10 21:18:10 UTC) #40
braveyao
emircan@, revised the cl a little bit, based on the recent refactoring and the fact ...
2 months, 1 week ago (2017-04-14 22:28:28 UTC) #60
emircan
Looking good % test comment below. I am also adding mcasas@ for review in case ...
2 months, 1 week ago (2017-04-17 17:24:06 UTC) #68
braveyao
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.cc#newcode510 media/ffmpeg/ffmpeg_common.cc:510: if (codec != ...
2 months, 1 week ago (2017-04-17 18:09:28 UTC) #69
emircan
https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media_recorder/video_track_recorder_unittest.cc File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2801803002/diff/220001/content/renderer/media_recorder/video_track_recorder_unittest.cc#newcode51 content/renderer/media_recorder/video_track_recorder_unittest.cc:51: // So H264 can't be included on Android because ...
2 months, 1 week ago (2017-04-17 18:39:59 UTC) #72
emircan
Also lgtm. But please wait for mcasas@ review.
2 months, 1 week ago (2017-04-17 19:44:05 UTC) #73
mcasas
https://codereview.chromium.org/2801803002/diff/220001/content/browser/webrtc/webrtc_media_recorder_browsertest.cc File content/browser/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/2801803002/diff/220001/content/browser/webrtc/webrtc_media_recorder_browsertest.cc#newcode25 content/browser/webrtc/webrtc_media_recorder_browsertest.cc:25: #if !defined(OS_ANDROID) Don't use negative conditions: they are hard ...
2 months, 1 week ago (2017-04-17 20:10:39 UTC) #74
braveyao
Thanks for the comments, mcasas@. All addressed. PTAL. https://codereview.chromium.org/2801803002/diff/220001/content/browser/webrtc/webrtc_media_recorder_browsertest.cc File content/browser/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/2801803002/diff/220001/content/browser/webrtc/webrtc_media_recorder_browsertest.cc#newcode25 content/browser/webrtc/webrtc_media_recorder_browsertest.cc:25: #if ...
2 months, 1 week ago (2017-04-18 16:15:39 UTC) #84
mcasas
https://codereview.chromium.org/2801803002/diff/320001/content/renderer/media_recorder/video_track_recorder_unittest.cc File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2801803002/diff/320001/content/renderer/media_recorder/video_track_recorder_unittest.cc#newcode51 content/renderer/media_recorder/video_track_recorder_unittest.cc:51: // So H264 can't be included on Android because ...
2 months ago (2017-04-19 22:32:46 UTC) #92
braveyao
mcasas@ PTAL. https://codereview.chromium.org/2801803002/diff/320001/content/renderer/media_recorder/video_track_recorder_unittest.cc File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2801803002/diff/320001/content/renderer/media_recorder/video_track_recorder_unittest.cc#newcode51 content/renderer/media_recorder/video_track_recorder_unittest.cc:51: // So H264 can't be included on ...
2 months ago (2017-04-20 18:17:29 UTC) #95
mcasas
https://codereview.chromium.org/2801803002/diff/320001/content/test/data/media/mediarecorder_test.html File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/2801803002/diff/320001/content/test/data/media/mediarecorder_test.html#newcode548 content/test/data/media/mediarecorder_test.html:548: canvas.height = 240; On 2017/04/20 18:17:29, braveyao wrote: > ...
2 months ago (2017-04-20 19:18:37 UTC) #96
braveyao
With the proposed method, one case will be missed: for H264, since it doesn't support ...
2 months ago (2017-04-20 20:52:20 UTC) #97
emircan
On 2017/04/20 20:52:20, braveyao wrote: > With the proposed method, one case will be missed: ...
2 months ago (2017-04-20 22:09:56 UTC) #100
braveyao
Thanks for the clarification! PTAL. https://codereview.chromium.org/2801803002/diff/320001/content/test/data/media/mediarecorder_test.html File content/test/data/media/mediarecorder_test.html (right): https://codereview.chromium.org/2801803002/diff/320001/content/test/data/media/mediarecorder_test.html#newcode548 content/test/data/media/mediarecorder_test.html:548: canvas.height = 240; On ...
2 months ago (2017-04-20 22:37:35 UTC) #105
mcasas
lgtm w/ nit below. https://codereview.chromium.org/2801803002/diff/360001/content/browser/webrtc/webrtc_media_recorder_browsertest.cc File content/browser/webrtc/webrtc_media_recorder_browsertest.cc (right): https://codereview.chromium.org/2801803002/diff/360001/content/browser/webrtc/webrtc_media_recorder_browsertest.cc#newcode132 content/browser/webrtc/webrtc_media_recorder_browsertest.cc:132: if (GetParam().mime_type.find("video/webm") != std::string::npos) { ...
2 months ago (2017-04-20 22:39:47 UTC) #106
braveyao
+ chcunningham@ for OWNER's review to media/filters/ffmpeg_demuxer.cc
2 months ago (2017-04-20 23:12:30 UTC) #112
chcunningham
On 2017/04/20 23:12:30, braveyao wrote: > + chcunningham@ for OWNER's review to > media/filters/ffmpeg_demuxer.cc Where ...
2 months ago (2017-04-21 01:11:03 UTC) #114
braveyao
On 2017/04/21 01:11:03, chcunningham wrote: > On 2017/04/20 23:12:30, braveyao wrote: > > + chcunningham@ ...
2 months ago (2017-04-21 16:23:24 UTC) #115
DaleCurtis
https://codereview.chromium.org/2801803002/diff/380001/media/filters/ffmpeg_demuxer.cc File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2801803002/diff/380001/media/filters/ffmpeg_demuxer.cc#newcode665 media/filters/ffmpeg_demuxer.cc:665: #if BUILDFLAG(USE_PROPRIETARY_CODECS) || defined(OS_ANDROID) I'm pretty sure this is ...
2 months ago (2017-04-21 16:43:04 UTC) #116
braveyao
https://codereview.chromium.org/2801803002/diff/380001/media/filters/ffmpeg_demuxer.cc File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2801803002/diff/380001/media/filters/ffmpeg_demuxer.cc#newcode665 media/filters/ffmpeg_demuxer.cc:665: #if BUILDFLAG(USE_PROPRIETARY_CODECS) || defined(OS_ANDROID) On 2017/04/21 16:43:03, DaleCurtis wrote: ...
2 months ago (2017-04-21 16:58:53 UTC) #117
chcunningham
Sounds like you just need to add a gn args to enable proprietary codecs in ...
2 months ago (2017-04-21 17:27:38 UTC) #118
braveyao
On 2017/04/21 17:27:38, chcunningham wrote: > Sounds like you just need to add a gn ...
2 months ago (2017-04-21 17:31:12 UTC) #119
DaleCurtis
On 2017/04/21 at 17:31:12, braveyao wrote: > On 2017/04/21 17:27:38, chcunningham wrote: > > Sounds ...
2 months ago (2017-04-21 17:45:54 UTC) #120
braveyao
Oh, I have no idea that we will set those buildflags for Android too. Good ...
2 months ago (2017-04-21 18:09:45 UTC) #123
chcunningham
media (or lack of ;)) lgtm
2 months ago (2017-04-21 18:12:40 UTC) #124
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801803002/400001
2 months ago (2017-04-21 19:32:22 UTC) #129
commit-bot: I haz the power
Committed patchset #12 (id:400001) as https://chromium.googlesource.com/chromium/src/+/feabf3776925e38aab3e7f5eb20e90860cc49b71
2 months ago (2017-04-21 19:37:31 UTC) #132
Yoland Yan(Google)
2 months ago (2017-04-21 22:38:03 UTC) #133
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....
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318