|
|
Created:
3 years, 10 months ago by emircan Modified:
3 years, 9 months ago 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. |
DescriptionSupport 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 : #Messages
Total messages: 84 (70 generated)
The CQ bit was checked by emircan@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by emircan@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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 emircan@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) 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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by emircan@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 #1 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 emircan@chromium.org to run a CQ dry run
Patchset #1 (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 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 emircan@chromium.org to run a CQ dry run
Patchset #1 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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 emircan@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 #1 (id:120001) has been deleted
Description was changed from ========== add alpha BUG= ========== to ========== 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..... Also, working on a alpha video based content_browsertest in a follow-up CL. ==========
emircan@chromium.org changed reviewers: + mcasas@chromium.org, vigneshv@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 emircan@chromium.org to run a CQ dry run
Patchset #1 (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...
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good overall. Just some nits and questions. Thanks! https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:286: // Drop alpha channel if |encoder does not support it yet. Supernit: accidental |. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:845: // It is more expensive to encode 0x00, so use 0x80 instead. nit: Move this line just above the std::fill below. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:858: std::fill(alpha_dummy_planes_.begin(), alpha_dummy_planes_.end(), 128); nit: can just use 0x80 here and avoid the mental conversion between comment and code. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:881: if (!IsInitialized(alpha_codec_config_) || Why is this initialization block needed? The same thing happens above know? https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:919: void VpxEncoder::DoEncode(vpx_codec_ctx_t* encoder, nit: *const ? https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:923: int y_stride, nit: const. ditto for other strides & |force_keyframe| https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:931: bool* keyframe) { nit: *const Ditto in the line above. https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... File media/muxers/webm_muxer.cc (right): https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer.cc:338: if (!encoded_alpha_data || encoded_alpha_data->empty()) { Just checking: Have you verified if this combination of Blocks and SimpleBlocks plays back ok in Chrome (please check both MSE and <video> tag as they use different demuxer implementations). https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... File media/muxers/webm_muxer_unittest.cc (right): https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. A test for video with alpha channel data here would be nice.
https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... File content/renderer/media_recorder/media_recorder_handler.h (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... 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 alpha must go together, but if so, could you add some comment here and in the webm_muxer declaration to state it? Otherwise I might think that alpha and non-alpha could be sent separately and marked in some way. Also what about s/encoded_alpha_data/encoded_alpha/ ? https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... File content/renderer/media_recorder/media_recorder_handler_unittest.cc (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/media_recorder_handler_unittest.cc:58: const bool handles_alpha; Since this test encompasses both encoder and muxer, it might be interesting to indicate here that the support or not is because of the codec, i.e. s/handles_alpha/encoder_supports_alpha/ or similar? (Because webm/mkv could support h264-encoded alpha, I guess...) https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/media_recorder_handler_unittest.cc:277: const size_t kEncodedSizeThreshold = 16; nit: can we add here just to make super sure and for future generations EXPECT_EQ(4u, media::VideoFrame::NumPlanes(alpha_frame->format())); ? https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:98: } Why change this part to a function ISO having it in the constructor? I recommend leaving it as it was. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:153: // If alpha_encode is enabled, encoder is expected to return a second output |alpha_encode|, but also, what about s/alpha_encode/use_alpha_channel/ or similar? "alpha_encode" sounds like a command. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:228: // Inserts an opaque frame followed by two transparent frames and expects them Do you mean that we expect the frame_with_alpha to force creation of a key frame, where otherwise it wouldn't be the case? "Synchronized" doesn't seem to convey that. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:233: const double kFrameRate = 60.0f; I'm not sure we need to specify |kFrameRate| for this test. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:253: EXPECT_CALL(*this, DoOnEncodedVideo(_, _, _, _, false)) We can add an expectation here and in l. 248 for the alpha data to be not null (::testing::NotNull()) ISO just "_". Correspondingly, please add a ::testing::IsNull() in l.241 for the alpha channel. (And of course feel free to add NotNull() expectations on the arguments in these EXPECT clauses). https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... File media/muxers/webm_muxer.cc (right): https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer.cc:249: video_track->SetAlphaMode(mkvmuxer::VideoTrack::kAlpha); Do we have to do it always, regardless of alpha being produced in the encoder or not? https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer.cc:250: video_track->set_max_block_additional_id(1); We need a comment as to why we make this additional thing. Link e.g. to http://wiki.webmproject.org/alpha-channel and say that Alpha would go as a BlockAdditional or so. Also, if you are (over)writing it just to make sure of the value, DCHECK() it instead as done in e.g. l254. https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer.cc:352: encoded_alpha_data->size(), 1, track_index, What's this "1"? It's /* add_id */ https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxer.h File media/muxers/webm_muxer.h (right): https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer.h:69: // to WebM Segment. Either one returns true on success. Say what |encoded_alpha_data| is and that it can perfectly be nullptr.
The CQ bit was checked by emircan@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 emircan@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...
Adressing vignesh@ comments. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:286: // Drop alpha channel if |encoder does not support it yet. On 2017/03/08 17:26:14, vignesh wrote: > Supernit: accidental |. Done. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:845: // It is more expensive to encode 0x00, so use 0x80 instead. On 2017/03/08 17:26:14, vignesh wrote: > nit: Move this line just above the std::fill below. Done. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:858: std::fill(alpha_dummy_planes_.begin(), alpha_dummy_planes_.end(), 128); On 2017/03/08 17:26:14, vignesh wrote: > nit: can just use 0x80 here and avoid the mental conversion between comment and > code. Done. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:881: if (!IsInitialized(alpha_codec_config_) || On 2017/03/08 17:26:14, vignesh wrote: > Why is this initialization block needed? The same thing happens above know? Ouch, I missed it after a refactor. Removing this one. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:919: void VpxEncoder::DoEncode(vpx_codec_ctx_t* encoder, On 2017/03/08 17:26:14, vignesh wrote: > nit: *const ? Done. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:923: int y_stride, On 2017/03/08 17:26:14, vignesh wrote: > nit: const. > > ditto for other strides & |force_keyframe| AFAIK we dont use const for primitive function params in Chromium. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:931: bool* keyframe) { On 2017/03/08 17:26:14, vignesh wrote: > nit: *const > > Ditto in the line above. Done. https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... File media/muxers/webm_muxer.cc (right): https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer.cc:338: if (!encoded_alpha_data || encoded_alpha_data->empty()) { On 2017/03/08 17:26:14, vignesh wrote: > Just checking: Have you verified if this combination of Blocks and SimpleBlocks > plays back ok in Chrome (please check both MSE and <video> tag as they use > different demuxer implementations). It tested a alpha-opaque-alpha canvas recording on <video> tag and it works fine. What do you refer to by MSE? https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... File media/muxers/webm_muxer_unittest.cc (right): https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/03/08 17:26:14, vignesh wrote: > A test for video with alpha channel data here would be nice. I added one. I actually worked on a test similar to OnEncodedVideoTwoFrames but found out that instead of an extra 6 bytes of kSimpleBlockSize, we get 21. I couldn't find the appropriate documentation for alpha describing these extra bytes. Can you help with describing the correct checks here?
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Patchset #3 (id:200001) 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: This issue passed the CQ dry run.
The CQ bit was checked by emircan@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...
Addressing mcasas@ comments. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... File content/renderer/media_recorder/media_recorder_handler.h (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... 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 wrote: > It's unfortunate that the frame and its alpha must go > together, but if so, could you add some comment here > and in the webm_muxer declaration to state it? > > Otherwise I might think that alpha and non-alpha > could be sent separately and marked in some way. > > Also what about s/encoded_alpha_data/encoded_alpha/ ? They are used together in the same function call in webm_muxer so I think it would make sense to send them together. Changed the name. Added a comment. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... File content/renderer/media_recorder/media_recorder_handler_unittest.cc (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/media_recorder_handler_unittest.cc:58: const bool handles_alpha; On 2017/03/08 22:38:48, mcasas wrote: > Since this test encompasses both encoder and muxer, > it might be interesting to indicate here that the support > or not is because of the codec, i.e. > s/handles_alpha/encoder_supports_alpha/ or similar? > (Because webm/mkv could support h264-encoded > alpha, I guess...) Done. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/media_recorder_handler_unittest.cc:277: const size_t kEncodedSizeThreshold = 16; On 2017/03/08 22:38:48, mcasas wrote: > nit: can we add here just to make super sure > and for future generations > EXPECT_EQ(4u, media::VideoFrame::NumPlanes(alpha_frame->format())); > ? Done. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:98: } On 2017/03/08 22:38:48, mcasas wrote: > Why change this part to a function ISO having it > in the constructor? I recommend leaving it as it > was. Earlier |testing::get<0>(GetParam())| was called from ctor(). As a result, it wasn't possible to add TEST_F(). I wanted to avoid adding another combination of tests here because SyncKeyframeOnAlphaSwitch() is actually looking at a very specific sequence. Alternatively, I can make a subclass of VideoTrackRecorderTest and use a default value. WDYT? https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:153: // If alpha_encode is enabled, encoder is expected to return a second output On 2017/03/08 22:38:48, mcasas wrote: > |alpha_encode|, but also, what about > s/alpha_encode/use_alpha_channel/ or similar? > "alpha_encode" sounds like a command. Changed it to encode_alpha_channel. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:228: // Inserts an opaque frame followed by two transparent frames and expects them On 2017/03/08 22:38:48, mcasas wrote: > Do you mean that we expect the frame_with_alpha > to force creation of a key frame, where otherwise it > wouldn't be the case? > "Synchronized" doesn't seem to convey that. I rewrote the description. I learned from the spec that, encoded YUV and AXX data should either be both keyframes or not, hence I added the logic for forcing keyframe. Here I am trying to test that opaque-alpha-alpha sequence results in keyframe-keyframe-non. Without the new logic, it would have been keyframe-non-non. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:233: const double kFrameRate = 60.0f; On 2017/03/08 22:38:48, mcasas wrote: > I'm not sure we need to specify |kFrameRate| for this test. Done. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:253: EXPECT_CALL(*this, DoOnEncodedVideo(_, _, _, _, false)) On 2017/03/08 22:38:48, mcasas wrote: > We can add an expectation here and in l. 248 for the alpha data > to be not null (::testing::NotNull()) ISO just "_". > Correspondingly, please add a ::testing::IsNull() in l.241 for the > alpha channel. > (And of course feel free to add NotNull() expectations on the > arguments in these EXPECT clauses). Mocked function DoOnEncodedVideo() expects std::string, so that is always not null. I tried changing that to unique_ptr but then we have problem with SaveArg. I will just save_arg and check the length of the string to see. https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... File media/muxers/webm_muxer.cc (right): https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer.cc:249: video_track->SetAlphaMode(mkvmuxer::VideoTrack::kAlpha); On 2017/03/08 22:38:48, mcasas wrote: > Do we have to do it always, regardless of > alpha being produced in the encoder or not? According to my offline discussion with vignesh@, yes. We need to set this before encode actually starts and we do not know if alpha is going to be included or not. https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer.cc:250: video_track->set_max_block_additional_id(1); On 2017/03/08 22:38:48, mcasas wrote: > We need a comment as to why we make this additional > thing. Link e.g. to http://wiki.webmproject.org/alpha-channel > and say that Alpha would go as a BlockAdditional or so. > > Also, if you are (over)writing it just to make sure > of the value, DCHECK() it instead as done in e.g. l254. Done. https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer.cc:352: encoded_alpha_data->size(), 1, track_index, On 2017/03/08 22:38:48, mcasas wrote: > What's this "1"? It's > /* add_id */ Added the comment. I just followed this from the webm sample. https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxer.h File media/muxers/webm_muxer.h (right): https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer.h:69: // to WebM Segment. Either one returns true on success. On 2017/03/08 22:38:48, mcasas wrote: > Say what |encoded_alpha_data| is and that it > can perfectly be nullptr. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
i'm ok with the overall working of this CL as far the encoder and container quirks are concerned. lgtm % muxer unit test (although my lgtm probably doesn't count for anything submit-related). I'll leave that to mcasas@ and other owners. https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/160001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:923: int y_stride, On 2017/03/08 23:53:58, emircan wrote: > On 2017/03/08 17:26:14, vignesh wrote: > > nit: const. > > > > ditto for other strides & |force_keyframe| > > AFAIK we dont use const for primitive function params in Chromium. Acknowledged. https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... File media/muxers/webm_muxer.cc (right): https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer.cc:338: if (!encoded_alpha_data || encoded_alpha_data->empty()) { On 2017/03/08 23:53:58, emircan wrote: > On 2017/03/08 17:26:14, vignesh wrote: > > Just checking: Have you verified if this combination of Blocks and > SimpleBlocks > > plays back ok in Chrome (please check both MSE and <video> tag as they use > > different demuxer implementations). > > It tested a alpha-opaque-alpha canvas recording on <video> tag and it works > fine. What do you refer to by MSE? MSE is Media Source Extensions [1]. It's how most javascript players playback video. Can you please share a sample file created this way with me? Just curious. We can take this offline. [1] https://developers.google.com/web/fundamentals/getting-started/primers/media-... https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... File media/muxers/webm_muxer_unittest.cc (right): https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/03/08 23:53:58, emircan wrote: > On 2017/03/08 17:26:14, vignesh wrote: > > A test for video with alpha channel data here would be nice. > > I added one. I actually worked on a test similar to OnEncodedVideoTwoFrames but > found out that instead of an extra 6 bytes of kSimpleBlockSize, we get 21. I > couldn't find the appropriate documentation for alpha describing these extra > bytes. Can you help with describing the correct checks here? Here's the element structure for a block with alpha data: BlockGroup Block ReferenceBlock BlockAdditions BlockMore BlockAdditionalID BlockAdditional (As opposed to a simpleblock without alpha which contains only one element, SimpleBlock). Considering all this, 21 seems a bit low. I'm calculating something near 40. Can we please talk about this offline? I think we should iron this out and add the test.
https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... File media/muxers/webm_muxer.cc (right): https://codereview.chromium.org/2691373005/diff/160001/media/muxers/webm_muxe... media/muxers/webm_muxer.cc:338: if (!encoded_alpha_data || encoded_alpha_data->empty()) { On 2017/03/09 07:13:46, vignesh wrote: > On 2017/03/08 23:53:58, emircan wrote: > > On 2017/03/08 17:26:14, vignesh wrote: > > > Just checking: Have you verified if this combination of Blocks and > > SimpleBlocks > > > plays back ok in Chrome (please check both MSE and <video> tag as they use > > > different demuxer implementations). > > > > It tested a alpha-opaque-alpha canvas recording on <video> tag and it works > > fine. What do you refer to by MSE? > > MSE is Media Source Extensions [1]. It's how most javascript players playback > video. > > Can you please share a sample file created this way with me? Just curious. We > can take this offline. > > [1] > https://developers.google.com/web/fundamentals/getting-started/primers/media-... Sure. The way we are playing back Blob in the demo page seems to be using MSE and it works fine. I am sending a hybrid opaque-transparent webm to you via email.
I would consider s/encoded_alpha_data/encoded_alpha/ everywhere because it's clear enough in this context what we're referring to. https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:229: base::TimeTicks capture_timestamp) = 0; I think we should keep ConfigureEncoderOnEncodingTaskRunner() since it forms part of the interface contract. https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:849: v_plane_offset_ = media::VideoFrame::PlaneSize( You use frame->visible_data(VideoFrame::kAPlane) in l.884, and IIUC, the strides for u and v plane are supposed to be equal to the input frame "normal" planes, so what I suggest is leave here the configuation and dummy planes buffer allocation, use e.g. for the u plane: frame->stride(VideoFrame::kUPlane) in l.885, and with a DCHECK_EQ(media::VideoFrame::RowBytes(VideoFrame::kUPlane, frame->format(), frame_size.width()), frame->stride(VideoFrame::kUPlane)); if you want, but I think it's superfluous; Also try to use as much as possible media::VideoFrame functions and methods to calculate allocation and sizes. https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.h (right): https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.h:80: bool CanEncodeAlphaChannelForTesting(); There is no need for this function since VideoTrackRecorderTest is a friend class, it can directly access encoder_->CanEncodeAlphaChannel() https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:123: return video_track_recorder_->CanEncodeAlphaChannelForTesting(); Yeah, I think here you can do directly: video_track_recorder_->encoder_->CanEncodeAlphaChannel() (but we still need this CanEncodeAlphaChannel() here). https://codereview.chromium.org/2691373005/diff/240001/media/muxers/webm_muxe... File media/muxers/webm_muxer.cc (right): https://codereview.chromium.org/2691373005/diff/240001/media/muxers/webm_muxe... media/muxers/webm_muxer.cc:250: // Encode output of alpha channel is set using BlockAdditional element, see This sound strange, what about: // Alpha channel, if present, is stored in a BlockAdditional next to the // associated opaque Block. And I'd say add as well that this follows Method 1 in [1] [1] http://wiki.webmproject.org/alpha-channel https://codereview.chromium.org/2691373005/diff/240001/media/muxers/webm_muxer.h File media/muxers/webm_muxer.h (right): https://codereview.chromium.org/2691373005/diff/240001/media/muxers/webm_muxe... media/muxers/webm_muxer.h:70: // |encoded_alpha| represents the encode output of alpha channel when nit: s/encode output/encoded output/
The CQ bit was checked by emircan@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/2691373005/diff/240001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:229: base::TimeTicks capture_timestamp) = 0; On 2017/03/09 21:10:50, mcasas wrote: > I think we should keep > ConfigureEncoderOnEncodingTaskRunner() > since it forms part of the interface contract. It is not called from any user of the interface though. Each implementation calls their own implementation of it in EncodeOnEncodingTaskRunner() which is pure virtual. I think it doesn't belong to the interface contract any more. Also, with these changes what would it represent for VpxEncoder? We shouldn't configure both encoders because we don't know if there is an alpha input yet. https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:849: v_plane_offset_ = media::VideoFrame::PlaneSize( On 2017/03/09 21:10:50, mcasas wrote: > You use > frame->visible_data(VideoFrame::kAPlane) > in l.884, and IIUC, the strides for u and v plane > are supposed to be equal to the input frame "normal" > planes, so what I suggest is leave here the configuation > and dummy planes buffer allocation, use e.g. for the > u plane: > > frame->stride(VideoFrame::kUPlane) > > in l.885, and with a > > DCHECK_EQ(media::VideoFrame::RowBytes(VideoFrame::kUPlane, frame->format(), > frame_size.width()), > > frame->stride(VideoFrame::kUPlane)); > > if you want, but I think it's superfluous; > > Also try to use as much as possible media::VideoFrame > functions and methods to calculate allocation and sizes. We are already using media::VideoFrame functions for every calculation here. I don't think your suggestion would work. Consider the case where |frame->stride(VideoFrame::kUPlane) == frame->coded_size.width()| and |frame->coded_size.width() != frame->visible_size.width()|. Here we allocate the dummy buffers based on visible_size so their strides must also be based on visible_size. It might not be equal to UV buffers' stride(which should be >=coded_size.width()) and that is fine. Also, I wanted to avoid recalculating same values for the same visible_size here so I kept the class members. https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.h (right): https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.h:80: bool CanEncodeAlphaChannelForTesting(); On 2017/03/09 21:10:50, mcasas wrote: > There is no need for this function since > VideoTrackRecorderTest is a friend class, it > can directly access > encoder_->CanEncodeAlphaChannel() Encoder is forward declared though, so it cannot access the methods. I didn't think it would be worth moving that whole chunk of info into the h file for this. WDYT? Also, I think VideoTrackRecorder is too large now and up for a refactor. I will make a bug. https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder_unittest.cc (right): https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... content/renderer/media_recorder/video_track_recorder_unittest.cc:123: return video_track_recorder_->CanEncodeAlphaChannelForTesting(); On 2017/03/09 21:10:50, mcasas wrote: > Yeah, I think here you can do directly: > video_track_recorder_->encoder_->CanEncodeAlphaChannel() > > (but we still need this CanEncodeAlphaChannel() here). See my reply before. https://codereview.chromium.org/2691373005/diff/240001/media/muxers/webm_muxe... File media/muxers/webm_muxer.cc (right): https://codereview.chromium.org/2691373005/diff/240001/media/muxers/webm_muxe... media/muxers/webm_muxer.cc:250: // Encode output of alpha channel is set using BlockAdditional element, see On 2017/03/09 21:10:50, mcasas wrote: > This sound strange, what about: > // Alpha channel, if present, is stored in a BlockAdditional next to the > // associated opaque Block. > And I'd say add as well that this follows Method 1 in [1] > > [1] http://wiki.webmproject.org/alpha-channel Done. https://codereview.chromium.org/2691373005/diff/240001/media/muxers/webm_muxer.h File media/muxers/webm_muxer.h (right): https://codereview.chromium.org/2691373005/diff/240001/media/muxers/webm_muxe... media/muxers/webm_muxer.h:70: // |encoded_alpha| represents the encode output of alpha channel when On 2017/03/09 21:10:50, mcasas wrote: > nit: s/encode output/encoded output/ Done.
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 emircan@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.
lgtm with a suggestion below. Also, would be interesting to have a LayoutTest for this? Can be added in a follow up CL w/ a <canvas>.captureStream() w/ alpha. https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:229: base::TimeTicks capture_timestamp) = 0; On 2017/03/10 00:22:47, emircan wrote: > On 2017/03/09 21:10:50, mcasas wrote: > > I think we should keep > > ConfigureEncoderOnEncodingTaskRunner() > > since it forms part of the interface contract. > > It is not called from any user of the interface though. Each implementation > calls their own implementation of it in EncodeOnEncodingTaskRunner() which is > pure virtual. I think it doesn't belong to the interface contract any more. > Also, with these changes what would it represent for VpxEncoder? We shouldn't > configure both encoders because we don't know if there is an alpha input yet. ok, IC https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.h (right): https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.h:80: bool CanEncodeAlphaChannelForTesting(); On 2017/03/10 00:22:47, emircan wrote: > On 2017/03/09 21:10:50, mcasas wrote: > > There is no need for this function since > > VideoTrackRecorderTest is a friend class, it > > can directly access > > encoder_->CanEncodeAlphaChannel() > > Encoder is forward declared though, so it cannot access the methods. I didn't > think it would be worth moving that whole chunk of info into the h file for > this. WDYT? OIC. Well, keep it in mind when splitting up the VTRecorders (maybe add a TODO() + bug) -- it's just a bit sad that we have a friend class VideoTrackRecorderTest _and_ a ForTesting() method, we should do one or the other. > Also, I think VideoTrackRecorder is too large now and up for a refactor. I will > make a bug. https://codereview.chromium.org/2691373005/diff/280001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/280001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:839: if (frame->format() == media::PIXEL_FORMAT_YV12A) { nit: Maybe do a const bool frame_has_alpha = frame->format() == media::PIXEL_FORMAT_YV12A; and use it here and in l.880
https://codereview.chromium.org/2691373005/diff/280001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/280001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:866: } Somehow rietveld threw away my comment. I wanted to say that |last_frame_alpha_| is a bit vague for a name. Can we s/last_frame_alpha_/last_frame_had_alpha_/ and take l.860-866 out of the initialization block and instead just be: const bool force_keyframe = frame_has_alpha && !last_frame_had_alpha_; last_frame_had_alpha_ = frame_has_alpha; essentially forcing a keyframe if the current frame has alpha and the previous one didn't have alpha. Makes sense? And |last_frame_had_alpha_| is init'ed to false like it is now.
https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.h (right): https://codereview.chromium.org/2691373005/diff/240001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.h:80: bool CanEncodeAlphaChannelForTesting(); On 2017/03/11 01:21:22, mcasas wrote: > On 2017/03/10 00:22:47, emircan wrote: > > On 2017/03/09 21:10:50, mcasas wrote: > > > There is no need for this function since > > > VideoTrackRecorderTest is a friend class, it > > > can directly access > > > encoder_->CanEncodeAlphaChannel() > > > > Encoder is forward declared though, so it cannot access the methods. I didn't > > think it would be worth moving that whole chunk of info into the h file for > > this. WDYT? > > OIC. Well, keep it in mind when splitting up the VTRecorders > (maybe add a TODO() + bug) -- it's just a bit sad > that we have a friend class VideoTrackRecorderTest > _and_ a ForTesting() method, we should do one or the > other. > > > Also, I think VideoTrackRecorder is too large now and up for a refactor. I > will > > make a bug. > Done. https://codereview.chromium.org/2691373005/diff/280001/content/renderer/media... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2691373005/diff/280001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:839: if (frame->format() == media::PIXEL_FORMAT_YV12A) { On 2017/03/11 01:21:22, mcasas wrote: > nit: Maybe do a > const bool frame_has_alpha = frame->format() == media::PIXEL_FORMAT_YV12A; > > and use it here and in l.880 Done. https://codereview.chromium.org/2691373005/diff/280001/content/renderer/media... content/renderer/media_recorder/video_track_recorder.cc:866: } On 2017/03/11 01:25:39, mcasas wrote: > Somehow rietveld threw away my comment. > > I wanted to say that |last_frame_alpha_| is a bit > vague for a name. Can we > s/last_frame_alpha_/last_frame_had_alpha_/ > and take l.860-866 out of the initialization > block and instead just be: > > const bool force_keyframe = frame_has_alpha && !last_frame_had_alpha_; > last_frame_had_alpha_ = frame_has_alpha; > > essentially forcing a keyframe if the current frame > has alpha and the previous one didn't have alpha. > Makes sense? > > And |last_frame_had_alpha_| is init'ed to false > like it is now. Done.
The CQ bit was checked by emircan@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 emircan@chromium.org
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vigneshv@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2691373005/#ps300001 (title: " ")
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": 300001, "attempt_start_ts": 1489384008792590, "parent_rev": "ac041cfab01f509ef22e0f6fedbad3b421bbe20a", "commit_rev": "9d369903d6cdd84aaaf70e2a73ada508e2b34eaa"}
Message was sent while issue was closed.
Description was changed from ========== 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..... Also, working on a alpha video based content_browsertest in a follow-up CL. ========== to ========== 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..... 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/+/9d369903d6cdd84aaaf70e2a73ad... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:300001) as https://chromium.googlesource.com/chromium/src/+/9d369903d6cdd84aaaf70e2a73ad... |