Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2701203003:
media: Prefer decrypting pipeline when CDM is attached (Closed)
|
Created:
3 years, 10 months ago by xhwang Modified:
3 years, 10 months ago CC:
chfremer+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, ddorwin, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Prefer decrypting pipeline when CDM is attached
In DecoderSelector, if a CDM is attached, always try the decrypting
pipeline first (decoders that support encrypted streams, or decrypting
demuxer stream plus regular decoders), so that the pipeline can handle
encrytped streams later.
BUG=597443
TEST=New tests enabled.
Review-Url: https://codereview.chromium.org/2701203003
Cr-Commit-Position: refs/heads/master@{#452975}
Committed: https://chromium.googlesource.com/chromium/src/+/b2827d2c7e7fb65a6936bedb3b08409448e4d737
Patch Set 1 #
Total comments: 1
Patch Set 2 #
Total comments: 3
Patch Set 3 : fix tests #
Total comments: 6
Patch Set 4 : comments addressed #
Total comments: 4
Patch Set 5 : comments addressed #Messages
Total messages: 60 (35 generated)
The CQ bit was checked by xhwang@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...
Description was changed from ========== media: DecoderSelector try decrypting pipeline first when CDM is attached In DecoderSelector, if a CDM is attached, always try the decrypting pipeline first (decoders that support encrypted streams, or decrypting demuxer stream plus regular decoders), so that the pipeline can handle encrytped streams later. BUG=597443 TEST=New tests enabled. ========== to ========== media: Prefer decrypting pipeline when CDM is attached In DecoderSelector, if a CDM is attached, always try the decrypting pipeline first (decoders that support encrypted streams, or decrypting demuxer stream plus regular decoders), so that the pipeline can handle encrytped streams later. BUG=597443 TEST=New tests enabled. ==========
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...)
The CQ bit was checked by xhwang@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...)
The CQ bit was checked by xhwang@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:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by xhwang@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.
Patchset #1 (id:40001) has been deleted
xhwang@chromium.org changed reviewers: + dalecurtis@chromium.org, jrummell@chromium.org
dalecurtis: Please review from overall DecoderSelector logic's perspective. jrummell: Please review everything. ddorwin: FYI https://codereview.chromium.org/2701203003/diff/60001/media/filters/decoder_s... File media/filters/decoder_selector.cc (right): https://codereview.chromium.org/2701203003/diff/60001/media/filters/decoder_s... media/filters/decoder_selector.cc:206: // MojoAudioDecoder on Android. See http://crbug.com/597443 This will actually cause trouble on Clank as the MojoAudioDecoder is the second decoder in the vector (after FFAD). I'll fix this in the next CL.
You'll need to set the is_encrypted flag on the decoder configs passed through to decoders for this to work. Otherwise GVD will reject the decoder config for non-encrypted streams for libvpx.
On 2017/02/21 18:26:26, DaleCurtis wrote: > You'll need to set the is_encrypted flag on the decoder configs passed through > to decoders for this to work. Otherwise GVD will reject the decoder config for > non-encrypted streams for libvpx. Ah, good catch. I forgot that GVD doesn't support real run-time reinitialization :( I thought about setting the is_encrypted flag. There's a corner case that makes things complicated in that case: - if the JS player attaches a CDM, but only serves clear streams - the clear stream is playable using one of our clear decoders, but not any decrypting decoders In this case, in theory, the stream should still be playable. But if we manually set the is_encrypted flag, the playback will fail. One way to fix this is that if all decoders failed to initialize for the new "encrypted" stream, we should flip the is_encrypted flag back and try all decoders again. But today we immediately drop the decoder after we tried it, so it requires some bigger change in the decoder selector. Or, maybe the corner case is trivial enough that we shouldn't really care?
On 2017/02/21 at 19:35:44, xhwang wrote: > On 2017/02/21 18:26:26, DaleCurtis wrote: > > You'll need to set the is_encrypted flag on the decoder configs passed through > > to decoders for this to work. Otherwise GVD will reject the decoder config for > > non-encrypted streams for libvpx. > > Ah, good catch. I forgot that GVD doesn't support real run-time reinitialization :( > > I thought about setting the is_encrypted flag. There's a corner case that makes things complicated in that case: > > - if the JS player attaches a CDM, but only serves clear streams > - the clear stream is playable using one of our clear decoders, but not any decrypting decoders Is this even possible for Android? On Android the encrypted player can play everything the non-encrypted ones can. Desktop, none of our decoders support encrypted streams, so they'll all be rejected if that flag is set.
On 2017/02/21 20:04:27, DaleCurtis wrote: > On 2017/02/21 at 19:35:44, xhwang wrote: > > On 2017/02/21 18:26:26, DaleCurtis wrote: > > > You'll need to set the is_encrypted flag on the decoder configs passed > through > > > to decoders for this to work. Otherwise GVD will reject the decoder config > for > > > non-encrypted streams for libvpx. > > > > Ah, good catch. I forgot that GVD doesn't support real run-time > reinitialization :( > > > > I thought about setting the is_encrypted flag. There's a corner case that > makes things complicated in that case: > > > > - if the JS player attaches a CDM, but only serves clear streams > > - the clear stream is playable using one of our clear decoders, but not any > decrypting decoders > > Is this even possible for Android? On Android the encrypted player can play > everything the non-encrypted ones can. Desktop, none of our decoders support > encrypted streams, so they'll all be rejected if that flag is set. Agreed. This is more of a hypothetical example to test the general logic in DecoderSelector, assuming we don't have any prior knowledge of our decoders. Also, it's possible in the future that we support some new codec in a clear decoder that we don't support in the CDM (using DecryptingVideoDecoder), that'll cause the same issue, though that problem will even exist in the current CL and is not specific to flipping the "is_encrypted" flag. In reality, I don't expect this to cause issues, especially since we require that the clear and encrypted streams should use the same contentType: https://docs.google.com/document/d/173ruVV4ofl0J4kNmz8E8d04gyI8U5sQ3_759-UCU9... An alternative to flipping the "is_encrypted" flag is to redefine the meaning of "cdm_context" in VideoDecdoer::Initialize() call. Basically, if "cdm_context" is non_null, even if the input stream is clear, the decoder should be able to handle encrypted stream as the result of a future config change, to declare initialization success. This makes the Initialize() semantics a little bit more complicated, but doesn't require API changes on DemuxerStream to flip the "is_encrypted" flag. WDYT? Ultimately, I imagine a world where we can freely switch decoders. We already have the fallback logic that if decoder reinitialization failed, we'll go back to DecoderSelector to choose a new decoder. The only problem now is that the DecoderSelector maintains a vector and we have less and less decoders available every time we choose a decoder. To fix this, the DecoderSelector should own a DecoderFactory so that it can always generate a full list of decoders, and we can select a new one from it. (For decoder error fallback, we should exclude the currently selected decoder.) This requires some refactoring, but I thought this should be doable, until you reminded me that GVD only does a fake reinitialization :( We can discuss on this more if you are interested.
On 2017/02/21 at 20:34:51, xhwang wrote: > On 2017/02/21 20:04:27, DaleCurtis wrote: > > On 2017/02/21 at 19:35:44, xhwang wrote: > > > On 2017/02/21 18:26:26, DaleCurtis wrote: > > > > You'll need to set the is_encrypted flag on the decoder configs passed > > through > > > > to decoders for this to work. Otherwise GVD will reject the decoder config > > for > > > > non-encrypted streams for libvpx. > > > > > > Ah, good catch. I forgot that GVD doesn't support real run-time > > reinitialization :( > > > > > > I thought about setting the is_encrypted flag. There's a corner case that > > makes things complicated in that case: > > > > > > - if the JS player attaches a CDM, but only serves clear streams > > > - the clear stream is playable using one of our clear decoders, but not any > > decrypting decoders > > > > Is this even possible for Android? On Android the encrypted player can play > > everything the non-encrypted ones can. Desktop, none of our decoders support > > encrypted streams, so they'll all be rejected if that flag is set. > > Agreed. This is more of a hypothetical example to test the general logic in DecoderSelector, assuming we don't have any prior knowledge of our decoders. Also, it's possible in the future that we support some new codec in a clear decoder that we don't support in the CDM (using DecryptingVideoDecoder), that'll cause the same issue, though that problem will even exist in the current CL and is not specific to flipping the "is_encrypted" flag. > > In reality, I don't expect this to cause issues, especially since we require that the clear and encrypted streams should use the same contentType: https://docs.google.com/document/d/173ruVV4ofl0J4kNmz8E8d04gyI8U5sQ3_759-UCU9... > > An alternative to flipping the "is_encrypted" flag is to redefine the meaning of "cdm_context" in VideoDecdoer::Initialize() call. Basically, if "cdm_context" is non_null, even if the input stream is clear, the decoder should be able to handle encrypted stream as the result of a future config change, to declare initialization success. This makes the Initialize() semantics a little bit more complicated, but doesn't require API changes on DemuxerStream to flip the "is_encrypted" flag. WDYT? This is also okay, though we'd probably want some way to test this. > > Ultimately, I imagine a world where we can freely switch decoders. We already have the fallback logic that if decoder reinitialization failed, we'll go back to DecoderSelector to choose a new decoder. The only problem now is that the DecoderSelector maintains a vector and we have less and less decoders available every time we choose a decoder. To fix this, the DecoderSelector should own a DecoderFactory so that it can always generate a full list of decoders, and we can select a new one from it. (For decoder error fallback, we should exclude the currently selected decoder.) This requires some refactoring, but I thought this should be doable, until you reminded me that GVD only does a fake reinitialization :( We can discuss on this more if you are interested. This would be best. As it would also allow us to seamlessly switch between hardware and software decoders under error conditions.
On 2017/02/21 20:39:14, DaleCurtis wrote: > On 2017/02/21 at 20:34:51, xhwang wrote: > > On 2017/02/21 20:04:27, DaleCurtis wrote: > > > On 2017/02/21 at 19:35:44, xhwang wrote: > > > > On 2017/02/21 18:26:26, DaleCurtis wrote: > > > > > You'll need to set the is_encrypted flag on the decoder configs passed > > > through > > > > > to decoders for this to work. Otherwise GVD will reject the decoder > config > > > for > > > > > non-encrypted streams for libvpx. > > > > > > > > Ah, good catch. I forgot that GVD doesn't support real run-time > > > reinitialization :( > > > > > > > > I thought about setting the is_encrypted flag. There's a corner case that > > > makes things complicated in that case: > > > > > > > > - if the JS player attaches a CDM, but only serves clear streams > > > > - the clear stream is playable using one of our clear decoders, but not > any > > > decrypting decoders > > > > > > Is this even possible for Android? On Android the encrypted player can play > > > everything the non-encrypted ones can. Desktop, none of our decoders support > > > encrypted streams, so they'll all be rejected if that flag is set. > > > > Agreed. This is more of a hypothetical example to test the general logic in > DecoderSelector, assuming we don't have any prior knowledge of our decoders. > Also, it's possible in the future that we support some new codec in a clear > decoder that we don't support in the CDM (using DecryptingVideoDecoder), that'll > cause the same issue, though that problem will even exist in the current CL and > is not specific to flipping the "is_encrypted" flag. > > > > In reality, I don't expect this to cause issues, especially since we require > that the clear and encrypted streams should use the same contentType: > https://docs.google.com/document/d/173ruVV4ofl0J4kNmz8E8d04gyI8U5sQ3_759-UCU9... > > > > An alternative to flipping the "is_encrypted" flag is to redefine the meaning > of "cdm_context" in VideoDecdoer::Initialize() call. Basically, if "cdm_context" > is non_null, even if the input stream is clear, the decoder should be able to > handle encrypted stream as the result of a future config change, to declare > initialization success. This makes the Initialize() semantics a little bit more > complicated, but doesn't require API changes on DemuxerStream to flip the > "is_encrypted" flag. WDYT? > > This is also okay, though we'd probably want some way to test this. Okay, let me experiment with this idea. Thanks! > > > > Ultimately, I imagine a world where we can freely switch decoders. We already > have the fallback logic that if decoder reinitialization failed, we'll go back > to DecoderSelector to choose a new decoder. The only problem now is that the > DecoderSelector maintains a vector and we have less and less decoders available > every time we choose a decoder. To fix this, the DecoderSelector should own a > DecoderFactory so that it can always generate a full list of decoders, and we > can select a new one from it. (For decoder error fallback, we should exclude the > currently selected decoder.) This requires some refactoring, but I thought this > should be doable, until you reminded me that GVD only does a fake > reinitialization :( We can discuss on this more if you are interested. > > This would be best. As it would also allow us to seamlessly switch between > hardware and software decoders under error conditions. I can write a doc on this later (after M58 time frame) and we can iterate on it. Actually GVD should still work, in a very weird way though. Assume we serve clear stream first, then switch to encrypted stream, on Android: 1. GVD is selected because it can handle clear stream 2. Upon config change, we reinitialize the selected GVD using the encrypted stream, which will fail. 3. In DecoderStream, we fallback to call DecoderSelector::SelectDecoder(), which will create a list of decoders, including a branding new GVD. 4. We reinitialize the new GVD with encrytped stream, and success!
On 2017/02/21 at 20:46:22, xhwang wrote: > On 2017/02/21 20:39:14, DaleCurtis wrote: > > On 2017/02/21 at 20:34:51, xhwang wrote: > > > On 2017/02/21 20:04:27, DaleCurtis wrote: > > > > On 2017/02/21 at 19:35:44, xhwang wrote: > > > > > On 2017/02/21 18:26:26, DaleCurtis wrote: > > > > > > You'll need to set the is_encrypted flag on the decoder configs passed > > > > through > > > > > > to decoders for this to work. Otherwise GVD will reject the decoder > > config > > > > for > > > > > > non-encrypted streams for libvpx. > > > > > > > > > > Ah, good catch. I forgot that GVD doesn't support real run-time > > > > reinitialization :( > > > > > > > > > > I thought about setting the is_encrypted flag. There's a corner case that > > > > makes things complicated in that case: > > > > > > > > > > - if the JS player attaches a CDM, but only serves clear streams > > > > > - the clear stream is playable using one of our clear decoders, but not > > any > > > > decrypting decoders > > > > > > > > Is this even possible for Android? On Android the encrypted player can play > > > > everything the non-encrypted ones can. Desktop, none of our decoders support > > > > encrypted streams, so they'll all be rejected if that flag is set. > > > > > > Agreed. This is more of a hypothetical example to test the general logic in > > DecoderSelector, assuming we don't have any prior knowledge of our decoders. > > Also, it's possible in the future that we support some new codec in a clear > > decoder that we don't support in the CDM (using DecryptingVideoDecoder), that'll > > cause the same issue, though that problem will even exist in the current CL and > > is not specific to flipping the "is_encrypted" flag. > > > > > > In reality, I don't expect this to cause issues, especially since we require > > that the clear and encrypted streams should use the same contentType: > > https://docs.google.com/document/d/173ruVV4ofl0J4kNmz8E8d04gyI8U5sQ3_759-UCU9... > > > > > > An alternative to flipping the "is_encrypted" flag is to redefine the meaning > > of "cdm_context" in VideoDecdoer::Initialize() call. Basically, if "cdm_context" > > is non_null, even if the input stream is clear, the decoder should be able to > > handle encrypted stream as the result of a future config change, to declare > > initialization success. This makes the Initialize() semantics a little bit more > > complicated, but doesn't require API changes on DemuxerStream to flip the > > "is_encrypted" flag. WDYT? > > > > This is also okay, though we'd probably want some way to test this. > > Okay, let me experiment with this idea. Thanks! > > > > > > > Ultimately, I imagine a world where we can freely switch decoders. We already > > have the fallback logic that if decoder reinitialization failed, we'll go back > > to DecoderSelector to choose a new decoder. The only problem now is that the > > DecoderSelector maintains a vector and we have less and less decoders available > > every time we choose a decoder. To fix this, the DecoderSelector should own a > > DecoderFactory so that it can always generate a full list of decoders, and we > > can select a new one from it. (For decoder error fallback, we should exclude the > > currently selected decoder.) This requires some refactoring, but I thought this > > should be doable, until you reminded me that GVD only does a fake > > reinitialization :( We can discuss on this more if you are interested. > > > > This would be best. As it would also allow us to seamlessly switch between > > hardware and software decoders under error conditions. > > I can write a doc on this later (after M58 time frame) and we can iterate on it. > > Actually GVD should still work, in a very weird way though. Assume we serve clear stream first, then switch to encrypted stream, on Android: > 1. GVD is selected because it can handle clear stream Aren't we worried about the case where libvpx is selected instead? > 2. Upon config change, we reinitialize the selected GVD using the encrypted stream, which will fail. > 3. In DecoderStream, we fallback to call DecoderSelector::SelectDecoder(), which will create a list of decoders, including a branding new GVD. > 4. We reinitialize the new GVD with encrytped stream, and success!
(old chats removed) > > I can write a doc on this later (after M58 time frame) and we can iterate on > it. > > > > Actually GVD should still work, in a very weird way though. Assume we serve > clear stream first, then switch to encrypted stream, on Android: > > 1. GVD is selected because it can handle clear stream > > Aren't we worried about the case where libvpx is selected instead? This should still work. We can choose libvpx first, which will fail the reinitialization in (2). Then (3) and (4) will stay the same. It's an open question on whether we prefer this way though. > > 2. Upon config change, we reinitialize the selected GVD using the encrypted > stream, which will fail. > > 3. In DecoderStream, we fallback to call DecoderSelector::SelectDecoder(), > which will create a list of decoders, including a branding new GVD. > > 4. We reinitialize the new GVD with encrytped stream, and success!
On 2017/02/21 at 21:24:35, xhwang wrote: > (old chats removed) > > > I can write a doc on this later (after M58 time frame) and we can iterate on > > it. > > > > > > Actually GVD should still work, in a very weird way though. Assume we serve > > clear stream first, then switch to encrypted stream, on Android: > > > 1. GVD is selected because it can handle clear stream > > > > Aren't we worried about the case where libvpx is selected instead? > > This should still work. We can choose libvpx first, which will fail the reinitialization in (2). Then (3) and (4) will stay the same. It's an open question on whether we prefer this way though. GVD is listed before libvpx decoder though. So it wouldn't be selected in #2; it's already destroyed.
On 2017/02/21 21:27:25, DaleCurtis wrote: > On 2017/02/21 at 21:24:35, xhwang wrote: > > (old chats removed) > > > > I can write a doc on this later (after M58 time frame) and we can iterate > on > > > it. > > > > > > > > Actually GVD should still work, in a very weird way though. Assume we > serve > > > clear stream first, then switch to encrypted stream, on Android: > > > > 1. GVD is selected because it can handle clear stream > > > > > > Aren't we worried about the case where libvpx is selected instead? > > > > This should still work. We can choose libvpx first, which will fail the > reinitialization in (2). Then (3) and (4) will stay the same. It's an open > question on whether we prefer this way though. > > GVD is listed before libvpx decoder though. So it wouldn't be selected in #2; > it's already destroyed. Let's chat on this tomorrow :) As proposed, the DecoderSelector will own a DecoderFactory, instead of a list of pre-created decoders. On (2), we'll have a brand-new list of decoders to select from, which includes a brand new GVD.
OIC, you're talking about in the new world; not the existing one. sgtm :)
The CQ bit was checked by xhwang@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...
As discussed, PTAL again I think it's actually pretty straightforward to keep decoders in the vector after initialization failure (instead of removing them) to either support trying clear decoders for clear stream even if a CDM is attached (after all decrypting decoders fail to initialize), or to support more runtime switch features. But I don't want to further complicate this CL and M58. So we can do that later. https://codereview.chromium.org/2701203003/diff/70001/media/filters/audio_dec... File media/filters/audio_decoder_selector_unittest.cc (left): https://codereview.chromium.org/2701203003/diff/70001/media/filters/audio_dec... media/filters/audio_decoder_selector_unittest.cc:172: // TODO(xhwang): Add kNoCdm tests for clear stream. This TODO is fixed now. https://codereview.chromium.org/2701203003/diff/70001/media/filters/audio_dec... media/filters/audio_decoder_selector_unittest.cc:175: // selected. I found these comments hard to maintain and don't provide much information (it's a duplication of the test name). So I removed them. https://codereview.chromium.org/2701203003/diff/70001/media/filters/decoder_s... File media/filters/decoder_selector.cc (right): https://codereview.chromium.org/2701203003/diff/70001/media/filters/decoder_s... media/filters/decoder_selector.cc:184: config_.set_is_encrypted(true); This is where we flip the |is_encrypted| flag as discussed.
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...)
apparently I need to fix more tests, on that now
The CQ bit was checked by xhwang@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.
dalecurtis / jrummell: tests fixed. PTAL again!
lgtm % some comments and method rename. https://codereview.chromium.org/2701203003/diff/90001/media/base/audio_decode... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/2701203003/diff/90001/media/base/audio_decode... media/base/audio_decoder_config.h:92: // Sets the config to be encrypted or not encrypted manually. This can be Can't use hacker_style() for such a big method; move to .cc and rename SetIsEncrypted(). https://codereview.chromium.org/2701203003/diff/90001/media/base/video_decode... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/2701203003/diff/90001/media/base/video_decode... media/base/video_decoder_config.h:123: // Sets the config to be encrypted or not encrypted manually. This can be Ditto. https://codereview.chromium.org/2701203003/diff/90001/media/filters/decryptin... File media/filters/decrypting_demuxer_stream.cc (right): https://codereview.chromium.org/2701203003/diff/90001/media/filters/decryptin... media/filters/decrypting_demuxer_stream.cc:380: audio_config_ = demuxer_stream_->audio_decoder_config(); Comments would be helpful for these. As it is, it looks like: http://i.imgur.com/esTFV36.gif
comments addressed
https://codereview.chromium.org/2701203003/diff/90001/media/base/audio_decode... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/2701203003/diff/90001/media/base/audio_decode... media/base/audio_decoder_config.h:92: // Sets the config to be encrypted or not encrypted manually. This can be On 2017/02/24 18:03:50, DaleCurtis wrote: > Can't use hacker_style() for such a big method; move to .cc and rename > SetIsEncrypted(). Done. https://codereview.chromium.org/2701203003/diff/90001/media/base/video_decode... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/2701203003/diff/90001/media/base/video_decode... media/base/video_decoder_config.h:123: // Sets the config to be encrypted or not encrypted manually. This can be On 2017/02/24 18:03:50, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/2701203003/diff/90001/media/filters/decryptin... File media/filters/decrypting_demuxer_stream.cc (right): https://codereview.chromium.org/2701203003/diff/90001/media/filters/decryptin... media/filters/decrypting_demuxer_stream.cc:380: audio_config_ = demuxer_stream_->audio_decoder_config(); On 2017/02/24 18:03:50, DaleCurtis wrote: > Comments would be helpful for these. As it is, it looks like: > > http://i.imgur.com/esTFV36.gif :) Done.
The CQ bit was checked by xhwang@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...
lgtm https://codereview.chromium.org/2701203003/diff/110001/media/base/audio_decod... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/2701203003/diff/110001/media/base/audio_decod... media/base/audio_decoder_config.h:20: #include "media/base/media_util.h" nit: Not sure why logging.h and media_util.h are added to this header. The .cc file needs media_util.h, but I don't see anything in the header that needs either. https://codereview.chromium.org/2701203003/diff/110001/media/base/video_decod... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/2701203003/diff/110001/media/base/video_decod... media/base/video_decoder_config.h:18: #include "media/base/media_util.h" nit: Same thing, this is needed in the .cc file.
comments addressed
https://codereview.chromium.org/2701203003/diff/110001/media/base/audio_decod... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/2701203003/diff/110001/media/base/audio_decod... media/base/audio_decoder_config.h:20: #include "media/base/media_util.h" On 2017/02/24 18:37:37, jrummell wrote: > nit: Not sure why logging.h and media_util.h are added to this header. The .cc > file needs media_util.h, but I don't see anything in the header that needs > either. Done. https://codereview.chromium.org/2701203003/diff/110001/media/base/video_decod... File media/base/video_decoder_config.h (right): https://codereview.chromium.org/2701203003/diff/110001/media/base/video_decod... media/base/video_decoder_config.h:18: #include "media/base/media_util.h" On 2017/02/24 18:37:37, jrummell wrote: > nit: Same thing, this is needed in the .cc file. Done.
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, jrummell@chromium.org Link to the patchset: https://codereview.chromium.org/2701203003/#ps130001 (title: "comments addressed")
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
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 xhwang@chromium.org
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": 130001, "attempt_start_ts": 1487971448290030,
"parent_rev": "a343d458ab14a2d85c17b9e845fdf56287559c0c", "commit_rev":
"b2827d2c7e7fb65a6936bedb3b08409448e4d737"}
Message was sent while issue was closed.
Description was changed from ========== media: Prefer decrypting pipeline when CDM is attached In DecoderSelector, if a CDM is attached, always try the decrypting pipeline first (decoders that support encrypted streams, or decrypting demuxer stream plus regular decoders), so that the pipeline can handle encrytped streams later. BUG=597443 TEST=New tests enabled. ========== to ========== media: Prefer decrypting pipeline when CDM is attached In DecoderSelector, if a CDM is attached, always try the decrypting pipeline first (decoders that support encrypted streams, or decrypting demuxer stream plus regular decoders), so that the pipeline can handle encrytped streams later. BUG=597443 TEST=New tests enabled. Review-Url: https://codereview.chromium.org/2701203003 Cr-Commit-Position: refs/heads/master@{#452975} Committed: https://chromium.googlesource.com/chromium/src/+/b2827d2c7e7fb65a6936bedb3b08... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:130001) as https://chromium.googlesource.com/chromium/src/+/b2827d2c7e7fb65a6936bedb3b08... |
