[Chromecast] Make sure that Stop() stops buffer decryption

[Chromecast] Make sure that Stop() stops buffer decryption

AvPipelineImpl's Stop() call must prevent any pending buffer decryption callback
from ever running. StartPlayingFrom() sets enable_feeding_ back to true, so if
a pending decryption callback from before Stop() is allowed to complete after
StartPlayingFrom() is called again, it will think everything is fine
and try to push a buffer, resulting in a double push.

BUG= internal b/31543110

Committed: https://crrev.com/5dd51b563246eed2fd0a8cdc7061169a664974f6
Cr-Commit-Position: refs/heads/master@{#420489}

[kmackay, 2016-09-22 17:19:32]

kmackay@chromium.org changed reviewers:
+ halliwell@chromium.org, yucliu@chromium.org

[alokp, 2016-09-22 17:25:17]

alokp@chromium.org changed reviewers:
+ alokp@chromium.org

[alokp, 2016-09-22 17:25:18]

https://codereview.chromium.org/2361103002/diff/1/chromecast/media/cma/pipeli...
File chromecast/media/cma/pipeline/av_pipeline_impl.h (right):

https://codereview.chromium.org/2361103002/diff/1/chromecast/media/cma/pipeli...
chromecast/media/cma/pipeline/av_pipeline_impl.h:175: // Special weak factory
used for asynchronous decryption. This allows us to
This comment does not seem accurate. IIUC it does not cancel the actual
decryption task but just drops the decryption completion callback. If this is
true, I do not see how this patch helps because we already early-return if
enable_feeding_ is false (which is set in Stop).

[kmackay, 2016-09-22 17:32:10]

On 2016/09/22 17:25:18, alokp wrote:
>
https://codereview.chromium.org/2361103002/diff/1/chromecast/media/cma/pipeli...
> File chromecast/media/cma/pipeline/av_pipeline_impl.h (right):
> 
>
https://codereview.chromium.org/2361103002/diff/1/chromecast/media/cma/pipeli...
> chromecast/media/cma/pipeline/av_pipeline_impl.h:175: // Special weak factory
> used for asynchronous decryption. This allows us to
> This comment does not seem accurate. IIUC it does not cancel the actual
> decryption task but just drops the decryption completion callback. If this is
> true, I do not see how this patch helps because we already early-return if
> enable_feeding_ is false (which is set in Stop).

StartPlayingFrom() sets enable_feeding_ back to true. If the decryption callback
(from before Flush) completes after StartPlayingFrom() is called again, it will
think everything is fine and try to push, resulting in a double push.

[yucliu1, 2016-09-22 17:44:43]

On 2016/09/22 17:32:10, kmackay wrote:
> On 2016/09/22 17:25:18, alokp wrote:
> >
>
https://codereview.chromium.org/2361103002/diff/1/chromecast/media/cma/pipeli...
> > File chromecast/media/cma/pipeline/av_pipeline_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/2361103002/diff/1/chromecast/media/cma/pipeli...
> > chromecast/media/cma/pipeline/av_pipeline_impl.h:175: // Special weak
factory
> > used for asynchronous decryption. This allows us to
> > This comment does not seem accurate. IIUC it does not cancel the actual
> > decryption task but just drops the decryption completion callback. If this
is
> > true, I do not see how this patch helps because we already early-return if
> > enable_feeding_ is false (which is set in Stop).
> 
> StartPlayingFrom() sets enable_feeding_ back to true. If the decryption
callback
> (from before Flush) completes after StartPlayingFrom() is called again, it
will
> think everything is fine and try to push, resulting in a double push.

lgtm

[alokp, 2016-09-22 17:47:30]

On 2016/09/22 17:32:10, kmackay wrote:
> On 2016/09/22 17:25:18, alokp wrote:
> >
>
https://codereview.chromium.org/2361103002/diff/1/chromecast/media/cma/pipeli...
> > File chromecast/media/cma/pipeline/av_pipeline_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/2361103002/diff/1/chromecast/media/cma/pipeli...
> > chromecast/media/cma/pipeline/av_pipeline_impl.h:175: // Special weak
factory
> > used for asynchronous decryption. This allows us to
> > This comment does not seem accurate. IIUC it does not cancel the actual
> > decryption task but just drops the decryption completion callback. If this
is
> > true, I do not see how this patch helps because we already early-return if
> > enable_feeding_ is false (which is set in Stop).
> 
> StartPlayingFrom() sets enable_feeding_ back to true. If the decryption
callback
> (from before Flush) completes after StartPlayingFrom() is called again, it
will
> think everything is fine and try to push, resulting in a double push.

I see. Can you add this in a comment? A test would be nice too :)

[kmackay, 2016-09-22 18:36:53]

On 2016/09/22 17:47:30, alokp wrote:
> On 2016/09/22 17:32:10, kmackay wrote:
> > On 2016/09/22 17:25:18, alokp wrote:
> > >
> >
>
https://codereview.chromium.org/2361103002/diff/1/chromecast/media/cma/pipeli...
> > > File chromecast/media/cma/pipeline/av_pipeline_impl.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2361103002/diff/1/chromecast/media/cma/pipeli...
> > > chromecast/media/cma/pipeline/av_pipeline_impl.h:175: // Special weak
> factory
> > > used for asynchronous decryption. This allows us to
> > > This comment does not seem accurate. IIUC it does not cancel the actual
> > > decryption task but just drops the decryption completion callback. If this
> is
> > > true, I do not see how this patch helps because we already early-return if
> > > enable_feeding_ is false (which is set in Stop).
> > 
> > StartPlayingFrom() sets enable_feeding_ back to true. If the decryption
> callback
> > (from before Flush) completes after StartPlayingFrom() is called again, it
> will
> > think everything is fine and try to push, resulting in a double push.
> 
> I see. Can you add this in a comment? A test would be nice too :)

Added a more explanatory comment. I don't have bandwidth to add a unit test for
asynchronous decryption right now; maybe Yuchen could add one?

[kmackay, 2016-09-22 18:38:35]

Description was changed from

==========
[Chromecast] Make sure that Flush stops buffer decryption

AvPipelineImpl's Flush() call must stop buffer decryption before
calling the done callback.

BUG= internal b/31543110
==========

to

==========
[Chromecast] Make sure that Stop() stops buffer decryption

AvPipelineImpl's Stop() call must prevent any pending buffer decryption callback
from ever running. StartPlayingFrom() sets enable_feeding_ back to true, so if a
pending decryption callback from before Stop() is allowed to complete after
StartPlayingFrom() is called again, it will think everything is fine and try to
push a buffer, resulting in a double push.

BUG= internal b/31543110
==========

[kmackay, 2016-09-22 18:40:20]

Description was changed from

==========
[Chromecast] Make sure that Stop() stops buffer decryption

AvPipelineImpl's Stop() call must prevent any pending buffer decryption callback
from ever running. StartPlayingFrom() sets enable_feeding_ back to true, so if a
pending decryption callback from before Stop() is allowed to complete after
StartPlayingFrom() is called again, it will think everything is fine and try to
push a buffer, resulting in a double push.

BUG= internal b/31543110
==========

to

==========
[Chromecast] Make sure that Stop() stops buffer decryption

AvPipelineImpl's Stop() call must prevent any pending buffer decryption callback
from ever running. StartPlayingFrom() sets enable_feeding_ back to true, so if
a pending decryption callback from before Stop() is allowed to complete after
StartPlayingFrom() is called again, it will think everything is fine
and try to push a buffer, resulting in a double push.

BUG= internal b/31543110
==========

[alokp, 2016-09-22 18:59:09]

On 2016/09/22 18:36:53, kmackay wrote:
> On 2016/09/22 17:47:30, alokp wrote:
> > On 2016/09/22 17:32:10, kmackay wrote:
> > > On 2016/09/22 17:25:18, alokp wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2361103002/diff/1/chromecast/media/cma/pipeli...
> > > > File chromecast/media/cma/pipeline/av_pipeline_impl.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2361103002/diff/1/chromecast/media/cma/pipeli...
> > > > chromecast/media/cma/pipeline/av_pipeline_impl.h:175: // Special weak
> > factory
> > > > used for asynchronous decryption. This allows us to
> > > > This comment does not seem accurate. IIUC it does not cancel the actual
> > > > decryption task but just drops the decryption completion callback. If
this
> > is
> > > > true, I do not see how this patch helps because we already early-return
if
> > > > enable_feeding_ is false (which is set in Stop).
> > > 
> > > StartPlayingFrom() sets enable_feeding_ back to true. If the decryption
> > callback
> > > (from before Flush) completes after StartPlayingFrom() is called again, it
> > will
> > > think everything is fine and try to push, resulting in a double push.
> > 
> > I see. Can you add this in a comment? A test would be nice too :)
> 
> Added a more explanatory comment. I don't have bandwidth to add a unit test
for
> asynchronous decryption right now; maybe Yuchen could add one?

I might be able to get rid of AvPipelineImpl::Stop in the patch I am working on
right now. If I am able to do that, I will include this logic in my patch add a
test.

[alokp, 2016-09-22 22:10:40]

My patch is getting bigger than anticipated, which I am not confident checking
into 1.21. You should land this. I will rebase my change on top.

lgtm

[kmackay, 2016-09-22 22:48:47]

On 2016/09/22 22:10:40, alokp wrote:
> My patch is getting bigger than anticipated, which I am not confident checking
> into 1.21. You should land this. I will rebase my change on top.
> 
> lgtm

Thanks.

[kmackay, 2016-09-22 22:48:52]

The CQ bit was checked by kmackay@chromium.org

[kmackay, 2016-09-22 22:48:52]

The patchset sent to the CQ was uploaded after l-g-t-m from yucliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2361103002/#ps40001
(title: "fix formatting")

[commit-bot: I haz the power, 2016-09-22 22:49:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 22:59:50]

Description was changed from

==========
[Chromecast] Make sure that Stop() stops buffer decryption

AvPipelineImpl's Stop() call must prevent any pending buffer decryption callback
from ever running. StartPlayingFrom() sets enable_feeding_ back to true, so if
a pending decryption callback from before Stop() is allowed to complete after
StartPlayingFrom() is called again, it will think everything is fine
and try to push a buffer, resulting in a double push.

BUG= internal b/31543110
==========

to

==========
[Chromecast] Make sure that Stop() stops buffer decryption

AvPipelineImpl's Stop() call must prevent any pending buffer decryption callback
from ever running. StartPlayingFrom() sets enable_feeding_ back to true, so if
a pending decryption callback from before Stop() is allowed to complete after
StartPlayingFrom() is called again, it will think everything is fine
and try to push a buffer, resulting in a double push.

BUG= internal b/31543110
==========

[commit-bot: I haz the power, 2016-09-22 22:59:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-22 23:01:57]

Description was changed from

==========
[Chromecast] Make sure that Stop() stops buffer decryption

AvPipelineImpl's Stop() call must prevent any pending buffer decryption callback
from ever running. StartPlayingFrom() sets enable_feeding_ back to true, so if
a pending decryption callback from before Stop() is allowed to complete after
StartPlayingFrom() is called again, it will think everything is fine
and try to push a buffer, resulting in a double push.

BUG= internal b/31543110
==========

to

==========
[Chromecast] Make sure that Stop() stops buffer decryption

AvPipelineImpl's Stop() call must prevent any pending buffer decryption callback
from ever running. StartPlayingFrom() sets enable_feeding_ back to true, so if
a pending decryption callback from before Stop() is allowed to complete after
StartPlayingFrom() is called again, it will think everything is fine
and try to push a buffer, resulting in a double push.

BUG= internal b/31543110

Committed: https://crrev.com/5dd51b563246eed2fd0a8cdc7061169a664974f6
Cr-Commit-Position: refs/heads/master@{#420489}
==========

[commit-bot: I haz the power, 2016-09-22 23:01:58]

Patchset 3 (id:??) landed as
https://crrev.com/5dd51b563246eed2fd0a8cdc7061169a664974f6
Cr-Commit-Position: refs/heads/master@{#420489}