Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10969028:
Add video decoding methods in Decryptor and add DecryptingVideoDecoder. (Closed)
|
Created:
8 years, 3 months ago by xhwang Modified:
8 years, 2 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd video decoding methods in Decryptor and add DecryptingVideoDecoder.
In Decryptor interface, video decoding methods are added to support CDMs that support both decryption and video decoding.
A new VideoDecoder, DecryptingVideoDecoder, is added to support video decoding by Decryptors in the media pipeline. DecryptingVideoDecoder uses adapter pattern to convert a Decryptor into a VideoDecoder.
In AddDefaultDecodersToCollection, add a DecryptingVideoDecoder object into the video decoder list.
BUG=141784
TEST=new tests added into media_unittest
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160183
Patch Set 1 #
Total comments: 52
Patch Set 2 : Added unitttests and resolved comments. #
Total comments: 6
Patch Set 3 : Move DVD to filters/ and rebase. #
Total comments: 81
Patch Set 4 : resolve comments #
Total comments: 33
Patch Set 5 : resolve ddorwin's comments and have a question about force posting task! #
Total comments: 41
Patch Set 6 : resolve comments and fix stop-during-pending-init. #
Total comments: 4
Patch Set 7 : nits and rebase #Patch Set 8 : reorder methods in the unittest #
Messages
Total messages: 29 (0 generated)
Hello, First prototyle CL for decrypt-and-decode (D&D). It only compiles so don't bother to review details. I want to send this out early to ensure I am not in the wrong direction. DecryptingVideoDecoder(DVD) mostly follows what FFmpegVideoDecoder(FFVD) is doing. It isolates threading and DemuxerStream handling from the Decryptor, so that the Decryptor only need to worry about processing input buffers and handle out output buffers/frames (async'ly). For now DVD only handles D&D. Decrypt-only is still done in FFVD. After this is working, I'll start to move decrypt-only from FFVD to DVD. PTAL and let me know if I should go ahead or I am on the wrong way! http://codereview.chromium.org/10969028/diff/1/webkit/media/filter_helpers.cc File webkit/media/filter_helpers.cc (right): http://codereview.chromium.org/10969028/diff/1/webkit/media/filter_helpers.cc... webkit/media/filter_helpers.cc:46: filter_collection->GetVideoDecoders()->push_back(ffmpeg_video_decoder); After this change, we have 3 VideoDecoders in the list (in the following order): GVD: only accepts unencrypted stream DVD: only accepts encrypted stream (decrypt-and-decode) FFVD: accepts both unencrypted and encrypted stream (decrypt-only). In later CLs decrypt-only will be moved from FFVD to DVD.
Seems fine overall. See my last comment about documenting the overall design somewhere. I did not review decrypting_video_decoder.cc since you said this was a prototype. http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode25 media/base/decryptor.h:25: // and nonblocking (key events should be fired asynchronously). Parenthesis implies this is an interpretation of the previous statement. I think it is in addition (the actions must be async too), so maybe it should be after a ';'. http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode30 media/base/decryptor.h:30: // Note that the all callbacks can be fired synchronously or asynchronously. Why? This can lead to confusion. http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode104 media/base/decryptor.h:104: typedef base::Callback<void(bool)> InitializeCB; I think "Initialize" is too generic here. InitializeDecoderCB is long but much more meaningful. http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode109 media/base/decryptor.h:109: // should only be called after InitializeVideoDecoder() succeeded. Hopefully we have a DCHECK for this too. http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode118 media/base/decryptor.h:118: // contains the decoded video frame or indicates the end of the stream. How does it indicate the end of the stream? If it's a function on VideFrame, maybe we can imply that more directly than "indicates". http://codereview.chromium.org/10969028/diff/1/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10969028/diff/1/media/crypto/aes_decryptor.cc#... media/crypto/aes_decryptor.cc:239: init_cb.Run(false); Why not NOTREACHED();? If this is used to determine whether this decryptor supports D&D, that might be worth a comment. http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... File media/crypto/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.cc:82: reset_cb_ = closure; What happens if Reset() is called twice before the first call executes? http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... File media/crypto/decrypting_video_decoder.h (right): http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:12: #include "media/base/video_decoder.h" And decoder_buffer.h to IWYU. http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:23: // Decryptor powered VideoDecoder implementation that can decrypt and decode Decryptor powered ==> Decryptor-based? Decryptor-backed? http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:24: // encrypted buffers and return unencrypted and uncompressed video frames. s/unencrypted and uncompressed/decrypted decompressed/ http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:54: void OnDecoderInitDone(bool success); Would OnDecoderInitialized not be accurate? http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:57: // Reads from the demuxer stream with corresponding callback method. I don't understand "corresponding callback method" http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:59: void DecryptAndDecodeBuffer(DemuxerStream::Status status, Since there is no white space, I would assume 57 also applies to this, but I don't think it does. So, add a comment or white space. Why are these grouped bug DeliverFrame is not? http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:62: // DecryptOrDecodeBuffer(). DecryptAndDecodeBuffer http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:66: void DeliverFrame(int buffer_size, Should this be a Do function as well since it is presumably called by another callback? http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:91: Decryptor* decryptor_; *const http://codereview.chromium.org/10969028/diff/1/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/10969028/diff/1/media/media.gyp#newcode236 media/media.gyp:236: 'crypto/decrypting_video_decoder.cc', I don't think this file actually depends on crypto/. It seems it should be in base. http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/ppapi_decr... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:16: #include "media/base/video_decoder_config.h" These don't seem necessary. http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:108: NOTIMPLEMENTED(); TODO(xhwang): Impelement.? It seems these are intended to be whereas the AesDecryptor ones are not. http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/proxy_decr... File webkit/media/crypto/proxy_decryptor.cc (right): http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/proxy_decr... webkit/media/crypto/proxy_decryptor.cc:15: #include "media/base/video_decoder_config.h" same http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/proxy_decr... webkit/media/crypto/proxy_decryptor.cc:193: NOTIMPLEMENTED(); Will be implemented or not? http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/proxy_decr... File webkit/media/crypto/proxy_decryptor.h (right): http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/proxy_decr... webkit/media/crypto/proxy_decryptor.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Does this class eventually go away? http://codereview.chromium.org/10969028/diff/1/webkit/media/filter_helpers.cc File webkit/media/filter_helpers.cc (right): http://codereview.chromium.org/10969028/diff/1/webkit/media/filter_helpers.cc... webkit/media/filter_helpers.cc:45: filter_collection->GetVideoDecoders()->push_back(decrypting_video_decoder); Does this order mean we will never decrypt with HW decoding enabled? I guess there isn't much we can do until you address your comment below since the GVD is provided outside this class http://codereview.chromium.org/10969028/diff/1/webkit/media/filter_helpers.cc... webkit/media/filter_helpers.cc:46: filter_collection->GetVideoDecoders()->push_back(ffmpeg_video_decoder); On 2012/09/20 23:26:34, xhwang wrote: > After this change, we have 3 VideoDecoders in the list (in the following order): > GVD: only accepts unencrypted stream > DVD: only accepts encrypted stream (decrypt-and-decode) > FFVD: accepts both unencrypted and encrypted stream (decrypt-only). > > In later CLs decrypt-only will be moved from FFVD to DVD. It's not obvious here how the correct one will be selected, especially wrt is_encrypted(). Probably worth an overall design comment somewhere.
On Thu, Sep 20, 2012 at 4:26 PM, <xhwang@chromium.org> wrote: > In Decryptor interface, video decoding methods are added to support CDMs > that > support both decryption and video decoding. > Why duplicate the VideoDecoder API inside of Decryptor instead of adding a single method to Decryptor that returns a VideoDecoder implementation (which, for D&D, would be an implementation of VD entirely inside the Decryptor impl)? IOW scoped_refptr<VideoDecoder> Decryptor::video_decoder() { NOTREACHED() << "Oops"; return NULL; } ?
On 2012/09/21 05:53:08, Ami Fischman wrote:
> On Thu, Sep 20, 2012 at 4:26 PM, <mailto:xhwang@chromium.org> wrote:
>
> > In Decryptor interface, video decoding methods are added to support CDMs
> > that
> > support both decryption and video decoding.
> >
>
> Why duplicate the VideoDecoder API inside of Decryptor instead of adding a
> single method to Decryptor that returns a VideoDecoder implementation
> (which, for D&D, would be an implementation of VD entirely inside the
> Decryptor impl)?
> IOW
> scoped_refptr<VideoDecoder> Decryptor::video_decoder() { NOTREACHED() <<
> "Oops"; return NULL; }
> ?
That's a fair question. The short answer is that I want to keep the Decryptor
simple.
The long answer:
If we do this, a Decryptor implementation that supports video decoding will need
to implement all logic needed for a functioning VideoDecoder, including:
1, Thread trampolining, due to how VideoDecoder interacts with VideoRendererBase
and DemuxerStream.
2, Dealing with DemuxerStream::Read(), including a) call DS::Read() again when
more data is needed, and b) handle config change.
3, Handle Reset() and Stop() when read_cb is in the flight.
Then when we add audio decrypt/decode, the Decryptor will need to implement a
full AudioDecoder, which will have similar logic as above.
Given that the decryptor also needs to implement key operations and the
Decrypt() call. It seems to me that the Decryptor is doing too much.
This also has some potential issues in the future:
- If we add more Decryptor implementations, we will essentially creating more
VideoDecoder and AudioDecoder implementations. We'll end up duplicating all the
logic in 1, 2 and 3 above. (These logics are already more or less duplicated in
FFVD and DVD).
- If we move to the EME v2 (Object-oriented design), the Decryptor will be
created by a MediaKey element, which will be attached to a Media element. It's
not clear to me how things will work then, but I'd like to keep the Decryptor
simple and keep the VideoDecoder logic close to media pipeline.
By adding those video decoder APIs in the Decryptor and having the DVD wrapping
a Decryptor, we put all logics in 1, 2 and 3 in a common place, which can be
shared by all Decryptor implementations.
WDYT?
On 2012/09/21 07:50:41, xhwang wrote:
> On 2012/09/21 05:53:08, Ami Fischman wrote:
> > On Thu, Sep 20, 2012 at 4:26 PM, <mailto:xhwang@chromium.org> wrote:
> >
> > > In Decryptor interface, video decoding methods are added to support CDMs
> > > that
> > > support both decryption and video decoding.
> > >
> >
> > Why duplicate the VideoDecoder API inside of Decryptor instead of adding a
> > single method to Decryptor that returns a VideoDecoder implementation
> > (which, for D&D, would be an implementation of VD entirely inside the
> > Decryptor impl)?
> > IOW
> > scoped_refptr<VideoDecoder> Decryptor::video_decoder() { NOTREACHED() <<
> > "Oops"; return NULL; }
> > ?
>
> That's a fair question. The short answer is that I want to keep the Decryptor
> simple.
>
> The long answer:
> If we do this, a Decryptor implementation that supports video decoding will
need
> to implement all logic needed for a functioning VideoDecoder, including:
> 1, Thread trampolining, due to how VideoDecoder interacts with
VideoRendererBase
> and DemuxerStream.
> 2, Dealing with DemuxerStream::Read(), including a) call DS::Read() again when
> more data is needed, and b) handle config change.
> 3, Handle Reset() and Stop() when read_cb is in the flight.
>
> Then when we add audio decrypt/decode, the Decryptor will need to implement a
> full AudioDecoder, which will have similar logic as above.
>
> Given that the decryptor also needs to implement key operations and the
> Decrypt() call. It seems to me that the Decryptor is doing too much.
>
> This also has some potential issues in the future:
> - If we add more Decryptor implementations, we will essentially creating more
> VideoDecoder and AudioDecoder implementations. We'll end up duplicating all
the
> logic in 1, 2 and 3 above. (These logics are already more or less duplicated
in
> FFVD and DVD).
> - If we move to the EME v2 (Object-oriented design), the Decryptor will be
> created by a MediaKey element, which will be attached to a Media element. It's
> not clear to me how things will work then, but I'd like to keep the Decryptor
> simple and keep the VideoDecoder logic close to media pipeline.
>
> By adding those video decoder APIs in the Decryptor and having the DVD
wrapping
> a Decryptor, we put all logics in 1, 2 and 3 in a common place, which can be
> shared by all Decryptor implementations.
>
> WDYT?
To clarify (ddorwin@ felt the above reply was not very clear; sorry for my 1 am
snooziness), if we do what fischman@ proposed, we'll still need to implement the
VideoDecoder (and AudioDecoder later) somewhere. So other than having less APIs,
we won't save any code. We'll probably need to implement the VideoDecoder in
PpapiDecryptor (or some friend class of it) so that it can access the
PluginInstance for video decoding calls. I feel bad if we have to do thread
hopping etc (1,2,3) in PpapiDecryptor.
In other words, by introducing these new APIs in the Decryptor, we actually
created a firewall to keep VideoDecoder specific code (1,2,3) out of Decryptor
implementations.
> > So other than having less APIs, we won't save any code. > I wasn't trying to save code; I'm trying to narrow the API choices involved. In other words, by introducing these new APIs in the Decryptor, we actually > created a firewall to keep VideoDecoder specific code (1,2,3) out of > Decryptor > implementations. > On the contrary, you're forcing every Decryptor implementation to implement your new methods, even when they don't support D&D. Given that there is likely going to be only one Decryptor supporting D&D (PpapiDecryptor) I don't see why you wouldn't rather put the 1,2,3 logic in there instead of in the DVD (where the 1,2,3 code will be dead for all other decryptors being wrapped)
On 2012/09/21 17:40:47, Ami Fischman wrote: > > > > So other than having less APIs, we won't save any code. > > > > I wasn't trying to save code; I'm trying to narrow the API choices involved. > > In other words, by introducing these new APIs in the Decryptor, we actually > > created a firewall to keep VideoDecoder specific code (1,2,3) out of > > Decryptor > > implementations. > > > > On the contrary, you're forcing every Decryptor implementation to implement > your new methods, even when they don't support D&D. That's true. But it's not a lot of work. They are pretty much done in this CL. What you proposed also requires us to implement two VideoDecoders, one in ProxyDecryptor and one in PpapiDecryptor. Or ProxyDecryptor::video_decoder() needs to be able to return the video_decoder() async'ly. > Given that there is likely going to be only one Decryptor supporting D&D > (PpapiDecryptor) I don't see why you wouldn't rather put the 1,2,3 logic in > there instead of in the DVD (where the 1,2,3 code will be dead for all > other decryptors being wrapped) Putting 1,2,3 logic in the PpapiDecryptor is doable, but I don't like it. As I said, it'll put too much code in PpapiDecryptor. Given the same amount of code, why don't we distribute them nicely (in clear layers) rather than putting everything in one class?
I think we just have different instincts about how this will turn out. Since you're the one doing the work here I'm going to back off and let you do it the way you want and then offer concrete suggestions/CLs later if I don't like how it turns out.
I do agree w/ fischman@ it looks a bit odd to embed a decoding API into decryptor but if you feel like this is the right way to go then that's OK with me. I do have a question... what about when it comes time to do Decrypt+Decode for audio? Is that on the horizon? http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode30 media/base/decryptor.h:30: // Note that the all callbacks can be fired synchronously or asynchronously. On 2012/09/21 00:36:08, ddorwin wrote: > Why? This can lead to confusion. Would be really, really nice to mandate one or the other.
On 2012/09/21 18:57:56, scherkus wrote: > I do agree w/ fischman@ it looks a bit odd to embed a decoding API into > decryptor but if you feel like this is the right way to go then that's OK with > me. Thanks for the understanding. I thought about this again over the weekend. One thing worth mentioning is that this new API is not exactly a copy of the VideoDecoder API. One difference is that the decoder can return kNoKey, which means there's no decryption key available for the current buffer. The caller can choose to error out or to hold the callback and wait. Currently the pause-and-wait-for-key-to-be-added logic is implemented in the ProxyDecryptor (to avoid duplicate code in AesDecryptor and PpapiDecryptor). If we use VideoDecoder interface on Decryptors directly, we need to find a new way to pass and handle kNoKey, which seems hairy to me. > > I do have a question... what about when it comes time to do Decrypt+Decode for > audio? Is that on the horizon? When it comes time to support Audio, my plan is to add another 4 APIs in the Decryptor interface :P I know it sounds not ideal, but based on all reasons I mentioned in this discussion, I don't have a better solution so far. > > http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h > File media/base/decryptor.h (right): > > http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode30 > media/base/decryptor.h:30: // Note that the all callbacks can be fired > synchronously or asynchronously. > On 2012/09/21 00:36:08, ddorwin wrote: > > Why? This can lead to confusion. > > Would be really, really nice to mandate one or the other.
http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode30 media/base/decryptor.h:30: // Note that the all callbacks can be fired synchronously or asynchronously. On 2012/09/21 18:57:56, scherkus wrote: > On 2012/09/21 00:36:08, ddorwin wrote: > > Why? This can lead to confusion. > > Would be really, really nice to mandate one or the other. Well, this isn't new. I just moved it from the old code (line 71) here. The reason for this is that a) decryptors don't have their own threads, and b) we need both cases: in AesDecryptor, we call those callbacks synchronously (and since (a), we can't make it asyn now). In PpapiDecryptor, callbacks are fired on the renderer's thread asynchronously. By specifying this clearly here, the caller must ensure that it supports both (pretty much by force posting the callbacks).
Added unit tests and resolved ddorwin's preliminary comments. Please do a full review now. Thanks! http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode25 media/base/decryptor.h:25: // and nonblocking (key events should be fired asynchronously). On 2012/09/21 00:36:08, ddorwin wrote: > Parenthesis implies this is an interpretation of the previous statement. I think > it is in addition (the actions must be async too), so maybe it should be after a > ';'. Done. http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode104 media/base/decryptor.h:104: typedef base::Callback<void(bool)> InitializeCB; On 2012/09/21 00:36:08, ddorwin wrote: > I think "Initialize" is too generic here. InitializeDecoderCB is long but much > more meaningful. Renamed to DecoderInitCB. http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode109 media/base/decryptor.h:109: // should only be called after InitializeVideoDecoder() succeeded. On 2012/09/21 00:36:08, ddorwin wrote: > Hopefully we have a DCHECK for this too. Will do in implementations in the next CL. http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode118 media/base/decryptor.h:118: // contains the decoded video frame or indicates the end of the stream. On 2012/09/21 00:36:08, ddorwin wrote: > How does it indicate the end of the stream? If it's a function on VideFrame, > maybe we can imply that more directly than "indicates". Done. http://codereview.chromium.org/10969028/diff/1/media/crypto/aes_decryptor.cc File media/crypto/aes_decryptor.cc (right): http://codereview.chromium.org/10969028/diff/1/media/crypto/aes_decryptor.cc#... media/crypto/aes_decryptor.cc:239: init_cb.Run(false); On 2012/09/21 00:36:08, ddorwin wrote: > Why not NOTREACHED();? > If this is used to determine whether this decryptor supports D&D, that might be > worth a comment. Done. http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... File media/crypto/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.cc:82: reset_cb_ = closure; On 2012/09/21 00:36:08, ddorwin wrote: > What happens if Reset() is called twice before the first call executes? Good call. I think VideoDecoder interface should disallow this (updated). Add a DCHECK here and in FFVD. http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... File media/crypto/decrypting_video_decoder.h (right): http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:12: #include "media/base/video_decoder.h" On 2012/09/21 00:36:08, ddorwin wrote: > And decoder_buffer.h to IWYU. DecoderBuffer is forward declared on line 20. The header is included in decrypting_video_decoder.cc. http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:23: // Decryptor powered VideoDecoder implementation that can decrypt and decode On 2012/09/21 00:36:08, ddorwin wrote: > Decryptor powered ==> > Decryptor-based? Decryptor-backed? Done. http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:24: // encrypted buffers and return unencrypted and uncompressed video frames. On 2012/09/21 00:36:08, ddorwin wrote: > s/unencrypted and uncompressed/decrypted decompressed/ Done. http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:54: void OnDecoderInitDone(bool success); On 2012/09/21 00:36:08, ddorwin wrote: > Would OnDecoderInitialized not be accurate? Done. http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:57: // Reads from the demuxer stream with corresponding callback method. On 2012/09/21 00:36:08, ddorwin wrote: > I don't understand "corresponding callback method" "corresponding callback method" is whatever callback the DVD passed into DemuxerStream::Read(). I agree it's too much implementation detail and doesn't need to be here. Remove the comment since the function name is pretty self-explanatory. http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:59: void DecryptAndDecodeBuffer(DemuxerStream::Status status, On 2012/09/21 00:36:08, ddorwin wrote: > Since there is no white space, I would assume 57 also applies to this, but I > don't think it does. So, add a comment or white space. Why are these grouped bug > DeliverFrame is not? Done. http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:62: // DecryptOrDecodeBuffer(). On 2012/09/21 00:36:08, ddorwin wrote: > DecryptAndDecodeBuffer Done. http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:66: void DeliverFrame(int buffer_size, On 2012/09/21 00:36:08, ddorwin wrote: > Should this be a Do function as well since it is presumably called by another > callback? I think we use "Do" for posted tasks, not on normal callbacks. http://codereview.chromium.org/10969028/diff/1/media/crypto/decrypting_video_... media/crypto/decrypting_video_decoder.h:91: Decryptor* decryptor_; On 2012/09/21 00:36:08, ddorwin wrote: > *const Done. http://codereview.chromium.org/10969028/diff/1/media/media.gyp File media/media.gyp (right): http://codereview.chromium.org/10969028/diff/1/media/media.gyp#newcode236 media/media.gyp:236: 'crypto/decrypting_video_decoder.cc', On 2012/09/21 00:36:08, ddorwin wrote: > I don't think this file actually depends on crypto/. It seems it should be in > base. It makes sense to be in media/base since it only depends on base. But at the same time it's a concrete implementation so I am not sure. +scherkus/fischman: comments? http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/ppapi_decr... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:16: #include "media/base/video_decoder_config.h" On 2012/09/21 00:36:08, ddorwin wrote: > These don't seem necessary. Surprise! After I removed these two everything still build. However, VideoDecoderConfig and VideoFrame are all forward declared in header files. Everything still build because VideoDecoderConfig and VideoFrame are declared indirectly through "webkit/plugins/ppapi/ppapi_plugin_instance.h". By IWYU, I think we should include them explicitly. http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:108: NOTIMPLEMENTED(); On 2012/09/21 00:36:08, ddorwin wrote: > TODO(xhwang): Impelement.? > It seems these are intended to be whereas the AesDecryptor ones are not. Added comments. http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/proxy_decr... File webkit/media/crypto/proxy_decryptor.cc (right): http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/proxy_decr... webkit/media/crypto/proxy_decryptor.cc:15: #include "media/base/video_decoder_config.h" On 2012/09/21 00:36:08, ddorwin wrote: > same same http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/proxy_decr... webkit/media/crypto/proxy_decryptor.cc:193: NOTIMPLEMENTED(); On 2012/09/21 00:36:08, ddorwin wrote: > Will be implemented or not? Done. http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/proxy_decr... File webkit/media/crypto/proxy_decryptor.h (right): http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/proxy_decr... webkit/media/crypto/proxy_decryptor.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/09/21 00:36:08, ddorwin wrote: > Does this class eventually go away? Not. This class is handling the kNoKey logic which is shared by all other Decryptor implementations. http://codereview.chromium.org/10969028/diff/1/webkit/media/filter_helpers.cc File webkit/media/filter_helpers.cc (right): http://codereview.chromium.org/10969028/diff/1/webkit/media/filter_helpers.cc... webkit/media/filter_helpers.cc:45: filter_collection->GetVideoDecoders()->push_back(decrypting_video_decoder); On 2012/09/21 00:36:08, ddorwin wrote: > Does this order mean we will never decrypt with HW decoding enabled? I guess > there isn't much we can do until you address your comment below since the GVD is > provided outside this class Not in this CL. Will enable encrypt-only + GVD/FFVD later. http://codereview.chromium.org/10969028/diff/1/webkit/media/filter_helpers.cc... webkit/media/filter_helpers.cc:46: filter_collection->GetVideoDecoders()->push_back(ffmpeg_video_decoder); On 2012/09/21 00:36:08, ddorwin wrote: > On 2012/09/20 23:26:34, xhwang wrote: > > After this change, we have 3 VideoDecoders in the list (in the following > order): > > GVD: only accepts unencrypted stream > > DVD: only accepts encrypted stream (decrypt-and-decode) > > FFVD: accepts both unencrypted and encrypted stream (decrypt-only). > > > > In later CLs decrypt-only will be moved from FFVD to DVD. > > It's not obvious here how the correct one will be selected, especially wrt > is_encrypted(). Probably worth an overall design comment somewhere. Added more comments for AddDecaultDecodersToCollection() to clarify this. Not sure it's the best place for these comments though.
Note that I'll rebase this CL to ToT and merge with http://codereview.chromium.org/10990039/ later.
I need to review dvd.cc and unittests, but I'll wait until we decide on the location. @scherkus @fischman: should decrypting_video_decoder files be in base or crypto/, base/, or filters/? http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode109 media/base/decryptor.h:109: // should only be called after InitializeVideoDecoder() succeeded. On 2012/09/25 23:52:32, xhwang wrote: > On 2012/09/21 00:36:08, ddorwin wrote: > > Hopefully we have a DCHECK for this too. > > Will do in implementations in the next CL. Add a TODO? http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/ppapi_decr... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:16: #include "media/base/video_decoder_config.h" On 2012/09/25 23:52:32, xhwang wrote: > On 2012/09/21 00:36:08, ddorwin wrote: > > These don't seem necessary. > > Surprise! After I removed these two everything still build. > > However, VideoDecoderConfig and VideoFrame are all forward declared in header > files. Everything still build because VideoDecoderConfig and VideoFrame are > declared indirectly through "webkit/plugins/ppapi/ppapi_plugin_instance.h". > > By IWYU, I think we should include them explicitly. We use VideoDecodeCB and DecoderInitCB, but not VideoDecoderConfig (just a const ref). So, we don't need definitions for everything. I'd have to look at where those two CB's are defined, though, to see what we need to include. http://codereview.chromium.org/10969028/diff/14001/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/14001/media/base/decryptor.h#new... media/base/decryptor.h:118: // 1) NULL, which means the operation has been aborted. Abort is success? http://codereview.chromium.org/10969028/diff/14001/media/base/decryptor.h#new... media/base/decryptor.h:143: virtual void CancelDecryptAndDecodeVideo() = 0; Is there a reason this shouldn't just be Cancel(), which would report kError and NULL data for all types of outstanding requests? Seems odd to have a D&D-specific function for this. http://codereview.chromium.org/10969028/diff/14001/media/crypto/decrypting_vi... File media/crypto/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/14001/media/crypto/decrypting_vi... media/crypto/decrypting_video_decoder.cc:51: // DecryptingVideoDecoder only accepts potentially encrypted stream. For now. Once we start requiring the decision beforehand, we'll need to remove this. http://codereview.chromium.org/10969028/diff/14001/media/crypto/decrypting_vi... media/crypto/decrypting_video_decoder.cc:239: if (status == Decryptor::kNoKey || status == Decryptor::kError) { Why is NoKey a decode (or decrypt) error? Don't we just report a needkey event somewhere? http://codereview.chromium.org/10969028/diff/14001/webkit/media/crypto/proxy_... File webkit/media/crypto/proxy_decryptor.cc (right): http://codereview.chromium.org/10969028/diff/14001/webkit/media/crypto/proxy_... webkit/media/crypto/proxy_decryptor.cc:16: #include "media/base/video_frame.h" What is used from this? http://codereview.chromium.org/10969028/diff/14001/webkit/media/filter_helper... File webkit/media/filter_helpers.cc (right): http://codereview.chromium.org/10969028/diff/14001/webkit/media/filter_helper... webkit/media/filter_helpers.cc:27: // DecryptingVideoDecoder only supports encrypted video stream. "streams" Probably a bad example since this will change, though. An example probably isn't necessary, so just remove.
On 2012/09/26 02:34:27, ddorwin wrote: > I need to review dvd.cc and unittests, but I'll wait until we decide on the > location. > > @scherkus @fischman: should decrypting_video_decoder files be in base or > crypto/, base/, or filters/? Chatted w/ scherkus@ offline and we agreed on media/filters. I'll start the move and will also rebase since these files aren't heavily reviewed yet. Please hold off reviewing before I upload the new patch. > > http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h > File media/base/decryptor.h (right): > > http://codereview.chromium.org/10969028/diff/1/media/base/decryptor.h#newcode109 > media/base/decryptor.h:109: // should only be called after > InitializeVideoDecoder() succeeded. > On 2012/09/25 23:52:32, xhwang wrote: > > On 2012/09/21 00:36:08, ddorwin wrote: > > > Hopefully we have a DCHECK for this too. > > > > Will do in implementations in the next CL. > > Add a TODO? > > http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/ppapi_decr... > File webkit/media/crypto/ppapi_decryptor.cc (right): > > http://codereview.chromium.org/10969028/diff/1/webkit/media/crypto/ppapi_decr... > webkit/media/crypto/ppapi_decryptor.cc:16: #include > "media/base/video_decoder_config.h" > On 2012/09/25 23:52:32, xhwang wrote: > > On 2012/09/21 00:36:08, ddorwin wrote: > > > These don't seem necessary. > > > > Surprise! After I removed these two everything still build. > > > > However, VideoDecoderConfig and VideoFrame are all forward declared in header > > files. Everything still build because VideoDecoderConfig and VideoFrame are > > declared indirectly through "webkit/plugins/ppapi/ppapi_plugin_instance.h". > > > > By IWYU, I think we should include them explicitly. > > We use VideoDecodeCB and DecoderInitCB, but not VideoDecoderConfig (just a const > ref). So, we don't need definitions for everything. I'd have to look at where > those two CB's are defined, though, to see what we need to include. > > http://codereview.chromium.org/10969028/diff/14001/media/base/decryptor.h > File media/base/decryptor.h (right): > > http://codereview.chromium.org/10969028/diff/14001/media/base/decryptor.h#new... > media/base/decryptor.h:118: // 1) NULL, which means the operation has been > aborted. > Abort is success? > > http://codereview.chromium.org/10969028/diff/14001/media/base/decryptor.h#new... > media/base/decryptor.h:143: virtual void CancelDecryptAndDecodeVideo() = 0; > Is there a reason this shouldn't just be Cancel(), which would report kError and > NULL data for all types of outstanding requests? Seems odd to have a > D&D-specific function for this. > > http://codereview.chromium.org/10969028/diff/14001/media/crypto/decrypting_vi... > File media/crypto/decrypting_video_decoder.cc (right): > > http://codereview.chromium.org/10969028/diff/14001/media/crypto/decrypting_vi... > media/crypto/decrypting_video_decoder.cc:51: // DecryptingVideoDecoder only > accepts potentially encrypted stream. > For now. Once we start requiring the decision beforehand, we'll need to remove > this. > > http://codereview.chromium.org/10969028/diff/14001/media/crypto/decrypting_vi... > media/crypto/decrypting_video_decoder.cc:239: if (status == Decryptor::kNoKey || > status == Decryptor::kError) { > Why is NoKey a decode (or decrypt) error? Don't we just report a needkey event > somewhere? > > http://codereview.chromium.org/10969028/diff/14001/webkit/media/crypto/proxy_... > File webkit/media/crypto/proxy_decryptor.cc (right): > > http://codereview.chromium.org/10969028/diff/14001/webkit/media/crypto/proxy_... > webkit/media/crypto/proxy_decryptor.cc:16: #include "media/base/video_frame.h" > What is used from this? > > http://codereview.chromium.org/10969028/diff/14001/webkit/media/filter_helper... > File webkit/media/filter_helpers.cc (right): > > http://codereview.chromium.org/10969028/diff/14001/webkit/media/filter_helper... > webkit/media/filter_helpers.cc:27: // DecryptingVideoDecoder only supports > encrypted video stream. > "streams" > Probably a bad example since this will change, though. An example probably isn't > necessary, so just remove.
I updated the CL and please review!
http://codereview.chromium.org/10969028/diff/31001/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/31001/media/base/decryptor.h#new... media/base/decryptor.h:95: // Cancel scheduled decryption operations and fires any pending DecryptCB Cancels http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:33: if (!message_loop_) { Initialize can be called multiple times? Or is this just for the self-post? It would seem better to have a DoInitialize rather than calling itself with this check. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:43: demuxer_stream_ = stream; Can this class be reused after one of the following errors? If so, this should probably be set at 56. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:51: // DecryptingVideoDecoder only accepts potentially encrypted stream. Newline http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:85: // Defer the reset if a read is pending. Why? When will it be called then? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:104: // Defer stopping if a read is pending. Same. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:111: DecryptingVideoDecoder::~DecryptingVideoDecoder() { Do we need this? Presumably the parent is already virtual. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:122: if (!success) { nit: newline since the above DCHECK is applicable to more than this line. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:171: DCHECK_NE(state_, kUninitialized); These two can be replaced with EQ kNormal. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:176: base::ResetAndReturn(&read_cb_).Run(kOk, NULL); Not related to this CL, but odd that we use the data in the second parameter as an indication of whether a frame was successfully decoded. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:198: DCHECK_EQ(DemuxerStream::kOk, status); nit: Did we decide the other order for non-unittests? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:211: // synchronously or asynchronously by the decryptor. Might be worth explaining why or pointing to an explanation so future readers don't wonder why. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:242: base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL); Why is kNoKey a decode error? Where is the wait and retry logic? Should we ever get kNoKey here? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:259: DCHECK_EQ(Decryptor::kSuccess, status); same http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:269: state_ = kNormal; DCHECK(state_ != kUninitialized). http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.h (right): http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.h:34: DecryptingVideoDecoder(const MessageLoopFactoryCB& message_loop_factory_cb, // Blah. // |decryptor| must to outlive this instance. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.h:73: void DeliverFrame(int buffer_size, The previous Decryptor callback started with "On". http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.h:102: // Pointer to a Decryptor that can decrypt and decode encrypted video buffers. This comment will need to be updated when you address the main TODO. Does it need to be this specific? Will you remember to update it? :) http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder_unittest.cc (right): http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:40: scoped_refptr<DecoderBuffer> buffer(new DecoderBuffer(buffer_size)); Why? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:45: DecryptConfig::kDecryptionKeySize), use arraysize() instead. It's not obvious this constant is related without looking at the definition. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:76: decoder_ = new StrictMock<DecryptingVideoDecoder>( Why not done on 71? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:87: virtual ~DecryptingVideoDecoderTest() {} Necessary? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:111: void InitializeWithConfig(const VideoDecoderConfig& config) { This is only used once. Do we really need it to avoid PIPELINE_OK at 97? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:115: void RunPendingVideoDecodeCB() { Returns a NULL frame. that should probably be clear or implied by the function name. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:143: VideoDecoder::Status status; Should be initialized to non-expected value. Same at 156. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:173: .WillRepeatedly(ReturnBuffer(end_of_stream_buffer_)); Does end_of_stream_buffer_ need to be a member, or can we just return DecoderBuffer::CreateEOSBuffer() here? Same for other fixed members. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:179: why newline here instead of after the last EXPECT_CALL? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:218: // Ensure that DecryptingVideoDecoder only acceptes encrypted video. accepts http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:228: // Ensure decoder handles unsupported video configs without crashing. s/unsupported/invalid/ http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:277: VideoDecoder::Status status; init http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:334: EnterEndOfStreamState(); Inconsistency: DecryptAndDecode_Normal could just use EnterDecodingState() but does not, but here you just use EnterEndOfStreamState(). I'm not sure which is better - explicitly testing functionality rather than relying on a helper function vs. code duplication. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:356: } Test multiple Reset()s (and multiple Stop()s below)? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:358: // Test resetting when there is a pending read on the demuxer. What causes the read to be pending? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:359: TEST_F(DecryptingVideoDecoderTest, Reset_DuringPendingRead) { Reset_DuringPendingDemuxerRead http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:377: // Test resetting when there is a pending decrypt on the decryptor. Same. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:418: // Test stopping when there is a pending read on the demuxer. Same question about how pending works here and below. Does it make sense to have SetUpPendingFoo() as helper functions? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:460: // Test aborted read on the demuxer stream. What makes it aborted? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:467: VideoDecoder::Status status; init http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:485: message_loop_.RunAllPending(); // Run so that the read_cb is obtained. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:488: Reset(); Why Reset before aborting?
Resolved ddrowin's comments (thanks!). PTAL again! http://codereview.chromium.org/10969028/diff/31001/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/31001/media/base/decryptor.h#new... media/base/decryptor.h:95: // Cancel scheduled decryption operations and fires any pending DecryptCB On 2012/09/28 17:36:40, ddorwin wrote: > Cancels Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:33: if (!message_loop_) { On 2012/09/28 17:36:40, ddorwin wrote: > Initialize can be called multiple times? Or is this just for the self-post? > It would seem better to have a DoInitialize rather than calling itself with this > check. This is the same code we have in FFVD. But it's a good point to use DoInitialize so that it's consistent w/ other function in this class. By the VideoDecoder API, Initialize() should not be called multiple times because when a VideoDecoder is stopped, it cannot be re-initialized. Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:43: demuxer_stream_ = stream; On 2012/09/28 17:36:40, ddorwin wrote: > Can this class be reused after one of the following errors? If so, this should > probably be set at 56. Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:51: // DecryptingVideoDecoder only accepts potentially encrypted stream. On 2012/09/28 17:36:40, ddorwin wrote: > Newline Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:85: // Defer the reset if a read is pending. On 2012/09/28 17:36:40, ddorwin wrote: > Why? When will it be called then? We can't finish the reset process if we have pending callback not returned. It will be called when the read callback passed to the demuxer stream is fired. See DoDecryptAndDecodeBuffer(). http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:104: // Defer stopping if a read is pending. On 2012/09/28 17:36:40, ddorwin wrote: > Same. See above answer. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:111: DecryptingVideoDecoder::~DecryptingVideoDecoder() { On 2012/09/28 17:36:40, ddorwin wrote: > Do we need this? Presumably the parent is already virtual. Correct. I added a DCHECK in the dtor so now we need it. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:122: if (!success) { On 2012/09/28 17:36:40, ddorwin wrote: > nit: newline since the above DCHECK is applicable to more than this line. Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:171: DCHECK_NE(state_, kUninitialized); On 2012/09/28 17:36:40, ddorwin wrote: > These two can be replaced with EQ kNormal. Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:176: base::ResetAndReturn(&read_cb_).Run(kOk, NULL); On 2012/09/28 17:36:40, ddorwin wrote: > Not related to this CL, but odd that we use the data in the second parameter as > an indication of whether a frame was successfully decoded. I agree. +scherkus/fischman: How about we add kAborted, kDecodeFinished into VideoDecoder::Status? http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:198: DCHECK_EQ(DemuxerStream::kOk, status); On 2012/09/28 17:36:40, ddorwin wrote: > nit: Did we decide the other order for non-unittests? Yeah, as summarized by scherkus in another thread: "So I'd sum up the rule as "Always prefer (foo == 0) for all comparisons except for EXPECT_EQ and ASSERT_EQ which require the order to be reversed for prettier error messages" So thanks for pointing out again. Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:211: // synchronously or asynchronously by the decryptor. On 2012/09/28 17:36:40, ddorwin wrote: > Might be worth explaining why or pointing to an explanation so future readers > don't wonder why. Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:242: base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL); On 2012/09/28 17:36:40, ddorwin wrote: > Why is kNoKey a decode error? Where is the wait and retry logic? Should we ever > get kNoKey here? The wait and retry logic is in ProxyDecryptor so it hides kNoKey intrenally. But not all Decryptor implementations hides kNoKey, e.g. AesDecryptor. The idea of DVD is that it can work with any Decryptor implementations, thus it needs to handle kNoKey here. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:259: DCHECK_EQ(Decryptor::kSuccess, status); On 2012/09/28 17:36:40, ddorwin wrote: > same Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:269: state_ = kNormal; On 2012/09/28 17:36:40, ddorwin wrote: > DCHECK(state_ != kUninitialized). Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.h (right): http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.h:34: DecryptingVideoDecoder(const MessageLoopFactoryCB& message_loop_factory_cb, On 2012/09/28 17:36:40, ddorwin wrote: > // Blah. > // |decryptor| must to outlive this instance. Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.h:73: void DeliverFrame(int buffer_size, On 2012/09/28 17:36:40, ddorwin wrote: > The previous Decryptor callback started with "On". We use both in media code. I use On* for callbacks that only acknowledge something has finished (e.g. by calling completion callback). And use imperative function names for those callbacks that do a lot of real work. But this is just my opinion and can be persuaded :) http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.h:102: // Pointer to a Decryptor that can decrypt and decode encrypted video buffers. On 2012/09/28 17:36:40, ddorwin wrote: > This comment will need to be updated when you address the main TODO. Does it > need to be this specific? Will you remember to update it? :) Removed. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder_unittest.cc (right): http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:40: scoped_refptr<DecoderBuffer> buffer(new DecoderBuffer(buffer_size)); On 2012/09/28 17:36:40, ddorwin wrote: > Why? Since the real decryptor is a mock, we don't really care what the input is. Make a fake encrypted buffer here so that all sanity checks (e.g. data isn't null, size isn't zero) can pass. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:45: DecryptConfig::kDecryptionKeySize), On 2012/09/28 17:36:40, ddorwin wrote: > use arraysize() instead. It's not obvious this constant is related without > looking at the definition. Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:76: decoder_ = new StrictMock<DecryptingVideoDecoder>( On 2012/09/28 17:36:40, ddorwin wrote: > Why not done on 71? Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:87: virtual ~DecryptingVideoDecoderTest() {} On 2012/09/28 17:36:40, ddorwin wrote: > Necessary? Added Stop() as per http://codereview.chromium.org/10990039/. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:111: void InitializeWithConfig(const VideoDecoderConfig& config) { On 2012/09/28 17:36:40, ddorwin wrote: > This is only used once. Do we really need it to avoid PIPELINE_OK at 97? Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:115: void RunPendingVideoDecodeCB() { On 2012/09/28 17:36:40, ddorwin wrote: > Returns a NULL frame. that should probably be clear or implied by the function > name. Rename to Abort* http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:143: VideoDecoder::Status status; On 2012/09/28 17:36:40, ddorwin wrote: > Should be initialized to non-expected value. Same at 156. Fixed here and everywhere else. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:173: .WillRepeatedly(ReturnBuffer(end_of_stream_buffer_)); On 2012/09/28 17:36:40, ddorwin wrote: > Does end_of_stream_buffer_ need to be a member, or can we just return > DecoderBuffer::CreateEOSBuffer() here? Same for other fixed members. Dropped end_of_stream_buffer_. But I need to keep decoded_video_frame_ to check that the frame the decoder received is the same as what the decryptor returns (see line 317 and 327). Keep encrypted_buffer_ for the same reason. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:179: On 2012/09/28 17:36:40, ddorwin wrote: > why newline here instead of after the last EXPECT_CALL? Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:218: // Ensure that DecryptingVideoDecoder only acceptes encrypted video. On 2012/09/28 17:36:40, ddorwin wrote: > accepts Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:228: // Ensure decoder handles unsupported video configs without crashing. On 2012/09/28 17:36:40, ddorwin wrote: > s/unsupported/invalid/ Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:277: VideoDecoder::Status status; On 2012/09/28 17:36:40, ddorwin wrote: > init Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:334: EnterEndOfStreamState(); On 2012/09/28 17:36:40, ddorwin wrote: > Inconsistency: DecryptAndDecode_Normal could just use EnterDecodingState() but > does not, but here you just use EnterEndOfStreamState(). I'm not sure which is > better - explicitly testing functionality rather than relying on a helper > function vs. code duplication. Changed DecryptAndDecode_Normal to use EnterDecodingState(). I think helper function is okay as long as the intent of that function is clear. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:356: } On 2012/09/28 17:36:40, ddorwin wrote: > Test multiple Reset()s (and multiple Stop()s below)? Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:358: // Test resetting when there is a pending read on the demuxer. On 2012/09/28 17:36:40, ddorwin wrote: > What causes the read to be pending? The read is pending because we save the read_cb_ into read_cb and don't fire it. Then we fire it after Reset() is called. Added more comments. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:359: TEST_F(DecryptingVideoDecoderTest, Reset_DuringPendingRead) { On 2012/09/28 17:36:40, ddorwin wrote: > Reset_DuringPendingDemuxerRead Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:377: // Test resetting when there is a pending decrypt on the decryptor. On 2012/09/28 17:36:40, ddorwin wrote: > Same. Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:418: // Test stopping when there is a pending read on the demuxer. On 2012/09/28 17:36:40, ddorwin wrote: > Same question about how pending works here and below. > Does it make sense to have SetUpPendingFoo() as helper functions? Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:460: // Test aborted read on the demuxer stream. On 2012/09/28 17:36:40, ddorwin wrote: > What makes it aborted? Added comments. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:467: VideoDecoder::Status status; On 2012/09/28 17:36:40, ddorwin wrote: > init Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:485: message_loop_.RunAllPending(); On 2012/09/28 17:36:40, ddorwin wrote: > // Run so that the read_cb is obtained. Done. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:488: Reset(); On 2012/09/28 17:36:40, ddorwin wrote: > Why Reset before aborting? Reset cannot complete (pending) when read callback is pending. This test tests the case when aborting during pending reset. So we have to enter the pending reset state (by calling Reset()) first then abort the demuxer read. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:191: DoReset(); This is to handle the case where Stop() is called during pending Reset(). But I feel it's not really very useful as they both depend on the pending read. Probably we should just specify in the API that Stop() shouldn't be called during a pending Reset()? http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder_unittest.cc (right): http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:82: Will reorder these functions, but in a later patch to make reviewing easier.
Still some test code to review, but I wanted to get this out sooner. http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/31001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:242: base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL); On 2012/09/30 19:58:51, xhwang wrote: > On 2012/09/28 17:36:40, ddorwin wrote: > > Why is kNoKey a decode error? Where is the wait and retry logic? Should we > ever > > get kNoKey here? > > The wait and retry logic is in ProxyDecryptor so it hides kNoKey intrenally. But > not all Decryptor implementations hides kNoKey, e.g. AesDecryptor. The idea of > DVD is that it can work with any Decryptor implementations, thus it needs to > handle kNoKey here. But wouldn't this class always wrap the real Decryptor in ProxyDecryptor? Should the kNoKey logic be here then? Thinking ahead to when we remove Decryptor from FFmpegDecoder, this will be the central point for all kNoKey returns. Regardless, it doesn't seem like we would expect kNoKey in this code, which means we should at least DCHECK that it is not received at 238. http://codereview.chromium.org/10969028/diff/36001/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/36001/media/base/decryptor.h#new... media/base/decryptor.h:117: // 1) NULL, which means the operation has been aborted. Copied from previous patch: Abort is success? http://codereview.chromium.org/10969028/diff/36001/media/base/decryptor.h#new... media/base/decryptor.h:142: virtual void CancelDecryptAndDecodeVideo() = 0; Copied from previous patch: Is there a reason this shouldn't just be Cancel(), which would report kError and NULL data for all types of outstanding requests? Seems odd to have a D&D-specific function for this. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:61: // Defer the reset if a read is pending. Reply to conversation in other patch: Okay. Might be nice to explain why and/or when it will happen. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:80: // Defer stopping if a read is pending. Same http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:95: DCHECK(stream); DCHECK thread. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:98: DLOG(ERROR) << "Invalid video stream - " << config.AsHumanReadableString(); ...stream config? http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:121: const PipelineStatusCB& status_cb, nit: Is there a reason the result comes AFTER the callback? http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:170: // In theory, we don't need to force post the task here, because we do a Isn't the old comment referring to the fact that this call is executed on the stack of demuxer_stream_->Read()? If so, isn't that different from the callback that DoDecryptAndDecodeBuffer calls? I also worry about removing this TODO because when it is fixed, it will affect all decoders. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:174: // logic more understandable and manageable, so why not? I would rephrase the "why not?" questions here and below to statements. They appear to be uncertain as to whether this is the right thing. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:191: DoReset(); On 2012/09/30 19:58:51, xhwang wrote: > This is to handle the case where Stop() is called during pending Reset(). But I > feel it's not really very useful as they both depend on the pending read. > Probably we should just specify in the API that Stop() shouldn't be called > during a pending Reset()? Does it make sense to call both callbacks? Do callers expect this? I'm fine with updating the API but we should see what others say. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:227: // synchronously in Reset()/Stop(). Instead of using more complicated logic in Oh, I didn't realize it was just for Reset and Stop. We're paying a cost in the normal case for the rare case of a reset/stop. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder_unittest.cc (right): http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:39: const int encrypted_frame_offset = 1; // This should be non-zero. Why should this be non-zero? (That was my original question, but "Why?" appeared on the wrong line for some reason.) http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:82: On 2012/09/30 19:58:51, xhwang wrote: > Will reorder these functions, but in a later patch to make reviewing easier. Thank you! http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:183: void ReadAndExpectToCompleteWith(VideoDecoder::Status* status, ReadAndExpectToCompleteWithStatus ^^^^^^ Maybe even better: ReadAndExpectFrameReadyWithStatus http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:296: VideoDecoder::Status status = VideoDecoder::kDecryptError; kSuccess would be fine in this case. Then you don't have to worry about what type of error it was. Same below.
Hello ddorwin/scherkus, Resolve comments and have a big question about force task post. Please see inline comments for the discussion and question! xhwang http://codereview.chromium.org/10969028/diff/36001/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/36001/media/base/decryptor.h#new... media/base/decryptor.h:117: // 1) NULL, which means the operation has been aborted. On 2012/10/01 18:43:20, ddorwin wrote: > Copied from previous patch: > Abort is success? Yeah, it's a little tricky. But this is for historical reasons: http://code.google.com/searchframe#OAMlx_jo-ck/src/media/base/video_decoder.h... It kind of makes sense because when demuxer read is aborted, the decoder is still in a good shape (no error happened). http://codereview.chromium.org/10969028/diff/36001/media/base/decryptor.h#new... media/base/decryptor.h:142: virtual void CancelDecryptAndDecodeVideo() = 0; On 2012/10/01 18:43:20, ddorwin wrote: > Copied from previous patch: > Is there a reason this shouldn't just be Cancel(), which would report kError and > NULL data for all types of outstanding requests? Seems odd to have a > D&D-specific function for this. As discussed in an offline email thread, since both video decoder and audio decoder issues Reset() separately, we need to handle them in the decryptor separately. If we have one general Cancel() function that cancels all types of outstanding requests, we may end up with the following call sequence: video_decoder->CancelDecrypt() // success! video_decoder->Decrypt() // new buffer after seek audio_decoder->CancelDecrypt() // oops, cancelled the new video buffer! Given the fact that we choose either decrypt-only or decrypt-and-decode for video (and audio), we may only need functions like CancelVideo() and CancelAudio(). But I'd like to address this in later CLs as it affects the existing implementation. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:61: // Defer the reset if a read is pending. On 2012/10/01 18:43:20, ddorwin wrote: > Reply to conversation in other patch: Okay. Might be nice to explain why and/or > when it will happen. Done. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:80: // Defer stopping if a read is pending. On 2012/10/01 18:43:20, ddorwin wrote: > Same Done. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:95: DCHECK(stream); On 2012/10/01 18:43:20, ddorwin wrote: > DCHECK thread. Done. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:98: DLOG(ERROR) << "Invalid video stream - " << config.AsHumanReadableString(); On 2012/10/01 18:43:20, ddorwin wrote: > ...stream config? Done. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:98: DLOG(ERROR) << "Invalid video stream - " << config.AsHumanReadableString(); On 2012/10/01 18:43:20, ddorwin wrote: > ...stream config? Done. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:121: const PipelineStatusCB& status_cb, On 2012/10/01 18:43:20, ddorwin wrote: > nit: Is there a reason the result comes AFTER the callback? This is because we need to bind the status_cb in the callback for decryptor_->InitializeVideoDecoder(). See line 116. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:170: // In theory, we don't need to force post the task here, because we do a On 2012/10/01 18:43:20, ddorwin wrote: > Isn't the old comment referring to the fact that this call is executed on the > stack of demuxer_stream_->Read()? If so, isn't that different from the callback > that DoDecryptAndDecodeBuffer calls? > I also worry about removing this TODO because when it is fixed, it will affect > all decoders. That's a long story :) The original comment is copied from FFVD (http://code.google.com/searchframe#OAMlx_jo-ck/src/media/filters/ffmpeg_video...). The reason for this is that if FFmpegDemuxerStream::Read() execute the read callback on the same stack (synchronously), we may end up with the following re-entrancing call sequence: ReadFromDemuxerStream() -> FFmpegDemuxerStream::Read() -> DecryptOrDecodeBuffer() -> DoDecryptOrDecodeBuffer -> ReadFromDemuxerStream(). In our case, we have one more callback, the video_decode_cb_. So to produce the above re-entrancing call sequence it has to be: ReadFromDemuxerStream() -> FFmpegDemuxerStream::Read() -> DecryptAndDecodeBuffer() -> DoDecryptAndDecodeBuffer() -> Decryptor::DecryptAndDecodeVideo() -> DeliverFrame() -> DoDeliverFrame() -> ReadFromDemuxerStream(). Note that this can happen because FFmpegDemuxerStream::Read() and Decryptor::DecryptAndDecodeVideo() can both call their callbacks synchronously (on the same call stack). To prevent the above call sequence, we can force post either in DecryptOrDecodeBuffer() (as we did in FFVD), or in DeliverFrame(). Other than that, we also could end up with the following call sequence: DecryptingVideoDecoder::Reset() -> Decryptor::CancelDecryptAndDecodeVideo() -> DeliverFrame() -> DoDeliverFrame() -> base::ResetAndReturn(&read_cb_).Run(kOk, NULL), because Decryptor::CancelDecryptAndDecodeVideo() can also call the video_decode_cb_ synchronously. Since we also check "read_cb_" in Reset() (line 62), this will be an issue. It can be fixed by adding more logic in Reset(). But since the long calling sequence isn't good anyways, I'd like to just force post in DeliverFrame() to prevent it. Since we force post in DeliverFrame(), it's not necessary in theory to force post in DecryptOrDecodeBuffer(). But for the same reason (to avoid super long call sequence), I also force post in DecryptOrDecodeBuffer(). Question: Since the ProxyDecryptor will have it's own message loop, does it make sense to enforce the ProxyDecryptor to always return callbacks asynchronously? (If we do that we won't need any for post task in DVD, but may need to always post Decrypt()/CancelDecrypt() calls to make sure callbacks are not returned in the same callstack.) Actually I wonder how expensive it is to have an extra post? Is this a big performance concern or just some minor optimization? +scherkus: WDYT? http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:174: // logic more understandable and manageable, so why not? On 2012/10/01 18:43:20, ddorwin wrote: > I would rephrase the "why not?" questions here and below to statements. They > appear to be uncertain as to whether this is the right thing. Agreed. The right explanation is too long (see above) and I didn't have a good way to summarize it. Will try again once we decided what to do (see Question above). http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:191: DoReset(); On 2012/10/01 18:43:20, ddorwin wrote: > On 2012/09/30 19:58:51, xhwang wrote: > > This is to handle the case where Stop() is called during pending Reset(). But > I > > feel it's not really very useful as they both depend on the pending read. > > Probably we should just specify in the API that Stop() shouldn't be called > > during a pending Reset()? > > Does it make sense to call both callbacks? Do callers expect this? > I'm fine with updating the API but we should see what others say. Yeah, I also prefer to updating the API to prevent this. This is kind of unnecessary complexity. +scherkus, what do you think? http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:227: // synchronously in Reset()/Stop(). Instead of using more complicated logic in On 2012/10/01 18:43:20, ddorwin wrote: > Oh, I didn't realize it was just for Reset and Stop. We're paying a cost in the > normal case for the rare case of a reset/stop. That's a valid point: paying a cost in the normal case for rare cases. This can be avoided by enforcing video_decode_cb_ is always fired asychronously in CancelDecryptAndDecodeVideo(). But I'd like to get consensus on this before updating (see above comments about force posting). http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder_unittest.cc (right): http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:39: const int encrypted_frame_offset = 1; // This should be non-zero. On 2012/10/01 18:43:20, ddorwin wrote: > Why should this be non-zero? > (That was my original question, but "Why?" appeared on the wrong line for some > reason.) I think this was required before but it seems not now. Maybe the requirement was dropped when we remove the hmac check. Done. http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:183: void ReadAndExpectToCompleteWith(VideoDecoder::Status* status, On 2012/10/01 18:43:20, ddorwin wrote: > ReadAndExpectToCompleteWithStatus > ^^^^^^ > Maybe even better: > ReadAndExpectFrameReadyWithStatus Changed to ReadAndExpectFrameReadyWithStatusAndFrame(). Not sure if it's too long :) http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:296: VideoDecoder::Status status = VideoDecoder::kDecryptError; On 2012/10/01 18:43:20, ddorwin wrote: > kSuccess would be fine in this case. Then you don't have to worry about what > type of error it was. Same below. Done.
Looking good, but we need to resolve a few items with scherkus. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:63: // after the read callback is fired (see DoDecryptAndDecodeBuffer() and nit: Might be cleaner to use a dash instead of parens. Below too. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder_unittest.cc (right): http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:380: } Should there be a Reset after Stop test? http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:389: Reset(); None of these Reset tests check the resulting state. Should they? http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:390: base::ResetAndReturn(&pending_demuxer_read_cb_).Run(DemuxerStream::kOk, No state is checked after here either. What is the point of running these callbacks? Should something WillOnce() them if we are trying to simulate real behavior? http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:432: config_.Initialize(kCodecVP8, VIDEO_CODEC_PROFILE_UNKNOWN, kVideoFormat, Why is this config necessary? http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:437: Stop(); Stop tests are not checking state either. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:479: // Test stopping after the decoder has been stopped. s/stopped/reset/
overall looking good http://codereview.chromium.org/10969028/diff/45001/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/45001/media/base/decryptor.h#new... media/base/decryptor.h:30: // Note that the all callbacks can be fired synchronously or asynchronously. "that the all" ? http://codereview.chromium.org/10969028/diff/45001/media/base/decryptor.h#new... media/base/decryptor.h:50: virtual ~Decryptor() {} can you move ctor/dtor impl to .cc? http://codereview.chromium.org/10969028/diff/45001/media/base/decryptor.h#new... media/base/decryptor.h:108: // should only be called after InitializeVideoDecoder() succeeded. /should/can/? http://codereview.chromium.org/10969028/diff/45001/media/base/decryptor.h#new... media/base/decryptor.h:144: // Stops decoder and sets it to an uninitialized state. is it permissible to re-initialize after calling this? http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:24: message_loop_(NULL), nit: remove http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:86: // DoDeliverFrame()). hmm.. is it worth mentioning that a pending reset_cb_ could happen? it caught me off guard when I saw us executing reset_cb_ in our stop handling code http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:122: decryptor_->InitializeVideoDecoder( style nit: move "config, base::Bind(" here then drop rest of params on next line http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:194: DCHECK_EQ(!!buffer, status == DemuxerStream::kOk) << status; we decided against !! style -- just compare against NULL http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:223: DCHECK(buffer); dcheck not useful http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:287: DCHECK(frame); dcheck not useful http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder_unittest.cc (right): http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:185: EXPECT_CALL(*this, FrameReady(_, _)) can't you pass in (status, video_frame) as the EXPECT_CALL params instead of saving the params + EXPECT_XX'ing them later? http://codereview.chromium.org/10969028/diff/45001/webkit/media/filter_helper... File webkit/media/filter_helpers.cc (right): http://codereview.chromium.org/10969028/diff/45001/webkit/media/filter_helper... webkit/media/filter_helpers.cc:39: base::Bind(&media::MessageLoopFactory::GetMessageLoop, afaik we can bind this once to a local var and pass it as an arg
resolved comments and fix the stop-during-pending-init behavior. PTAL! http://codereview.chromium.org/10969028/diff/45001/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/45001/media/base/decryptor.h#new... media/base/decryptor.h:30: // Note that the all callbacks can be fired synchronously or asynchronously. On 2012/10/02 19:45:41, scherkus wrote: > "that the all" ? Done. http://codereview.chromium.org/10969028/diff/45001/media/base/decryptor.h#new... media/base/decryptor.h:50: virtual ~Decryptor() {} On 2012/10/02 19:45:41, scherkus wrote: > can you move ctor/dtor impl to .cc? Done. http://codereview.chromium.org/10969028/diff/45001/media/base/decryptor.h#new... media/base/decryptor.h:108: // should only be called after InitializeVideoDecoder() succeeded. On 2012/10/02 19:45:41, scherkus wrote: > /should/can/? Done. http://codereview.chromium.org/10969028/diff/45001/media/base/decryptor.h#new... media/base/decryptor.h:144: // Stops decoder and sets it to an uninitialized state. On 2012/10/02 19:45:41, scherkus wrote: > is it permissible to re-initialize after calling this? The answer is "yes", and that's the plan for supporting kConfigChanged. Added comments to make this explicit. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:24: message_loop_(NULL), On 2012/10/02 19:45:41, scherkus wrote: > nit: remove Done. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:63: // after the read callback is fired (see DoDecryptAndDecodeBuffer() and On 2012/10/02 06:10:41, ddorwin wrote: > nit: Might be cleaner to use a dash instead of parens. Below too. Done. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:86: // DoDeliverFrame()). On 2012/10/02 19:45:41, scherkus wrote: > hmm.. is it worth mentioning that a pending reset_cb_ could happen? > > it caught me off guard when I saw us executing reset_cb_ in our stop handling > code Done. Also fixed a bug that stopping during initialization won't block if init_cb is not returned. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:122: decryptor_->InitializeVideoDecoder( On 2012/10/02 19:45:41, scherkus wrote: > style nit: move "config, base::Bind(" here then drop rest of params on next line Done. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:194: DCHECK_EQ(!!buffer, status == DemuxerStream::kOk) << status; On 2012/10/02 19:45:41, scherkus wrote: > we decided against !! style -- just compare against NULL Done. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:223: DCHECK(buffer); On 2012/10/02 19:45:41, scherkus wrote: > dcheck not useful Done. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:287: DCHECK(frame); On 2012/10/02 19:45:41, scherkus wrote: > dcheck not useful Done. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder_unittest.cc (right): http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:185: EXPECT_CALL(*this, FrameReady(_, _)) On 2012/10/02 19:45:41, scherkus wrote: > can't you pass in (status, video_frame) as the EXPECT_CALL params instead of > saving the params + EXPECT_XX'ing them later? That's an awesome advice. Thanks! Done. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:380: } On 2012/10/02 06:10:41, ddorwin wrote: > Should there be a Reset after Stop test? That's not allowed by the API. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:389: Reset(); On 2012/10/02 06:10:41, ddorwin wrote: > None of these Reset tests check the resulting state. Should they? Do you mean the internal state? That can't be checked from outside. If you mean the state returned by the callback. Reset callback is a closure and doesn't have a return value. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:390: base::ResetAndReturn(&pending_demuxer_read_cb_).Run(DemuxerStream::kOk, On 2012/10/02 06:10:41, ddorwin wrote: > No state is checked after here either. > What is the point of running these callbacks? Should something WillOnce() them > if we are trying to simulate real behavior? The reset callback is NewExpectedClosure() which we already have a EXPECT_CALL inside. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:432: config_.Initialize(kCodecVP8, VIDEO_CODEC_PROFILE_UNKNOWN, kVideoFormat, On 2012/10/02 06:10:41, ddorwin wrote: > Why is this config necessary? If the config is not valid or not encrypted, the initiliaztion will return right away and then we can't reach the init-pending state. http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:437: Stop(); On 2012/10/02 06:10:41, ddorwin wrote: > Stop tests are not checking state either. ditto http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:479: // Test stopping after the decoder has been stopped. On 2012/10/02 06:10:41, ddorwin wrote: > s/stopped/reset/ Done.
lgtm w/ nits let's get this in and iterate http://codereview.chromium.org/10969028/diff/61002/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/61002/media/base/decryptor.h#new... media/base/decryptor.h:30: // Note that all callbacks can be fired synchronously or asynchronously. hmm.. how about "Depending on the implementation callbacks may be fired synchronously or asynchronously." http://codereview.chromium.org/10969028/diff/61002/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/61002/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:200: DCHECK(status == DemuxerStream::kOk ? buffer : !buffer) << status; this is even more confusing I was suggesting DCHECK_EQ(buffer != NULL, status == DemuxerStream::kOk)
lgtm http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder_unittest.cc (right): http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:380: } On 2012/10/03 02:15:08, xhwang wrote: > On 2012/10/02 06:10:41, ddorwin wrote: > > Should there be a Reset after Stop test? > > That's not allowed by the API. So it would DCHECK? http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:389: Reset(); On 2012/10/03 02:15:08, xhwang wrote: > On 2012/10/02 06:10:41, ddorwin wrote: > > None of these Reset tests check the resulting state. Should they? > > Do you mean the internal state? That can't be checked from outside. If you mean > the state returned by the callback. Reset callback is a closure and doesn't have > a return value. Both I guess. Don't want to make the test too rigid, but how do we know that this does anything other than not crash?
http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder_unittest.cc (right): http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:380: } On 2012/10/04 00:57:11, ddorwin wrote: > On 2012/10/03 02:15:08, xhwang wrote: > > On 2012/10/02 06:10:41, ddorwin wrote: > > > Should there be a Reset after Stop test? > > > > That's not allowed by the API. > > So it would DCHECK? Yes, in Reset() we have "DCHECK_NE(state_, kUninitialized);" http://codereview.chromium.org/10969028/diff/45001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:389: Reset(); On 2012/10/04 00:57:11, ddorwin wrote: > On 2012/10/03 02:15:08, xhwang wrote: > > On 2012/10/02 06:10:41, ddorwin wrote: > > > None of these Reset tests check the resulting state. Should they? > > > > Do you mean the internal state? That can't be checked from outside. If you > mean > > the state returned by the callback. Reset callback is a closure and doesn't > have > > a return value. > > Both I guess. Don't want to make the test too rigid, but how do we know that > this does anything other than not crash? We make sure the reset callback waits for the read callback to complete to finish. http://codereview.chromium.org/10969028/diff/61002/media/base/decryptor.h File media/base/decryptor.h (right): http://codereview.chromium.org/10969028/diff/61002/media/base/decryptor.h#new... media/base/decryptor.h:30: // Note that all callbacks can be fired synchronously or asynchronously. On 2012/10/04 00:15:13, scherkus wrote: > hmm.. how about "Depending on the implementation callbacks may be fired > synchronously or asynchronously." Done. http://codereview.chromium.org/10969028/diff/61002/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/10969028/diff/61002/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:200: DCHECK(status == DemuxerStream::kOk ? buffer : !buffer) << status; On 2012/10/04 00:15:13, scherkus wrote: > this is even more confusing > > I was suggesting > DCHECK_EQ(buffer != NULL, status == DemuxerStream::kOk) Done.
http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... File media/filters/decrypting_video_decoder_unittest.cc (right): http://codereview.chromium.org/10969028/diff/36001/media/filters/decrypting_v... media/filters/decrypting_video_decoder_unittest.cc:82: On 2012/10/01 18:43:20, ddorwin wrote: > On 2012/09/30 19:58:51, xhwang wrote: > > Will reorder these functions, but in a later patch to make reviewing easier. > > Thank you! Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10969028/51020
Change committed as 160183 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
