Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11492003:
Encrypted Media: Support Audio Decrypt-Only. (Closed)
|
Created:
8 years ago by xhwang Modified:
8 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEncrypted Media: Support Audio Decrypt-Only.
- Add AudioDecoderSelector to facilitate AudioDecoder selection.
- Add AudioDecoderSelectorTest.
- Update key_system.cc to support audio in clearkey key system.
- Update content_browsertests to test audio decrypt-only.
BUG=123421
TEST=both decrypt-only and decrypt-and-decode works for audio!
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173235
Patch Set 1 #
Total comments: 1
Patch Set 2 : this CL is working now, in a very hacky way though ;) #Patch Set 3 : working and not hacky; need to update comments and tests #
Total comments: 52
Patch Set 4 : comments mostly resolved (I believe); need to add/update tests if this looks good #
Total comments: 21
Patch Set 5 : resolved comments and fixed unittest #
Total comments: 22
Patch Set 6 : added unittest; no comments resolved yet #Patch Set 7 : rebase only #Patch Set 8 : comments resolved; will update key_system #Patch Set 9 : updated key_system.cc and content_browsertests #
Total comments: 8
Patch Set 10 : update player_x11 and key_system unittest #Patch Set 11 : comments resolved #
Total comments: 7
Patch Set 12 : nits #Patch Set 13 : rebase #
Messages
Total messages: 28 (0 generated)
Hello, This CL only compiles and needs a lot of work. Please review it only from a design perspective. Thanks! xhwang
This CL looks much better (IMHO) now. PTAL! https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h File media/base/decryptor.h (right): https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h#new... media/base/decryptor.h:207: // Callback to notify decryptor creation. I moved this block here since it's duplicated in a lot of places. Also I feel the name is too long... How about SetDecryptorCB and RegisterSetDecryptorCB? https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.cc:103: decrypting_demuxer_stream_->Reset(base::Bind(&base::DoNothing)); Similar to what we are doing in media pipeline, the decoder->Reset() doesn't need to wait for demuxer_stream->Reset(). If we stick to this pattern, we could probably remove the callback parameter in DecyrptingDemuxerStream::Reset(). Otherwise, I can add another callback to make sure the two Reset() are called in sequence.
Thanks. I like this approach better. I did not review ADF.cc closely. I will look closer in the next round. https://codereview.chromium.org/11492003/diff/1/media/base/pipeline.h File media/base/pipeline.h (right): https://codereview.chromium.org/11492003/diff/1/media/base/pipeline.h#newcode131 media/base/pipeline.h:131: // pipeline's buffering state changes. add new param? https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h File media/base/decryptor.h (right): https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h#new... media/base/decryptor.h:207: // Callback to notify decryptor creation. On 2012/12/11 02:27:40, xhwang wrote: > I moved this block here since it's duplicated in a lot of places. Also I feel > the name is too long... How about SetDecryptorCB and RegisterSetDecryptorCB? I agree about the length (plus the lack of meaning). Do we name CBs based on the function name or what they signal? Would DecryptorCreatedCB work? https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h#new... media/base/decryptor.h:209: // Callback to request/cancel decryptor creation notification. "...notification of decryptor creation." https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h#new... media/base/decryptor.h:216: typedef base::Callback<void(const DecryptorNotificationCB&)> This is a callback for a callback? Seems weird. Maybe this is why naming is so hard. See also comment about naming of the actual function in wmpi.cc. Maybe fixing that will help. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... File media/filters/audio_decoder_factory.h (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:30: class MEDIA_EXPORT AudioDecoderFactory { It's odd for a factory to have state. This factory uses multiple threads I assume, so we'll need state somewhere. Does someone own the factory instance? (Thinking out loud, I'll answer this question myself.) https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:44: const scoped_refptr<DecryptingDemuxerStream>&)> InitDoneCB; Should the callback be DecoderCreatedCB? https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:47: // available to perform decryption. I don't understand? The factory will not support decrypted streams? Why would a callback parameter have control implications? https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:50: const AudioDecoderList& precreated_decoders, "precreated" is an odd term. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:54: void InitAudioDecoder(const scoped_refptr<DemuxerStream>& stream, Is this Init or Create? https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.cc:103: decrypting_demuxer_stream_->Reset(base::Bind(&base::DoNothing)); On 2012/12/11 02:27:40, xhwang wrote: > Similar to what we are doing in media pipeline, the decoder->Reset() doesn't > need to wait for demuxer_stream->Reset(). If we stick to this pattern, we could > probably remove the callback parameter in DecyrptingDemuxerStream::Reset(). > Otherwise, I can add another callback to make sure the two Reset() are called in > sequence. I defer to scherkus. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.cc:205: const scoped_refptr<AudioDecoder>& audio_decoder, Would selected_decoder be a more helpful name? https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.cc:211: factory_.reset(); Should we have the factory manage its own lifetime? See comment in ADF.h. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.cc:214: DCHECK(!sink_); Should we have stored decoder_ at 209 in this case? I'm thinking we should have 211 then 213-216 then 218-221 then 209-210. WDYT? https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... File media/filters/audio_renderer_impl.h (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.h:158: scoped_ptr<AudioDecoderFactory> factory_; decoder_factory_ https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.h:160: scoped_refptr<DecryptingDemuxerStream> decrypting_demuxer_stream_; Maybe explain when this is populated/valid. https://codereview.chromium.org/11492003/diff/5001/webkit/media/webmediaplaye... File webkit/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/11492003/diff/5001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.cc:1131: base::Bind(&ProxyDecryptor::RequestDecryptorNotification, ProxyDecryptor::RequestDecryptorNotification() doesn't seem to request notification. It looks like it saves a notification request. Should it be Register...?
replied to comments only. no updates yet. PTAL! https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h File media/base/decryptor.h (right): https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h#new... media/base/decryptor.h:216: typedef base::Callback<void(const DecryptorNotificationCB&)> On 2012/12/11 05:13:34, ddorwin wrote: > This is a callback for a callback? Seems weird. Maybe this is why naming is so > hard. See also comment about naming of the actual function in wmpi.cc. Maybe > fixing that will help. The original naming (proposed by fischman) actually looks good to me, except it's too long. We don't care if the decryptor is created, or how the decryptor will be used. It's all about notifying the requester when a decryptor is available. That said, I like the word "Register" too. How about DecryptorReadyCB and RegisterDecryptorReadyCB? https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... File media/filters/audio_decoder_factory.h (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:47: // available to perform decryption. On 2012/12/11 05:13:34, ddorwin wrote: > I don't understand? The factory will not support decrypted streams? Why would a > callback parameter have control implications When --enable-encrypted-media is not set, we don't create the ProxyDecryptor at all (http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/media/webmediaplaye...). That's when this callback could be null. In that case, we want the factory to just initialize decoders from the precreated list. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:50: const AudioDecoderList& precreated_decoders, On 2012/12/11 05:13:34, ddorwin wrote: > "precreated" is an odd term. How about "default", to match "AddDefaultDecodersToCollection" in filter_helper? https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:54: void InitAudioDecoder(const scoped_refptr<DemuxerStream>& stream, On 2012/12/11 05:13:34, ddorwin wrote: > Is this Init or Create? Well, this is tricky. The accurate name should be "CreateIfNeededAndInitializeAudioDecoder". The complication comes from the fact that some decoders are pre-created. So I just took the common part "Init" and skipped the tricky part "Create"... https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.cc:211: factory_.reset(); On 2012/12/11 05:13:34, ddorwin wrote: > Should we have the factory manage its own lifetime? See comment in ADF.h. After the audio decoder (and decrypting_demuxer_stream) is/are populated, the factory is not needed anymore. That's why it's destroyed here. I am not sure how it could manage it's own lifetime... https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.cc:214: DCHECK(!sink_); On 2012/12/11 05:13:34, ddorwin wrote: > Should we have stored decoder_ at 209 in this case? > I'm thinking we should have 211 then 213-216 then 218-221 then 209-210. WDYT? It's a little tricky. We are passing const-ref of the scoped_refptr, so we are not adding ref counts to the audio_decoder and decrypting_demuxer_stream. If we destroy the factory early, we could end up destroying the audio_decoder and/or decrypting_demuxer_stream as well. Other than that I agree the order can be improved. Will work on that. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... File media/filters/audio_renderer_impl.h (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.h:158: scoped_ptr<AudioDecoderFactory> factory_; On 2012/12/11 05:13:34, ddorwin wrote: > decoder_factory_ Will do. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.h:160: scoped_refptr<DecryptingDemuxerStream> decrypting_demuxer_stream_; On 2012/12/11 05:13:34, ddorwin wrote: > Maybe explain when this is populated/valid. Will add comment.
https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h File media/base/decryptor.h (right): https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h#new... media/base/decryptor.h:216: typedef base::Callback<void(const DecryptorNotificationCB&)> On 2012/12/11 19:43:04, xhwang wrote: > On 2012/12/11 05:13:34, ddorwin wrote: > > This is a callback for a callback? Seems weird. Maybe this is why naming is so > > hard. See also comment about naming of the actual function in wmpi.cc. Maybe > > fixing that will help. > > The original naming (proposed by fischman) actually looks good to me, except > it's too long. > > We don't care if the decryptor is created, or how the decryptor will be used. > It's all about notifying the requester when a decryptor is available. That said, > I like the word "Register" too. How about DecryptorReadyCB and > RegisterDecryptorReadyCB? Well this is actually setting / unsetting an observer callback. In other words: class DecryptorCreator { public void SetObserver(const DecryptorCB& cb) = 0; }; I kind of like the word "set" as it doesn't make you think you can register multiple callbacks. How about: DecryptorCreatedCB SetDecryptorCreatedCB ... alternatively perhaps it would be clearer w/o callbacks and using something more old fashioned like observer_list.h and C++ interfaces https://codereview.chromium.org/11492003/diff/5001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/11492003/diff/5001/media/base/pipeline.cc#new... media/base/pipeline.cc:898: request_decryptor_notification_cb_, why does this have to go in via Initialize() versus ctor? https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... File media/filters/audio_decoder_factory.h (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:37: // AudioDecoder initialization failed. nit: this style of indentation is a waste of valuable whitespace my suggestion is to simply add an empty line inbetween first and second parameter descriptions https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.cc:103: decrypting_demuxer_stream_->Reset(base::Bind(&base::DoNothing)); On 2012/12/11 05:13:34, ddorwin wrote: > On 2012/12/11 02:27:40, xhwang wrote: > > Similar to what we are doing in media pipeline, the decoder->Reset() doesn't > > need to wait for demuxer_stream->Reset(). If we stick to this pattern, we > could > > probably remove the callback parameter in DecyrptingDemuxerStream::Reset(). > > Otherwise, I can add another callback to make sure the two Reset() are called > in > > sequence. > > I defer to scherkus. Why do we not need to wait? Is it because it's synchronous or something else? What are you referring to when you say "similar to what we are doing in media pipeline"?
Mostly replies, but at least one new comment. https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h File media/base/decryptor.h (right): https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h#new... media/base/decryptor.h:216: typedef base::Callback<void(const DecryptorNotificationCB&)> On 2012/12/11 20:52:15, scherkus wrote: > On 2012/12/11 19:43:04, xhwang wrote: > > On 2012/12/11 05:13:34, ddorwin wrote: > > > This is a callback for a callback? Seems weird. Maybe this is why naming is > so > > > hard. See also comment about naming of the actual function in wmpi.cc. Maybe > > > fixing that will help. > > > > The original naming (proposed by fischman) actually looks good to me, except > > it's too long. > > > > We don't care if the decryptor is created, or how the decryptor will be used. > > It's all about notifying the requester when a decryptor is available. That > said, > > I like the word "Register" too. How about DecryptorReadyCB and > > RegisterDecryptorReadyCB? > > Well this is actually setting / unsetting an observer callback. In other words: > > class DecryptorCreator { > public void SetObserver(const DecryptorCB& cb) = 0; > }; > > I kind of like the word "set" as it doesn't make you think you can register > multiple callbacks. > > How about: > DecryptorCreatedCB > SetDecryptorCreatedCB > > > ... alternatively perhaps it would be clearer w/o callbacks and using something > more old fashioned like observer_list.h and C++ interfaces Either of these SGTM. Combining scherkus's and xhwang's comments, we end up with: DecryptorReadyCB SetDecryptorReadyCB I think we should avoid pulling in new interfaces to this CL. If we want to refactor (other than naming), let's do it in a separate CL. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... File media/filters/audio_decoder_factory.h (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:47: // available to perform decryption. On 2012/12/11 19:43:04, xhwang wrote: > On 2012/12/11 05:13:34, ddorwin wrote: > > I don't understand? The factory will not support decrypted streams? Why would > a > > callback parameter have control implications > > When --enable-encrypted-media is not set, we don't create the ProxyDecryptor at > all > (http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/media/webmediaplaye...). > That's when this callback could be null. In that case, we want the factory to > just initialize decoders from the precreated list. Why not use an explicit control (Boolean)? It doesn't matter too much since this condition will go away soonish. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:49: const scoped_refptr<base::MessageLoopProxy>& message_loop, Is there a method/pattern to who parameters are passed to the constructor or Init? Is this designed such that the factory could be used to obtain multiple decoders? In the current use, the class is constructed then Init is called, so we could even have a simple constructor or eliminate Init. (I'm not saying we should, just pointing out the usage.) https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:50: const AudioDecoderList& precreated_decoders, On 2012/12/11 19:43:04, xhwang wrote: > On 2012/12/11 05:13:34, ddorwin wrote: > > "precreated" is an odd term. > > How about "default", to match "AddDefaultDecodersToCollection" in filter_helper? Sounds better, BUT not all are default. When we do this for video, GPU is not created by that function. What about standard or normal or clear? https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:54: void InitAudioDecoder(const scoped_refptr<DemuxerStream>& stream, On 2012/12/11 19:43:04, xhwang wrote: > On 2012/12/11 05:13:34, ddorwin wrote: > > Is this Init or Create? > > Well, this is tricky. The accurate name should be > "CreateIfNeededAndInitializeAudioDecoder". The complication comes from the fact > that some decoders are pre-created. So I just took the common part "Init" and > skipped the tricky part "Create"... Select? Get? Obtain? On a related note, Factories create things so maybe this class is misnamed. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.cc:211: factory_.reset(); On 2012/12/11 19:43:04, xhwang wrote: > On 2012/12/11 05:13:34, ddorwin wrote: > > Should we have the factory manage its own lifetime? See comment in ADF.h. > > After the audio decoder (and decrypting_demuxer_stream) is/are populated, the > factory is not needed anymore. That's why it's destroyed here. I am not sure how > it could manage it's own lifetime... Init() AddRef()'s then errors or success DeRef() after calling the callback. That's a bit odd too the way the factory has a constructor. Really we don't need a factory object - we just want this class to do something and notify us when it's done. This happens to need state due to all the callbacks. Maybe we can hide or avoid this. Possibilities, respectively: * Factory::Get() creates a self-managing pimpl. * Pass a state object to each function in the flow. Maybe as a scoped_ptr or something so it is always deleted. WDYT?
Most comments resolved. PTAL! If you have comments that is a followup of an old comments. Please comment it on the new patch. It's really hard for me to track comments on old patches. Thanks! I'll start working on tests if this looks good. https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h File media/base/decryptor.h (right): https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h#new... media/base/decryptor.h:207: // Callback to notify decryptor creation. On 2012/12/11 05:13:34, ddorwin wrote: > On 2012/12/11 02:27:40, xhwang wrote: > > I moved this block here since it's duplicated in a lot of places. Also I feel > > the name is too long... How about SetDecryptorCB and RegisterSetDecryptorCB? > > I agree about the length (plus the lack of meaning). > Do we name CBs based on the function name or what they signal? Would > DecryptorCreatedCB work? Renamed. https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h#new... media/base/decryptor.h:209: // Callback to request/cancel decryptor creation notification. On 2012/12/11 05:13:34, ddorwin wrote: > "...notification of decryptor creation." Done. https://codereview.chromium.org/11492003/diff/5001/media/base/decryptor.h#new... media/base/decryptor.h:216: typedef base::Callback<void(const DecryptorNotificationCB&)> On 2012/12/12 00:30:25, ddorwin wrote: > On 2012/12/11 20:52:15, scherkus wrote: > > On 2012/12/11 19:43:04, xhwang wrote: > > > On 2012/12/11 05:13:34, ddorwin wrote: > > > > This is a callback for a callback? Seems weird. Maybe this is why naming > is > > so > > > > hard. See also comment about naming of the actual function in wmpi.cc. > Maybe > > > > fixing that will help. > > > > > > The original naming (proposed by fischman) actually looks good to me, except > > > it's too long. > > > > > > We don't care if the decryptor is created, or how the decryptor will be > used. > > > It's all about notifying the requester when a decryptor is available. That > > said, > > > I like the word "Register" too. How about DecryptorReadyCB and > > > RegisterDecryptorReadyCB? > > > > Well this is actually setting / unsetting an observer callback. In other > words: > > > > class DecryptorCreator { > > public void SetObserver(const DecryptorCB& cb) = 0; > > }; > > > > I kind of like the word "set" as it doesn't make you think you can register > > multiple callbacks. > > > > How about: > > DecryptorCreatedCB > > SetDecryptorCreatedCB > > > > > > ... alternatively perhaps it would be clearer w/o callbacks and using > something > > more old fashioned like observer_list.h and C++ interfaces > > Either of these SGTM. Combining scherkus's and xhwang's comments, we end up > with: > DecryptorReadyCB > SetDecryptorReadyCB > > I think we should avoid pulling in new interfaces to this CL. If we want to > refactor (other than naming), let's do it in a separate CL. Done. https://codereview.chromium.org/11492003/diff/5001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/11492003/diff/5001/media/base/pipeline.cc#new... media/base/pipeline.cc:898: request_decryptor_notification_cb_, On 2012/12/11 20:52:15, scherkus wrote: > why does this have to go in via Initialize() versus ctor? That's actually my first attempt. It didn't work well because in render_view_impl (http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_vi...), we create the audio_renderer_impl also. In RenderViewImpl we don't have the proxy_decryptor and the callback to pass in the ctor of AudioRendererImpl. After I went the current approach, I started to like it. The callback is only used in initialization of decoders and it makes sense to me to pass it in the Init() call. WDYT? https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... File media/filters/audio_decoder_factory.h (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:30: class MEDIA_EXPORT AudioDecoderFactory { On 2012/12/11 05:13:34, ddorwin wrote: > It's odd for a factory to have state. This factory uses multiple threads I > assume, so we'll need state somewhere. Does someone own the factory instance? > (Thinking out loud, I'll answer this question myself.) It's not factory anymore. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:37: // AudioDecoder initialization failed. On 2012/12/11 20:52:15, scherkus wrote: > nit: this style of indentation is a waste of valuable whitespace > > my suggestion is to simply add an empty line inbetween first and second > parameter descriptions Done. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:44: const scoped_refptr<DecryptingDemuxerStream>&)> InitDoneCB; On 2012/12/11 05:13:34, ddorwin wrote: > Should the callback be DecoderCreatedCB? Changed to DecoderSlectedCB. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:44: const scoped_refptr<DecryptingDemuxerStream>&)> InitDoneCB; On 2012/12/11 05:13:34, ddorwin wrote: > Should the callback be DecoderCreatedCB? Done. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:47: // available to perform decryption. On 2012/12/12 00:30:25, ddorwin wrote: > On 2012/12/11 19:43:04, xhwang wrote: > > On 2012/12/11 05:13:34, ddorwin wrote: > > > I don't understand? The factory will not support decrypted streams? Why > would > > a > > > callback parameter have control implications > > > > When --enable-encrypted-media is not set, we don't create the ProxyDecryptor > at > > all > > > (http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/media/webmediaplaye...). > > That's when this callback could be null. In that case, we want the factory to > > just initialize decoders from the precreated list. > > Why not use an explicit control (Boolean)? It doesn't matter too much since this > condition will go away soonish. Yep, will keep what I have here. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:49: const scoped_refptr<base::MessageLoopProxy>& message_loop, On 2012/12/12 00:30:25, ddorwin wrote: > Is there a method/pattern to who parameters are passed to the constructor or > Init? Is this designed such that the factory could be used to obtain multiple > decoders? In the current use, the class is constructed then Init is called, so > we could even have a simple constructor or eliminate Init. (I'm not saying we > should, just pointing out the usage.) Not sure, keep the code as is unless anybody objects again. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_decode... media/filters/audio_decoder_factory.h:54: void InitAudioDecoder(const scoped_refptr<DemuxerStream>& stream, On 2012/12/12 00:30:25, ddorwin wrote: > On 2012/12/11 19:43:04, xhwang wrote: > > On 2012/12/11 05:13:34, ddorwin wrote: > > > Is this Init or Create? > > > > Well, this is tricky. The accurate name should be > > "CreateIfNeededAndInitializeAudioDecoder". The complication comes from the > fact > > that some decoders are pre-created. So I just took the common part "Init" and > > skipped the tricky part "Create"... > > Select? Get? Obtain? > > On a related note, Factories create things so maybe this class is misnamed. Renamed to Select. Class name renamed also. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.cc:103: decrypting_demuxer_stream_->Reset(base::Bind(&base::DoNothing)); On 2012/12/11 20:52:15, scherkus wrote: > On 2012/12/11 05:13:34, ddorwin wrote: > > On 2012/12/11 02:27:40, xhwang wrote: > > > Similar to what we are doing in media pipeline, the decoder->Reset() doesn't > > > need to wait for demuxer_stream->Reset(). If we stick to this pattern, we > > could > > > probably remove the callback parameter in DecyrptingDemuxerStream::Reset(). > > > Otherwise, I can add another callback to make sure the two Reset() are > called > > in > > > sequence. > > > > I defer to scherkus. > > Why do we not need to wait? Is it because it's synchronous or something else? > > What are you referring to when you say "similar to what we are doing in media > pipeline"? Added another callback so we actually wait. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.cc:205: const scoped_refptr<AudioDecoder>& audio_decoder, On 2012/12/11 05:13:34, ddorwin wrote: > Would selected_decoder be a more helpful name? Done. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.cc:211: factory_.reset(); On 2012/12/12 00:30:25, ddorwin wrote: > On 2012/12/11 19:43:04, xhwang wrote: > > On 2012/12/11 05:13:34, ddorwin wrote: > > > Should we have the factory manage its own lifetime? See comment in ADF.h. > > > > After the audio decoder (and decrypting_demuxer_stream) is/are populated, the > > factory is not needed anymore. That's why it's destroyed here. I am not sure > how > > it could manage it's own lifetime... > > Init() AddRef()'s then errors or success DeRef() after calling the callback. > > That's a bit odd too the way the factory has a constructor. Really we don't need > a factory object - we just want this class to do something and notify us when > it's done. This happens to need state due to all the callbacks. Maybe we can > hide or avoid this. Possibilities, respectively: > * Factory::Get() creates a self-managing pimpl. > * Pass a state object to each function in the flow. Maybe as a scoped_ptr or > something so it is always deleted. > > WDYT? Pass the scoped_ptr in the callback to keep it alive. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... File media/filters/audio_renderer_impl.h (right): https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.h:158: scoped_ptr<AudioDecoderFactory> factory_; On 2012/12/11 19:43:04, xhwang wrote: > On 2012/12/11 05:13:34, ddorwin wrote: > > decoder_factory_ > > Will do. Done. https://codereview.chromium.org/11492003/diff/5001/media/filters/audio_render... media/filters/audio_renderer_impl.h:160: scoped_refptr<DecryptingDemuxerStream> decrypting_demuxer_stream_; On 2012/12/11 19:43:04, xhwang wrote: > On 2012/12/11 05:13:34, ddorwin wrote: > > Maybe explain when this is populated/valid. > > Will add comment. Done. https://codereview.chromium.org/11492003/diff/5001/webkit/media/webmediaplaye... File webkit/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/11492003/diff/5001/webkit/media/webmediaplaye... webkit/media/webmediaplayer_impl.cc:1131: base::Bind(&ProxyDecryptor::RequestDecryptorNotification, On 2012/12/11 05:13:34, ddorwin wrote: > ProxyDecryptor::RequestDecryptorNotification() doesn't seem to request > notification. It looks like it saves a notification request. Should it be > Register...? Done with Set...
(only replying to single question) Re: Initialize() versus ARI()... you should take a look at my pending https://codereview.chromium.org/11468033/ I accidentally rolled in a refactoring that moves ARI creation into WMPI (technically it's a completely separate change). I would much rather prefer we do that + pass into ctor then add new plumbing in Pipeline via Initialize(). Do you want me to land that change first or is it easier for you to incorporate those bits into this CL?
On 2012/12/13 03:05:43, scherkus wrote: > (only replying to single question) > > Re: Initialize() versus ARI()... you should take a look at my pending > https://codereview.chromium.org/11468033/ > > I accidentally rolled in a refactoring that moves ARI creation into WMPI > (technically it's a completely separate change). > > I would much rather prefer we do that + pass into ctor then add new plumbing in > Pipeline via Initialize(). > > > Do you want me to land that change first or is it easier for you to incorporate > those bits into this CL? Agreed with you on passing into ctor. Do you plan to land your CL any time soon? If yes, I'll wait. Otherwise, I can definitely merge those changes into my CL.
Thanks! LG. Just some minor stuff. https://codereview.chromium.org/11492003/diff/18001/media/base/pipeline.h File media/base/pipeline.h (right): https://codereview.chromium.org/11492003/diff/18001/media/base/pipeline.h#new... media/base/pipeline.h:131: // pipeline's buffering state changes. add comment for new param? https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... File media/filters/audio_decoder_selector.cc (right): https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Update CL description. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:5: #include "media/filters/audio_decoder_selector.h" Does this really belong in filters/? I know we have a lot of non-filter stuff in there already. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:38: DVLOG(2) << "SelectAudioDecoder()"; I always log above checks, though it probably doesn't matter much since the check prints the line number. Still nice to have a function name, though, I guess. Regardless, It shouldn't be between checks. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:41: select_decoder_cb_ = BindToCurrentLoop(select_decoder_cb); Maybe explain why. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:71: message_loop_, set_decryptor_ready_cb_); better indentation. Can you fit with one above? https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... File media/filters/audio_decoder_selector.h (right): https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... media/filters/audio_decoder_selector.h:39: // decryption for the initialized AudioDecoder. What should a caller do with the DDS? https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_rende... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_rende... media/filters/audio_renderer_impl.cc:107: void AudioRendererImpl::OnDecryptingDemuxerStreamReset( How about calling this DoReset() and just calling it at 104? OnDDSReset is a strange name. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_rende... media/filters/audio_renderer_impl.cc:206: // |decoder_selector| in the same line. I'm not sure what the issue you're trying to avoid is. Would a simple comment saying that ownership of the pointer is passed to the callback suffice without the local variable, which just adds another potentially dangling pointer? https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_rende... File media/filters/audio_renderer_impl.h (right): https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_rende... media/filters/audio_renderer_impl.h:147: // created to help decrypt the encrypted stream. is/was ... https://codereview.chromium.org/11492003/diff/18001/webkit/media/crypto/proxy... File webkit/media/crypto/proxy_decryptor.h (right): https://codereview.chromium.org/11492003/diff/18001/webkit/media/crypto/proxy... webkit/media/crypto/proxy_decryptor.h:49: void RegisterDecryptorReadyNotification( I thought we were using "Set".
SetDecryptorReadyCB renaming is moved to another CL to reduce the noise level of this CL. Also moved the SetDecryptorReadyCB to AudioRendererImpl's ctor instead of updating Pipeline->Start(). Most comments resolved. Unittests are working. I'll add new tests later... PTAL again! https://codereview.chromium.org/11492003/diff/18001/media/base/pipeline.h File media/base/pipeline.h (right): https://codereview.chromium.org/11492003/diff/18001/media/base/pipeline.h#new... media/base/pipeline.h:131: // pipeline's buffering state changes. On 2012/12/13 05:08:25, ddorwin wrote: > add comment for new param? Pipeline will not be used to pass the SetDecryptorReadyCB anymore... https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... File media/filters/audio_decoder_selector.cc (right): https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/12/13 05:08:25, ddorwin wrote: > Update CL description. Done. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:5: #include "media/filters/audio_decoder_selector.h" On 2012/12/13 05:08:25, ddorwin wrote: > Does this really belong in filters/? I know we have a lot of non-filter stuff in > there already. Well, it's affliated with AudioRendererImpl, which is in filters.. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:38: DVLOG(2) << "SelectAudioDecoder()"; On 2012/12/13 05:08:25, ddorwin wrote: > I always log above checks, though it probably doesn't matter much since the > check prints the line number. Still nice to have a function name, though, I > guess. > Regardless, It shouldn't be between checks. Done. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:41: select_decoder_cb_ = BindToCurrentLoop(select_decoder_cb); On 2012/12/13 05:08:25, ddorwin wrote: > Maybe explain why. Done. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:71: message_loop_, set_decryptor_ready_cb_); On 2012/12/13 05:08:25, ddorwin wrote: > better indentation. Can you fit with one above? Done. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... File media/filters/audio_decoder_selector.h (right): https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_decod... media/filters/audio_decoder_selector.h:39: // decryption for the initialized AudioDecoder. On 2012/12/13 05:08:25, ddorwin wrote: > What should a caller do with the DDS? Done. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_rende... File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_rende... media/filters/audio_renderer_impl.cc:107: void AudioRendererImpl::OnDecryptingDemuxerStreamReset( On 2012/12/13 05:08:25, ddorwin wrote: > How about calling this DoReset() and just calling it at 104? OnDDSReset is a > strange name. Done. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_rende... media/filters/audio_renderer_impl.cc:206: // |decoder_selector| in the same line. On 2012/12/13 05:08:25, ddorwin wrote: > I'm not sure what the issue you're trying to avoid is. > Would a simple comment saying that ownership of the pointer is passed to the > callback suffice without the local variable, which just adds another potentially > dangling pointer? The problem is that the evaluation order is undefined. Suppose base::Passed() is evaluated first, then decoder_selector will become a NULL scoped_ptr. decoder_selector->SelectAudioDecoder() will be evaluated later and will crash. Using a raw pointer here is weird, but it's not dangling as we are sure the point is valid when we call it. https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_rende... File media/filters/audio_renderer_impl.h (right): https://codereview.chromium.org/11492003/diff/18001/media/filters/audio_rende... media/filters/audio_renderer_impl.h:147: // created to help decrypt the encrypted stream. On 2012/12/13 05:08:25, ddorwin wrote: > is/was ... Done.
I'm a little confused about the sink changes. We should probably address those in a different CL (except not changing behavior in this CL). https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... File media/filters/audio_decoder_selector.h (right): https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.h:40: // Note: DecryptingDemuxerStream::Reset() should be called before Does the caller own the DDS? "The caller should call ... before...." https://codereview.chromium.org/11492003/diff/26003/webkit/media/webmediaplay... File webkit/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/11492003/diff/26003/webkit/media/webmediaplay... webkit/media/webmediaplayer_impl.cc:155: audio_renderer_sink_(audio_renderer_sink), Is there a reason for this member if we will pass ownership to AudioRendererImpl? Maybe we can eliminate the shared_ptr altogether in a subsequent CL. https://codereview.chromium.org/11492003/diff/26003/webkit/media/webmediaplay... webkit/media/webmediaplayer_impl.cc:196: base::Bind(&ProxyDecryptor::RegisterDecryptorReadyNotification, Name needs to be updated per other CL. https://codereview.chromium.org/11492003/diff/26003/webkit/media/webmediaplay... webkit/media/webmediaplayer_impl.cc:200: // Create default audio renderer using the null sink if no sink was provided. This is a change in behavior. Why? OR, how did this used to work?
https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... File media/filters/audio_decoder_selector.cc (right): https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:28: weak_this_(weak_ptr_factory_.GetWeakPtr()) { nit: if this is created on the same thread as SelectAudioDecoder() is called, you can drop the weak_this_ and just have the factory and hand out weakptrs when you base::Bind() https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:51: input_stream_ = stream; nit: your call, but I think we can be fancy here and bind stream + stats cb for each callback and keep passing it through instead of having member variables up to you if you find it's clearer / less stateful https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:76: BindToCurrentLoop(base::Bind( hmmm... we really shouldn't be so paranoid about binding everything AFAIK all our audio decoders run on the same thread. are you finding this isn't the case? https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... File media/filters/audio_decoder_selector.h (right): https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.h:47: // to perform decryption. This could happen if --enable-encrypted-media nit: don't talk about the switch, just say it's an optional parameter and that decryption will be unavailable if null https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.h:51: const AudioDecoderList& clear_decoders, no clue what "clear_decoders" means why not simply "decoders" ? https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.h:62: void InitializeNextClearDecoder(); I'm really not finding the usage of "Clear" to be a helpful term here https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.h:79: DISALLOW_COPY_AND_ASSIGN(AudioDecoderSelector); s/C_A_A/IMPLICIT_CONSTRUCTORS/ (since you don't define a default ctor) https://codereview.chromium.org/11492003/diff/26003/webkit/media/webmediaplay... File webkit/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/11492003/diff/26003/webkit/media/webmediaplay... webkit/media/webmediaplayer_impl.cc:155: audio_renderer_sink_(audio_renderer_sink), On 2012/12/13 23:09:43, ddorwin wrote: > Is there a reason for this member if we will pass ownership to > AudioRendererImpl? Maybe we can eliminate the shared_ptr altogether in a > subsequent CL. there is a lot of silliness w.r.t. audio_source_provider_ and audio_renderer_sink_, see https://code.google.com/p/chromium/issues/detail?id=136442
https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... File media/filters/audio_decoder_selector.cc (right): https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:28: weak_this_(weak_ptr_factory_.GetWeakPtr()) { On 2012/12/13 23:39:07, scherkus wrote: > nit: if this is created on the same thread as SelectAudioDecoder() is called, > you can drop the weak_this_ and just have the factory and hand out weakptrs when > you base::Bind() Done. https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:51: input_stream_ = stream; On 2012/12/13 23:39:07, scherkus wrote: > nit: your call, but I think we can be fancy here and bind stream + stats cb for > each callback and keep passing it through instead of having member variables > > up to you if you find it's clearer / less stateful hmm, I'd keep the current way.. Passing them around seems not as clean... https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:76: BindToCurrentLoop(base::Bind( On 2012/12/13 23:39:07, scherkus wrote: > hmmm... we really shouldn't be so paranoid about binding everything > > AFAIK all our audio decoders run on the same thread. are you finding this isn't > the case? Now FFmpegAudioDecoder and AudioDecoderImpl all run on the pipeline thread, correct? In that cast, FFmpegAudioDecoder can call the callback on the same execution stack. http://code.google.com/searchframe#OAMlx_jo-ck/src/media/filters/ffmpeg_audio... I want to prevent the reentrance caused by that. This is also related to the discussion of who should post the task, the caller or the callee? https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... File media/filters/audio_decoder_selector.h (right): https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.h:40: // Note: DecryptingDemuxerStream::Reset() should be called before On 2012/12/13 23:09:43, ddorwin wrote: > Does the caller own the DDS? > "The caller should call ... before...." Done. https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.h:47: // to perform decryption. This could happen if --enable-encrypted-media On 2012/12/13 23:39:07, scherkus wrote: > nit: don't talk about the switch, just say it's an optional parameter and that > decryption will be unavailable if null Done. https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.h:51: const AudioDecoderList& clear_decoders, On 2012/12/13 23:39:07, scherkus wrote: > no clue what "clear_decoders" means > > why not simply "decoders" ? Done. https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.h:62: void InitializeNextClearDecoder(); On 2012/12/13 23:39:07, scherkus wrote: > I'm really not finding the usage of "Clear" to be a helpful term here Done. https://codereview.chromium.org/11492003/diff/26003/media/filters/audio_decod... media/filters/audio_decoder_selector.h:79: DISALLOW_COPY_AND_ASSIGN(AudioDecoderSelector); On 2012/12/13 23:39:07, scherkus wrote: > s/C_A_A/IMPLICIT_CONSTRUCTORS/ (since you don't define a default ctor) Done. https://codereview.chromium.org/11492003/diff/26003/webkit/media/webmediaplay... File webkit/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/11492003/diff/26003/webkit/media/webmediaplay... webkit/media/webmediaplayer_impl.cc:196: base::Bind(&ProxyDecryptor::RegisterDecryptorReadyNotification, On 2012/12/13 23:09:43, ddorwin wrote: > Name needs to be updated per other CL. Done. https://codereview.chromium.org/11492003/diff/26003/webkit/media/webmediaplay... webkit/media/webmediaplayer_impl.cc:200: // Create default audio renderer using the null sink if no sink was provided. On 2012/12/13 23:09:43, ddorwin wrote: > This is a change in behavior. Why? OR, how did this used to work? Please see: https://codereview.chromium.org/11564007/
Added unittest, rebased, resolved comments, updated key_system.cc and updated content_browser test. PTAL!
Thanks. Just a few more things. https://codereview.chromium.org/11492003/diff/31012/content/browser/encrypted... File content/browser/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/11492003/diff/31012/content/browser/encrypted... content/browser/encrypted_media_browsertest.cc:133: // TODO(shadi): Make these three IN_PROC_BROWSER_TEST_P() when internal Clear Remove https://codereview.chromium.org/11492003/diff/31012/media/filters/audio_decod... File media/filters/audio_decoder_selector.h (right): https://codereview.chromium.org/11492003/diff/31012/media/filters/audio_decod... media/filters/audio_decoder_selector.h:76: base::WeakPtrFactory<AudioDecoderSelector> weak_this_; shouldn't this be named ...factory_? The type is not obvious when reading the .cc file. Is this common? https://codereview.chromium.org/11492003/diff/31012/media/filters/audio_decod... File media/filters/audio_decoder_selector_unittest.cc (right): https://codereview.chromium.org/11492003/diff/31012/media/filters/audio_decod... media/filters/audio_decoder_selector_unittest.cc:163: // There is a decryptor but the stream is not encrypted. The decoder will be Why does the decryptor get initialized (line 77) in this case? No DAD should be created if the stream is not encrypted, right? https://codereview.chromium.org/11492003/diff/31012/webkit/media/crypto/key_s... File webkit/media/crypto/key_systems.cc (right): https://codereview.chromium.org/11492003/diff/31012/webkit/media/crypto/key_s... webkit/media/crypto/key_systems.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. There is a unit tests for this. You need to update the clear key expectations audio/* and video/* with an audio codec.
https://codereview.chromium.org/11492003/diff/31012/content/browser/encrypted... File content/browser/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/11492003/diff/31012/content/browser/encrypted... content/browser/encrypted_media_browsertest.cc:133: // TODO(shadi): Make these three IN_PROC_BROWSER_TEST_P() when internal Clear On 2012/12/14 05:17:01, ddorwin wrote: > Remove Done. https://codereview.chromium.org/11492003/diff/31012/media/filters/audio_decod... File media/filters/audio_decoder_selector.h (right): https://codereview.chromium.org/11492003/diff/31012/media/filters/audio_decod... media/filters/audio_decoder_selector.h:76: base::WeakPtrFactory<AudioDecoderSelector> weak_this_; On 2012/12/14 05:17:01, ddorwin wrote: > shouldn't this be named ...factory_? The type is not obvious when reading the > .cc file. Is this common? I was following http://code.google.com/searchframe#OAMlx_jo-ck/src/media/audio/audio_output_c... Searched again. That's not the common use. Change it back. Thanks. https://codereview.chromium.org/11492003/diff/31012/media/filters/audio_decod... File media/filters/audio_decoder_selector_unittest.cc (right): https://codereview.chromium.org/11492003/diff/31012/media/filters/audio_decod... media/filters/audio_decoder_selector_unittest.cc:163: // There is a decryptor but the stream is not encrypted. The decoder will be On 2012/12/14 05:17:01, ddorwin wrote: > Why does the decryptor get initialized (line 77) in this case? No DAD should be > created if the stream is not encrypted, right? line 77 has WillRepeatedly(), so we allow the function to be called or not called. In this case, it's not called. I didn't enforce to check if decryptor->InitializeAudioDecoder() is called or not since that's implementation detail. For the same reason, in this test, DAD is not created, but I didn't check. https://codereview.chromium.org/11492003/diff/31012/webkit/media/crypto/key_s... File webkit/media/crypto/key_systems.cc (right): https://codereview.chromium.org/11492003/diff/31012/webkit/media/crypto/key_s... webkit/media/crypto/key_systems.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/12/14 05:17:01, ddorwin wrote: > There is a unit tests for this. You need to update the clear key expectations > audio/* and video/* with an audio codec. Done.
lgtm
lgtm w/ nits https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_decod... File media/filters/audio_decoder_selector.cc (right): https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:40: // Make sure the |selected_decoder_cb| on a different execution stack. s/selected/select/ I'd probably reword this to say "Make sure |select_decoder_cb| runs on a ..." https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_decod... File media/filters/audio_decoder_selector.h (right): https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_decod... media/filters/audio_decoder_selector.h:55: void SelectAudioDecoder(const scoped_refptr<DemuxerStream>& stream, needs docs https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_rende... File media/filters/audio_renderer_impl.h (right): https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_rende... media/filters/audio_renderer_impl.h:48: const SetDecryptorReadyCB& set_decryptor_ready_cb); it's a bit unfortunate to have to pass this in only to create an AudioDecoderSelector perhaps one day instead of populating a FilterCollection + passing in an AudioDecoderList to Initialize(), WMPI et al can populate AudioDecoderSelectors and pass _that_ in to Initialize() nothing to change now, but something to think about...
https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_decod... File media/filters/audio_decoder_selector.cc (right): https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_decod... media/filters/audio_decoder_selector.cc:40: // Make sure the |selected_decoder_cb| on a different execution stack. On 2012/12/14 06:39:12, scherkus wrote: > s/selected/select/ > > I'd probably reword this to say "Make sure |select_decoder_cb| runs on a ..." Done. https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_decod... File media/filters/audio_decoder_selector.h (right): https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_decod... media/filters/audio_decoder_selector.h:55: void SelectAudioDecoder(const scoped_refptr<DemuxerStream>& stream, On 2012/12/14 06:39:12, scherkus wrote: > needs docs Done. https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_rende... File media/filters/audio_renderer_impl.h (right): https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_rende... media/filters/audio_renderer_impl.h:48: const SetDecryptorReadyCB& set_decryptor_ready_cb); On 2012/12/14 06:39:12, scherkus wrote: > it's a bit unfortunate to have to pass this in only to create an > AudioDecoderSelector > > perhaps one day instead of populating a FilterCollection + passing in an > AudioDecoderList to Initialize(), WMPI et al can populate AudioDecoderSelectors > and pass _that_ in to Initialize() > > nothing to change now, but something to think about... I actually like this idea ;)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11492003/32011
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11492003/32011
Sorry for I got bad news for ya. Compile failed with a clobber build on win_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11492003/32011
https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_rende... File media/filters/audio_renderer_impl.h (right): https://codereview.chromium.org/11492003/diff/37030/media/filters/audio_rende... media/filters/audio_renderer_impl.h:48: const SetDecryptorReadyCB& set_decryptor_ready_cb); On 2012/12/14 07:45:06, xhwang wrote: > On 2012/12/14 06:39:12, scherkus wrote: > > it's a bit unfortunate to have to pass this in only to create an > > AudioDecoderSelector > > > > perhaps one day instead of populating a FilterCollection + passing in an > > AudioDecoderList to Initialize(), WMPI et al can populate > AudioDecoderSelectors > > and pass _that_ in to Initialize() > > > > nothing to change now, but something to think about... > > I actually like this idea ;) ...actually I thought about this a bit more. If we never have more than a single AudioRenderer/VideoRenderer implementation... couldn't we construct and pass in the list of decoders + cbs into the constructor instead of Initialize()? We could rip out more code from Pipeline... Hmm.....
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11492003/32011 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
