Add DecryptingDemuxerStream::Stop().

Add DecryptingDemuxerStream::Stop().

During the teardown process, media pipeline will be waiting on the
render main thread. If a Decryptor depends on the render main thread
(e.g. PpapiDecryptor), the pending DecryptCB will not be satisfied.

This CL adds a DecryptingDemuxerStream::Stop() method so that the
stopping process doesn't wait for any pending operation.

BUG=343748
TEST=Add unittests to cover this case.
R=ddorwin@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251277

[xhwang, 2014-02-14 01:33:19]

PTAL

https://codereview.chromium.org/165733002/diff/1/media/filters/decoder_select...
File media/filters/decoder_selector.cc (right):

https://codereview.chromium.org/165733002/diff/1/media/filters/decoder_select...
media/filters/decoder_selector.cc:133: decrypted_stream_->Stop(
DecoderSelector is new in M34. So I need to do a manual merge for M33.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.cc:83: if (state_ ==
kDecryptorRequested) {
This should not happen and in theory can be removed. But I am thinking keeping
this here so that this CL is easier to merge.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.cc:141:
BindToCurrentLoop(closure).Run();
l.124-141 is pretty much the same as DAD and DVD.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
File media/filters/decrypting_demuxer_stream_unittest.cc (right):

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream_unittest.cc:501: }
I'll add more tests shortly.

[ddorwin, 2014-02-14 01:55:13]

LG. Some questions and nits; also waiting for the new tests.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.cc:83: if (state_ ==
kDecryptorRequested) {
On 2014/02/14 01:33:19, xhwang wrote:
> This should not happen and in theory can be removed. But I am thinking keeping
> this here so that this CL is easier to merge.

TODO?

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.cc:117: // Invalid all weak pointers so
that pending callbacks won't fire.
Invalid*at*e

callbacks won't *be fired into this object*?

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.cc:122: // render thread to be
processing messages to complete (such as PPAPI
nit: ...to process messages...

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.cc:123: // callbacks).
nit: They aren't really "PPAPI callbacks" (e.g. completion callbacks) - they are
just PPAPI methods, right?

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.cc:125:
decryptor_->RegisterNewKeyCB(GetDecryptorStreamType(),
// Clear the callback.

Why do we need to do this? Why doesn't the weak pointer handle this?

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
File media/filters/decrypting_demuxer_stream.h (right):

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:41: // operation to finish before
satisfying |closure|. Sets |this| to
nit: |this| can't be set to kUninitialized

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:47: // and sets |this| to kStopped
state. Do NOT wait for any pending operations.
ditto

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:47: // and sets |this| to kStopped
state. Do NOT wait for any pending operations.
s/Do/Does/

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:50: // (e.g. PpapiDecryptor), the
pending DecryptCB will not be satisfied.
s/will/would/?

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
File media/filters/decrypting_demuxer_stream_unittest.cc (right):

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream_unittest.cc:111:
.WillRepeatedly(SaveArg<1>(&key_added_cb_));
Why the change?

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream_unittest.cc:235: // This function
simulates this case.
nit: Does it really simulate this case or just ensure that CancelDecrypt is
called, which means we're doing something other than waiting for the decryptor
to complete?

Does this comment and expected call really stand alone for all Stop() calls or
does it only apply to the actual test case with a pending decrypt? Maybe just
combine this into the test below?

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream_unittest.cc:501: }
On 2014/02/14 01:33:19, xhwang wrote:
> I'll add more tests shortly.

Okay - at least need a test for calling Stop() when doing nothing.

[xhwang, 2014-02-14 03:00:52]

PTAL again

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.cc:83: if (state_ ==
kDecryptorRequested) {
On 2014/02/14 01:55:13, ddorwin wrote:
> On 2014/02/14 01:33:19, xhwang wrote:
> > This should not happen and in theory can be removed. But I am thinking
keeping
> > this here so that this CL is easier to merge.
> 
> TODO?

Done.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.cc:117: // Invalid all weak pointers so
that pending callbacks won't fire.
On 2014/02/14 01:55:13, ddorwin wrote:
> Invalid*at*e
> 
> callbacks won't *be fired into this object*?

Done.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.cc:122: // render thread to be
processing messages to complete (such as PPAPI
On 2014/02/14 01:55:13, ddorwin wrote:
> nit: ...to process messages...

Done.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.cc:123: // callbacks).
On 2014/02/14 01:55:13, ddorwin wrote:
> nit: They aren't really "PPAPI callbacks" (e.g. completion callbacks) - they
are
> just PPAPI methods, right?

Done.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.cc:125:
decryptor_->RegisterNewKeyCB(GetDecryptorStreamType(),
On 2014/02/14 01:55:13, ddorwin wrote:
> // Clear the callback.

Done.
> 
> Why do we need to do this? Why doesn't the weak pointer handle this?

Decryptor won't call into this class anymore due to the invalidated weak
pointer. But it will still try to make the call (i.e. evaluate the weak ptr and
then give up). The API allows we to cancel the NewKeyCB so that the decryptor
won't even try to make the call. But I agree that these are done in the
pre-weak-ptr era and now I feel it has little value.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
File media/filters/decrypting_demuxer_stream.h (right):

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:41: // operation to finish before
satisfying |closure|. Sets |this| to
On 2014/02/14 01:55:13, ddorwin wrote:
> nit: |this| can't be set to kUninitialized

Done.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:47: // and sets |this| to kStopped
state. Do NOT wait for any pending operations.
On 2014/02/14 01:55:13, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:47: // and sets |this| to kStopped
state. Do NOT wait for any pending operations.
On 2014/02/14 01:55:13, ddorwin wrote:
> s/Do/Does/

Done.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:50: // (e.g. PpapiDecryptor), the
pending DecryptCB will not be satisfied.
On 2014/02/14 01:55:13, ddorwin wrote:
> s/will/would/?

Done.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
File media/filters/decrypting_demuxer_stream_unittest.cc (right):

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream_unittest.cc:111:
.WillRepeatedly(SaveArg<1>(&key_added_cb_));
On 2014/02/14 01:55:13, ddorwin wrote:
> Why the change?

Because now we may call RegisterNewKeyCB() twice (register and cancel).

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream_unittest.cc:235: // This function
simulates this case.
On 2014/02/14 01:55:13, ddorwin wrote:
> nit: Does it really simulate this case or just ensure that CancelDecrypt is
> called, which means we're doing something other than waiting for the decryptor
> to complete?

AbortPendingDecryptCB() is not in this case, so the pending_decrypt_cb will
never be satisfied.
> 
> Does this comment and expected call really stand alone for all Stop() calls or
> does it only apply to the actual test case with a pending decrypt? Maybe just
> combine this into the test below?

Updated the comment.

https://codereview.chromium.org/165733002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream_unittest.cc:501: }
On 2014/02/14 01:55:13, ddorwin wrote:
> On 2014/02/14 01:33:19, xhwang wrote:
> > I'll add more tests shortly.
> 
> Okay - at least need a test for calling Stop() when doing nothing.

Done.

[xhwang, 2014-02-14 03:07:09]

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
File media/filters/decrypting_demuxer_stream_unittest.cc (right):

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream_unittest.cc:240: if (is_decryptor_set_)
CancelDecrypt() won't be called if the decryptor wasn't set.

[ddorwin, 2014-02-14 04:17:58]

lgtm

LGTM with nits comments, which can be addressed in another CL.

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream.cc:119: // Invalidate all weak pointers
so that pending callbacks won't fire.
...so that callbacks won't be fired into this object?

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream.cc:127:
decryptor_->RegisterNewKeyCB(GetDecryptorStreamType(),
Per the discussion in PS1, maybe we should remove this.

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
File media/filters/decrypting_demuxer_stream_unittest.cc (right):

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream_unittest.cc:53:
ACTION_P2(SetDecryptorIfNotNull, decryptor, is_decryptor_set) {
This could probably use a comment. I'm not exactly sure what it's doing or about
the logic for is_decryptor_set.

Is it really setting a decryptor if (what is?) not null? Or is it setting the
output parameter if something is not null?

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream_unittest.cc:114:
.WillRepeatedly(SaveArg<1>(&key_added_cb_));
I wonder if we should just have two expects, the second with the "null" CB. Or,
just revert this after removing the CB clearing per the discussion there.

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream_unittest.cc:254: // Whether a valid
Decryptor is set to the |demuxer_stream_|.
... has been set in the...?

[commit-bot: I haz the power, 2014-02-14 04:19:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/165733002/170001

[commit-bot: I haz the power, 2014-02-14 08:45:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 08:45:24]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[xhwang, 2014-02-14 08:46:43]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-02-14 08:47:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/165733002/170001

[xhwang, 2014-02-14 09:27:15]

Committed patchset #3 manually as r251277 (tree was closed).

[xhwang, 2014-02-14 18:47:26]

Comments addressed in https://codereview.chromium.org/163433004/.

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream.cc:119: // Invalidate all weak pointers
so that pending callbacks won't fire.
On 2014/02/14 04:17:59, ddorwin wrote:
> ...so that callbacks won't be fired into this object?

Done.

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream.cc:127:
decryptor_->RegisterNewKeyCB(GetDecryptorStreamType(),
On 2014/02/14 04:17:59, ddorwin wrote:
> Per the discussion in PS1, maybe we should remove this.

Added TODO since we also need to clean up other Decrypting* classes.

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
File media/filters/decrypting_demuxer_stream_unittest.cc (right):

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream_unittest.cc:53:
ACTION_P2(SetDecryptorIfNotNull, decryptor, is_decryptor_set) {
On 2014/02/14 04:17:59, ddorwin wrote:
> This could probably use a comment. I'm not exactly sure what it's doing or
about
> the logic for is_decryptor_set.
> 
> Is it really setting a decryptor if (what is?) not null? Or is it setting the
> output parameter if something is not null?

Done.

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream_unittest.cc:114:
.WillRepeatedly(SaveArg<1>(&key_added_cb_));
On 2014/02/14 04:17:59, ddorwin wrote:
> I wonder if we should just have two expects, the second with the "null" CB.
Or,
> just revert this after removing the CB clearing per the discussion there.

Added TODO.

https://codereview.chromium.org/165733002/diff/170001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream_unittest.cc:254: // Whether a valid
Decryptor is set to the |demuxer_stream_|.
On 2014/02/14 04:17:59, ddorwin wrote:
> ... has been set in the...?

Done.