Implement disabling and enabling media tracks

Implement disabling and enabling media tracks

This CL implements disabling and enabling audio and video streams via
Javascript media tracks API. When a media track is disabled, the
corresponding DemuxerStream generates an EOS notification, thus ending
further decoding/rendering of that stream. When a media track is
enabled, the corresponding DemuxerStream is seeked to the current
playback position and we notify the renderers to resume reading from
the demuxer stream and rendering its contents.

BUG=249427
Committed: https://crrev.com/f25ceedfcb1e403c96eb80702cd29aa1dc370619
Cr-Commit-Position: refs/heads/master@{#403403}

[servolk, 2016-05-12 22:04:22]

Description was changed from

==========
Implement disabling/enabling video stream.
==========

to

==========
Implement disabling and enabling media tracks

This CL implements disabling and enabling audio and video streams via
Javascript media tracks API. When a media track is disabled, the
corresponding DemuxerStream generates an EOS notification, thus ending
further decoding/rendering of that stream. When a media track is
enabled, the corresponding DemuxerStream is seeked to the current
playback position and we notify the renderers to resume reading from
the demuxer stream and rendering its contents.

BUG=249427
==========

[servolk, 2016-05-12 22:04:23]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, wolenetz@chromium.org, xhwang@chromium.org

[wolenetz, 2016-05-26 16:02:54]

I'm offsite at a w3c meeting today, but I'll do a pass on this sometime before
midnight :)

[servolk, 2016-05-27 20:28:14]

On 2016/05/26 16:02:54, wolenetz_slow_thursday_may_26 wrote:
> I'm offsite at a w3c meeting today, but I'll do a pass on this sometime before
> midnight :)

Matt, did you get a chance to look at this?

[wolenetz, 2016-05-27 20:29:35]

On 2016/05/27 20:28:14, servolk wrote:
> On 2016/05/26 16:02:54, wolenetz_slow_thursday_may_26 wrote:
> > I'm offsite at a w3c meeting today, but I'll do a pass on this sometime
before
> > midnight :)
> 
> Matt, did you get a chance to look at this?

Sorry - I'm looking now.

[wolenetz, 2016-05-27 20:49:42]

Some initial clarifying questions. More deep CR to follow.

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

https://codereview.chromium.org/1935873002/diff/380001/media/base/demuxer_str...
media/base/demuxer_stream.h:98: // again after that will yield a kEnabled result
if the stream is later
nit: IIUC, s/When a stream is disabled/When a stream is set to disabled/ and
s/will yield/will eventually yield either/

https://codereview.chromium.org/1935873002/diff/380001/media/base/demuxer_str...
media/base/demuxer_stream.h:99: // reenabled, or end_of_stream buffer if stream
ends without being reenabled.
nit: How precisely is it determined that a stream ends, especially if it is
disabled? I assume you mean one of the following, but they have different
implications:
1) MediaSource.endOfStream(...) has occurred, regardless of last buffer read
position or current media playback time.
2) MediaSource.endOfStream(...) has occurred, and the last read position from
the disabled stream (from when it was enabled) was the last buffered coded
frame.
3) Or something else?

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

https://codereview.chromium.org/1935873002/diff/380001/media/renderers/render...
media/renderers/renderer_impl.cc:224: // StartPlaying that dchecks we are not in
playing state.
nit: we usually don't refer to implementation detail ("that dchecks"), rather
something like "that requires that"

[wolenetz, 2016-05-27 21:02:37]

wolenetz@chromium.org changed reviewers:
+ chcunningham@chromium.org

[wolenetz, 2016-05-27 21:02:38]

To improve our upstream turnaround on servolk@'s CRs while I'm engaged in
driving MSE v1 spec to completion in the short-term:
+chcunningham@ and xhwang@: Would you please take a look at this CL and provide
comments?  See also a design doc for this particular change that servolk@
provided to us earlier in this CL comment chain.

[servolk, 2016-05-27 21:23:28]

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

https://codereview.chromium.org/1935873002/diff/380001/media/base/demuxer_str...
media/base/demuxer_stream.h:99: // reenabled, or end_of_stream buffer if stream
ends without being reenabled.
On 2016/05/27 20:49:42, wolenetz_slow_reviews wrote:
> nit: How precisely is it determined that a stream ends, especially if it is
> disabled? I assume you mean one of the following, but they have different
> implications:
> 1) MediaSource.endOfStream(...) has occurred, regardless of last buffer read
> position or current media playback time.
> 2) MediaSource.endOfStream(...) has occurred, and the last read position from
> the disabled stream (from when it was enabled) was the last buffered coded
> frame.
> 3) Or something else?

Oh, sorry this comment was out of date, I've just updated it. This CL actually
implements the approach that we discussed - when a stream is disabled it will
start returning EOS. So it becomes indistinguishable from a regular ended
stream. If a stream becomes re-enabled before playback of other remaining
streams finishes, then reading from it again will resume returning buffers from
approximately |timestamp| position.

[servolk, 2016-06-01 17:36:16]

Ping. Chris, Xiaohan any comments?

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

https://codereview.chromium.org/1935873002/diff/380001/media/renderers/render...
media/renderers/renderer_impl.cc:224: // StartPlaying that dchecks we are not in
playing state.
On 2016/05/27 20:49:42, wolenetz_slow_reviews wrote:
> nit: we usually don't refer to implementation detail ("that dchecks"), rather
> something like "that requires that"

Done.

[xhwang, 2016-06-02 20:35:11]

Sorry for the delay. I haven't been following this work recently (was fighting
with another P0 task). I'll start with your design doc and hopefully will
provide feedback by EOW.

[chcunningham, 2016-06-02 21:25:54]

I've reviewed the base CL with comments. Will start on this one now

[chcunningham, 2016-06-02 23:49:43]

Some initial comments. Still working my way through

https://codereview.chromium.org/1935873002/diff/440001/media/base/fake_demuxe...
File media/base/fake_demuxer_stream.cc (right):

https://codereview.chromium.org/1935873002/diff/440001/media/base/fake_demuxe...
media/base/fake_demuxer_stream.cc:111: void FakeDemuxerStream::set_enabled(bool
enabled, base::TimeDelta timestamp) {}
ditto for the other fakes

https://codereview.chromium.org/1935873002/diff/440001/media/base/fake_demuxe...
media/base/fake_demuxer_stream.cc:111: void FakeDemuxerStream::set_enabled(bool
enabled, base::TimeDelta timestamp) {}
NOTIMPLEMENTED()

https://codereview.chromium.org/1935873002/diff/440001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1935873002/diff/440001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:612: media_task_runner_->PostTask(
This is bit bouncy. 

Main Thread:   JS sets enabled on stream
Media Thread: calls set_enabled on demuxer_stream
Main Thread:   Call pipeline_->RestartStreamPlayback
Media Thread: Pipeline calls renderer->RestartStreamPlayback

...

Could we instead push more of the work to the pipeline and make just one post
from main -> media thread?

What if Pipeline::RestartStreamPlayback() instead became
Pipeline::OnEnableTracksChanged()? Maybe the name could be better (since video
is onSelectedTrackChanged), but the idea is pass the type, selected/enabled
id(s), and let pipeline post the task to media thread to set the enabled states
on the appropriate demuxer streams and restart renderers if needed. 

Also, pipeline can call its own GetMediaTime once on the media thread which
should improve its accuracy a little.

https://codereview.chromium.org/1935873002/diff/440001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1935873002/diff/440001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:691: void FFmpegDemuxerStream::set_enabled(bool
enabled, base::TimeDelta timestamp) {
I feel like we need some logic here to drop frames after we enable until we
observe the first key frame. I get that we have one file position, so we cant
seek like we did in chunk demuxer.

[chcunningham, 2016-06-03 01:38:50]

Finished my review. I'm pretty clueless on the mojo stuff, so defer to xhwang.

https://codereview.chromium.org/1935873002/diff/440001/media/mojo/services/mo...
File media/mojo/services/mojo_demuxer_stream_adapter.h (right):

https://codereview.chromium.org/1935873002/diff/440001/media/mojo/services/mo...
media/mojo/services/mojo_demuxer_stream_adapter.h:80: bool enabled_ = true;
This class' style is to set defaults in the ctor

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

https://codereview.chromium.org/1935873002/diff/440001/media/renderers/render...
media/renderers/renderer_impl.cc:557: } else if (new_buffering_state ==
BUFFERING_HAVE_ENOUGH) {
What if the new buffering state never reaches HAVE_ENOUGH? If MSE apps decide to
not bother appending video while the video track is disabled, but then later
enable the track, we should probably signal that we don't have any video data at
the current time. The same argument could be made for audio.

I probably haven't thought about this enough, but one option would be to
recognize the HAVE_NOTHING as potentially legit, but defer surfacing this via
deferred post task (like we do when video underflows below). Cancel the posting
of the task if the buffering state comes in as HAVE_ENOUGH under the threshold.

https://codereview.chromium.org/1935873002/diff/440001/media/renderers/render...
media/renderers/renderer_impl.cc:563: video_renderer_->OnTimeStateChanged(true);
Not really related to this line, but it triggered a thought...

I think we should make video renderer call OnTimeStateChanged(false) on itself
when it observes an EOS. Or we could do it in the renderer_impl when video
renderer signals its playback has ended. As it stands, I think the video sink's
timer will continue firing in spite of the track being disabled.

https://codereview.chromium.org/1935873002/diff/440001/media/renderers/render...
media/renderers/renderer_impl.cc:583: SetPlaybackRate(0);
Why does this help sync? I think you're already paused because you stopped
ticking prior to flush. I think you just need to call
time_source_->SetMediaTime() in OnAudioRendererFlushDone_ForRestart, and then
here you just need to StartTicking.

[servolk, 2016-06-03 02:53:15]

https://codereview.chromium.org/1935873002/diff/440001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1935873002/diff/440001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:612: media_task_runner_->PostTask(
On 2016/06/02 23:49:42, chcunningham wrote:
> This is bit bouncy. 
> 
> Main Thread:   JS sets enabled on stream
> Media Thread: calls set_enabled on demuxer_stream
> Main Thread:   Call pipeline_->RestartStreamPlayback
> Media Thread: Pipeline calls renderer->RestartStreamPlayback
> 
> ...
> 
> Could we instead push more of the work to the pipeline and make just one post
> from main -> media thread?
> 
> What if Pipeline::RestartStreamPlayback() instead became
> Pipeline::OnEnableTracksChanged()? Maybe the name could be better (since video
> is onSelectedTrackChanged), but the idea is pass the type, selected/enabled
> id(s), and let pipeline post the task to media thread to set the enabled
states
> on the appropriate demuxer streams and restart renderers if needed. 
> 
> Also, pipeline can call its own GetMediaTime once on the media thread which
> should improve its accuracy a little. 

Yes, we could do what you suggested, but one significant downside that I can
foresee is that we would either need to introduce external/blink track ids into
media::Pipeline public interface, probably not a good idea, since IIUC
blink-specific stuff is limited to media/blink/.
Or we'd need to find an alternative way of somehow delivering the information
from WMPI to Pipeline about which tracks exactly got enabled or disabled. I
guess we could cheat a little and instead of actually checking the track ids
given to us from blink level, we could just assume that there's max 1 audio and
1 video track, and then we can just check if audio |enabledTrackIds| vector or
video |selectedTrackId| are empty or not and then ask media::Pipeline to enable
or disable audio or video track without explicitly giving its blink id to
media::Pipeline. This solution would work (and would make the other CL that
implements mapping between blink track ids and demuxer streams unnecessary). But
it would require significant rework if we ever want to support multiple audio or
video tracks. I'm not sure if we want to go down that route. WDYT?

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

https://codereview.chromium.org/1935873002/diff/440001/media/renderers/render...
media/renderers/renderer_impl.cc:557: } else if (new_buffering_state ==
BUFFERING_HAVE_ENOUGH) {
On 2016/06/03 01:38:49, chcunningham wrote:
> What if the new buffering state never reaches HAVE_ENOUGH? If MSE apps decide
to
> not bother appending video while the video track is disabled, but then later
> enable the track, we should probably signal that we don't have any video data
at
> the current time. The same argument could be made for audio.
> 
> I probably haven't thought about this enough, but one option would be to
> recognize the HAVE_NOTHING as potentially legit, but defer surfacing this via
> deferred post task (like we do when video underflows below). Cancel the
posting
> of the task if the buffering state comes in as HAVE_ENOUGH under the
threshold.

If MSE apps decide to not bother appending video then they can already do that
today without even disabling the video track explicitly. That would result in
stalled playback of course, but that's an application bug and we can't prevent
them from doing that. So I think we don't need to be worried about this here.

https://codereview.chromium.org/1935873002/diff/440001/media/renderers/render...
media/renderers/renderer_impl.cc:563: video_renderer_->OnTimeStateChanged(true);
On 2016/06/03 01:38:49, chcunningham wrote:
> Not really related to this line, but it triggered a thought...
> 
> I think we should make video renderer call OnTimeStateChanged(false) on itself
> when it observes an EOS. Or we could do it in the renderer_impl when video
> renderer signals its playback has ended. As it stands, I think the video
sink's
> timer will continue firing in spite of the track being disabled.

Yes, it sounds reasonable to me. I'm not sure why that hasn't been done yet,
perhaps there are reasons why we need to keep ticking. I'll let Xiaohan or Matt
comment on this.

[chcunningham, 2016-06-03 20:54:59]

https://codereview.chromium.org/1935873002/diff/440001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1935873002/diff/440001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:612: media_task_runner_->PostTask(
Let me see how our discussion here shakes out

https://codereview.chromium.org/1922333002/diff/220001/media/blink/webmediapl...

Perhaps we'll just use bytstream track ids everywhere, which would address your
concern with this proposal.

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

https://codereview.chromium.org/1935873002/diff/440001/media/renderers/render...
media/renderers/renderer_impl.cc:557: } else if (new_buffering_state ==
BUFFERING_HAVE_ENOUGH) {
I don't think its a matter of worrying just to help the app devs - its more that
I think we need to do this to meet the spec (which hopefully helps app devs
too). The spec would have us change our readyState to less-than HAVE_ENOUGH
whenever we don't HAVE_ENOUGH. 

IIUC, the current code would actually *not* stall playback in the scenario where
the track is re-enabled without having data buffered. This is asymmetric
behavior from the case you described where they leave the track enabled the
whole time but never append. I think the behavior of just-enabled-tracks should
match that of always-enabled-tracks.

[chcunningham, 2016-06-03 20:55:37]

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

https://codereview.chromium.org/1935873002/diff/440001/media/renderers/render...
media/renderers/renderer_impl.cc:583: SetPlaybackRate(0);
(reminder about this one in case you missed it)

[xhwang, 2016-06-04 00:09:43]

Thanks for doing this and sorry for the delay.

A few random nits I had while looking at the code. Then one big comment about
some ideas I had. Let's think about it more and see what's the best way to do
this.

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/base/demu...
File media/base/demuxer_stream.h (right):

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/base/demu...
media/base/demuxer_stream.h:97: virtual bool enabled() const = 0;
nit: add empty line

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/base/pipe...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/base/pipe...
media/base/pipeline.h:81: base::TimeDelta time) = 0;
What if our current media time is at t1, and |time| != t1. What is the Pipeline
supposed to do?

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/blink/web...
File media/blink/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/blink/web...
media/blink/webmediaplayer_impl.cc:612: media_task_runner_->PostTask(
On 2016/06/03 02:53:15, servolk wrote:
> On 2016/06/02 23:49:42, chcunningham wrote:
> > This is bit bouncy. 
> > 
> > Main Thread:   JS sets enabled on stream
> > Media Thread: calls set_enabled on demuxer_stream
> > Main Thread:   Call pipeline_->RestartStreamPlayback
> > Media Thread: Pipeline calls renderer->RestartStreamPlayback
> > 
> > ...
> > 
> > Could we instead push more of the work to the pipeline and make just one
post
> > from main -> media thread?
> > 
> > What if Pipeline::RestartStreamPlayback() instead became
> > Pipeline::OnEnableTracksChanged()? Maybe the name could be better (since
video
> > is onSelectedTrackChanged), but the idea is pass the type, selected/enabled
> > id(s), and let pipeline post the task to media thread to set the enabled
> states
> > on the appropriate demuxer streams and restart renderers if needed. 
> > 
> > Also, pipeline can call its own GetMediaTime once on the media thread which
> > should improve its accuracy a little. 
> 
> Yes, we could do what you suggested, but one significant downside that I can
> foresee is that we would either need to introduce external/blink track ids
into
> media::Pipeline public interface, probably not a good idea, since IIUC
> blink-specific stuff is limited to media/blink/.
> Or we'd need to find an alternative way of somehow delivering the information
> from WMPI to Pipeline about which tracks exactly got enabled or disabled. I
> guess we could cheat a little and instead of actually checking the track ids
> given to us from blink level, we could just assume that there's max 1 audio
and
> 1 video track, and then we can just check if audio |enabledTrackIds| vector or
> video |selectedTrackId| are empty or not and then ask media::Pipeline to
enable
> or disable audio or video track without explicitly giving its blink id to
> media::Pipeline. This solution would work (and would make the other CL that
> implements mapping between blink track ids and demuxer streams unnecessary).
But
> it would require significant rework if we ever want to support multiple audio
or
> video tracks. I'm not sure if we want to go down that route. WDYT?

For the thread hopping chcunningham pointed out, we don't really call demuxer
directly in WMPI, nor do we deal with the media_task_runner_ directly (the only
exception is SetSinkIdOnMediaThread() which I need to take another look). It
seems to me we should notify the Demuxer through PipelineController, similar to
how we do Seek().

Then you can probably just call something like demuxer->set_enabled(track_id,
enabled). (I am proposing to get rid of GetDemuxerStreamByTrackId(), see below.)

Then it would be nice if we could avoid the round trip of
OnSelectedVideoTrackChanged -> GetDemuxerStreamByTrackId ->
restart_stream_playback_cb -> RestartStreamPlayback. 

Thinking wildly, I wonder whether this can be solved mostly in the demuxer. For
example, we could have some like:

DemuxerStream::SetEnableNotification(cb);

Then in VideoRendererImpl:
demuxer_stream_->SetEnableDisableNotification(&OnDemuxerStreamEnabled);

void OnDemuxerStreamEnabled() {
  StartResumePlayback();
}

Then you don't have to go through the pipeline and renderer to resume playback.
When you enable the demuxer stream/track, the corresponding renderer will be
notified.

I haven't thought about all the details. Just something for us to think about...

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/renderers...
File media/renderers/renderer_impl.h (right):

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/renderers...
media/renderers/renderer_impl.h:107: void
OnAudioRendererFlushDone_ForRestart(base::TimeDelta time);
nit: We don't use underscore in function names.

[servolk, 2016-06-09 21:00:13]

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/base/demu...
File media/base/demuxer_stream.h (right):

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/base/demu...
media/base/demuxer_stream.h:97: virtual bool enabled() const = 0;
On 2016/06/04 00:09:43, xhwang wrote:
> nit: add empty line

Done.

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/base/pipe...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/base/pipe...
media/base/pipeline.h:81: base::TimeDelta time) = 0;
On 2016/06/04 00:09:43, xhwang wrote:
> What if our current media time is at t1, and |time| != t1. What is the
Pipeline
> supposed to do?

Well, I suppose if we move the 'resume playback' logic deeper into
PipelineController or Pipeline as discussed in other comments, then we won't
need to pass |time| here explicitly and can pass just a set of track ids and
their status.

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/blink/web...
File media/blink/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/1935873002/diff/440001/media/blink/web...
media/blink/webmediaplayer_impl.cc:612: media_task_runner_->PostTask(
On 2016/06/04 00:09:43, xhwang wrote:
> On 2016/06/03 02:53:15, servolk wrote:
> > On 2016/06/02 23:49:42, chcunningham wrote:
> > > This is bit bouncy. 
> > > 
> > > Main Thread:   JS sets enabled on stream
> > > Media Thread: calls set_enabled on demuxer_stream
> > > Main Thread:   Call pipeline_->RestartStreamPlayback
> > > Media Thread: Pipeline calls renderer->RestartStreamPlayback
> > > 
> > > ...
> > > 
> > > Could we instead push more of the work to the pipeline and make just one
> post
> > > from main -> media thread?
> > > 
> > > What if Pipeline::RestartStreamPlayback() instead became
> > > Pipeline::OnEnableTracksChanged()? Maybe the name could be better (since
> video
> > > is onSelectedTrackChanged), but the idea is pass the type,
selected/enabled
> > > id(s), and let pipeline post the task to media thread to set the enabled
> > states
> > > on the appropriate demuxer streams and restart renderers if needed. 
> > > 
> > > Also, pipeline can call its own GetMediaTime once on the media thread
which
> > > should improve its accuracy a little. 
> > 
> > Yes, we could do what you suggested, but one significant downside that I can
> > foresee is that we would either need to introduce external/blink track ids
> into
> > media::Pipeline public interface, probably not a good idea, since IIUC
> > blink-specific stuff is limited to media/blink/.
> > Or we'd need to find an alternative way of somehow delivering the
information
> > from WMPI to Pipeline about which tracks exactly got enabled or disabled. I
> > guess we could cheat a little and instead of actually checking the track ids
> > given to us from blink level, we could just assume that there's max 1 audio
> and
> > 1 video track, and then we can just check if audio |enabledTrackIds| vector
or
> > video |selectedTrackId| are empty or not and then ask media::Pipeline to
> enable
> > or disable audio or video track without explicitly giving its blink id to
> > media::Pipeline. This solution would work (and would make the other CL that
> > implements mapping between blink track ids and demuxer streams unnecessary).
> But
> > it would require significant rework if we ever want to support multiple
audio
> or
> > video tracks. I'm not sure if we want to go down that route. WDYT?
> 
> For the thread hopping chcunningham pointed out, we don't really call demuxer
> directly in WMPI, nor do we deal with the media_task_runner_ directly (the
only
> exception is SetSinkIdOnMediaThread() which I need to take another look). It
> seems to me we should notify the Demuxer through PipelineController, similar
to
> how we do Seek().
> 
> Then you can probably just call something like demuxer->set_enabled(track_id,
> enabled). (I am proposing to get rid of GetDemuxerStreamByTrackId(), see
below.)
> 
> Then it would be nice if we could avoid the round trip of
> OnSelectedVideoTrackChanged -> GetDemuxerStreamByTrackId ->
> restart_stream_playback_cb -> RestartStreamPlayback. 
> 
> Thinking wildly, I wonder whether this can be solved mostly in the demuxer.
For
> example, we could have some like:
> 
> DemuxerStream::SetEnableNotification(cb);
> 
> Then in VideoRendererImpl:
> demuxer_stream_->SetEnableDisableNotification(&OnDemuxerStreamEnabled);
> 
> void OnDemuxerStreamEnabled() {
>   StartResumePlayback();
> }
> 
> Then you don't have to go through the pipeline and renderer to resume
playback.
> When you enable the demuxer stream/track, the corresponding renderer will be
> notified.
> 
> I haven't thought about all the details. Just something for us to think
about...

There is a lot of different points here, let me try to address them in order
(can't find an easy way to quote here):

1. Yes, I guess we could go through pipeline controller instead of calling
demuxer directly in WMPI, that should be a relatively minor change, I'll give it
a try.

2. I'm not sure getting rid of GetDemuxerStreamByTrackId is a good idea (or
perhaps I just don't fully understand what you are suggesting instead). We get
media stack status notifications in blink as a set of media track ids. We need
to translate those ids into demuxerstreams somewhere. So I see two possible
options:
a. Pass the track ids sets into demuxer and make it do the translations
internally - is that what you are suggesting? I.e. we could add
Demuxer::OnEnabledAudioStreamChanged(<set of ids>) /
OnSelectedVideoStreamChanged(<id>) ? But that would mean we still introduce two
new functions into the public Demuxer interface. Those functions would operate
with track ids, but would not allow you to get access to demuxer streams by
knowing the track ids, which would make the usefulness of those functions very
limited (you wouldn't be able to do anything else with media track ids, only
enable/disable them).
b. (what's currently implemented in this CL) We could have a generic function in
the demuxer that would allow you to translate media track ids into demuxer
streams. Then you could do whatever you choose with those streams, you could
disable/enable them + you could also figure out other demuxer stream/media track
properties by knowing just the media track id (track type, liveness, a/v decoder
configs, etc). I think that would be much more useful function to have in the
public demuxer API.

3. Re your last point. We cannot avoid going at least through RendererImpl,
because we need to notify RendererImpl that the stream playback is being
restarted and that stream should be no longer considered 'ended'. That's why I
chose to implement it this way - it's already easy to recover A/VRendererImpls
from the 'ended' state by using their public APIs by just calling Flush +
StartPlaying(From), no further changes are necessary to the A/V renderers. But
there is currently no direct way to recover the media::RendererImpl from playing
just one stream (while the other one has been disabled/ended) back to playing
two streams.
And since we need to make some changes to the media::RendererImpl in any case,
and media::Renderer is already accessible from media::Pipeline(Controller), we
could propage the necessary changes through the pipeline(controller).
I guess we could make it slightly differently, along the lines of what you
described. I.e. introduce DemuxerStream::OnEnabledNotification, then in the
RendererImpl subscribe to those notifications through
RendererImpl::demuxer_stream_provider_, and when the media track status changes
we could enable/disable individual streams in the pipeline(controller) without
directly notifying media::Renderer. But I'm not sure how that would be better
than the current implementation.

[servolk, 2016-06-16 21:01:54]

> > 
> > Then you can probably just call something like
demuxer->set_enabled(track_id,
> > enabled). (I am proposing to get rid of GetDemuxerStreamByTrackId(), see
> below.)
> > 
> > Then it would be nice if we could avoid the round trip of
> > OnSelectedVideoTrackChanged -> GetDemuxerStreamByTrackId ->
> > restart_stream_playback_cb -> RestartStreamPlayback. 
> > 
> > Thinking wildly, I wonder whether this can be solved mostly in the demuxer.
> For
> > example, we could have some like:
> > 
> > DemuxerStream::SetEnableNotification(cb);
> > 
> > Then in VideoRendererImpl:
> > demuxer_stream_->SetEnableDisableNotification(&OnDemuxerStreamEnabled);
> > 
> > void OnDemuxerStreamEnabled() {
> >   StartResumePlayback();
> > }
> > 
> > Then you don't have to go through the pipeline and renderer to resume
> playback.
> > When you enable the demuxer stream/track, the corresponding renderer will be
> > notified.
> > 
> > I haven't thought about all the details. Just something for us to think
> about...
> 
> There is a lot of different points here, let me try to address them in order
> (can't find an easy way to quote here):
> 
> 1. Yes, I guess we could go through pipeline controller instead of calling
> demuxer directly in WMPI, that should be a relatively minor change, I'll give
it
> a try.
> 
> 2. I'm not sure getting rid of GetDemuxerStreamByTrackId is a good idea (or
> perhaps I just don't fully understand what you are suggesting instead). We get
> media stack status notifications in blink as a set of media track ids. We need
> to translate those ids into demuxerstreams somewhere. So I see two possible
> options:
> a. Pass the track ids sets into demuxer and make it do the translations
> internally - is that what you are suggesting? I.e. we could add
> Demuxer::OnEnabledAudioStreamChanged(<set of ids>) /
> OnSelectedVideoStreamChanged(<id>) ? But that would mean we still introduce
two
> new functions into the public Demuxer interface. Those functions would operate
> with track ids, but would not allow you to get access to demuxer streams by
> knowing the track ids, which would make the usefulness of those functions very
> limited (you wouldn't be able to do anything else with media track ids, only
> enable/disable them).
> b. (what's currently implemented in this CL) We could have a generic function
in
> the demuxer that would allow you to translate media track ids into demuxer
> streams. Then you could do whatever you choose with those streams, you could
> disable/enable them + you could also figure out other demuxer stream/media
track
> properties by knowing just the media track id (track type, liveness, a/v
decoder
> configs, etc). I think that would be much more useful function to have in the
> public demuxer API.
> 
> 3. Re your last point. We cannot avoid going at least through RendererImpl,
> because we need to notify RendererImpl that the stream playback is being
> restarted and that stream should be no longer considered 'ended'. That's why I
> chose to implement it this way - it's already easy to recover A/VRendererImpls
> from the 'ended' state by using their public APIs by just calling Flush +
> StartPlaying(From), no further changes are necessary to the A/V renderers. But
> there is currently no direct way to recover the media::RendererImpl from
playing
> just one stream (while the other one has been disabled/ended) back to playing
> two streams.
> And since we need to make some changes to the media::RendererImpl in any case,
> and media::Renderer is already accessible from media::Pipeline(Controller), we
> could propage the necessary changes through the pipeline(controller).
> I guess we could make it slightly differently, along the lines of what you
> described. I.e. introduce DemuxerStream::OnEnabledNotification, then in the
> RendererImpl subscribe to those notifications through
> RendererImpl::demuxer_stream_provider_, and when the media track status
changes
> we could enable/disable individual streams in the pipeline(controller) without
> directly notifying media::Renderer. But I'm not sure how that would be better
> than the current implementation.

Ok, now that https://codereview.chromium.org/2050043002 (which was a
prerequisite for this) has landed, let's get back to this one. I've rebased it
to the current ToT and made a few changes to address previous feedback from
xhwang@ and chcunningham@:
1. I've moved the logic for actually enabling/disabling demuxer streams deeper
into media::Pipeline, so we no longer access |demuxer_| directly from WMPI (and
no longer jump hop on different threads excessively), and instead in
WMPI::onA/Vtrackschanged we'll just convert track ids from blink::WebStrings
into MediaTrack::Ids and pass that down further to the media::Pipeline which
will deal with the demuxer. I have considered going through PipelineController,
but for now decided not to do it, as it don't seem to make any sense. IIUC
PipelineController still runs on the WMPI thread, whereas we want to access
demuxer streams on the media thread. In fact now that alokp@ split the
PipelineImpl into PipelineImpl which runs on WMPI thread and RendererWrapper
that runs on the media thread, perhaps it would make sense to merge
PipelineController with PipelineImpl, as now both of them run on WMPI thread.
2. I've added a logic in FFmpegDemuxerStream to enter a |waiting_for_keyframe_|
mode when the stream is re-enabled after being disabled. That will ensure that
the next buffer that renderers will see from that DemuxerStream is a keyframe. I
haven't changed the logic in the ChunkDemuxerStream yet, since for ChunkDemuxer
case we do an explicit Seek on the stream and that ensures that reading starts
with a key frame. Although that means for now there's a difference. The FFmpeg
case will resume playback from the next keyframe after the current playback
position, whereas MSE/ChunkDemuxer case will pick up from the _previous_
keyframe before the current playback position and will try to catch up with the
playback. I'm pondering if I should change the MSE case as well for consistency
with FFmpeg.

Also, I haven't made any significant further changes yet, as I've explained in
the previous comment I'm not sure if it would make more sense to provide
enabled/disabled notifications through the DemuxerStream interface instead of
simply giving that information from the PipelineImpl to the renderer (see point
#3 in my previous comment).

[wolenetz, 2016-06-16 22:20:53]

On 2016/06/16 21:01:54, servolk wrote:
> > > 
> > > Then you can probably just call something like
> demuxer->set_enabled(track_id,
> > > enabled). (I am proposing to get rid of GetDemuxerStreamByTrackId(), see
> > below.)
> > > 
> > > Then it would be nice if we could avoid the round trip of
> > > OnSelectedVideoTrackChanged -> GetDemuxerStreamByTrackId ->
> > > restart_stream_playback_cb -> RestartStreamPlayback. 
> > > 
> > > Thinking wildly, I wonder whether this can be solved mostly in the
demuxer.
> > For
> > > example, we could have some like:
> > > 
> > > DemuxerStream::SetEnableNotification(cb);
> > > 
> > > Then in VideoRendererImpl:
> > > demuxer_stream_->SetEnableDisableNotification(&OnDemuxerStreamEnabled);
> > > 
> > > void OnDemuxerStreamEnabled() {
> > >   StartResumePlayback();
> > > }
> > > 
> > > Then you don't have to go through the pipeline and renderer to resume
> > playback.
> > > When you enable the demuxer stream/track, the corresponding renderer will
be
> > > notified.
> > > 
> > > I haven't thought about all the details. Just something for us to think
> > about...
> > 
> > There is a lot of different points here, let me try to address them in order
> > (can't find an easy way to quote here):
> > 
> > 1. Yes, I guess we could go through pipeline controller instead of calling
> > demuxer directly in WMPI, that should be a relatively minor change, I'll
give
> it
> > a try.
> > 
> > 2. I'm not sure getting rid of GetDemuxerStreamByTrackId is a good idea (or
> > perhaps I just don't fully understand what you are suggesting instead). We
get
> > media stack status notifications in blink as a set of media track ids. We
need
> > to translate those ids into demuxerstreams somewhere. So I see two possible
> > options:
> > a. Pass the track ids sets into demuxer and make it do the translations
> > internally - is that what you are suggesting? I.e. we could add
> > Demuxer::OnEnabledAudioStreamChanged(<set of ids>) /
> > OnSelectedVideoStreamChanged(<id>) ? But that would mean we still introduce
> two
> > new functions into the public Demuxer interface. Those functions would
operate
> > with track ids, but would not allow you to get access to demuxer streams by
> > knowing the track ids, which would make the usefulness of those functions
very
> > limited (you wouldn't be able to do anything else with media track ids, only
> > enable/disable them).
> > b. (what's currently implemented in this CL) We could have a generic
function
> in
> > the demuxer that would allow you to translate media track ids into demuxer
> > streams. Then you could do whatever you choose with those streams, you could
> > disable/enable them + you could also figure out other demuxer stream/media
> track
> > properties by knowing just the media track id (track type, liveness, a/v
> decoder
> > configs, etc). I think that would be much more useful function to have in
the
> > public demuxer API.
> > 
> > 3. Re your last point. We cannot avoid going at least through RendererImpl,
> > because we need to notify RendererImpl that the stream playback is being
> > restarted and that stream should be no longer considered 'ended'. That's why
I
> > chose to implement it this way - it's already easy to recover
A/VRendererImpls
> > from the 'ended' state by using their public APIs by just calling Flush +
> > StartPlaying(From), no further changes are necessary to the A/V renderers.
But
> > there is currently no direct way to recover the media::RendererImpl from
> playing
> > just one stream (while the other one has been disabled/ended) back to
playing
> > two streams.
> > And since we need to make some changes to the media::RendererImpl in any
case,
> > and media::Renderer is already accessible from media::Pipeline(Controller),
we
> > could propage the necessary changes through the pipeline(controller).
> > I guess we could make it slightly differently, along the lines of what you
> > described. I.e. introduce DemuxerStream::OnEnabledNotification, then in the
> > RendererImpl subscribe to those notifications through
> > RendererImpl::demuxer_stream_provider_, and when the media track status
> changes
> > we could enable/disable individual streams in the pipeline(controller)
without
> > directly notifying media::Renderer. But I'm not sure how that would be
better
> > than the current implementation.
> 
> Ok, now that https://codereview.chromium.org/2050043002 (which was a
> prerequisite for this) has landed, let's get back to this one. I've rebased it
> to the current ToT and made a few changes to address previous feedback from
> xhwang@ and chcunningham@:
> 1. I've moved the logic for actually enabling/disabling demuxer streams deeper
> into media::Pipeline, so we no longer access |demuxer_| directly from WMPI
(and
> no longer jump hop on different threads excessively), and instead in
> WMPI::onA/Vtrackschanged we'll just convert track ids from blink::WebStrings
> into MediaTrack::Ids and pass that down further to the media::Pipeline which
> will deal with the demuxer. I have considered going through
PipelineController,
> but for now decided not to do it, as it don't seem to make any sense. IIUC
> PipelineController still runs on the WMPI thread, whereas we want to access
> demuxer streams on the media thread. In fact now that alokp@ split the
> PipelineImpl into PipelineImpl which runs on WMPI thread and RendererWrapper
> that runs on the media thread, perhaps it would make sense to merge
> PipelineController with PipelineImpl, as now both of them run on WMPI thread.
> 2. I've added a logic in FFmpegDemuxerStream to enter a
|waiting_for_keyframe_|
> mode when the stream is re-enabled after being disabled. That will ensure that
> the next buffer that renderers will see from that DemuxerStream is a keyframe.
I
> haven't changed the logic in the ChunkDemuxerStream yet, since for
ChunkDemuxer
> case we do an explicit Seek on the stream and that ensures that reading starts
> with a key frame. Although that means for now there's a difference. The FFmpeg
> case will resume playback from the next keyframe after the current playback
> position, whereas MSE/ChunkDemuxer case will pick up from the _previous_
> keyframe before the current playback position and will try to catch up with
the
> playback. I'm pondering if I should change the MSE case as well for
consistency
> with FFmpeg.
> 
> Also, I haven't made any significant further changes yet, as I've explained in
> the previous comment I'm not sure if it would make more sense to provide
> enabled/disabled notifications through the DemuxerStream interface instead of
> simply giving that information from the PipelineImpl to the renderer (see
point
> #3 in my previous comment).

Thanks for updating this CL. I'll begin my CR of it after completing CR of
https://codereview.chromium.org/1678523003/ (likely tomorrow).

[wolenetz, 2016-06-17 20:07:29]

On 2016/06/16 22:20:53, wolenetz wrote:
> On 2016/06/16 21:01:54, servolk wrote:
> > > > 
> > > > Then you can probably just call something like
> > demuxer->set_enabled(track_id,
> > > > enabled). (I am proposing to get rid of GetDemuxerStreamByTrackId(), see
> > > below.)
> > > > 
> > > > Then it would be nice if we could avoid the round trip of
> > > > OnSelectedVideoTrackChanged -> GetDemuxerStreamByTrackId ->
> > > > restart_stream_playback_cb -> RestartStreamPlayback. 
> > > > 
> > > > Thinking wildly, I wonder whether this can be solved mostly in the
> demuxer.
> > > For
> > > > example, we could have some like:
> > > > 
> > > > DemuxerStream::SetEnableNotification(cb);
> > > > 
> > > > Then in VideoRendererImpl:
> > > > demuxer_stream_->SetEnableDisableNotification(&OnDemuxerStreamEnabled);
> > > > 
> > > > void OnDemuxerStreamEnabled() {
> > > >   StartResumePlayback();
> > > > }
> > > > 
> > > > Then you don't have to go through the pipeline and renderer to resume
> > > playback.
> > > > When you enable the demuxer stream/track, the corresponding renderer
will
> be
> > > > notified.
> > > > 
> > > > I haven't thought about all the details. Just something for us to think
> > > about...
> > > 
> > > There is a lot of different points here, let me try to address them in
order
> > > (can't find an easy way to quote here):
> > > 
> > > 1. Yes, I guess we could go through pipeline controller instead of calling
> > > demuxer directly in WMPI, that should be a relatively minor change, I'll
> give
> > it
> > > a try.
> > > 
> > > 2. I'm not sure getting rid of GetDemuxerStreamByTrackId is a good idea
(or
> > > perhaps I just don't fully understand what you are suggesting instead). We
> get
> > > media stack status notifications in blink as a set of media track ids. We
> need
> > > to translate those ids into demuxerstreams somewhere. So I see two
possible
> > > options:
> > > a. Pass the track ids sets into demuxer and make it do the translations
> > > internally - is that what you are suggesting? I.e. we could add
> > > Demuxer::OnEnabledAudioStreamChanged(<set of ids>) /
> > > OnSelectedVideoStreamChanged(<id>) ? But that would mean we still
introduce
> > two
> > > new functions into the public Demuxer interface. Those functions would
> operate
> > > with track ids, but would not allow you to get access to demuxer streams
by
> > > knowing the track ids, which would make the usefulness of those functions
> very
> > > limited (you wouldn't be able to do anything else with media track ids,
only
> > > enable/disable them).
> > > b. (what's currently implemented in this CL) We could have a generic
> function
> > in
> > > the demuxer that would allow you to translate media track ids into demuxer
> > > streams. Then you could do whatever you choose with those streams, you
could
> > > disable/enable them + you could also figure out other demuxer stream/media
> > track
> > > properties by knowing just the media track id (track type, liveness, a/v
> > decoder
> > > configs, etc). I think that would be much more useful function to have in
> the
> > > public demuxer API.
> > > 
> > > 3. Re your last point. We cannot avoid going at least through
RendererImpl,
> > > because we need to notify RendererImpl that the stream playback is being
> > > restarted and that stream should be no longer considered 'ended'. That's
why
> I
> > > chose to implement it this way - it's already easy to recover
> A/VRendererImpls
> > > from the 'ended' state by using their public APIs by just calling Flush +
> > > StartPlaying(From), no further changes are necessary to the A/V renderers.
> But
> > > there is currently no direct way to recover the media::RendererImpl from
> > playing
> > > just one stream (while the other one has been disabled/ended) back to
> playing
> > > two streams.
> > > And since we need to make some changes to the media::RendererImpl in any
> case,
> > > and media::Renderer is already accessible from
media::Pipeline(Controller),
> we
> > > could propage the necessary changes through the pipeline(controller).
> > > I guess we could make it slightly differently, along the lines of what you
> > > described. I.e. introduce DemuxerStream::OnEnabledNotification, then in
the
> > > RendererImpl subscribe to those notifications through
> > > RendererImpl::demuxer_stream_provider_, and when the media track status
> > changes
> > > we could enable/disable individual streams in the pipeline(controller)
> without
> > > directly notifying media::Renderer. But I'm not sure how that would be
> better
> > > than the current implementation.
> > 
> > Ok, now that https://codereview.chromium.org/2050043002 (which was a
> > prerequisite for this) has landed, let's get back to this one. I've rebased
it
> > to the current ToT and made a few changes to address previous feedback from
> > xhwang@ and chcunningham@:
> > 1. I've moved the logic for actually enabling/disabling demuxer streams
deeper
> > into media::Pipeline, so we no longer access |demuxer_| directly from WMPI
> (and
> > no longer jump hop on different threads excessively), and instead in
> > WMPI::onA/Vtrackschanged we'll just convert track ids from blink::WebStrings
> > into MediaTrack::Ids and pass that down further to the media::Pipeline which
> > will deal with the demuxer. I have considered going through
> PipelineController,
> > but for now decided not to do it, as it don't seem to make any sense. IIUC
> > PipelineController still runs on the WMPI thread, whereas we want to access
> > demuxer streams on the media thread. In fact now that alokp@ split the
> > PipelineImpl into PipelineImpl which runs on WMPI thread and RendererWrapper
> > that runs on the media thread, perhaps it would make sense to merge
> > PipelineController with PipelineImpl, as now both of them run on WMPI
thread.
> > 2. I've added a logic in FFmpegDemuxerStream to enter a
> |waiting_for_keyframe_|
> > mode when the stream is re-enabled after being disabled. That will ensure
that
> > the next buffer that renderers will see from that DemuxerStream is a
keyframe.
> I
> > haven't changed the logic in the ChunkDemuxerStream yet, since for
> ChunkDemuxer
> > case we do an explicit Seek on the stream and that ensures that reading
starts
> > with a key frame. Although that means for now there's a difference. The
FFmpeg
> > case will resume playback from the next keyframe after the current playback
> > position, whereas MSE/ChunkDemuxer case will pick up from the _previous_
> > keyframe before the current playback position and will try to catch up with
> the
> > playback. I'm pondering if I should change the MSE case as well for
> consistency
> > with FFmpeg.
> > 
> > Also, I haven't made any significant further changes yet, as I've explained
in
> > the previous comment I'm not sure if it would make more sense to provide
> > enabled/disabled notifications through the DemuxerStream interface instead
of
> > simply giving that information from the PipelineImpl to the renderer (see
> point
> > #3 in my previous comment).
> 
> Thanks for updating this CL. I'll begin my CR of it after completing CR of
> https://codereview.chromium.org/1678523003/ (likely tomorrow).

CR of https://codereview.chromium.org/1678523003/ led to finding a prereq bug
(https://bugs.chromium.org/p/chromium/issues/detail?id=620903) that I'm fixing,
so my CR of this CL might take just a bit longer.

[xhwang, 2016-06-20 19:50:53]

The chat history is a bit long so let me only quote recent comments. Again, high
level discussions only.

I won't have time to review all details. wolenetz@, are you back and available
to review this CL now?

> 2. I'm not sure getting rid of GetDemuxerStreamByTrackId is a good idea (or
> perhaps I just don't fully understand what you are suggesting instead). We get
> media stack status notifications in blink as a set of media track ids. We need
> to translate those ids into demuxerstreams somewhere. So I see two possible
> options:
> a. Pass the track ids sets into demuxer and make it do the translations
> internally - is that what you are suggesting? I.e. we could add
> Demuxer::OnEnabledAudioStreamChanged(<set of ids>) /
> OnSelectedVideoStreamChanged(<id>) ? But that would mean we still introduce
two
> new functions into the public Demuxer interface. Those functions would operate
> with track ids, but would not allow you to get access to demuxer streams by
> knowing the track ids, which would make the usefulness of those functions very
> limited (you wouldn't be able to do anything else with media track ids, only
> enable/disable them).
> b. (what's currently implemented in this CL) We could have a generic function
in
> the demuxer that would allow you to translate media track ids into demuxer
> streams. Then you could do whatever you choose with those streams, you could
> disable/enable them + you could also figure out other demuxer stream/media
track
> properties by knowing just the media track id (track type, liveness, a/v
decoder
> configs, etc). I think that would be much more useful function to have in the
> public demuxer API.

See comment below, I don't really like the trip of

OnSelectedVideoTrackChanged -> GetDemuxerStreamByTrackId ->
restart_stream_playback_cb -> RestartStreamPlayback. 

Instead I am thinking of a model where pipeline notifies the demuxer that a
track is enabled, and then the demuxer notifies the RendererImpl directly. If
that works, then Demuxers don't need to expose DemuxerStreams to the pipeline,
then we don't need GetDemuxerStreamByTrackId(). What we need is similar to
Demuxer::EnabledTracks(<set of ids>). (And we don't need
OnSelectedVideoStreamChanged()). So it's more like:

Demuxer::EnabledTracks(ids) -> (internally enable DemuxerStreams) -> notify
RendererImpl (see below).

> 3. Re your last point. We cannot avoid going at least through RendererImpl,
> because we need to notify RendererImpl that the stream playback is being
> restarted and that stream should be no longer considered 'ended'. That's why I
> chose to implement it this way - it's already easy to recover A/VRendererImpls
> from the 'ended' state by using their public APIs by just calling Flush +
> StartPlaying(From), no further changes are necessary to the A/V renderers. But
> there is currently no direct way to recover the media::RendererImpl from
playing
> just one stream (while the other one has been disabled/ended) back to playing
> two streams.
> And since we need to make some changes to the media::RendererImpl in any case,
> and media::Renderer is already accessible from media::Pipeline(Controller), we
> could propage the necessary changes through the pipeline(controller).
> I guess we could make it slightly differently, along the lines of what you
> described. I.e. introduce DemuxerStream::OnEnabledNotification, then in the
> RendererImpl subscribe to those notifications through
> RendererImpl::demuxer_stream_provider_, and when the media track status
changes
> we could enable/disable individual streams in the pipeline(controller) without
> directly notifying media::Renderer. But I'm not sure how that would be better
> than the current implementation.

Yeah, the changes within RendererImpl (that uses public APIs of
A/VRendererImpls) looks nice. I am just not a big fan of having two paths when
enabling a track (one on the demuxer, another on the renderer). Also, passing
the DemuxerStream down the pipeline/Renderer path seems a bit hacky (). I am
thinking of a model where pipeline notifies the demuxer that a track is enabled,
and then the demuxer notifies the RendererImpl. This is more consistent with the
current design where demuxer related changes goes from the demuxer or
demuxerstream to Renderers directly.

Can we have something like
DemuxerStreamProvider::SetDemuxerStreamEnabledNotification(), so that
RendererImpl would get notified when a stream/track is enabled on
DemuxerStreamProvider? For now, we could just pass DemuxerStream::AUDIO and/or
DemuxerStream::VIDEO in the notification, passing down the full "DemuxerStream*
stream" only to call stream->type() seems unnecessary. This won't work well
multiple audio/video streams, but we can iterate on this later I guess. Even if
we want to pass DemuxerStream* directly, it'll be much easier to pass it from
the DemuxerStreamProvider than from the pipeline.

[servolk, 2016-06-21 23:41:05]

On 2016/06/20 19:50:53, xhwang wrote:
> The chat history is a bit long so let me only quote recent comments. Again,
high
> level discussions only.
> 
> I won't have time to review all details. wolenetz@, are you back and available
> to review this CL now?
> 
> > 2. I'm not sure getting rid of GetDemuxerStreamByTrackId is a good idea (or
> > perhaps I just don't fully understand what you are suggesting instead). We
get
> > media stack status notifications in blink as a set of media track ids. We
need
> > to translate those ids into demuxerstreams somewhere. So I see two possible
> > options:
> > a. Pass the track ids sets into demuxer and make it do the translations
> > internally - is that what you are suggesting? I.e. we could add
> > Demuxer::OnEnabledAudioStreamChanged(<set of ids>) /
> > OnSelectedVideoStreamChanged(<id>) ? But that would mean we still introduce
> two
> > new functions into the public Demuxer interface. Those functions would
operate
> > with track ids, but would not allow you to get access to demuxer streams by
> > knowing the track ids, which would make the usefulness of those functions
very
> > limited (you wouldn't be able to do anything else with media track ids, only
> > enable/disable them).
> > b. (what's currently implemented in this CL) We could have a generic
function
> in
> > the demuxer that would allow you to translate media track ids into demuxer
> > streams. Then you could do whatever you choose with those streams, you could
> > disable/enable them + you could also figure out other demuxer stream/media
> track
> > properties by knowing just the media track id (track type, liveness, a/v
> decoder
> > configs, etc). I think that would be much more useful function to have in
the
> > public demuxer API.
> 
> See comment below, I don't really like the trip of
> 
> OnSelectedVideoTrackChanged -> GetDemuxerStreamByTrackId ->
> restart_stream_playback_cb -> RestartStreamPlayback. 
> 
> Instead I am thinking of a model where pipeline notifies the demuxer that a
> track is enabled, and then the demuxer notifies the RendererImpl directly. If
> that works, then Demuxers don't need to expose DemuxerStreams to the pipeline,
> then we don't need GetDemuxerStreamByTrackId(). What we need is similar to
> Demuxer::EnabledTracks(<set of ids>). (And we don't need
> OnSelectedVideoStreamChanged()). So it's more like:
> 
> Demuxer::EnabledTracks(ids) -> (internally enable DemuxerStreams) -> notify
> RendererImpl (see below).

Well, in that model what would be the set of tracks passed into
Demuxer::EnabledTracks?
Would that be a set of all currently enabled audio AND video tracks? But where
would we get that?
To remind you, currently we receive a set of either audio OR video tracks from
blink via WMPI::enabledAudioTracksChanged/selectedVideoTrackChanged. We don't
keep track of media track status on WMPI level. So if we wanted to have a
unified notification that would contain the merged set of audio and video track,
we'd need to either modify the interface between Chromium and blink to give us
all the information. Or we would need to maintain track status information on
WMPI or media pipeline level in addition to the demuxer. Both choices don't seem
to be good ideas to me. I can certainly implement it that way, but I can already
see that it will be uglier than the current code.
 
> > 3. Re your last point. We cannot avoid going at least through RendererImpl,
> > because we need to notify RendererImpl that the stream playback is being
> > restarted and that stream should be no longer considered 'ended'. That's why
I
> > chose to implement it this way - it's already easy to recover
A/VRendererImpls
> > from the 'ended' state by using their public APIs by just calling Flush +
> > StartPlaying(From), no further changes are necessary to the A/V renderers.
But
> > there is currently no direct way to recover the media::RendererImpl from
> playing
> > just one stream (while the other one has been disabled/ended) back to
playing
> > two streams.
> > And since we need to make some changes to the media::RendererImpl in any
case,
> > and media::Renderer is already accessible from media::Pipeline(Controller),
we
> > could propage the necessary changes through the pipeline(controller).
> > I guess we could make it slightly differently, along the lines of what you
> > described. I.e. introduce DemuxerStream::OnEnabledNotification, then in the
> > RendererImpl subscribe to those notifications through
> > RendererImpl::demuxer_stream_provider_, and when the media track status
> changes
> > we could enable/disable individual streams in the pipeline(controller)
without
> > directly notifying media::Renderer. But I'm not sure how that would be
better
> > than the current implementation.
> 
> Yeah, the changes within RendererImpl (that uses public APIs of
> A/VRendererImpls) looks nice. I am just not a big fan of having two paths when
> enabling a track (one on the demuxer, another on the renderer). Also, passing
> the DemuxerStream down the pipeline/Renderer path seems a bit hacky (). I am
> thinking of a model where pipeline notifies the demuxer that a track is
enabled,
> and then the demuxer notifies the RendererImpl. This is more consistent with
the
> current design where demuxer related changes goes from the demuxer or
> demuxerstream to Renderers directly.
> 
> Can we have something like
> DemuxerStreamProvider::SetDemuxerStreamEnabledNotification(), so that
> RendererImpl would get notified when a stream/track is enabled on
> DemuxerStreamProvider? For now, we could just pass DemuxerStream::AUDIO and/or
> DemuxerStream::VIDEO in the notification, passing down the full
"DemuxerStream*
> stream" only to call stream->type() seems unnecessary. This won't work well
> multiple audio/video streams, but we can iterate on this later I guess. Even
if
> we want to pass DemuxerStream* directly, it'll be much easier to pass it from
> the DemuxerStreamProvider than from the pipeline.

Yes, we could do this. I'll give it a try. But I think we would still want to
keep the RestartStreamPlayback method in the public media::Renderer interface,
since it would be useful for proper implementation of
WebMediaSourceImpl::unmarkEndOfStream.

[xhwang, 2016-06-22 18:00:24]

On 2016/06/21 23:41:05, servolk wrote:
> On 2016/06/20 19:50:53, xhwang wrote:
> > The chat history is a bit long so let me only quote recent comments. Again,
> high
> > level discussions only.
> > 
> > I won't have time to review all details. wolenetz@, are you back and
available
> > to review this CL now?
> > 
> > > 2. I'm not sure getting rid of GetDemuxerStreamByTrackId is a good idea
(or
> > > perhaps I just don't fully understand what you are suggesting instead). We
> get
> > > media stack status notifications in blink as a set of media track ids. We
> need
> > > to translate those ids into demuxerstreams somewhere. So I see two
possible
> > > options:
> > > a. Pass the track ids sets into demuxer and make it do the translations
> > > internally - is that what you are suggesting? I.e. we could add
> > > Demuxer::OnEnabledAudioStreamChanged(<set of ids>) /
> > > OnSelectedVideoStreamChanged(<id>) ? But that would mean we still
introduce
> > two
> > > new functions into the public Demuxer interface. Those functions would
> operate
> > > with track ids, but would not allow you to get access to demuxer streams
by
> > > knowing the track ids, which would make the usefulness of those functions
> very
> > > limited (you wouldn't be able to do anything else with media track ids,
only
> > > enable/disable them).
> > > b. (what's currently implemented in this CL) We could have a generic
> function
> > in
> > > the demuxer that would allow you to translate media track ids into demuxer
> > > streams. Then you could do whatever you choose with those streams, you
could
> > > disable/enable them + you could also figure out other demuxer stream/media
> > track
> > > properties by knowing just the media track id (track type, liveness, a/v
> > decoder
> > > configs, etc). I think that would be much more useful function to have in
> the
> > > public demuxer API.
> > 
> > See comment below, I don't really like the trip of
> > 
> > OnSelectedVideoTrackChanged -> GetDemuxerStreamByTrackId ->
> > restart_stream_playback_cb -> RestartStreamPlayback. 
> > 
> > Instead I am thinking of a model where pipeline notifies the demuxer that a
> > track is enabled, and then the demuxer notifies the RendererImpl directly.
If
> > that works, then Demuxers don't need to expose DemuxerStreams to the
pipeline,
> > then we don't need GetDemuxerStreamByTrackId(). What we need is similar to
> > Demuxer::EnabledTracks(<set of ids>). (And we don't need
> > OnSelectedVideoStreamChanged()). So it's more like:
> > 
> > Demuxer::EnabledTracks(ids) -> (internally enable DemuxerStreams) -> notify
> > RendererImpl (see below).
> 
> Well, in that model what would be the set of tracks passed into
> Demuxer::EnabledTracks?
> Would that be a set of all currently enabled audio AND video tracks? But where
> would we get that?
> To remind you, currently we receive a set of either audio OR video tracks from
> blink via WMPI::enabledAudioTracksChanged/selectedVideoTrackChanged. We don't
> keep track of media track status on WMPI level. So if we wanted to have a
> unified notification that would contain the merged set of audio and video
track,
> we'd need to either modify the interface between Chromium and blink to give us
> all the information. Or we would need to maintain track status information on
> WMPI or media pipeline level in addition to the demuxer. Both choices don't
seem
> to be good ideas to me. I can certainly implement it that way, but I can
already
> see that it will be uglier than the current code.

The API I proposed is just for the idea and could be over simplified. At the
very least, you could have Demuxer::EnabledAudioTrack(ids) and
Demuxer::SelectVideoTrack(id). The implementation would be mostly similar to
what you have in PipelineImpl::RendererWrapper::OnEnabledAudioTracksChanged and
PipelineImpl::RendererWrapper::OnSelectedVideoTrackChanged.
  
> > > 3. Re your last point. We cannot avoid going at least through
RendererImpl,
> > > because we need to notify RendererImpl that the stream playback is being
> > > restarted and that stream should be no longer considered 'ended'. That's
why
> I
> > > chose to implement it this way - it's already easy to recover
> A/VRendererImpls
> > > from the 'ended' state by using their public APIs by just calling Flush +
> > > StartPlaying(From), no further changes are necessary to the A/V renderers.
> But
> > > there is currently no direct way to recover the media::RendererImpl from
> > playing
> > > just one stream (while the other one has been disabled/ended) back to
> playing
> > > two streams.
> > > And since we need to make some changes to the media::RendererImpl in any
> case,
> > > and media::Renderer is already accessible from
media::Pipeline(Controller),
> we
> > > could propage the necessary changes through the pipeline(controller).
> > > I guess we could make it slightly differently, along the lines of what you
> > > described. I.e. introduce DemuxerStream::OnEnabledNotification, then in
the
> > > RendererImpl subscribe to those notifications through
> > > RendererImpl::demuxer_stream_provider_, and when the media track status
> > changes
> > > we could enable/disable individual streams in the pipeline(controller)
> without
> > > directly notifying media::Renderer. But I'm not sure how that would be
> better
> > > than the current implementation.
> > 
> > Yeah, the changes within RendererImpl (that uses public APIs of
> > A/VRendererImpls) looks nice. I am just not a big fan of having two paths
when
> > enabling a track (one on the demuxer, another on the renderer). Also,
passing
> > the DemuxerStream down the pipeline/Renderer path seems a bit hacky (). I am
> > thinking of a model where pipeline notifies the demuxer that a track is
> enabled,
> > and then the demuxer notifies the RendererImpl. This is more consistent with
> the
> > current design where demuxer related changes goes from the demuxer or
> > demuxerstream to Renderers directly.
> > 
> > Can we have something like
> > DemuxerStreamProvider::SetDemuxerStreamEnabledNotification(), so that
> > RendererImpl would get notified when a stream/track is enabled on
> > DemuxerStreamProvider? For now, we could just pass DemuxerStream::AUDIO
and/or
> > DemuxerStream::VIDEO in the notification, passing down the full
> "DemuxerStream*
> > stream" only to call stream->type() seems unnecessary. This won't work well
> > multiple audio/video streams, but we can iterate on this later I guess. Even
> if
> > we want to pass DemuxerStream* directly, it'll be much easier to pass it
from
> > the DemuxerStreamProvider than from the pipeline.
> 
> Yes, we could do this. I'll give it a try. But I think we would still want to
> keep the RestartStreamPlayback method in the public media::Renderer interface,
> since it would be useful for proper implementation of
> WebMediaSourceImpl::unmarkEndOfStream.

WebMediaSourceImpl::unmarkEndOfStream() talks to the demuxer directly. Why a
demuxer-based solution won't work and we have to call media::Renderer?

I'll be OOO soon but I think I have expressed my ideas clearly. I'll let you and
other reviewers (+chcunningham/dalecurtis/wolenetz) to decide what's the best
way to implement this.

[servolk, 2016-06-22 18:17:56]

On 2016/06/22 18:00:24, xhwang wrote:
> On 2016/06/21 23:41:05, servolk wrote:
> > On 2016/06/20 19:50:53, xhwang wrote:
> > > The chat history is a bit long so let me only quote recent comments.
Again,
> > high
> > > level discussions only.
> > > 
> > > I won't have time to review all details. wolenetz@, are you back and
> available
> > > to review this CL now?
> > > 
> > > > 2. I'm not sure getting rid of GetDemuxerStreamByTrackId is a good idea
> (or
> > > > perhaps I just don't fully understand what you are suggesting instead).
We
> > get
> > > > media stack status notifications in blink as a set of media track ids.
We
> > need
> > > > to translate those ids into demuxerstreams somewhere. So I see two
> possible
> > > > options:
> > > > a. Pass the track ids sets into demuxer and make it do the translations
> > > > internally - is that what you are suggesting? I.e. we could add
> > > > Demuxer::OnEnabledAudioStreamChanged(<set of ids>) /
> > > > OnSelectedVideoStreamChanged(<id>) ? But that would mean we still
> introduce
> > > two
> > > > new functions into the public Demuxer interface. Those functions would
> > operate
> > > > with track ids, but would not allow you to get access to demuxer streams
> by
> > > > knowing the track ids, which would make the usefulness of those
functions
> > very
> > > > limited (you wouldn't be able to do anything else with media track ids,
> only
> > > > enable/disable them).
> > > > b. (what's currently implemented in this CL) We could have a generic
> > function
> > > in
> > > > the demuxer that would allow you to translate media track ids into
demuxer
> > > > streams. Then you could do whatever you choose with those streams, you
> could
> > > > disable/enable them + you could also figure out other demuxer
stream/media
> > > track
> > > > properties by knowing just the media track id (track type, liveness, a/v
> > > decoder
> > > > configs, etc). I think that would be much more useful function to have
in
> > the
> > > > public demuxer API.
> > > 
> > > See comment below, I don't really like the trip of
> > > 
> > > OnSelectedVideoTrackChanged -> GetDemuxerStreamByTrackId ->
> > > restart_stream_playback_cb -> RestartStreamPlayback. 
> > > 
> > > Instead I am thinking of a model where pipeline notifies the demuxer that
a
> > > track is enabled, and then the demuxer notifies the RendererImpl directly.
> If
> > > that works, then Demuxers don't need to expose DemuxerStreams to the
> pipeline,
> > > then we don't need GetDemuxerStreamByTrackId(). What we need is similar to
> > > Demuxer::EnabledTracks(<set of ids>). (And we don't need
> > > OnSelectedVideoStreamChanged()). So it's more like:
> > > 
> > > Demuxer::EnabledTracks(ids) -> (internally enable DemuxerStreams) ->
notify
> > > RendererImpl (see below).
> > 
> > Well, in that model what would be the set of tracks passed into
> > Demuxer::EnabledTracks?
> > Would that be a set of all currently enabled audio AND video tracks? But
where
> > would we get that?
> > To remind you, currently we receive a set of either audio OR video tracks
from
> > blink via WMPI::enabledAudioTracksChanged/selectedVideoTrackChanged. We
don't
> > keep track of media track status on WMPI level. So if we wanted to have a
> > unified notification that would contain the merged set of audio and video
> track,
> > we'd need to either modify the interface between Chromium and blink to give
us
> > all the information. Or we would need to maintain track status information
on
> > WMPI or media pipeline level in addition to the demuxer. Both choices don't
> seem
> > to be good ideas to me. I can certainly implement it that way, but I can
> already
> > see that it will be uglier than the current code.
> 
> The API I proposed is just for the idea and could be over simplified. At the
> very least, you could have Demuxer::EnabledAudioTrack(ids) and
> Demuxer::SelectVideoTrack(id). The implementation would be mostly similar to
> what you have in PipelineImpl::RendererWrapper::OnEnabledAudioTracksChanged
and
> PipelineImpl::RendererWrapper::OnSelectedVideoTrackChanged.

Ok, this might be more viable, I'll try it today.

> > > > 3. Re your last point. We cannot avoid going at least through
> RendererImpl,
> > > > because we need to notify RendererImpl that the stream playback is being
> > > > restarted and that stream should be no longer considered 'ended'. That's
> why
> > I
> > > > chose to implement it this way - it's already easy to recover
> > A/VRendererImpls
> > > > from the 'ended' state by using their public APIs by just calling Flush
+
> > > > StartPlaying(From), no further changes are necessary to the A/V
renderers.
> > But
> > > > there is currently no direct way to recover the media::RendererImpl from
> > > playing
> > > > just one stream (while the other one has been disabled/ended) back to
> > playing
> > > > two streams.
> > > > And since we need to make some changes to the media::RendererImpl in any
> > case,
> > > > and media::Renderer is already accessible from
> media::Pipeline(Controller),
> > we
> > > > could propage the necessary changes through the pipeline(controller).
> > > > I guess we could make it slightly differently, along the lines of what
you
> > > > described. I.e. introduce DemuxerStream::OnEnabledNotification, then in
> the
> > > > RendererImpl subscribe to those notifications through
> > > > RendererImpl::demuxer_stream_provider_, and when the media track status
> > > changes
> > > > we could enable/disable individual streams in the pipeline(controller)
> > without
> > > > directly notifying media::Renderer. But I'm not sure how that would be
> > better
> > > > than the current implementation.
> > > 
> > > Yeah, the changes within RendererImpl (that uses public APIs of
> > > A/VRendererImpls) looks nice. I am just not a big fan of having two paths
> when
> > > enabling a track (one on the demuxer, another on the renderer). Also,
> passing
> > > the DemuxerStream down the pipeline/Renderer path seems a bit hacky (). I
am
> > > thinking of a model where pipeline notifies the demuxer that a track is
> > enabled,
> > > and then the demuxer notifies the RendererImpl. This is more consistent
with
> > the
> > > current design where demuxer related changes goes from the demuxer or
> > > demuxerstream to Renderers directly.
> > > 
> > > Can we have something like
> > > DemuxerStreamProvider::SetDemuxerStreamEnabledNotification(), so that
> > > RendererImpl would get notified when a stream/track is enabled on
> > > DemuxerStreamProvider? For now, we could just pass DemuxerStream::AUDIO
> and/or
> > > DemuxerStream::VIDEO in the notification, passing down the full
> > "DemuxerStream*
> > > stream" only to call stream->type() seems unnecessary. This won't work
well
> > > multiple audio/video streams, but we can iterate on this later I guess.
Even
> > if
> > > we want to pass DemuxerStream* directly, it'll be much easier to pass it
> from
> > > the DemuxerStreamProvider than from the pipeline.
> > 
> > Yes, we could do this. I'll give it a try. But I think we would still want
to
> > keep the RestartStreamPlayback method in the public media::Renderer
interface,
> > since it would be useful for proper implementation of
> > WebMediaSourceImpl::unmarkEndOfStream.
> 
> WebMediaSourceImpl::unmarkEndOfStream() talks to the demuxer directly. Why a
> demuxer-based solution won't work and we have to call media::Renderer?

It doesn't work for the same reason: after MSE markEndOfStream happens, the a/v
renderers might read an EOS buffer from their demuxer streams, and that will
cause a/v renderers and media::RendererImpl to remain in the 'one of the streams
has ended' state indefinitely. Currently there's nothing that demuxer can do to
tell renderers to get out of that state. There is an existing bug that tracks
this, see crbug.com/530698 for more context.
 
> I'll be OOO soon but I think I have expressed my ideas clearly. I'll let you
and
> other reviewers (+chcunningham/dalecurtis/wolenetz) to decide what's the best
> way to implement this.

Ok, sounds good. Thanks for your comments!

[xhwang, 2016-06-22 18:22:29]

On 2016/06/22 18:17:56, servolk wrote:
> On 2016/06/22 18:00:24, xhwang wrote:
> > On 2016/06/21 23:41:05, servolk wrote:
> > > On 2016/06/20 19:50:53, xhwang wrote:
> > > > The chat history is a bit long so let me only quote recent comments.
> Again,
> > > high
> > > > level discussions only.
> > > > 
> > > > I won't have time to review all details. wolenetz@, are you back and
> > available
> > > > to review this CL now?
> > > > 
> > > > > 2. I'm not sure getting rid of GetDemuxerStreamByTrackId is a good
idea
> > (or
> > > > > perhaps I just don't fully understand what you are suggesting
instead).
> We
> > > get
> > > > > media stack status notifications in blink as a set of media track ids.
> We
> > > need
> > > > > to translate those ids into demuxerstreams somewhere. So I see two
> > possible
> > > > > options:
> > > > > a. Pass the track ids sets into demuxer and make it do the
translations
> > > > > internally - is that what you are suggesting? I.e. we could add
> > > > > Demuxer::OnEnabledAudioStreamChanged(<set of ids>) /
> > > > > OnSelectedVideoStreamChanged(<id>) ? But that would mean we still
> > introduce
> > > > two
> > > > > new functions into the public Demuxer interface. Those functions would
> > > operate
> > > > > with track ids, but would not allow you to get access to demuxer
streams
> > by
> > > > > knowing the track ids, which would make the usefulness of those
> functions
> > > very
> > > > > limited (you wouldn't be able to do anything else with media track
ids,
> > only
> > > > > enable/disable them).
> > > > > b. (what's currently implemented in this CL) We could have a generic
> > > function
> > > > in
> > > > > the demuxer that would allow you to translate media track ids into
> demuxer
> > > > > streams. Then you could do whatever you choose with those streams, you
> > could
> > > > > disable/enable them + you could also figure out other demuxer
> stream/media
> > > > track
> > > > > properties by knowing just the media track id (track type, liveness,
a/v
> > > > decoder
> > > > > configs, etc). I think that would be much more useful function to have
> in
> > > the
> > > > > public demuxer API.
> > > > 
> > > > See comment below, I don't really like the trip of
> > > > 
> > > > OnSelectedVideoTrackChanged -> GetDemuxerStreamByTrackId ->
> > > > restart_stream_playback_cb -> RestartStreamPlayback. 
> > > > 
> > > > Instead I am thinking of a model where pipeline notifies the demuxer
that
> a
> > > > track is enabled, and then the demuxer notifies the RendererImpl
directly.
> > If
> > > > that works, then Demuxers don't need to expose DemuxerStreams to the
> > pipeline,
> > > > then we don't need GetDemuxerStreamByTrackId(). What we need is similar
to
> > > > Demuxer::EnabledTracks(<set of ids>). (And we don't need
> > > > OnSelectedVideoStreamChanged()). So it's more like:
> > > > 
> > > > Demuxer::EnabledTracks(ids) -> (internally enable DemuxerStreams) ->
> notify
> > > > RendererImpl (see below).
> > > 
> > > Well, in that model what would be the set of tracks passed into
> > > Demuxer::EnabledTracks?
> > > Would that be a set of all currently enabled audio AND video tracks? But
> where
> > > would we get that?
> > > To remind you, currently we receive a set of either audio OR video tracks
> from
> > > blink via WMPI::enabledAudioTracksChanged/selectedVideoTrackChanged. We
> don't
> > > keep track of media track status on WMPI level. So if we wanted to have a
> > > unified notification that would contain the merged set of audio and video
> > track,
> > > we'd need to either modify the interface between Chromium and blink to
give
> us
> > > all the information. Or we would need to maintain track status information
> on
> > > WMPI or media pipeline level in addition to the demuxer. Both choices
don't
> > seem
> > > to be good ideas to me. I can certainly implement it that way, but I can
> > already
> > > see that it will be uglier than the current code.
> > 
> > The API I proposed is just for the idea and could be over simplified. At the
> > very least, you could have Demuxer::EnabledAudioTrack(ids) and
> > Demuxer::SelectVideoTrack(id). The implementation would be mostly similar to
> > what you have in PipelineImpl::RendererWrapper::OnEnabledAudioTracksChanged
> and
> > PipelineImpl::RendererWrapper::OnSelectedVideoTrackChanged.
> 
> Ok, this might be more viable, I'll try it today.
> 
> > > > > 3. Re your last point. We cannot avoid going at least through
> > RendererImpl,
> > > > > because we need to notify RendererImpl that the stream playback is
being
> > > > > restarted and that stream should be no longer considered 'ended'.
That's
> > why
> > > I
> > > > > chose to implement it this way - it's already easy to recover
> > > A/VRendererImpls
> > > > > from the 'ended' state by using their public APIs by just calling
Flush
> +
> > > > > StartPlaying(From), no further changes are necessary to the A/V
> renderers.
> > > But
> > > > > there is currently no direct way to recover the media::RendererImpl
from
> > > > playing
> > > > > just one stream (while the other one has been disabled/ended) back to
> > > playing
> > > > > two streams.
> > > > > And since we need to make some changes to the media::RendererImpl in
any
> > > case,
> > > > > and media::Renderer is already accessible from
> > media::Pipeline(Controller),
> > > we
> > > > > could propage the necessary changes through the pipeline(controller).
> > > > > I guess we could make it slightly differently, along the lines of what
> you
> > > > > described. I.e. introduce DemuxerStream::OnEnabledNotification, then
in
> > the
> > > > > RendererImpl subscribe to those notifications through
> > > > > RendererImpl::demuxer_stream_provider_, and when the media track
status
> > > > changes
> > > > > we could enable/disable individual streams in the pipeline(controller)
> > > without
> > > > > directly notifying media::Renderer. But I'm not sure how that would be
> > > better
> > > > > than the current implementation.
> > > > 
> > > > Yeah, the changes within RendererImpl (that uses public APIs of
> > > > A/VRendererImpls) looks nice. I am just not a big fan of having two
paths
> > when
> > > > enabling a track (one on the demuxer, another on the renderer). Also,
> > passing
> > > > the DemuxerStream down the pipeline/Renderer path seems a bit hacky ().
I
> am
> > > > thinking of a model where pipeline notifies the demuxer that a track is
> > > enabled,
> > > > and then the demuxer notifies the RendererImpl. This is more consistent
> with
> > > the
> > > > current design where demuxer related changes goes from the demuxer or
> > > > demuxerstream to Renderers directly.
> > > > 
> > > > Can we have something like
> > > > DemuxerStreamProvider::SetDemuxerStreamEnabledNotification(), so that
> > > > RendererImpl would get notified when a stream/track is enabled on
> > > > DemuxerStreamProvider? For now, we could just pass DemuxerStream::AUDIO
> > and/or
> > > > DemuxerStream::VIDEO in the notification, passing down the full
> > > "DemuxerStream*
> > > > stream" only to call stream->type() seems unnecessary. This won't work
> well
> > > > multiple audio/video streams, but we can iterate on this later I guess.
> Even
> > > if
> > > > we want to pass DemuxerStream* directly, it'll be much easier to pass it
> > from
> > > > the DemuxerStreamProvider than from the pipeline.
> > > 
> > > Yes, we could do this. I'll give it a try. But I think we would still want
> to
> > > keep the RestartStreamPlayback method in the public media::Renderer
> interface,
> > > since it would be useful for proper implementation of
> > > WebMediaSourceImpl::unmarkEndOfStream.
> > 
> > WebMediaSourceImpl::unmarkEndOfStream() talks to the demuxer directly. Why a
> > demuxer-based solution won't work and we have to call media::Renderer?
> 
> It doesn't work for the same reason: after MSE markEndOfStream happens, the
a/v
> renderers might read an EOS buffer from their demuxer streams, and that will
> cause a/v renderers and media::RendererImpl to remain in the 'one of the
streams
> has ended' state indefinitely. Currently there's nothing that demuxer can do
to
> tell renderers to get out of that state. There is an existing bug that tracks
> this, see crbug.com/530698 for more context.

I see. But as proposed, the DemuxerStreamProvider could notify the RendererImpl
about this change, right?

Quoting my previous comment here:

"Can we have something like
DemuxerStreamProvider::SetDemuxerStreamEnabledNotification(), so that
RendererImpl would get notified when a stream/track is enabled on
DemuxerStreamProvider?"
  
> > I'll be OOO soon but I think I have expressed my ideas clearly. I'll let you
> and
> > other reviewers (+chcunningham/dalecurtis/wolenetz) to decide what's the
best
> > way to implement this.
> 
> Ok, sounds good. Thanks for your comments!

[chromium-reviews, 2016-06-22 18:29:31]

Yes, but what I'm saying is that DSP should notify RendererImpl about
something like 'this stream playback needs to be restarted' instead of
something like 'this stream got reenabled'. Since in case of unmarkEOS the
stream doesn't get reenabled, is simply goes from ended state back to the
'more data available' state. If we had a notification about 'restart stream
playback' we could use it for both cases.

On Wed, Jun 22, 2016 at 11:22 AM, <xhwang@chromium.org> wrote:

> On 2016/06/22 18:17:56, servolk wrote:
> > On 2016/06/22 18:00:24, xhwang wrote:
> > > On 2016/06/21 23:41:05, servolk wrote:
> > > > On 2016/06/20 19:50:53, xhwang wrote:
> > > > > The chat history is a bit long so let me only quote recent
> comments.
> > Again,
> > > > high
> > > > > level discussions only.
> > > > >
> > > > > I won't have time to review all details. wolenetz@, are you back
> and
> > > available
> > > > > to review this CL now?
> > > > >
> > > > > > 2. I'm not sure getting rid of GetDemuxerStreamByTrackId is a
> good
> idea
> > > (or
> > > > > > perhaps I just don't fully understand what you are suggesting
> instead).
> > We
> > > > get
> > > > > > media stack status notifications in blink as a set of media
> track ids.
> > We
> > > > need
> > > > > > to translate those ids into demuxerstreams somewhere. So I see
> two
> > > possible
> > > > > > options:
> > > > > > a. Pass the track ids sets into demuxer and make it do the
> translations
> > > > > > internally - is that what you are suggesting? I.e. we could add
> > > > > > Demuxer::OnEnabledAudioStreamChanged(<set of ids>) /
> > > > > > OnSelectedVideoStreamChanged(<id>) ? But that would mean we still
> > > introduce
> > > > > two
> > > > > > new functions into the public Demuxer interface. Those functions
> would
> > > > operate
> > > > > > with track ids, but would not allow you to get access to demuxer
> streams
> > > by
> > > > > > knowing the track ids, which would make the usefulness of those
> > functions
> > > > very
> > > > > > limited (you wouldn't be able to do anything else with media
> track
> ids,
> > > only
> > > > > > enable/disable them).
> > > > > > b. (what's currently implemented in this CL) We could have a
> generic
> > > > function
> > > > > in
> > > > > > the demuxer that would allow you to translate media track ids
> into
> > demuxer
> > > > > > streams. Then you could do whatever you choose with those
> streams, you
> > > could
> > > > > > disable/enable them + you could also figure out other demuxer
> > stream/media
> > > > > track
> > > > > > properties by knowing just the media track id (track type,
> liveness,
> a/v
> > > > > decoder
> > > > > > configs, etc). I think that would be much more useful function
> to have
> > in
> > > > the
> > > > > > public demuxer API.
> > > > >
> > > > > See comment below, I don't really like the trip of
> > > > >
> > > > > OnSelectedVideoTrackChanged -> GetDemuxerStreamByTrackId ->
> > > > > restart_stream_playback_cb -> RestartStreamPlayback.
> > > > >
> > > > > Instead I am thinking of a model where pipeline notifies the
> demuxer
> that
> > a
> > > > > track is enabled, and then the demuxer notifies the RendererImpl
> directly.
> > > If
> > > > > that works, then Demuxers don't need to expose DemuxerStreams to
> the
> > > pipeline,
> > > > > then we don't need GetDemuxerStreamByTrackId(). What we need is
> similar
> to
> > > > > Demuxer::EnabledTracks(<set of ids>). (And we don't need
> > > > > OnSelectedVideoStreamChanged()). So it's more like:
> > > > >
> > > > > Demuxer::EnabledTracks(ids) -> (internally enable DemuxerStreams)
> ->
> > notify
> > > > > RendererImpl (see below).
> > > >
> > > > Well, in that model what would be the set of tracks passed into
> > > > Demuxer::EnabledTracks?
> > > > Would that be a set of all currently enabled audio AND video tracks?
> But
> > where
> > > > would we get that?
> > > > To remind you, currently we receive a set of either audio OR video
> tracks
> > from
> > > > blink via WMPI::enabledAudioTracksChanged/selectedVideoTrackChanged.
> We
> > don't
> > > > keep track of media track status on WMPI level. So if we wanted to
> have a
> > > > unified notification that would contain the merged set of audio and
> video
> > > track,
> > > > we'd need to either modify the interface between Chromium and blink
> to
> give
> > us
> > > > all the information. Or we would need to maintain track status
> information
> > on
> > > > WMPI or media pipeline level in addition to the demuxer. Both choices
> don't
> > > seem
> > > > to be good ideas to me. I can certainly implement it that way, but I
> can
> > > already
> > > > see that it will be uglier than the current code.
> > >
> > > The API I proposed is just for the idea and could be over simplified.
> At the
> > > very least, you could have Demuxer::EnabledAudioTrack(ids) and
> > > Demuxer::SelectVideoTrack(id). The implementation would be mostly
> similar to
> > > what you have in
> PipelineImpl::RendererWrapper::OnEnabledAudioTracksChanged
> > and
> > > PipelineImpl::RendererWrapper::OnSelectedVideoTrackChanged.
> >
> > Ok, this might be more viable, I'll try it today.
> >
> > > > > > 3. Re your last point. We cannot avoid going at least through
> > > RendererImpl,
> > > > > > because we need to notify RendererImpl that the stream playback
> is
> being
> > > > > > restarted and that stream should be no longer considered 'ended'.
> That's
> > > why
> > > > I
> > > > > > chose to implement it this way - it's already easy to recover
> > > > A/VRendererImpls
> > > > > > from the 'ended' state by using their public APIs by just calling
> Flush
> > +
> > > > > > StartPlaying(From), no further changes are necessary to the A/V
> > renderers.
> > > > But
> > > > > > there is currently no direct way to recover the
> media::RendererImpl
> from
> > > > > playing
> > > > > > just one stream (while the other one has been disabled/ended)
> back to
> > > > playing
> > > > > > two streams.
> > > > > > And since we need to make some changes to the
> media::RendererImpl in
> any
> > > > case,
> > > > > > and media::Renderer is already accessible from
> > > media::Pipeline(Controller),
> > > > we
> > > > > > could propage the necessary changes through the
> pipeline(controller).
> > > > > > I guess we could make it slightly differently, along the lines
> of what
> > you
> > > > > > described. I.e. introduce DemuxerStream::OnEnabledNotification,
> then
> in
> > > the
> > > > > > RendererImpl subscribe to those notifications through
> > > > > > RendererImpl::demuxer_stream_provider_, and when the media track
> status
> > > > > changes
> > > > > > we could enable/disable individual streams in the
> pipeline(controller)
> > > > without
> > > > > > directly notifying media::Renderer. But I'm not sure how that
> would be
> > > > better
> > > > > > than the current implementation.
> > > > >
> > > > > Yeah, the changes within RendererImpl (that uses public APIs of
> > > > > A/VRendererImpls) looks nice. I am just not a big fan of having two
> paths
> > > when
> > > > > enabling a track (one on the demuxer, another on the renderer).
> Also,
> > > passing
> > > > > the DemuxerStream down the pipeline/Renderer path seems a bit
> hacky ().
> I
> > am
> > > > > thinking of a model where pipeline notifies the demuxer that a
> track is
> > > > enabled,
> > > > > and then the demuxer notifies the RendererImpl. This is more
> consistent
> > with
> > > > the
> > > > > current design where demuxer related changes goes from the demuxer
> or
> > > > > demuxerstream to Renderers directly.
> > > > >
> > > > > Can we have something like
> > > > > DemuxerStreamProvider::SetDemuxerStreamEnabledNotification(), so
> that
> > > > > RendererImpl would get notified when a stream/track is enabled on
> > > > > DemuxerStreamProvider? For now, we could just pass
> DemuxerStream::AUDIO
> > > and/or
> > > > > DemuxerStream::VIDEO in the notification, passing down the full
> > > > "DemuxerStream*
> > > > > stream" only to call stream->type() seems unnecessary. This won't
> work
> > well
> > > > > multiple audio/video streams, but we can iterate on this later I
> guess.
> > Even
> > > > if
> > > > > we want to pass DemuxerStream* directly, it'll be much easier to
> pass it
> > > from
> > > > > the DemuxerStreamProvider than from the pipeline.
> > > >
> > > > Yes, we could do this. I'll give it a try. But I think we would
> still want
> > to
> > > > keep the RestartStreamPlayback method in the public media::Renderer
> > interface,
> > > > since it would be useful for proper implementation of
> > > > WebMediaSourceImpl::unmarkEndOfStream.
> > >
> > > WebMediaSourceImpl::unmarkEndOfStream() talks to the demuxer directly.
> Why a
> > > demuxer-based solution won't work and we have to call media::Renderer?
> >
> > It doesn't work for the same reason: after MSE markEndOfStream happens,
> the
> a/v
> > renderers might read an EOS buffer from their demuxer streams, and that
> will
> > cause a/v renderers and media::RendererImpl to remain in the 'one of the
> streams
> > has ended' state indefinitely. Currently there's nothing that demuxer
> can do
> to
> > tell renderers to get out of that state. There is an existing bug that
> tracks
> > this, see crbug.com/530698 for more context.
>
> I see. But as proposed, the DemuxerStreamProvider could notify the
> RendererImpl
> about this change, right?
>
> Quoting my previous comment here:
>
> "Can we have something like
> DemuxerStreamProvider::SetDemuxerStreamEnabledNotification(), so that
> RendererImpl would get notified when a stream/track is enabled on
> DemuxerStreamProvider?"
>
> > > I'll be OOO soon but I think I have expressed my ideas clearly. I'll
> let you
> > and
> > > other reviewers (+chcunningham/dalecurtis/wolenetz) to decide what's
> the
> best
> > > way to implement this.
> >
> > Ok, sounds good. Thanks for your comments!
>
>
>
> https://chromiumcodereview.appspot.com/1935873002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xhwang, 2016-06-23 00:35:36]

On 2016/06/22 18:29:31, chromium-reviews wrote:
> Yes, but what I'm saying is that DSP should notify RendererImpl about
> something like 'this stream playback needs to be restarted' instead of
> something like 'this stream got reenabled'. Since in case of unmarkEOS the
> stream doesn't get reenabled, is simply goes from ended state back to the
> 'more data available' state. If we had a notification about 'restart stream
> playback' we could use it for both cases.

Yeah, whatever notification that makes sense to you :)

[servolk, 2016-06-23 00:38:14]

On 2016/06/22 18:29:31, chromium-reviews wrote:
> Yes, but what I'm saying is that DSP should notify RendererImpl about
> something like 'this stream playback needs to be restarted' instead of
> something like 'this stream got reenabled'. Since in case of unmarkEOS the
> stream doesn't get reenabled, is simply goes from ended state back to the
> 'more data available' state. If we had a notification about 'restart stream
> playback' we could use it for both cases.
> 
> On Wed, Jun 22, 2016 at 11:22 AM, <mailto:xhwang@chromium.org> wrote:
> 

... Trimmed previous discussion to fit in 10000 chars limit ...

> > I see. But as proposed, the DemuxerStreamProvider could notify the
> > RendererImpl
> > about this change, right?
> >
> > Quoting my previous comment here:
> >
> > "Can we have something like
> > DemuxerStreamProvider::SetDemuxerStreamEnabledNotification(), so that
> > RendererImpl would get notified when a stream/track is enabled on
> > DemuxerStreamProvider?"
> >
> > > > I'll be OOO soon but I think I have expressed my ideas clearly. I'll
> > let you
> > > and
> > > > other reviewers (+chcunningham/dalecurtis/wolenetz) to decide what's
> > the
> > best
> > > > way to implement this.
> > >
> > > Ok, sounds good. Thanks for your comments!
> >
> >
> >
> > https://chromiumcodereview.appspot.com/1935873002/
> >
> 

Ok, so I've reworked this CL so that now:
1. PipelineImpl passes track status updates to the Demuxer implementation, where
they are handled.
2. Renderer is notified about media track status changes by subscribing to
demuxerstream notifications.

I've also removed the previous mojo changes for now, since that'll need to be
reworked to match the new model. I'll do that later in a separate CL.

[servolk, 2016-06-24 17:59:49]

On 2016/06/23 00:38:14, servolk wrote:
> On 2016/06/22 18:29:31, chromium-reviews wrote:
> > Yes, but what I'm saying is that DSP should notify RendererImpl about
> > something like 'this stream playback needs to be restarted' instead of
> > something like 'this stream got reenabled'. Since in case of unmarkEOS the
> > stream doesn't get reenabled, is simply goes from ended state back to the
> > 'more data available' state. If we had a notification about 'restart stream
> > playback' we could use it for both cases.
> > 
> > On Wed, Jun 22, 2016 at 11:22 AM, <mailto:xhwang@chromium.org> wrote:
> > 
> 
> ... Trimmed previous discussion to fit in 10000 chars limit ...
> 
> > > I see. But as proposed, the DemuxerStreamProvider could notify the
> > > RendererImpl
> > > about this change, right?
> > >
> > > Quoting my previous comment here:
> > >
> > > "Can we have something like
> > > DemuxerStreamProvider::SetDemuxerStreamEnabledNotification(), so that
> > > RendererImpl would get notified when a stream/track is enabled on
> > > DemuxerStreamProvider?"
> > >
> > > > > I'll be OOO soon but I think I have expressed my ideas clearly. I'll
> > > let you
> > > > and
> > > > > other reviewers (+chcunningham/dalecurtis/wolenetz) to decide what's
> > > the
> > > best
> > > > > way to implement this.
> > > >
> > > > Ok, sounds good. Thanks for your comments!
> > >
> > >
> > >
> > > https://chromiumcodereview.appspot.com/1935873002/
> > >
> > 
> 
> Ok, so I've reworked this CL so that now:
> 1. PipelineImpl passes track status updates to the Demuxer implementation,
where
> they are handled.
> 2. Renderer is notified about media track status changes by subscribing to
> demuxerstream notifications.
> 
> I've also removed the previous mojo changes for now, since that'll need to be
> reworked to match the new model. I'll do that later in a separate CL.

Ping. Matt, Cris could you take a look? Let's get this landed before m53
branches.

[chcunningham, 2016-06-24 21:20:10]

Looking now.

[chcunningham, 2016-06-24 23:32:56]

This is much nicer now! Thanks for re-working. Kudos to Xhwang for great
feedback.

Big concern is testing. Do we have any tests that disable/enable tracks  and see
that renders end/resume? I think we need some! Its probably gonna take some
re-working of pipeline integration tests to achieve. Perhaps you have a more
clever idea?

My other main concerns are with stuff in the renderer impl. Beyond that its
mostly nits.

https://codereview.chromium.org/1935873002/diff/760001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/1935873002/diff/760001/media/base/demuxer.h#n...
media/base/demuxer.h:142: // |track_ids| is either empty or contains a single
video track id.
Line break above this?

https://codereview.chromium.org/1935873002/diff/760001/media/base/pipeline_im...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/1935873002/diff/760001/media/base/pipeline_im...
media/base/pipeline_impl.cc:571: // first video track gets selected by default
before the playback has been
What if something other than the first/default track is selected prior to
starting playback? Should we save the selection and apply once the demuxer comes
available? 

I'm not sure this is possible, but from a quick glance it seems it may sometimes
take a moment for the async initializition of the data_source in WMPI to
complete, meaning we may have a window of opportunity for non-default tracks to
be enabled.

https://codereview.chromium.org/1935873002/diff/760001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1935873002/diff/760001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:584: DVLOG(1) << __FUNCTION__ << " enabled:"
<< logstr.str();
Maybe promote this to a MEDIA_LOG - seems it could be helpful for app debugging.
Would need to drop the __FUNCTION__ and make it more verbose (web-dev friendly).

https://codereview.chromium.org/1935873002/diff/760001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:598: DVLOG(1) << __FUNCTION__ << " selected:
" << logstr.str();
Ditto

https://codereview.chromium.org/1935873002/diff/760001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1935873002/diff/760001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:637: // Note: We intentionally don't lock here,
since we are not accessing any
You are locking though ;)

https://codereview.chromium.org/1935873002/diff/760001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:642: if (track_ids.size() > 0) {
IIUC size should never exceed 1 as currently implemented. DCHECK?

https://codereview.chromium.org/1935873002/diff/760001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:643: base::AutoLock auto_lock(lock_);
Seems you're not settled on this part. I wonder if you could put the lock and
the dcheck inside an ifdef for when DCHECKING is enabled. I forget the specific
define... something like DCHECK_ON

https://codereview.chromium.org/1935873002/diff/760001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:653: // Note: We intentionally don't lock here,
since we are not accessing any
This method has the same unsettled state around the locking. Maybe take the same
approach here as I suggest above?

https://codereview.chromium.org/1935873002/diff/760001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1935873002/diff/760001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:367: return;
This would be good to DVLOG - "Dropping non-key frame pts:X"

https://codereview.chromium.org/1935873002/diff/800001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/1935873002/diff/800001/media/base/demuxer.h#n...
media/base/demuxer.h:142: // |track_ids| is either empty or contains a single
video track id.
New line above this comment?

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/audio_...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:294: ended_timestamp_ =
kInfiniteDuration();
This seems like the right thing - just curious how you found this wasn't being
done?

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

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:564: if (type == DemuxerStream::VIDEO &&
restarting_video_) {
For both audio and video, my previous comments still apply. You need to consider
the case where the stream never reaches HAVE_ENOUGH because of buffer underflow.
As a first idea, you could try something like the deferred underflow callback we
have for video underflow - basically, treat this as a potential underflow, but
cancel it if HAVE_ENOUGH comes in quickly thereafter. If the deferred callback
fails to become cancelled, it should re-enter this method, setting
restarting_{video/audio}_ to false, and ultimately pause playback via "//
Renderer underflowed" below. For video, it make sense to reuse the existing
deferred callback. For audio, a new one is needed.

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:573: video_renderer_->OnTimeStateChanged(true);
Re-posting discussion from earlier - ping xhwang or wolenetz. I think this may
have significant power implications. It wasn't really a big deal before this CL
(an EOS from video was almost always likely followed by a EOS from audio and
pipepline stop), but now that video could potentially be disabled for a long
time while audio runs, its important to stop the video sink's timers from
firing. 


me 2016/06/03 01:38:49
======================
Not really related to this line, but it triggered a thought...

I think we should make video renderer call OnTimeStateChanged(false) on itself
when it observes an EOS. Or we could do it in the renderer_impl when video
renderer signals its playback has ended. As it stands, I think the video sink's
timer will continue firing in spite of the track being disabled.


servolk 2016/06/03 02:53:15
===========================
Yes, it sounds reasonable to me. I'm not sure why that hasn't been done yet,
perhaps there are reasons why we need to keep ticking. I'll let Xiaohan or Matt
comment on this.

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:591: SetPlaybackRate(0);
My previous comment is not addressed. Reposting

Why does this help sync? I think you're already paused because you stopped
ticking prior to flush. I think you just need to call
time_source_->SetMediaTime() in OnAudioRendererFlushDone_ForRestart, and then
here you just need to StartTicking.

[chcunningham, 2016-06-24 23:46:06]

chcunningham@chromium.org changed reviewers:
- wolenetz@chromium.org

[chcunningham, 2016-06-24 23:46:08]

wolenetz and I have agreed to divide and conquer. I'll be on point for MSE for
this one. Moving him to CC

[servolk, 2016-06-25 00:36:33]

https://codereview.chromium.org/1935873002/diff/760001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/1935873002/diff/760001/media/base/demuxer.h#n...
media/base/demuxer.h:142: // |track_ids| is either empty or contains a single
video track id.
On 2016/06/24 23:32:55, chcunningham wrote:
> Line break above this?

Done.

https://codereview.chromium.org/1935873002/diff/760001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1935873002/diff/760001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:584: DVLOG(1) << __FUNCTION__ << " enabled:"
<< logstr.str();
On 2016/06/24 23:32:55, chcunningham wrote:
> Maybe promote this to a MEDIA_LOG - seems it could be helpful for app
debugging.
> Would need to drop the __FUNCTION__ and make it more verbose (web-dev
friendly).

Done.

https://codereview.chromium.org/1935873002/diff/760001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:598: DVLOG(1) << __FUNCTION__ << " selected:
" << logstr.str();
On 2016/06/24 23:32:55, chcunningham wrote:
> Ditto

Done.

https://codereview.chromium.org/1935873002/diff/760001/media/filters/chunk_de...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1935873002/diff/760001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:637: // Note: We intentionally don't lock here,
since we are not accessing any
On 2016/06/24 23:32:55, chcunningham wrote:
> You are locking though ;)

But only while accessing |track_id_to_demux_stream_map_|. I agree we can do that
only when DCHECK_IS_ON.

https://codereview.chromium.org/1935873002/diff/760001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:642: if (track_ids.size() > 0) {
On 2016/06/24 23:32:55, chcunningham wrote:
> IIUC size should never exceed 1 as currently implemented. DCHECK?

Done.

https://codereview.chromium.org/1935873002/diff/760001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:643: base::AutoLock auto_lock(lock_);
On 2016/06/24 23:32:55, chcunningham wrote:
> Seems you're not settled on this part. I wonder if you could put the lock and
> the dcheck inside an ifdef for when DCHECKING is enabled. I forget the
specific
> define... something like DCHECK_ON 

Done.

https://codereview.chromium.org/1935873002/diff/760001/media/filters/chunk_de...
media/filters/chunk_demuxer.cc:653: // Note: We intentionally don't lock here,
since we are not accessing any
On 2016/06/24 23:32:55, chcunningham wrote:
> This method has the same unsettled state around the locking. Maybe take the
same
> approach here as I suggest above?

Done.

https://codereview.chromium.org/1935873002/diff/760001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1935873002/diff/760001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:367: return;
On 2016/06/24 23:32:55, chcunningham wrote:
> This would be good to DVLOG - "Dropping non-key frame pts:X"

Done.

https://codereview.chromium.org/1935873002/diff/800001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://codereview.chromium.org/1935873002/diff/800001/media/base/demuxer.h#n...
media/base/demuxer.h:142: // |track_ids| is either empty or contains a single
video track id.
On 2016/06/24 23:32:55, chcunningham wrote:
> New line above this comment?

Done.

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/audio_...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:294: ended_timestamp_ =
kInfiniteDuration();
On 2016/06/24 23:32:55, chcunningham wrote:
> This seems like the right thing - just curious how you found this wasn't being
> done?

I've been testing this CL manually, trying a bunch of different things
(enable/disable a/v streams, seek/pause/resume while one of the streams is
disabled, etc), and noticed that audio stream playback would sometimes stall
when re-enabling and audio stream after it's been disabled and a seek to a
different position has occured. I've debugged that stall and noticed that we
need to reset |ended_timestamp_| during the Flush even if that Flush is not
followed by SetMediaTime, since when we are restarting the audio stream, we do a
flush, but we don't do an explicit SetMediaTime.

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

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:564: if (type == DemuxerStream::VIDEO &&
restarting_video_) {
On 2016/06/24 23:32:55, chcunningham wrote:
> For both audio and video, my previous comments still apply. You need to
consider
> the case where the stream never reaches HAVE_ENOUGH because of buffer
underflow.
> As a first idea, you could try something like the deferred underflow callback
we
> have for video underflow - basically, treat this as a potential underflow, but
> cancel it if HAVE_ENOUGH comes in quickly thereafter. If the deferred callback
> fails to become cancelled, it should re-enter this method, setting
> restarting_{video/audio}_ to false, and ultimately pause playback via "//
> Renderer underflowed" below. For video, it make sense to reuse the existing
> deferred callback. For audio, a new one is needed. 

Well, I have looked into this some more after your comment, but then I've
noticed that we never actually report the HAVE_NOTHING to the RendererClient
(see the comment at lines 641-642 below). So even if we do use the deferred
underflow callback here and it gets triggered, then what happens next? We never
report that condition up the media stack. We will just sit in the paused state
until more video data becomes available, and at that point we'll resume
playback. How is this better than simply continuing to play audio until more
video becomes available? I think by not underflowing/pausing due to the resuming
stream we would get better use experience.

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:573: video_renderer_->OnTimeStateChanged(true);
On 2016/06/24 23:32:55, chcunningham wrote:
> Re-posting discussion from earlier - ping xhwang or wolenetz. I think this may
> have significant power implications. It wasn't really a big deal before this
CL
> (an EOS from video was almost always likely followed by a EOS from audio and
> pipepline stop), but now that video could potentially be disabled for a long
> time while audio runs, its important to stop the video sink's timers from
> firing. 
> 
> 
> me 2016/06/03 01:38:49
> ======================
> Not really related to this line, but it triggered a thought...
> 
> I think we should make video renderer call OnTimeStateChanged(false) on itself
> when it observes an EOS. Or we could do it in the renderer_impl when video
> renderer signals its playback has ended. As it stands, I think the video
sink's
> timer will continue firing in spite of the track being disabled.
> 
> 
> servolk 2016/06/03 02:53:15
> ===========================
> Yes, it sounds reasonable to me. I'm not sure why that hasn't been done yet,
> perhaps there are reasons why we need to keep ticking. I'll let Xiaohan or
Matt
> comment on this.

Acknowledged. Should we open a separate bug for this? I agree that this might
have implications beyond the scenario of enabled/disabled tracks. It think it
should be safe to stop ticking when eos is reached.

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:591: SetPlaybackRate(0);
On 2016/06/24 23:32:55, chcunningham wrote:
> My previous comment is not addressed. Reposting
> 
> Why does this help sync? I think you're already paused because you stopped
> ticking prior to flush. I think you just need to call
> time_source_->SetMediaTime() in OnAudioRendererFlushDone_ForRestart, and then
> here you just need to StartTicking. 

Prior to this point only the audio renderer is paused. The video renderer is
still rendering and ticking! So SetPlaybackRate(0) pauses both renderers and
then SetPlaybackRate(curr_playback_rate) restarts both renderers at the same
time. I have noticed that without this (if I simply resumed ticking on the
time_source_) a/v sync was slightly off. To be honest I didn't get to the bottom
of why the sync was off, I just figured that if we did pause and resume here,
that would go through all the necessary steps that we usually take to ensure a/v
sync after restarting playback from a fully paused state (where both streams
were paused). I tried this and it worked :)

[chcunningham, 2016-06-27 20:31:52]

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

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:564: if (type == DemuxerStream::VIDEO &&
restarting_video_) {
> Well, I have looked into this some more after your comment, but then I've
noticed that we never actually report the HAVE_NOTHING

This is true, but its also a bug that I'm soon fixing. See
http://crbug.com/144683. The current situation is not to spec and is not
interoperable. I don't see why a track being disabled/enabled absolves it of
responsibility to correctly report its readyState. 

> How is this better than simply continuing to play audio until more video
becomes available?

Its better because doing so matches the spec and helps interop. 

Furthermore, I can imagine scenarios where after enabling the video track where
not enough is buffered and I continue hearing audio play through some content
for which video is hugely important to the overall value. In such a case, it
would be better for playback to have paused while the video is buffered.

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:573: video_renderer_->OnTimeStateChanged(true);
On 2016/06/25 00:36:33, servolk wrote:
> On 2016/06/24 23:32:55, chcunningham wrote:
> > Re-posting discussion from earlier - ping xhwang or wolenetz. I think this
may
> > have significant power implications. It wasn't really a big deal before this
> CL
> > (an EOS from video was almost always likely followed by a EOS from audio and
> > pipepline stop), but now that video could potentially be disabled for a long
> > time while audio runs, its important to stop the video sink's timers from
> > firing. 
> > 
> > 
> > me 2016/06/03 01:38:49
> > ======================
> > Not really related to this line, but it triggered a thought...
> > 
> > I think we should make video renderer call OnTimeStateChanged(false) on
itself
> > when it observes an EOS. Or we could do it in the renderer_impl when video
> > renderer signals its playback has ended. As it stands, I think the video
> sink's
> > timer will continue firing in spite of the track being disabled.
> > 
> > 
> > servolk 2016/06/03 02:53:15
> > ===========================
> > Yes, it sounds reasonable to me. I'm not sure why that hasn't been done yet,
> > perhaps there are reasons why we need to keep ticking. I'll let Xiaohan or
> Matt
> > comment on this.
> 
> Acknowledged. Should we open a separate bug for this? I agree that this might
> have implications beyond the scenario of enabled/disabled tracks. It think it
> should be safe to stop ticking when eos is reached.

Since xhwang is AFK and wolenetz is just CC, I reached out to Dale to see if
this was deliberately not being done or if there were any expected side effects.
He says this additional call to OnTimeStateChanged(false) (or done internally in
response to rendering EOS) is probably fine for us to do. 

>> I agree that this might have implications beyond the scenario of
enabled/disabled tracks.

Sorry, this is the opposite of the meaning I intended. I think beyond
enabled/disabled tracks the implications are very narrow if any at all. I think
your introducing this feature *introduces* significant reasons to stop the sink
on EOS. Can you do it in this CL?

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:591: SetPlaybackRate(0);
Can you instead try to audio_renderer_->SetMediaTime before
audio_renderer_->StartPlaying in RestartAudioRenderer above? Then here, I think
you'll be able to remove these SetPlaybackRate() calls and just call
time_source_->StartTicking().

I *think* whats happening here is the value of currentTime has grown a little
stale in the time it takes to seek and get packets out of the demuxers. This
means that we rendered some silence at time currentTime and by the time we
actually start rendering the audio intended for currentTime its really rendering
at currentTime + delta. 

I'm not certain why pausing the streams works, but my suggestion *should* also
work in a less magical fashion. LMK if you find its not working or has some side
effect.

[chcunningham, 2016-06-27 20:35:14]

Also, this feature still needs automated testing.

[servolk, 2016-06-28 00:15:58]

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

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:564: if (type == DemuxerStream::VIDEO &&
restarting_video_) {
On 2016/06/27 20:31:52, chcunningham wrote:
> > Well, I have looked into this some more after your comment, but then I've
> noticed that we never actually report the HAVE_NOTHING
> 
> This is true, but its also a bug that I'm soon fixing. See
> http://crbug.com/144683. The current situation is not to spec and is not
> interoperable. I don't see why a track being disabled/enabled absolves it of
> responsibility to correctly report its readyState. 
> 
> > How is this better than simply continuing to play audio until more video
> becomes available?
> 
> Its better because doing so matches the spec and helps interop. 
> 
> Furthermore, I can imagine scenarios where after enabling the video track
where
> not enough is buffered and I continue hearing audio play through some content
> for which video is hugely important to the overall value. In such a case, it
> would be better for playback to have paused while the video is buffered. 

Ok, sure, if you are fixing the delivery of HAVE_NOTHING notifications to
renderer clients, then yes, we can implement the deferral logic here.
PS In fact, after I've tried to do this, I've realized that the deferral logic
here (when we are restarting video playback) would be exactly the same as in the
regular continuing playback case, so we can just to allow fall through to the
code below, on lines 603-618, to allow the deferred cb to be created and posted.
The only change that we need is when we get HAVE_ENOUGH for the video stream
which is being restarted. Then we need to check if the deferred cb has been
triggered (in which case playback has been fully paused, i.e. both audio and
video were paused) or not (in which case we need to resume playback of only
video renderer).

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:573: video_renderer_->OnTimeStateChanged(true);
On 2016/06/27 20:31:52, chcunningham wrote:
> On 2016/06/25 00:36:33, servolk wrote:
> > On 2016/06/24 23:32:55, chcunningham wrote:
> > > Re-posting discussion from earlier - ping xhwang or wolenetz. I think this
> may
> > > have significant power implications. It wasn't really a big deal before
this
> > CL
> > > (an EOS from video was almost always likely followed by a EOS from audio
and
> > > pipepline stop), but now that video could potentially be disabled for a
long
> > > time while audio runs, its important to stop the video sink's timers from
> > > firing. 
> > > 
> > > 
> > > me 2016/06/03 01:38:49
> > > ======================
> > > Not really related to this line, but it triggered a thought...
> > > 
> > > I think we should make video renderer call OnTimeStateChanged(false) on
> itself
> > > when it observes an EOS. Or we could do it in the renderer_impl when video
> > > renderer signals its playback has ended. As it stands, I think the video
> > sink's
> > > timer will continue firing in spite of the track being disabled.
> > > 
> > > 
> > > servolk 2016/06/03 02:53:15
> > > ===========================
> > > Yes, it sounds reasonable to me. I'm not sure why that hasn't been done
yet,
> > > perhaps there are reasons why we need to keep ticking. I'll let Xiaohan or
> > Matt
> > > comment on this.
> > 
> > Acknowledged. Should we open a separate bug for this? I agree that this
might
> > have implications beyond the scenario of enabled/disabled tracks. It think
it
> > should be safe to stop ticking when eos is reached.
> 
> Since xhwang is AFK and wolenetz is just CC, I reached out to Dale to see if
> this was deliberately not being done or if there were any expected side
effects.
> He says this additional call to OnTimeStateChanged(false) (or done internally
in
> response to rendering EOS) is probably fine for us to do. 
> 
> >> I agree that this might have implications beyond the scenario of
> enabled/disabled tracks.
> 
> Sorry, this is the opposite of the meaning I intended. I think beyond
> enabled/disabled tracks the implications are very narrow if any at all. I
think
> your introducing this feature *introduces* significant reasons to stop the
sink
> on EOS. Can you do it in this CL?

Done.

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:591: SetPlaybackRate(0);
On 2016/06/27 20:31:52, chcunningham wrote:
> Can you instead try to audio_renderer_->SetMediaTime before
> audio_renderer_->StartPlaying in RestartAudioRenderer above? Then here, I
think
> you'll be able to remove these SetPlaybackRate() calls and just call
> time_source_->StartTicking().
> 
> I *think* whats happening here is the value of currentTime has grown a little
> stale in the time it takes to seek and get packets out of the demuxers. This
> means that we rendered some silence at time currentTime and by the time we
> actually start rendering the audio intended for currentTime its really
rendering
> at currentTime + delta. 
> 
> I'm not certain why pausing the streams works, but my suggestion *should* also
> work in a less magical fashion. LMK if you find its not working or has some
side
> effect.

We need to be very careful here. audio_renderer_->SetMediaTime recreates the
audio clock (see
https://cs.chromium.org/chromium/src/media/renderers/audio_renderer_impl.cc?r...).
I'm not sure if it's safe to do that while video is still rendering (and
potentially might try to access that clock any time).
I'll need to ponder this some more and play with different implementations to
see what can be done here.

[servolk, 2016-06-28 02:05:43]

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

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:591: SetPlaybackRate(0);
On 2016/06/28 00:15:58, servolk wrote:
> On 2016/06/27 20:31:52, chcunningham wrote:
> > Can you instead try to audio_renderer_->SetMediaTime before
> > audio_renderer_->StartPlaying in RestartAudioRenderer above? Then here, I
> think
> > you'll be able to remove these SetPlaybackRate() calls and just call
> > time_source_->StartTicking().
> > 
> > I *think* whats happening here is the value of currentTime has grown a
little
> > stale in the time it takes to seek and get packets out of the demuxers. This
> > means that we rendered some silence at time currentTime and by the time we
> > actually start rendering the audio intended for currentTime its really
> rendering
> > at currentTime + delta. 
> > 
> > I'm not certain why pausing the streams works, but my suggestion *should*
also
> > work in a less magical fashion. LMK if you find its not working or has some
> side
> > effect.
> 
> We need to be very careful here. audio_renderer_->SetMediaTime recreates the
> audio clock (see
>
https://cs.chromium.org/chromium/src/media/renderers/audio_renderer_impl.cc?r...).
> I'm not sure if it's safe to do that while video is still rendering (and
> potentially might try to access that clock any time).
> I'll need to ponder this some more and play with different implementations to
> see what can be done here.

Btw, just FYI: When I tried to set audio_renderer_->SetMediaTime in
RestartAudioRenderer, I got a dcheck when restarting an audio stream:

[1:3:0627/185900:FATAL:video_renderer_algorithm.cc(317)] Check failed:
frame_queue_[last_frame_index_].drop_count <=
frame_queue_[last_frame_index_].render_count (2 vs. 1)
#0 0x7fd0700a161e base::debug::StackTrace::StackTrace()
#1 0x7fd07010575f logging::LogMessage::~LogMessage()
#2 0x7fd066ad3b9b media::VideoRendererAlgorithm::OnLastFrameDropped()
#3 0x7fd066b38d84 media::VideoRendererImpl::OnFrameDropped()
#4 0x7fd05a89e648 media::VideoFrameCompositor::CallRender()
#5 0x7fd05a89e340 media::VideoFrameCompositor::UpdateCurrentFrame()
#6 0x7fd0676050f9 cc::VideoFrameProviderClientImpl::OnBeginFrame()
#7 0x7fd067813872 cc::LayerTreeHostImpl::WillBeginImplFrame()
#8 0x7fd0678a371b cc::ProxyImpl::WillBeginImplFrame()
#9 0x7fd06774dc94 cc::Scheduler::BeginImplFrame()
#10 0x7fd06774cd09 cc::Scheduler::BeginImplFrameWithDeadline()
#11 0x7fd06774c32c cc::Scheduler::OnBeginFrameDerivedImpl()
#12 0x7fd067737f3a cc::BeginFrameObserverBase::OnBeginFrame()
#13 0x7fd06ab4cd78 content::CompositorExternalBeginFrameSource::OnBeginFrame()

I also tried to change this to be more similar to how we handle the video stream
restart (i.e. allow the HAVE_NOTHING to fall through to the regular audio
underflow code path below), but that's also problematic, since regular underflow
calls time_source_->StopTicking() and that DCHECKs, since StopTicking expects
the audio renderer to be in the kPlaying state, and not in kFlushed state.
Getting this right is very tricky, and so far this has been the only way I could
make it work reliably. I'll try some more tomorrow, but I'm running out of
ideas. How else can we resume playback here without stopping/flushing the video
renderer?

[servolk, 2016-06-28 18:42:22]

Improve audio stream restart handling

[servolk, 2016-06-28 18:51:44]

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

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:591: SetPlaybackRate(0);
On 2016/06/28 02:05:43, servolk wrote:
> On 2016/06/28 00:15:58, servolk wrote:
> > On 2016/06/27 20:31:52, chcunningham wrote:
> > > Can you instead try to audio_renderer_->SetMediaTime before
> > > audio_renderer_->StartPlaying in RestartAudioRenderer above? Then here, I
> > think
> > > you'll be able to remove these SetPlaybackRate() calls and just call
> > > time_source_->StartTicking().
> > > 
> > > I *think* whats happening here is the value of currentTime has grown a
> little
> > > stale in the time it takes to seek and get packets out of the demuxers.
This
> > > means that we rendered some silence at time currentTime and by the time we
> > > actually start rendering the audio intended for currentTime its really
> > rendering
> > > at currentTime + delta. 
> > > 
> > > I'm not certain why pausing the streams works, but my suggestion *should*
> also
> > > work in a less magical fashion. LMK if you find its not working or has
some
> > side
> > > effect.
> > 
> > We need to be very careful here. audio_renderer_->SetMediaTime recreates the
> > audio clock (see
> >
>
https://cs.chromium.org/chromium/src/media/renderers/audio_renderer_impl.cc?r...).
> > I'm not sure if it's safe to do that while video is still rendering (and
> > potentially might try to access that clock any time).
> > I'll need to ponder this some more and play with different implementations
to
> > see what can be done here.
> 
> Btw, just FYI: When I tried to set audio_renderer_->SetMediaTime in
> RestartAudioRenderer, I got a dcheck when restarting an audio stream:
> 
> [1:3:0627/185900:FATAL:video_renderer_algorithm.cc(317)] Check failed:
> frame_queue_[last_frame_index_].drop_count <=
> frame_queue_[last_frame_index_].render_count (2 vs. 1)
> #0 0x7fd0700a161e base::debug::StackTrace::StackTrace()
> #1 0x7fd07010575f logging::LogMessage::~LogMessage()
> #2 0x7fd066ad3b9b media::VideoRendererAlgorithm::OnLastFrameDropped()
> #3 0x7fd066b38d84 media::VideoRendererImpl::OnFrameDropped()
> #4 0x7fd05a89e648 media::VideoFrameCompositor::CallRender()
> #5 0x7fd05a89e340 media::VideoFrameCompositor::UpdateCurrentFrame()
> #6 0x7fd0676050f9 cc::VideoFrameProviderClientImpl::OnBeginFrame()
> #7 0x7fd067813872 cc::LayerTreeHostImpl::WillBeginImplFrame()
> #8 0x7fd0678a371b cc::ProxyImpl::WillBeginImplFrame()
> #9 0x7fd06774dc94 cc::Scheduler::BeginImplFrame()
> #10 0x7fd06774cd09 cc::Scheduler::BeginImplFrameWithDeadline()
> #11 0x7fd06774c32c cc::Scheduler::OnBeginFrameDerivedImpl()
> #12 0x7fd067737f3a cc::BeginFrameObserverBase::OnBeginFrame()
> #13 0x7fd06ab4cd78 content::CompositorExternalBeginFrameSource::OnBeginFrame()
> 
> I also tried to change this to be more similar to how we handle the video
stream
> restart (i.e. allow the HAVE_NOTHING to fall through to the regular audio
> underflow code path below), but that's also problematic, since regular
underflow
> calls time_source_->StopTicking() and that DCHECKs, since StopTicking expects
> the audio renderer to be in the kPlaying state, and not in kFlushed state.
> Getting this right is very tricky, and so far this has been the only way I
could
> make it work reliably. I'll try some more tomorrow, but I'm running out of
> ideas. How else can we resume playback here without stopping/flushing the
video
> renderer?

I think we can make this better by allowing PausePlayback to work correctly even
if ticking of the time_source_ has been stopped already. Then we can just pause
the video playback here via PausePlayback and since we have just got HAVE_ENOUGH
for audio here, both audio and video playback will be resumed immediately below
on the line 648. PTAL at patchset 47 and see if you like this better.
Next I'll see if it would make sense to introduce a deferred audio overflow cb,
similar to what we do for video. The video underflow handling logic is already
pretty complicated (take a look at 603-608 below), adding another deferred
underflow for audio might make it even more complex, since the number of
possible states will increase.

[servolk, 2016-06-28 23:03:40]

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

https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
media/renderers/renderer_impl.cc:591: SetPlaybackRate(0);
On 2016/06/28 18:51:44, servolk wrote:
> On 2016/06/28 02:05:43, servolk wrote:
> > On 2016/06/28 00:15:58, servolk wrote:
> > > On 2016/06/27 20:31:52, chcunningham wrote:
> > > > Can you instead try to audio_renderer_->SetMediaTime before
> > > > audio_renderer_->StartPlaying in RestartAudioRenderer above? Then here,
I
> > > think
> > > > you'll be able to remove these SetPlaybackRate() calls and just call
> > > > time_source_->StartTicking().
> > > > 
> > > > I *think* whats happening here is the value of currentTime has grown a
> > little
> > > > stale in the time it takes to seek and get packets out of the demuxers.
> This
> > > > means that we rendered some silence at time currentTime and by the time
we
> > > > actually start rendering the audio intended for currentTime its really
> > > rendering
> > > > at currentTime + delta. 
> > > > 
> > > > I'm not certain why pausing the streams works, but my suggestion
*should*
> > also
> > > > work in a less magical fashion. LMK if you find its not working or has
> some
> > > side
> > > > effect.
> > > 
> > > We need to be very careful here. audio_renderer_->SetMediaTime recreates
the
> > > audio clock (see
> > >
> >
>
https://cs.chromium.org/chromium/src/media/renderers/audio_renderer_impl.cc?r...).
> > > I'm not sure if it's safe to do that while video is still rendering (and
> > > potentially might try to access that clock any time).
> > > I'll need to ponder this some more and play with different implementations
> to
> > > see what can be done here.
> > 
> > Btw, just FYI: When I tried to set audio_renderer_->SetMediaTime in
> > RestartAudioRenderer, I got a dcheck when restarting an audio stream:
> > 
> > [1:3:0627/185900:FATAL:video_renderer_algorithm.cc(317)] Check failed:
> > frame_queue_[last_frame_index_].drop_count <=
> > frame_queue_[last_frame_index_].render_count (2 vs. 1)
> > #0 0x7fd0700a161e base::debug::StackTrace::StackTrace()
> > #1 0x7fd07010575f logging::LogMessage::~LogMessage()
> > #2 0x7fd066ad3b9b media::VideoRendererAlgorithm::OnLastFrameDropped()
> > #3 0x7fd066b38d84 media::VideoRendererImpl::OnFrameDropped()
> > #4 0x7fd05a89e648 media::VideoFrameCompositor::CallRender()
> > #5 0x7fd05a89e340 media::VideoFrameCompositor::UpdateCurrentFrame()
> > #6 0x7fd0676050f9 cc::VideoFrameProviderClientImpl::OnBeginFrame()
> > #7 0x7fd067813872 cc::LayerTreeHostImpl::WillBeginImplFrame()
> > #8 0x7fd0678a371b cc::ProxyImpl::WillBeginImplFrame()
> > #9 0x7fd06774dc94 cc::Scheduler::BeginImplFrame()
> > #10 0x7fd06774cd09 cc::Scheduler::BeginImplFrameWithDeadline()
> > #11 0x7fd06774c32c cc::Scheduler::OnBeginFrameDerivedImpl()
> > #12 0x7fd067737f3a cc::BeginFrameObserverBase::OnBeginFrame()
> > #13 0x7fd06ab4cd78
content::CompositorExternalBeginFrameSource::OnBeginFrame()
> > 
> > I also tried to change this to be more similar to how we handle the video
> stream
> > restart (i.e. allow the HAVE_NOTHING to fall through to the regular audio
> > underflow code path below), but that's also problematic, since regular
> underflow
> > calls time_source_->StopTicking() and that DCHECKs, since StopTicking
expects
> > the audio renderer to be in the kPlaying state, and not in kFlushed state.
> > Getting this right is very tricky, and so far this has been the only way I
> could
> > make it work reliably. I'll try some more tomorrow, but I'm running out of
> > ideas. How else can we resume playback here without stopping/flushing the
> video
> > renderer?
> 
> I think we can make this better by allowing PausePlayback to work correctly
even
> if ticking of the time_source_ has been stopped already. Then we can just
pause
> the video playback here via PausePlayback and since we have just got
HAVE_ENOUGH
> for audio here, both audio and video playback will be resumed immediately
below
> on the line 648. PTAL at patchset 47 and see if you like this better.
> Next I'll see if it would make sense to introduce a deferred audio overflow
cb,
> similar to what we do for video. The video underflow handling logic is already
> pretty complicated (take a look at 603-608 below), adding another deferred
> underflow for audio might make it even more complex, since the number of
> possible states will increase.

Ok, and patchset #48 implements deferred reporting for audio underflows when
audio stream is being restarted. So I believe functionally this CL should be
complete. Please take another look at it. In the meantime I'll see into adding
some tests for this functionality.

[servolk, 2016-06-29 21:55:37]

On 2016/06/28 23:03:40, servolk wrote:
>
https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
> File media/renderers/renderer_impl.cc (right):
> 
>
https://codereview.chromium.org/1935873002/diff/820001/media/renderers/render...
> media/renderers/renderer_impl.cc:591: SetPlaybackRate(0);
> On 2016/06/28 18:51:44, servolk wrote:
> > On 2016/06/28 02:05:43, servolk wrote:
> > > On 2016/06/28 00:15:58, servolk wrote:
> > > > On 2016/06/27 20:31:52, chcunningham wrote:
> > > > > Can you instead try to audio_renderer_->SetMediaTime before
> > > > > audio_renderer_->StartPlaying in RestartAudioRenderer above? Then
here,
> I
> > > > think
> > > > > you'll be able to remove these SetPlaybackRate() calls and just call
> > > > > time_source_->StartTicking().
> > > > > 
> > > > > I *think* whats happening here is the value of currentTime has grown a
> > > little
> > > > > stale in the time it takes to seek and get packets out of the
demuxers.
> > This
> > > > > means that we rendered some silence at time currentTime and by the
time
> we
> > > > > actually start rendering the audio intended for currentTime its really
> > > > rendering
> > > > > at currentTime + delta. 
> > > > > 
> > > > > I'm not certain why pausing the streams works, but my suggestion
> *should*
> > > also
> > > > > work in a less magical fashion. LMK if you find its not working or has
> > some
> > > > side
> > > > > effect.
> > > > 
> > > > We need to be very careful here. audio_renderer_->SetMediaTime recreates
> the
> > > > audio clock (see
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/media/renderers/audio_renderer_impl.cc?r...).
> > > > I'm not sure if it's safe to do that while video is still rendering (and
> > > > potentially might try to access that clock any time).
> > > > I'll need to ponder this some more and play with different
implementations
> > to
> > > > see what can be done here.
> > > 
> > > Btw, just FYI: When I tried to set audio_renderer_->SetMediaTime in
> > > RestartAudioRenderer, I got a dcheck when restarting an audio stream:
> > > 
> > > [1:3:0627/185900:FATAL:video_renderer_algorithm.cc(317)] Check failed:
> > > frame_queue_[last_frame_index_].drop_count <=
> > > frame_queue_[last_frame_index_].render_count (2 vs. 1)
> > > #0 0x7fd0700a161e base::debug::StackTrace::StackTrace()
> > > #1 0x7fd07010575f logging::LogMessage::~LogMessage()
> > > #2 0x7fd066ad3b9b media::VideoRendererAlgorithm::OnLastFrameDropped()
> > > #3 0x7fd066b38d84 media::VideoRendererImpl::OnFrameDropped()
> > > #4 0x7fd05a89e648 media::VideoFrameCompositor::CallRender()
> > > #5 0x7fd05a89e340 media::VideoFrameCompositor::UpdateCurrentFrame()
> > > #6 0x7fd0676050f9 cc::VideoFrameProviderClientImpl::OnBeginFrame()
> > > #7 0x7fd067813872 cc::LayerTreeHostImpl::WillBeginImplFrame()
> > > #8 0x7fd0678a371b cc::ProxyImpl::WillBeginImplFrame()
> > > #9 0x7fd06774dc94 cc::Scheduler::BeginImplFrame()
> > > #10 0x7fd06774cd09 cc::Scheduler::BeginImplFrameWithDeadline()
> > > #11 0x7fd06774c32c cc::Scheduler::OnBeginFrameDerivedImpl()
> > > #12 0x7fd067737f3a cc::BeginFrameObserverBase::OnBeginFrame()
> > > #13 0x7fd06ab4cd78
> content::CompositorExternalBeginFrameSource::OnBeginFrame()
> > > 
> > > I also tried to change this to be more similar to how we handle the video
> > stream
> > > restart (i.e. allow the HAVE_NOTHING to fall through to the regular audio
> > > underflow code path below), but that's also problematic, since regular
> > underflow
> > > calls time_source_->StopTicking() and that DCHECKs, since StopTicking
> expects
> > > the audio renderer to be in the kPlaying state, and not in kFlushed state.
> > > Getting this right is very tricky, and so far this has been the only way I
> > could
> > > make it work reliably. I'll try some more tomorrow, but I'm running out of
> > > ideas. How else can we resume playback here without stopping/flushing the
> > video
> > > renderer?
> > 
> > I think we can make this better by allowing PausePlayback to work correctly
> even
> > if ticking of the time_source_ has been stopped already. Then we can just
> pause
> > the video playback here via PausePlayback and since we have just got
> HAVE_ENOUGH
> > for audio here, both audio and video playback will be resumed immediately
> below
> > on the line 648. PTAL at patchset 47 and see if you like this better.
> > Next I'll see if it would make sense to introduce a deferred audio overflow
> cb,
> > similar to what we do for video. The video underflow handling logic is
already
> > pretty complicated (take a look at 603-608 below), adding another deferred
> > underflow for audio might make it even more complex, since the number of
> > possible states will increase.
> 
> Ok, and patchset #48 implements deferred reporting for audio underflows when
> audio stream is being restarted. So I believe functionally this CL should be
> complete. Please take another look at it. In the meantime I'll see into adding
> some tests for this functionality.

And in the patchset #51 I've added a couple of integration tests to provide test
coverage for this functionality. So I think it's ready to land now. PTAL.

[chcunningham, 2016-06-29 22:49:13]

https://codereview.chromium.org/1935873002/diff/980001/media/renderers/render...
File media/renderers/renderer_impl.cc (left):

https://codereview.chromium.org/1935873002/diff/980001/media/renderers/render...
media/renderers/renderer_impl.cc:491: void
RendererImpl::OnBufferingStateChange(DemuxerStream::Type type,
This method was once very simple, but it now has lots of distinct paths for
audio and video. Can you break this into the sub methods along those lines? It
should be possible to still have in this method or some other submethod the
common ending where we pause/play and callback to the client_

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

https://codereview.chromium.org/1935873002/diff/980001/media/renderers/render...
media/renderers/renderer_impl.cc:264: audio_renderer_->StartPlaying();
I think you still need to call time_source_->SetMediaTime(time) right before you
call StartPlaying. 

As you pointed out this will destroy the audio_clock and make a new one while
the video renderer is still playing, but I believe this is actually safe to do
because both AudioRendererImpl::SetMediaTime and ARI::GetWallClockTimes (which
video renderer calls for AV sync) are guarded by the same lock. I've tried
making this call here (patched your cl) and do not observe any dcheck/check
errors.

I wasn't able to develop a theory about how PausePlayback is improving AV
sync... the fact that it doesn't change the state of the audio_clock means that
any wrongness in the audio_clock should be preserved accross the pause. So I
patched your CL and tried to investigate ... In my testing I was not able to
introduce any noticeable drift with OR without your PausePlayback call. I
disabled the audio track in a 1 second interval for 5 straight minutes...
couldn't see anything significant. Still, I am confident that it is
theoretically possible to introduce drift if we do not reset the clock (see my
previous theory about staleness of currentTime). I could probably get this occur
with an hours-long video, but you're hoping to land soon.

https://codereview.chromium.org/1935873002/diff/980001/media/renderers/render...
media/renderers/renderer_impl.cc:607: if (deferred_underflow_cb_.IsCancelled()
&&
can you rename this to make it clear that this one is for video (now that we
have one for audio too)

https://codereview.chromium.org/1935873002/diff/980001/media/renderers/render...
media/renderers/renderer_impl.cc:629: PausePlayback();
As mentioned above, I think this should go. I don't think it helps sync.

https://codereview.chromium.org/1935873002/diff/980001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1935873002/diff/980001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:981: EXPECT_HASH_EQ(kNullVideoHash,
GetVideoHash());
These are a great start. Can you add some test for the restart-renderer paths?

[chcunningham, 2016-06-29 22:58:29]

Tests look good

https://codereview.chromium.org/1935873002/diff/980001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1935873002/diff/980001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:981: EXPECT_HASH_EQ(kNullVideoHash,
GetVideoHash());
On 2016/06/29 22:49:13, chcunningham wrote:
> These are a great start. Can you add some test for the restart-renderer paths?

Ignore me - I see you did this in your latest PS

https://codereview.chromium.org/1935873002/diff/1000001/media/test/pipeline_i...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1935873002/diff/1000001/media/test/pipeline_i...
media/test/pipeline_integration_test.cc:1021:
EXPECT_HASH_EQ("fd59357dfd9c144ab4fb8181b2de32c3", GetVideoHash());
Make sure you run these 10 times or so on your local machine. I've hit bugs
where checking a hash after a seek is not reliable. See http://crbug.com/583190
(though it may just be the mp3 demuxer... not sure).

[servolk, 2016-06-29 23:40:13]

https://codereview.chromium.org/1935873002/diff/980001/media/renderers/render...
File media/renderers/renderer_impl.cc (left):

https://codereview.chromium.org/1935873002/diff/980001/media/renderers/render...
media/renderers/renderer_impl.cc:491: void
RendererImpl::OnBufferingStateChange(DemuxerStream::Type type,
On 2016/06/29 22:49:13, chcunningham wrote:
> This method was once very simple, but it now has lots of distinct paths for
> audio and video. Can you break this into the sub methods along those lines? It
> should be possible to still have in this method or some other submethod the
> common ending where we pause/play and callback to the client_

Done.

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

https://codereview.chromium.org/1935873002/diff/980001/media/renderers/render...
media/renderers/renderer_impl.cc:264: audio_renderer_->StartPlaying();
On 2016/06/29 22:49:13, chcunningham wrote:
> I think you still need to call time_source_->SetMediaTime(time) right before
you
> call StartPlaying. 
> 
> As you pointed out this will destroy the audio_clock and make a new one while
> the video renderer is still playing, but I believe this is actually safe to do
> because both AudioRendererImpl::SetMediaTime and ARI::GetWallClockTimes (which
> video renderer calls for AV sync) are guarded by the same lock. I've tried
> making this call here (patched your cl) and do not observe any dcheck/check
> errors.
> 
> I wasn't able to develop a theory about how PausePlayback is improving AV
> sync... the fact that it doesn't change the state of the audio_clock means
that
> any wrongness in the audio_clock should be preserved accross the pause. So I
> patched your CL and tried to investigate ... In my testing I was not able to
> introduce any noticeable drift with OR without your PausePlayback call. I
> disabled the audio track in a 1 second interval for 5 straight minutes...
> couldn't see anything significant. Still, I am confident that it is
> theoretically possible to introduce drift if we do not reset the clock (see my
> previous theory about staleness of currentTime). I could probably get this
occur
> with an hours-long video, but you're hoping to land soon. 

Ok, maybe that was a stray unrelated dcheck then, and I didn't get it on the
first try either (but still fairly easily, on like a second or third attempt).
I'll give it another try.

https://codereview.chromium.org/1935873002/diff/980001/media/renderers/render...
media/renderers/renderer_impl.cc:607: if (deferred_underflow_cb_.IsCancelled()
&&
On 2016/06/29 22:49:13, chcunningham wrote:
> can you rename this to make it clear that this one is for video (now that we
> have one for audio too)

Done.

https://codereview.chromium.org/1935873002/diff/980001/media/renderers/render...
media/renderers/renderer_impl.cc:629: PausePlayback();
On 2016/06/29 22:49:13, chcunningham wrote:
> As mentioned above, I think this should go. I don't think it helps sync. 

Acknowledged.

https://codereview.chromium.org/1935873002/diff/980001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1935873002/diff/980001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:981: EXPECT_HASH_EQ(kNullVideoHash,
GetVideoHash());
On 2016/06/29 22:58:29, chcunningham wrote:
> On 2016/06/29 22:49:13, chcunningham wrote:
> > These are a great start. Can you add some test for the restart-renderer
paths?
> 
> Ignore me - I see you did this in your latest PS

Yep, added more complete tests in latest PS.

https://codereview.chromium.org/1935873002/diff/1000001/media/test/pipeline_i...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1935873002/diff/1000001/media/test/pipeline_i...
media/test/pipeline_integration_test.cc:1021:
EXPECT_HASH_EQ("fd59357dfd9c144ab4fb8181b2de32c3", GetVideoHash());
On 2016/06/29 22:58:29, chcunningham wrote:
> Make sure you run these 10 times or so on your local machine. I've hit bugs
> where checking a hash after a seek is not reliable. See
http://crbug.com/583190
> (though it may just be the mp3 demuxer... not sure).

Yes, my first attempts were also flaky at first, since I didn't take into
account that video preroll might result in video frames being processed (hashed)
even before playback is started. That's why I had to introduce those extra seeks
and ResetVideoHash. Now it's been stable, I ran those tests individually and as
part of the whole media_unittests suite about ~20 times now and haven't seen a
single failure.

[servolk, 2016-06-30 00:06:11]

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

https://codereview.chromium.org/1935873002/diff/980001/media/renderers/render...
media/renderers/renderer_impl.cc:264: audio_renderer_->StartPlaying();
On 2016/06/29 23:40:13, servolk wrote:
> On 2016/06/29 22:49:13, chcunningham wrote:
> > I think you still need to call time_source_->SetMediaTime(time) right before
> you
> > call StartPlaying. 
> > 
> > As you pointed out this will destroy the audio_clock and make a new one
while
> > the video renderer is still playing, but I believe this is actually safe to
do
> > because both AudioRendererImpl::SetMediaTime and ARI::GetWallClockTimes
(which
> > video renderer calls for AV sync) are guarded by the same lock. I've tried
> > making this call here (patched your cl) and do not observe any dcheck/check
> > errors.
> > 
> > I wasn't able to develop a theory about how PausePlayback is improving AV
> > sync... the fact that it doesn't change the state of the audio_clock means
> that
> > any wrongness in the audio_clock should be preserved accross the pause. So I
> > patched your CL and tried to investigate ... In my testing I was not able to
> > introduce any noticeable drift with OR without your PausePlayback call. I
> > disabled the audio track in a 1 second interval for 5 straight minutes...
> > couldn't see anything significant. Still, I am confident that it is
> > theoretically possible to introduce drift if we do not reset the clock (see
my
> > previous theory about staleness of currentTime). I could probably get this
> occur
> > with an hours-long video, but you're hoping to land soon. 
> 
> Ok, maybe that was a stray unrelated dcheck then, and I didn't get it on the
> first try either (but still fairly easily, on like a second or third attempt).
> I'll give it another try.

No, unfortunately that wasn't a stray dcheck. I don't get it every time, but I
consistently get it at least once out of 5-10 times when disabling/reenabling
audio with an explicit SetMediaTime.
Re PausePlayback I believe it improves a/v sync because here we are at a point
where we have just Flushed audio renderer. So now video renderer is still
playing and rendering, but audio renderer needs some time to decode enough
frames to reach the HAVE_ENOUGH buffering state. So I think regardless of
whether we call SetMediaTime here, we should still pause and unpause playback in
OnBufferingStateChange when we get a notification that the audio renderer
reached HAVE_ENOUGH status. At that point we'll know that both audio and video
streams are in HAVE_ENOUGH state and there should be enough data for smooth
playback. Only then can we be sure that they'll be in sync.
Re calling SetMediaTime. I'll try to investigate further. But TBH it's not clear
to me why do we need to call it here in the first place?
Remember that time_source_ keeps ticking regardless of whether audio stream has
ended or not. So when restarting audio stream we want time source to keep
ticking uninterrupted to allow smooth video playback to continue until audio
renderer decodes enough to get HAVE_ENOUGH state.

[chcunningham, 2016-06-30 20:58:32]

LGTM % filing bug to follow up on the audio drift issue. Here's some notes from
our VC discussion:

1. Drift seems to be more frequent in src= (ffmpeg) cases. 
2. The pause call seems to have reduced the frequency of drift, but neither of
us can explain how/why.
3. We agree that you should call SetMediaTime after you StopTicking in the
RestartAudioRenderer method. This will be crucial to ensure that audio clock is
reset to match the time of the packets coming out of the demuxer.
4. However, a big concern is with ffmpeg, we aren't actually seeking the demuxer
(one read head for both audio and video), so calling SetMediaTime does not
actually ensure that the clock matches the next audio packets after re-enabling.
In fact, SetMediaTime may reduce the likelihood that things line up for ffmpeg. 

TODO(servolk): file a bug to capture the above and find a fix ;)

[chcunningham, 2016-06-30 21:10:42]

Almost forgot...

5. Need to figure out why that video renderer DCHECK is hit when calling
SetMediaTime. From Dale, "Means that the frame was reported as dropped more
times than it was renderered. Which shouldn't be possible."... I bet it has
something to do with SetMediaTime causing time to move backward just slightly
(which is what we want to make the audio clock line up with the new audio
packets from demxuer).

[servolk, 2016-06-30 22:34:39]

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

[commit-bot: I haz the power, 2016-06-30 22:35:21]

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-06-30 22:40:10]

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

[commit-bot: I haz the power, 2016-06-30 22:40:12]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-06-30 23:46:33]

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

[servolk, 2016-06-30 23:47:05]

On 2016/06/30 22:40:12, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

+slan@ for chromecast/ changes.

[alokp, 2016-07-01 00:17:37]

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

[alokp, 2016-07-01 00:17:39]

lgtm

[servolk, 2016-07-01 00:18:36]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-07-01 00:19:06]

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-07-01 00:32:08]

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

[commit-bot: I haz the power, 2016-07-01 00:32:11]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[servolk, 2016-07-01 00:41:34]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-07-01 00:41:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org, alokp@chromium.org
Link to the patchset: https://codereview.chromium.org/1935873002/#ps1080001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-01 00:42:00]

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-07-01 02:07:01]

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

[commit-bot: I haz the power, 2016-07-01 02:07:03]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[servolk, 2016-07-01 02:31:35]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-07-01 02:31:55]

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-07-01 03:44:55]

Description was changed from

==========
Implement disabling and enabling media tracks

This CL implements disabling and enabling audio and video streams via
Javascript media tracks API. When a media track is disabled, the
corresponding DemuxerStream generates an EOS notification, thus ending
further decoding/rendering of that stream. When a media track is
enabled, the corresponding DemuxerStream is seeked to the current
playback position and we notify the renderers to resume reading from
the demuxer stream and rendering its contents.

BUG=249427
==========

to

==========
Implement disabling and enabling media tracks

This CL implements disabling and enabling audio and video streams via
Javascript media tracks API. When a media track is disabled, the
corresponding DemuxerStream generates an EOS notification, thus ending
further decoding/rendering of that stream. When a media track is
enabled, the corresponding DemuxerStream is seeked to the current
playback position and we notify the renderers to resume reading from
the demuxer stream and rendering its contents.

BUG=249427
==========

[commit-bot: I haz the power, 2016-07-01 03:44:57]

Committed patchset #55 (id:1080001)

[commit-bot: I haz the power, 2016-07-01 03:47:59]

Description was changed from

==========
Implement disabling and enabling media tracks

This CL implements disabling and enabling audio and video streams via
Javascript media tracks API. When a media track is disabled, the
corresponding DemuxerStream generates an EOS notification, thus ending
further decoding/rendering of that stream. When a media track is
enabled, the corresponding DemuxerStream is seeked to the current
playback position and we notify the renderers to resume reading from
the demuxer stream and rendering its contents.

BUG=249427
==========

to

==========
Implement disabling and enabling media tracks

This CL implements disabling and enabling audio and video streams via
Javascript media tracks API. When a media track is disabled, the
corresponding DemuxerStream generates an EOS notification, thus ending
further decoding/rendering of that stream. When a media track is
enabled, the corresponding DemuxerStream is seeked to the current
playback position and we notify the renderers to resume reading from
the demuxer stream and rendering its contents.

BUG=249427

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

[commit-bot: I haz the power, 2016-07-01 03:48:01]

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