Notify demux stream clients when a stream is disabled.

Notify demux stream clients when a stream is disabled.

Currently DemuxerStream client are notified about the stream being
disabled by an EOS buffer being returned from the Read call. But that
is problematic on media pipelines with deep buffering (like Chromecast)
where the client would have to read and process all previous buffers
before getting the EOS buffer. In order to ensure that disabling a
demuxer stream happens instantly, we need to notify clients via the
same mechanism that is currently being used for notifying them about
stream being re-enabled. This change will make it so that the
corresponding renderer gets flushed as soon as the DemuxerStream is
disabled. And then the renderer is going to be restarted, will read
the EOS buffer from the stream and will go into the 'ended' state just
like before.

BUG=633299
Committed: https://crrev.com/af21ccc6ee855dda09fd437637759071b04439e6
Cr-Commit-Position: refs/heads/master@{#409941}

[servolk, 2016-08-01 18:54:19]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 18:55:01]

Dry run: 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-08-01 18:58:02]

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

[commit-bot: I haz the power, 2016-08-01 18:58:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[servolk, 2016-08-01 19:02:06]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 19:02:45]

Dry run: 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-08-01 19:15:14]

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

[commit-bot: I haz the power, 2016-08-01 19:15:15]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[servolk, 2016-08-01 22:30:57]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 22:31:56]

Dry run: 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-08-02 00:32:13]

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

[commit-bot: I haz the power, 2016-08-02 00:32:13]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-08-02 00:32:46]

Description was changed from

==========
Notify demux stream clients when a stream is disabled.

Currently DemuxerStream client are notified about the stream being
disabled by an EOS buffer being returned from the Read call. But that
is problematic on media pipelines with deep buffering (like Chromecast)
where the client would have to read and process all previous buffers
before getting the EOS buffer. In order to ensure that disabling a
demuxer stream happens instantly, we need to notify clients via the
same mechanism that is currently being used for notifying them about
stream being re-enabled. This change will make it so that the
corresponding renderer gets flushed as soon as the DemuxerStream is
disabled. And then the renderer is going to be restarted, will read
the EOS buffer from the stream and will go into the 'ended' state just
like before.

BUG=633299
==========

to

==========
Notify demux stream clients when a stream is disabled.

Currently DemuxerStream client are notified about the stream being
disabled by an EOS buffer being returned from the Read call. But that
is problematic on media pipelines with deep buffering (like Chromecast)
where the client would have to read and process all previous buffers
before getting the EOS buffer. In order to ensure that disabling a
demuxer stream happens instantly, we need to notify clients via the
same mechanism that is currently being used for notifying them about
stream being re-enabled. This change will make it so that the
corresponding renderer gets flushed as soon as the DemuxerStream is
disabled. And then the renderer is going to be restarted, will read
the EOS buffer from the stream and will go into the 'ended' state just
like before.

BUG=633299
==========

[servolk, 2016-08-02 00:32:46]

servolk@chromium.org changed reviewers:
+ chcunningham@chromium.org, xhwang@chromium.org

[servolk, 2016-08-03 00:00:08]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[chcunningham1, 2016-08-03 00:27:16]

chcunningham@google.com changed reviewers:
+ chcunningham@google.com

[chcunningham1, 2016-08-03 00:27:17]

Dale can you stand in for xhwang?

I think this change generally looks good, but I'd like some new unit tests to
confirm that the flush immediately happens on disable. I propose adding tests
for the following:

renderer test:
- verify that each of demuxer streams are provided the stream change callback 
- save that callback and setup mock ST when mock.set_enabled().Then(call the
callback)
- call set_enabled(false) on the mock demuxer stream
- verify that renderers get immediately flushed 

chunk demuxer stream test:
- set the callback
- disable the stream - verify callback comes through
- enable the stream - verify callback again comes through

[servolk, 2016-08-03 23:55:51]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 23:56:18]

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

[servolk, 2016-08-04 00:15:45]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 00:16:33]

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

[DaleCurtis, 2016-08-04 00:23:35]

Mechanically lg2m, but I don't see that you're adding any information to the
callback. If you're going to rename it to StreamStatusChangeCB you need to add
some status :)

[servolk, 2016-08-04 00:28:25]

On 2016/08/04 00:23:35, DaleCurtis wrote:
> Mechanically lg2m, but I don't see that you're adding any information to the
> callback. If you're going to rename it to StreamStatusChangeCB you need to add
> some status :)

That would be redundant, since we are already passing the DemuxerStream into the
CB, one can simply call DemuxerStream::enabled to check its current status.

[DaleCurtis, 2016-08-04 00:32:14]

OnStatusChange is pretty generic though; do you want this to only ever mean the
stream is either enabled or disabled? I feel like the current naming suggests
other status could be communicated; in which case it becomes a needle in the
haystack game :)

[servolk, 2016-08-04 00:36:59]

On 2016/08/04 00:32:14, DaleCurtis wrote:
> OnStatusChange is pretty generic though; do you want this to only ever mean
the
> stream is either enabled or disabled? I feel like the current naming suggests
> other status could be communicated; in which case it becomes a needle in the
> haystack game :)

I agree that OnStatusChange is a somewhat generic name and I'm open to better
naming suggestions. I've thought that StreamGotDisabledOrEnabled would be an
even worse name (although it's more clear in what it really means), so I've used
the StreamStatusChanged for now for the lack of better ideas on my side.

[DaleCurtis, 2016-08-04 00:49:33]

OnStreamAvailabilityChanged? Though that's a bit misleading since the stream is
still there, it's just disabled. Why is it a problem to return the status that
changed? Sometimes these types of methods even return the old status and new
status. In this case, just a bool enabled seems fine.

Is every client that receives this call going to go and lookup the status
immediately after? If so I think providing it immediately is a cleaner option.

[servolk, 2016-08-04 01:14:21]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[servolk, 2016-08-04 01:14:29]

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

[commit-bot: I haz the power, 2016-08-04 01:14:44]

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

[servolk, 2016-08-04 01:15:55]

On 2016/08/04 00:49:33, DaleCurtis wrote:
> OnStreamAvailabilityChanged? Though that's a bit misleading since the stream
is
> still there, it's just disabled. Why is it a problem to return the status that
> changed? Sometimes these types of methods even return the old status and new
> status. In this case, just a bool enabled seems fine.
> 
> Is every client that receives this call going to go and lookup the status
> immediately after? If so I think providing it immediately is a cleaner option.

Ok, I've added a bool parameter to the StreamStatusChangeCB and added comments
explaining each param.
+alokp@ for chromecast/

[DaleCurtis, 2016-08-04 01:17:39]

lgtm

[commit-bot: I haz the power, 2016-08-04 02:40:49]

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

[commit-bot: I haz the power, 2016-08-04 02:40:50]

Dry run: This issue passed the CQ dry run.

[alokp, 2016-08-04 19:02:37]

lgtm % nits

https://codereview.chromium.org/2199913002/diff/100001/media/base/demuxer_str...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/2199913002/diff/100001/media/base/demuxer_str...
media/base/demuxer_stream.h:110: using StreamStatusChangeCB =
nit: is it necessary for the callback to specify the DemuxerStream? Can't the
client bind the demuxer-stream locally when constructing the callback?

https://codereview.chromium.org/2199913002/diff/100001/media/renderers/render...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/2199913002/diff/100001/media/renderers/render...
media/renderers/renderer_impl.cc:218: bool enabled,
I do not see this new argument being used. Am I missing something?

https://codereview.chromium.org/2199913002/diff/100001/media/renderers/render...
media/renderers/renderer_impl.cc:253: DVLOG(3) << __func__;
any particular reason for changing the logging level?

https://codereview.chromium.org/2199913002/diff/100001/media/renderers/render...
media/renderers/renderer_impl.cc:256: DCHECK_EQ(state_, STATE_PLAYING);
nit: I usually prefer the expected values specified in the same order as
EXPECT_EQ

https://codereview.chromium.org/2199913002/diff/100001/media/renderers/render...
media/renderers/renderer_impl.cc:262: DVLOG(3) << __func__;
ditto

[servolk, 2016-08-04 20:31:36]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[servolk, 2016-08-04 20:31:48]

https://codereview.chromium.org/2199913002/diff/100001/media/base/demuxer_str...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/2199913002/diff/100001/media/base/demuxer_str...
media/base/demuxer_stream.h:110: using StreamStatusChangeCB =
On 2016/08/04 19:02:36, alokp wrote:
> nit: is it necessary for the callback to specify the DemuxerStream? Can't the
> client bind the demuxer-stream locally when constructing the callback?

Done.

https://codereview.chromium.org/2199913002/diff/100001/media/renderers/render...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/2199913002/diff/100001/media/renderers/render...
media/renderers/renderer_impl.cc:218: bool enabled,
On 2016/08/04 19:02:36, alokp wrote:
> I do not see this new argument being used. Am I missing something?

Yes, it is unused for now. I've added it per Dale suggestion in
https://codereview.chromium.org/2199913002/#msg27

https://codereview.chromium.org/2199913002/diff/100001/media/renderers/render...
media/renderers/renderer_impl.cc:253: DVLOG(3) << __func__;
On 2016/08/04 19:02:37, alokp wrote:
> any particular reason for changing the logging level?

No particular reason. It was more useful during the initial bring up of this
functionality, and now that it's more stable I think we can reduce the priority
to make the logs less verbose.

https://codereview.chromium.org/2199913002/diff/100001/media/renderers/render...
media/renderers/renderer_impl.cc:256: DCHECK_EQ(state_, STATE_PLAYING);
On 2016/08/04 19:02:36, alokp wrote:
> nit: I usually prefer the expected values specified in the same order as
> EXPECT_EQ

I have no strong opinion about which order is better in general, but in this
case I prefer it this way for consistency with other code in this file and
elsewhere (see
https://cs.chromium.org/search/?q=%22DCHECK_EQ(state_,+STATE_PLAYING)%22&sq=p...).

[commit-bot: I haz the power, 2016-08-04 20:32:04]

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

[servolk, 2016-08-04 20:32:55]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 20:33:22]

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

[servolk, 2016-08-04 22:18:00]

The CQ bit was unchecked by servolk@chromium.org

[servolk, 2016-08-04 22:18:01]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-08-04 22:18:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, alokp@chromium.org
Link to the patchset: https://codereview.chromium.org/2199913002/#ps120001
(title: "Don't pass DemuxerStream into the status callback")

[commit-bot: I haz the power, 2016-08-04 22:18:20]

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-08-05 00:19:18]

Description was changed from

==========
Notify demux stream clients when a stream is disabled.

Currently DemuxerStream client are notified about the stream being
disabled by an EOS buffer being returned from the Read call. But that
is problematic on media pipelines with deep buffering (like Chromecast)
where the client would have to read and process all previous buffers
before getting the EOS buffer. In order to ensure that disabling a
demuxer stream happens instantly, we need to notify clients via the
same mechanism that is currently being used for notifying them about
stream being re-enabled. This change will make it so that the
corresponding renderer gets flushed as soon as the DemuxerStream is
disabled. And then the renderer is going to be restarted, will read
the EOS buffer from the stream and will go into the 'ended' state just
like before.

BUG=633299
==========

to

==========
Notify demux stream clients when a stream is disabled.

Currently DemuxerStream client are notified about the stream being
disabled by an EOS buffer being returned from the Read call. But that
is problematic on media pipelines with deep buffering (like Chromecast)
where the client would have to read and process all previous buffers
before getting the EOS buffer. In order to ensure that disabling a
demuxer stream happens instantly, we need to notify clients via the
same mechanism that is currently being used for notifying them about
stream being re-enabled. This change will make it so that the
corresponding renderer gets flushed as soon as the DemuxerStream is
disabled. And then the renderer is going to be restarted, will read
the EOS buffer from the stream and will go into the 'ended' state just
like before.

BUG=633299
==========

[commit-bot: I haz the power, 2016-08-05 00:19:19]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-05 00:21:09]

Description was changed from

==========
Notify demux stream clients when a stream is disabled.

Currently DemuxerStream client are notified about the stream being
disabled by an EOS buffer being returned from the Read call. But that
is problematic on media pipelines with deep buffering (like Chromecast)
where the client would have to read and process all previous buffers
before getting the EOS buffer. In order to ensure that disabling a
demuxer stream happens instantly, we need to notify clients via the
same mechanism that is currently being used for notifying them about
stream being re-enabled. This change will make it so that the
corresponding renderer gets flushed as soon as the DemuxerStream is
disabled. And then the renderer is going to be restarted, will read
the EOS buffer from the stream and will go into the 'ended' state just
like before.

BUG=633299
==========

to

==========
Notify demux stream clients when a stream is disabled.

Currently DemuxerStream client are notified about the stream being
disabled by an EOS buffer being returned from the Read call. But that
is problematic on media pipelines with deep buffering (like Chromecast)
where the client would have to read and process all previous buffers
before getting the EOS buffer. In order to ensure that disabling a
demuxer stream happens instantly, we need to notify clients via the
same mechanism that is currently being used for notifying them about
stream being re-enabled. This change will make it so that the
corresponding renderer gets flushed as soon as the DemuxerStream is
disabled. And then the renderer is going to be restarted, will read
the EOS buffer from the stream and will go into the 'ended' state just
like before.

BUG=633299

Committed: https://crrev.com/af21ccc6ee855dda09fd437637759071b04439e6
Cr-Commit-Position: refs/heads/master@{#409941}
==========

[commit-bot: I haz the power, 2016-08-05 00:21:11]

Patchset 7 (id:??) landed as
https://crrev.com/af21ccc6ee855dda09fd437637759071b04439e6
Cr-Commit-Position: refs/heads/master@{#409941}