Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(28)

Issue 2691373005: Support alpha channel recording for VPX in MediaRecorder (Closed)

Created:
3 years, 10 months ago by emircan
Modified:
3 years, 9 months ago
Reviewers:
mcasas, vignesh
CC:
chromium-reviews, posciak+watch_chromium.org, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, feature-media-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Support alpha channel recording for VPX in MediaRecorder This CL adds support for alpha channel recording in VpxEncoder in VideoTrackRecorder. - OnEncodedVideoCB is modified to handle extraneous alpha channel output. - VpxEncoder is refactored to use two encoder instances and encode alpha channel padded with dummy data. - WebmMuxer is modified to add the optional alpha channel data using AdditionalBlock. BUG=690968 TEST=Added unit tests. Demo page on https://cdn.rawgit.com/uysalere/js-demos/master/canvascapture_alpharecording.html. Also, working on a alpha video based content_browsertest in a follow-up CL. Review-Url: https://codereview.chromium.org/2691373005 Cr-Commit-Position: refs/heads/master@{#456331} Committed: https://chromium.googlesource.com/chromium/src/+/9d369903d6cdd84aaaf70e2a73ada508e2b34eaa

Patch Set 1 : #

Total comments: 46

Patch Set 2 : vignesh@ comments. #

Patch Set 3 : Add webm_muxer_unittest. #

Patch Set 4 : mcasas@ comments. #

Total comments: 15

Patch Set 5 : #

Patch Set 6 : Modify webm_muxer_unittest. #

Total comments: 4

Patch Set 7 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+448 lines, -139 lines) Patch
M content/renderer/media_recorder/media_recorder_handler.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/media_recorder/media_recorder_handler.cc View 1 2 3 4 2 chunks +3 lines, -1 line 0 comments Download
M content/renderer/media_recorder/media_recorder_handler_unittest.cc View 1 2 3 3 chunks +34 lines, -5 lines 0 comments Download
M content/renderer/media_recorder/video_track_recorder.h View 1 2 3 4 5 6 2 chunks +4 lines, -0 lines 0 comments Download
M content/renderer/media_recorder/video_track_recorder.cc View 1 2 3 4 5 6 14 chunks +185 lines, -91 lines 0 comments Download
M content/renderer/media_recorder/video_track_recorder_unittest.cc View 1 2 3 4 7 chunks +101 lines, -21 lines 0 comments Download
M media/muxers/webm_muxer.h View 1 2 3 3 chunks +6 lines, -0 lines 0 comments Download
M media/muxers/webm_muxer.cc View 1 2 3 4 8 chunks +39 lines, -8 lines 0 comments Download
M media/muxers/webm_muxer_fuzzertest.cc View 1 chunk +6 lines, -3 lines 0 comments Download
M media/muxers/webm_muxer_unittest.cc View 1 2 3 4 5 4 chunks +66 lines, -10 lines 0 comments Download

Messages

Total messages: 84 (70 generated)
emircan
PTAL.
3 years, 9 months ago (2017-03-08 07:16:28 UTC) #41
vignesh
Looks good overall. Just some nits and questions. Thanks! https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media_recorder/video_track_recorder.cc File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media_recorder/video_track_recorder.cc#newcode286 content/renderer/media_recorder/video_track_recorder.cc:286: ...
3 years, 9 months ago (2017-03-08 17:26:15 UTC) #44
mcasas
https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media_recorder/media_recorder_handler.h File content/renderer/media_recorder/media_recorder_handler.h (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media_recorder/media_recorder_handler.h#newcode70 content/renderer/media_recorder/media_recorder_handler.h:70: std::unique_ptr<std::string> encoded_alpha_data, It's unfortunate that the frame and its ...
3 years, 9 months ago (2017-03-08 22:38:49 UTC) #45
emircan
Adressing vignesh@ comments. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media_recorder/video_track_recorder.cc File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media_recorder/video_track_recorder.cc#newcode286 content/renderer/media_recorder/video_track_recorder.cc:286: // Drop alpha channel if |encoder ...
3 years, 9 months ago (2017-03-08 23:53:58 UTC) #50
emircan
Addressing mcasas@ comments. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media_recorder/media_recorder_handler.h File content/renderer/media_recorder/media_recorder_handler.h (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media_recorder/media_recorder_handler.h#newcode70 content/renderer/media_recorder/media_recorder_handler.h:70: std::unique_ptr<std::string> encoded_alpha_data, On 2017/03/08 22:38:48, mcasas ...
3 years, 9 months ago (2017-03-09 01:45:41 UTC) #58
vignesh
i'm ok with the overall working of this CL as far the encoder and container ...
3 years, 9 months ago (2017-03-09 07:13:47 UTC) #61
emircan
https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxer.cc File media/muxers/webm_muxer.cc (right): https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxer.cc#newcode338 media/muxers/webm_muxer.cc:338: if (!encoded_alpha_data || encoded_alpha_data->empty()) { On 2017/03/09 07:13:46, vignesh ...
3 years, 9 months ago (2017-03-09 19:18:54 UTC) #62
mcasas
I would consider s/encoded_alpha_data/encoded_alpha/ everywhere because it's clear enough in this context what we're referring ...
3 years, 9 months ago (2017-03-09 21:10:51 UTC) #63
emircan
https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media_recorder/video_track_recorder.cc File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media_recorder/video_track_recorder.cc#newcode229 content/renderer/media_recorder/video_track_recorder.cc:229: base::TimeTicks capture_timestamp) = 0; On 2017/03/09 21:10:50, mcasas wrote: ...
3 years, 9 months ago (2017-03-10 00:22:48 UTC) #66
mcasas
lgtm with a suggestion below. Also, would be interesting to have a LayoutTest for this? ...
3 years, 9 months ago (2017-03-11 01:21:23 UTC) #73
mcasas
https://codereview.chromium.org/2691373005/diff/280001/content/renderer/media_recorder/video_track_recorder.cc File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/280001/content/renderer/media_recorder/video_track_recorder.cc#newcode866 content/renderer/media_recorder/video_track_recorder.cc:866: } Somehow rietveld threw away my comment. I wanted ...
3 years, 9 months ago (2017-03-11 01:25:39 UTC) #74
emircan
https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media_recorder/video_track_recorder.h File content/renderer/media_recorder/video_track_recorder.h (right): https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media_recorder/video_track_recorder.h#newcode80 content/renderer/media_recorder/video_track_recorder.h:80: bool CanEncodeAlphaChannelForTesting(); On 2017/03/11 01:21:22, mcasas wrote: > On ...
3 years, 9 months ago (2017-03-13 05:43:54 UTC) #75
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/2691373005/300001
3 years, 9 months ago (2017-03-13 05:47:05 UTC) #81
commit-bot: I haz the power
3 years, 9 months ago (2017-03-13 07:14:23 UTC) #84
Message was sent while issue was closed.
Committed patchset #7 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/9d369903d6cdd84aaaf70e2a73ad...

Powered by Google App Engine
This is Rietveld 408576698