Freeze the reported media time while audio is restarted

Freeze the reported media time while audio is restarted

Audio renderer clock is used as a time source for the RendererImpl when
we have an audio stream. But audio renderer clock gets recreated when
switching audio streams, so we must avoid calling ARI::CurrentMediaTime
while an audio renderer is being restarted. This fixes a crash in the
media/avtrack/track-switching.html layout test.

BUG=715191
Review-Url: https://codereview.chromium.org/2890603004
Cr-Commit-Position: refs/heads/master@{#474175}
Committed: https://chromium.googlesource.com/chromium/src/+/9dd6cc8ed582ed45422be5c4a0dad00464d2639a

[servolk, 2017-05-17 16:42:44]

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

[commit-bot: I haz the power, 2017-05-17 16:43:18]

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, 2017-05-17 18:35:08]

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

[commit-bot: I haz the power, 2017-05-17 18:35:08]

Dry run: 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, 2017-05-17 18:36:05]

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

[commit-bot: I haz the power, 2017-05-17 18:36:47]

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, 2017-05-17 20:46:34]

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

[commit-bot: I haz the power, 2017-05-17 20:46:35]

Dry run: 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, 2017-05-17 21:15:51]

Description was changed from

==========
Freeze the reported media time while audio is restarted

BUG=
==========

to

==========
Freeze the reported media time while audio is restarted

Audio renderer clock is used as a time source for the RendererImpl when
we have an audio stream. But audio renderer clock gets recreated when
switching audio streams, so we must avoid calling ARI::CurrentMediaTime
while an audio renderer is being restarted. This fixes a crash in the
media/avtrack/track-switching.html layout test.

BUG=715191
==========

[servolk, 2017-05-17 21:15:51]

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

[servolk, 2017-05-17 21:15:57]

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

[commit-bot: I haz the power, 2017-05-17 21:17:25]

Dry run: CQ is trying da patch.

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

[servolk, 2017-05-17 21:52:30]

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

[servolk, 2017-05-17 21:54:41]

Previous discussion (with a bit more context) is in
https://codereview.chromium.org/2837303003/

[DaleCurtis, 2017-05-17 21:56:09]

std::atomic is banned. Also is it possible to get other calls into the VRI/ARI
then current time? We need to ensure no calls whatsover go into the renderer
while it's initializing.

[servolk, 2017-05-17 22:10:31]

On 2017/05/17 21:56:09, DaleCurtis_OOO_May_5_To_May23 wrote:
> std::atomic is banned. Also is it possible to get other calls into the VRI/ARI
> then current time? We need to ensure no calls whatsover go into the renderer
> while it's initializing.

Hmm, I can see std::atomic being used in quite a few places in base:
https://cs.chromium.org/search/?q=%22std::atomic%3C%22+file:base+package:%5Ec...

So is it still banned or did we forget to update the Chromium guidelines for
std::atomic? I've also found a few thread in chromium-dev discussing problems
with using std::atomic on certain platforms (which seems to confirm that
std::atomic is actually being used in Chromium). I guess we can ask on
chromium-dev to clarify.

Re other calls, I think we could add a few DCHECKs throughout RendererImpl to
ensure that. I guess if we wanted to be extra safe we could maybe hold ARI/VRI
in some temp variables inside RendererImpl while ARI/VRI are being reinitialized
and return them back to RendererImpl::audio_/video_renderer_ when the reinit is
done. WDYT?

[DaleCurtis, 2017-05-17 22:14:37]

check chromium-cxx@ it's listed as banned.

[servolk, 2017-05-17 22:27:50]

On 2017/05/17 22:14:37, DaleCurtis_OOO_May_5_To_May23 wrote:
> check chromium-cxx@ it's listed as banned.

Would it be ok to use base::subtle::Atomic32 instead here?

[DaleCurtis, 2017-05-17 22:31:54]

Anytime you start looking at base::subtle you're likely doing it wrong -- it's
in "subtle" for a reason. :)

That said, is this the right place for this? We have similar logic in
pipeline_impl for handling this during seeks(). Should we just put this logic
there for track changes too? No lock should be necessary then.

Is it possible for the video renderer to call into the time source during this
time (via GetWallclockTimes)? Is that why we need this logic here instead of
elsewhere?

[servolk, 2017-05-17 22:47:56]

On 2017/05/17 22:31:54, DaleCurtis_OOO_May_5_To_May23 wrote:
> Anytime you start looking at base::subtle you're likely doing it wrong -- it's
> in "subtle" for a reason. :)
> 
> That said, is this the right place for this? We have similar logic in
> pipeline_impl for handling this during seeks(). Should we just put this logic
> there for track changes too? No lock should be necessary then.
> 
> Is it possible for the video renderer to call into the time source during this
> time (via GetWallclockTimes)? Is that why we need this logic here instead of
> elsewhere?

Yeah, VRI might call GetWallclockTime, plus I've noticed that
Renderer::GetMediaTime is called from other places, besides PipelineImpl (e.g.
MojoRendererService::UpdateMediaTime and from
media::remoting::Receiver::SendMediaTimeUpdate), so I thought if we could fix
this within RendererImpl that would be ideal and would guarantee proper calling
sequence even if PipelineImpl is bypassed somehow. But perhaps I'm missing
something and PipelineImpl is still involved in those cases too?

[DaleCurtis, 2017-05-17 23:02:59]

On 2017/05/17 at 22:47:56, servolk wrote:
> On 2017/05/17 22:31:54, DaleCurtis_OOO_May_5_To_May23 wrote:
> > Anytime you start looking at base::subtle you're likely doing it wrong --
it's
> > in "subtle" for a reason. :)
> > 
> > That said, is this the right place for this? We have similar logic in
> > pipeline_impl for handling this during seeks(). Should we just put this
logic
> > there for track changes too? No lock should be necessary then.
> > 
> > Is it possible for the video renderer to call into the time source during
this
> > time (via GetWallclockTimes)? Is that why we need this logic here instead of
> > elsewhere?
> 
> Yeah, VRI might call GetWallclockTime, plus I've noticed that
Renderer::GetMediaTime is called from other places, besides PipelineImpl (e.g.
MojoRendererService::UpdateMediaTime and from
media::remoting::Receiver::SendMediaTimeUpdate), so I thought if we could fix
this within RendererImpl that would be ideal and would guarantee proper calling
sequence even if PipelineImpl is bypassed somehow. But perhaps I'm missing
something and PipelineImpl is still involved in those cases too?

Ah yeah those other points are tricky. It does seem like we should do it here.
xhwang, WDYT?

[commit-bot: I haz the power, 2017-05-18 00:32:29]

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

[commit-bot: I haz the power, 2017-05-18 00:32:30]

Dry run: 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, 2017-05-19 01:38:28]

On 2017/05/17 23:02:59, DaleCurtis_OOO_May_5_To_May23 wrote:
> On 2017/05/17 at 22:47:56, servolk wrote:
> > On 2017/05/17 22:31:54, DaleCurtis_OOO_May_5_To_May23 wrote:
> > > Anytime you start looking at base::subtle you're likely doing it wrong --
> it's
> > > in "subtle" for a reason. :)
> > > 
> > > That said, is this the right place for this? We have similar logic in
> > > pipeline_impl for handling this during seeks(). Should we just put this
> logic
> > > there for track changes too? No lock should be necessary then.
> > > 
> > > Is it possible for the video renderer to call into the time source during
> this
> > > time (via GetWallclockTimes)? Is that why we need this logic here instead
of
> > > elsewhere?
> > 
> > Yeah, VRI might call GetWallclockTime, plus I've noticed that
> Renderer::GetMediaTime is called from other places, besides PipelineImpl (e.g.
> MojoRendererService::UpdateMediaTime and from
> media::remoting::Receiver::SendMediaTimeUpdate), so I thought if we could fix
> this within RendererImpl that would be ideal and would guarantee proper
calling
> sequence even if PipelineImpl is bypassed somehow. But perhaps I'm missing
> something and PipelineImpl is still involved in those cases too?
> 
> Ah yeah those other points are tricky. It does seem like we should do it here.
> xhwang, WDYT?

ping, xhwang@ any thoughts?

[xhwang, 2017-05-19 03:46:17]

On 2017/05/19 01:38:28, servolk wrote:
> On 2017/05/17 23:02:59, DaleCurtis_OOO_May_5_To_May23 wrote:
> > On 2017/05/17 at 22:47:56, servolk wrote:
> > > On 2017/05/17 22:31:54, DaleCurtis_OOO_May_5_To_May23 wrote:
> > > > Anytime you start looking at base::subtle you're likely doing it wrong
--
> > it's
> > > > in "subtle" for a reason. :)
> > > > 
> > > > That said, is this the right place for this? We have similar logic in
> > > > pipeline_impl for handling this during seeks(). Should we just put this
> > logic
> > > > there for track changes too? No lock should be necessary then.
> > > > 
> > > > Is it possible for the video renderer to call into the time source
during
> > this
> > > > time (via GetWallclockTimes)? Is that why we need this logic here
instead
> of
> > > > elsewhere?
> > > 
> > > Yeah, VRI might call GetWallclockTime, plus I've noticed that
> > Renderer::GetMediaTime is called from other places, besides PipelineImpl
(e.g.
> > MojoRendererService::UpdateMediaTime and from
> > media::remoting::Receiver::SendMediaTimeUpdate), so I thought if we could
fix
> > this within RendererImpl that would be ideal and would guarantee proper
> calling
> > sequence even if PipelineImpl is bypassed somehow. But perhaps I'm missing
> > something and PipelineImpl is still involved in those cases too?
> > 
> > Ah yeah those other points are tricky. It does seem like we should do it
here.
> > xhwang, WDYT?
> 
> ping, xhwang@ any thoughts?

Sorry for the delay.

1. Renderer::GetMediaTime() is a Renderer API and it should be implemented by
each Renderer subclass. So we should fix this in RendererImpl, instead of push
it up to PipelineImpl.
2. <atomic> is still banned per https://chromium-cpp.appspot.com/. We should
follow our style guide even though it's used in a few places in the code base
IMHO.
3. That said, can we just use a lock to protect restarting_audio_time_ and
restarting_audio_?

[servolk, 2017-05-20 00:25:56]

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

[commit-bot: I haz the power, 2017-05-20 00:26:27]

Dry run: CQ is trying da patch.

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

[servolk, 2017-05-20 00:31:26]

On 2017/05/19 03:46:17, xhwang wrote:
> On 2017/05/19 01:38:28, servolk wrote:
> > On 2017/05/17 23:02:59, DaleCurtis_OOO_May_5_To_May23 wrote:
> > > On 2017/05/17 at 22:47:56, servolk wrote:
> > > > On 2017/05/17 22:31:54, DaleCurtis_OOO_May_5_To_May23 wrote:
> > > > > Anytime you start looking at base::subtle you're likely doing it wrong
> --
> > > it's
> > > > > in "subtle" for a reason. :)
> > > > > 
> > > > > That said, is this the right place for this? We have similar logic in
> > > > > pipeline_impl for handling this during seeks(). Should we just put
this
> > > logic
> > > > > there for track changes too? No lock should be necessary then.
> > > > > 
> > > > > Is it possible for the video renderer to call into the time source
> during
> > > this
> > > > > time (via GetWallclockTimes)? Is that why we need this logic here
> instead
> > of
> > > > > elsewhere?
> > > > 
> > > > Yeah, VRI might call GetWallclockTime, plus I've noticed that
> > > Renderer::GetMediaTime is called from other places, besides PipelineImpl
> (e.g.
> > > MojoRendererService::UpdateMediaTime and from
> > > media::remoting::Receiver::SendMediaTimeUpdate), so I thought if we could
> fix
> > > this within RendererImpl that would be ideal and would guarantee proper
> > calling
> > > sequence even if PipelineImpl is bypassed somehow. But perhaps I'm missing
> > > something and PipelineImpl is still involved in those cases too?
> > > 
> > > Ah yeah those other points are tricky. It does seem like we should do it
> here.
> > > xhwang, WDYT?
> > 
> > ping, xhwang@ any thoughts?
> 
> Sorry for the delay.
> 
> 1. Renderer::GetMediaTime() is a Renderer API and it should be implemented by
> each Renderer subclass. So we should fix this in RendererImpl, instead of push
> it up to PipelineImpl.
> 2. <atomic> is still banned per https://chromium-cpp.appspot.com/. We should
> follow our style guide even though it's used in a few places in the code base
> IMHO.
> 3. That said, can we just use a lock to protect restarting_audio_time_ and
> restarting_audio_?

Yes, I think we can use a lock for now, though it makes the code uglier. Since
the |restarting_audio_| flag is always set from the media thread and the only
place where it's read from non-media thread is RendererImpl::GetMediaTime, I
think we'll only need to have locks in places where it's set and in the
RendererImpl::GetMediaTime. All other read of that flag are from the media
thread and they don't need to rely on the lock to get correct values.

[commit-bot: I haz the power, 2017-05-20 03:58:20]

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

[commit-bot: I haz the power, 2017-05-20 03:58:21]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[servolk, 2017-05-22 15:49:15]

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

[commit-bot: I haz the power, 2017-05-22 15:49:35]

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, 2017-05-22 17:48:41]

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

[commit-bot: I haz the power, 2017-05-22 17:48:42]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2017-05-23 23:35:53]

lgtm

https://codereview.chromium.org/2890603004/diff/40001/media/renderers/rendere...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/2890603004/diff/40001/media/renderers/rendere...
media/renderers/renderer_impl.cc:97: restarting_audio_time_(kNoTimestamp),
Can inline initialize this too?

[servolk, 2017-05-23 23:45:10]

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

[servolk, 2017-05-23 23:45:23]

https://codereview.chromium.org/2890603004/diff/40001/media/renderers/rendere...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/2890603004/diff/40001/media/renderers/rendere...
media/renderers/renderer_impl.cc:97: restarting_audio_time_(kNoTimestamp),
On 2017/05/23 23:35:53, DaleCurtis wrote:
> Can inline initialize this too?

Done.

[commit-bot: I haz the power, 2017-05-23 23:47:14]

Dry run: CQ is trying da patch.

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

[xhwang, 2017-05-24 00:23:41]

https://codereview.chromium.org/2890603004/diff/60001/media/renderers/rendere...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/2890603004/diff/60001/media/renderers/rendere...
media/renderers/renderer_impl.cc:271: }
If time_source_->CurrentMediaTime() calls back into this class we could end up
in a deadlock. I don't think this will happen, but it's better to err on the
safe side.

To fix that, can we put l.267-271 in a {} block?

https://codereview.chromium.org/2890603004/diff/60001/media/renderers/rendere...
File media/renderers/renderer_impl.h (right):

https://codereview.chromium.org/2890603004/diff/60001/media/renderers/rendere...
media/renderers/renderer_impl.h:232: // Lock used to protect access to the
|restarting_audio_| flag.
nit: Also protecting restarting_audio_time_?

https://codereview.chromium.org/2890603004/diff/60001/media/renderers/rendere...
media/renderers/renderer_impl.h:237: base::TimeDelta restarting_audio_time_ =
kNoTimestamp;
nit: add an empty line here since restarting_video_ isn't protected by the lock.

[servolk, 2017-05-24 00:28:00]

https://codereview.chromium.org/2890603004/diff/60001/media/renderers/rendere...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/2890603004/diff/60001/media/renderers/rendere...
media/renderers/renderer_impl.cc:271: }
On 2017/05/24 00:23:41, xhwang wrote:
> If time_source_->CurrentMediaTime() calls back into this class we could end up
> in a deadlock. I don't think this will happen, but it's better to err on the
> safe side.
> 
> To fix that, can we put l.267-271 in a {} block?

Done.

https://codereview.chromium.org/2890603004/diff/60001/media/renderers/rendere...
File media/renderers/renderer_impl.h (right):

https://codereview.chromium.org/2890603004/diff/60001/media/renderers/rendere...
media/renderers/renderer_impl.h:232: // Lock used to protect access to the
|restarting_audio_| flag.
On 2017/05/24 00:23:41, xhwang wrote:
> nit: Also protecting restarting_audio_time_?

Done.

https://codereview.chromium.org/2890603004/diff/60001/media/renderers/rendere...
media/renderers/renderer_impl.h:237: base::TimeDelta restarting_audio_time_ =
kNoTimestamp;
On 2017/05/24 00:23:41, xhwang wrote:
> nit: add an empty line here since restarting_video_ isn't protected by the
lock.

Done.

[servolk, 2017-05-24 00:28:07]

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

[commit-bot: I haz the power, 2017-05-24 00:28:36]

Dry run: CQ is trying da patch.

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

[xhwang, 2017-05-24 00:32:48]

wow, that's quick

LGTM!

[commit-bot: I haz the power, 2017-05-24 00:39:51]

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

[commit-bot: I haz the power, 2017-05-24 00:39:52]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[servolk, 2017-05-24 00:55:54]

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

[commit-bot: I haz the power, 2017-05-24 00:56:40]

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, 2017-05-24 03:49:47]

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

[commit-bot: I haz the power, 2017-05-24 03:49:48]

Dry run: This issue passed the CQ dry run.

[servolk1, 2017-05-24 05:31:16]

The CQ bit was checked by servolk@google.com

[servolk1, 2017-05-24 05:31:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2890603004/#ps80001
(title: "Fixed nits")

[commit-bot: I haz the power, 2017-05-24 05:31:57]

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, 2017-05-24 05:39:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495603876946200,
"parent_rev": "a71bdb09d0b4e789a045820895ab3535bffddabf", "commit_rev":
"9dd6cc8ed582ed45422be5c4a0dad00464d2639a"}

[commit-bot: I haz the power, 2017-05-24 05:39:22]

Description was changed from

==========
Freeze the reported media time while audio is restarted

Audio renderer clock is used as a time source for the RendererImpl when
we have an audio stream. But audio renderer clock gets recreated when
switching audio streams, so we must avoid calling ARI::CurrentMediaTime
while an audio renderer is being restarted. This fixes a crash in the
media/avtrack/track-switching.html layout test.

BUG=715191
==========

to

==========
Freeze the reported media time while audio is restarted

Audio renderer clock is used as a time source for the RendererImpl when
we have an audio stream. But audio renderer clock gets recreated when
switching audio streams, so we must avoid calling ARI::CurrentMediaTime
while an audio renderer is being restarted. This fixes a crash in the
media/avtrack/track-switching.html layout test.

BUG=715191

Review-Url: https://codereview.chromium.org/2890603004
Cr-Commit-Position: refs/heads/master@{#474175}
Committed:
https://chromium.googlesource.com/chromium/src/+/9dd6cc8ed582ed45422be5c4a0da...
==========

[commit-bot: I haz the power, 2017-05-24 05:39:23]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9dd6cc8ed582ed45422be5c4a0da...