Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11745026:
Encrypted Media: Add config change support in DecryptingVideoDecoder. (Closed)
|
Created:
7 years, 11 months ago by xhwang Modified:
7 years, 11 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEncrypted Media: Add config change support in DecryptingVideoDecoder.
BUG=168128, 168129
TEST=Added unittests; demo page works.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175299
Patch Set 1 #
Total comments: 12
Patch Set 2 : rename tests for consistency; will move tests in the next patch set #Patch Set 3 : reordered tests; fix a missing return in DVD #
Total comments: 2
Patch Set 4 : #Patch Set 5 : remove ffvd changes; will do that in another CL #
Messages
Total messages: 15 (0 generated)
PTAL! If this looks good, I'll apply similar change to the DecryptingAudioDecoder.
Looks good. You should probably add a test like PipelineIntegrationTest.MediaSource_ConfigChange_WebM as well just to make sure all the pieces interact with eachother as expected. https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... File media/filters/decrypting_video_decoder.cc (right): https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... media/filters/decrypting_video_decoder.cc:215: } else { nit: add return above and drop else https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... media/filters/decrypting_video_decoder.cc:259: decryptor_->InitializeVideoDecoder( nit: In a different CL, consider changing the signature of this method to take a const VideoDecoderConfig& instead of a scoped_ptr<VideoDecoderConfig>. The conversion to a scoped_ptr above seems like an annoying dance for this caller to go through with no apparent benefit. https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... File media/filters/decrypting_video_decoder_unittest.cc (right): https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... media/filters/decrypting_video_decoder_unittest.cc:672: TEST_F(DecryptingVideoDecoderTest, DemuxerRead_ConfigChangeDuringReset) { How is this test different from Reset_DuringPendingConfigChange? The code looks different, but it looks like they are testing the same thing.
Looks fine. I'll let Aaron wrap it up. https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... File media/filters/decrypting_video_decoder.cc (right): https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... media/filters/decrypting_video_decoder.cc:212: if (reset_cb_.is_null()) { Could you also eliminate this indent by moving 222-226 up there and inverting the logic? https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... media/filters/decrypting_video_decoder.cc:215: } else { On 2013/01/04 16:03:20, acolwell wrote: > nit: add return above and drop else Maybe even invert the logic to if (!reset_cb_.is_null()), which seems to be the more common pattern in this file.
comments mostly resolved. PTAL again! https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... File media/filters/decrypting_video_decoder.cc (right): https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... media/filters/decrypting_video_decoder.cc:212: if (reset_cb_.is_null()) { On 2013/01/04 17:59:01, ddorwin wrote: > Could you also eliminate this indent by moving 222-226 up there and inverting > the logic? Done. https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... media/filters/decrypting_video_decoder.cc:215: } else { On 2013/01/04 16:03:20, acolwell wrote: > nit: add return above and drop else Done. https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... media/filters/decrypting_video_decoder.cc:215: } else { On 2013/01/04 17:59:01, ddorwin wrote: > On 2013/01/04 16:03:20, acolwell wrote: > > nit: add return above and drop else > > Maybe even invert the logic to if (!reset_cb_.is_null()), which seems to be the > more common pattern in this file. Done. https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... media/filters/decrypting_video_decoder.cc:259: decryptor_->InitializeVideoDecoder( On 2013/01/04 16:03:20, acolwell wrote: > nit: In a different CL, consider changing the signature of this method to take a > const VideoDecoderConfig& instead of a scoped_ptr<VideoDecoderConfig>. The > conversion to a scoped_ptr above seems like an annoying dance for this caller to > go through with no apparent benefit. The original reason that I use the scoped_ptr<> here is that XxxDecoderConfig is not copyable. I don't really see why we chose to do that. Probably we can eliminate that restriction and then remove the use of scoped_ptr<> here. https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... File media/filters/decrypting_video_decoder_unittest.cc (right): https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... media/filters/decrypting_video_decoder_unittest.cc:672: TEST_F(DecryptingVideoDecoderTest, DemuxerRead_ConfigChangeDuringReset) { On 2013/01/04 16:03:20, acolwell wrote: > How is this test different from Reset_DuringPendingConfigChange? The code looks > different, but it looks like they are testing the same thing. Renamed (reordered) these tests so that they are more consistent with the Reset_XXX tests. Also added more comments to make it clear what we are testing. For DVD those two tests go through the same code path. But from an API's perspective, the reset is called at different time, so IMHO we still need two tests to cover them.
BTW, ddorwin is adding a config change for encrypted media in pipeline integration change. That'll test the decrypt-only path (using DecryptingDemuxerStream and normal video decoders). We don't have a good way to test DecryptingXxxDecoders in that test and will think about it.
lgtm % comments https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... File media/filters/decrypting_video_decoder.cc (right): https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... media/filters/decrypting_video_decoder.cc:259: decryptor_->InitializeVideoDecoder( On 2013/01/05 00:09:55, xhwang wrote: > On 2013/01/04 16:03:20, acolwell wrote: > > nit: In a different CL, consider changing the signature of this method to take > a > > const VideoDecoderConfig& instead of a scoped_ptr<VideoDecoderConfig>. The > > conversion to a scoped_ptr above seems like an annoying dance for this caller > to > > go through with no apparent benefit. > > The original reason that I use the scoped_ptr<> here is that XxxDecoderConfig is > not copyable. I don't really see why we chose to do that. Probably we can > eliminate that restriction and then remove the use of scoped_ptr<> here. I don't understand. If you make the parameter const& a copy isn't needed. If code in InitializeVideoDecoder() needs to save a copy then it can use CopyFrom() just like you do here. It seemed cleaner to me to have the callee do the copy if necessary instead of forcing the caller to do it. https://codereview.chromium.org/11745026/diff/11001/media/filters/ffmpeg_vide... File media/filters/ffmpeg_video_decoder_unittest.cc (left): https://codereview.chromium.org/11745026/diff/11001/media/filters/ffmpeg_vide... media/filters/ffmpeg_video_decoder_unittest.cc:424: // Test resetting when decoder has initialized but not decoded. I'd prefer these changes be left out of this CL since they are unrelated to the primary purpose of this change. It would be nice if a different CL actually created a base class for common test cases that DVD & FVD unit tests could share.
https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... File media/filters/decrypting_video_decoder.cc (right): https://codereview.chromium.org/11745026/diff/1/media/filters/decrypting_vide... media/filters/decrypting_video_decoder.cc:259: decryptor_->InitializeVideoDecoder( On 2013/01/05 01:17:12, acolwell wrote: > On 2013/01/05 00:09:55, xhwang wrote: > > On 2013/01/04 16:03:20, acolwell wrote: > > > nit: In a different CL, consider changing the signature of this method to > take > > a > > > const VideoDecoderConfig& instead of a scoped_ptr<VideoDecoderConfig>. The > > > conversion to a scoped_ptr above seems like an annoying dance for this > caller > > to > > > go through with no apparent benefit. > > > > The original reason that I use the scoped_ptr<> here is that XxxDecoderConfig > is > > not copyable. I don't really see why we chose to do that. Probably we can > > eliminate that restriction and then remove the use of scoped_ptr<> here. > > I don't understand. If you make the parameter const& a copy isn't needed. If > code in InitializeVideoDecoder() needs to save a copy then it can use CopyFrom() > just like you do here. It seemed cleaner to me to have the callee do the copy if > necessary instead of forcing the caller to do it. I remember the original reason is that in PpapiDecryptor I need to do a post, where the parameters needs to be copied: http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/media/crypto/ppapi_... I agree that even in this case the callee could manually copy the config. But backing a bit, why in the first place we make them uncopiable? Is it because it could have extra_data which requires deep copy? https://codereview.chromium.org/11745026/diff/11001/media/filters/ffmpeg_vide... File media/filters/ffmpeg_video_decoder_unittest.cc (left): https://codereview.chromium.org/11745026/diff/11001/media/filters/ffmpeg_vide... media/filters/ffmpeg_video_decoder_unittest.cc:424: // Test resetting when decoder has initialized but not decoded. On 2013/01/05 01:17:12, acolwell wrote: > I'd prefer these changes be left out of this CL since they are unrelated to the > primary purpose of this change. > > It would be nice if a different CL actually created a base class for common test > cases that DVD & FVD unit tests could share. reverted these changes. will think about if we can consolidate some common cases here.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11745026/21002
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11745026/21002
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11745026/21002
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11745026/21002
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
