Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2837303003:
Fix the crash due to AudioRendererImpl::audio_clock_ being null (Closed)
DescriptionFix the crash due to AudioRendererImpl::audio_clock_ being null
Previously AudioRendererImpl::Initialize used to be called only once
before the renderer was started, so we didn't need a lock in there. But
now AudioRenderer::Initialize might get called again when switching
audio tracks, so we need to have a lock in there to be safe. The lock
will prevent audio_clock_ from being accessed from CurrentMediaTime
while the audio renderer is being reinitialized.
BUG=715191
Patch Set 1 #Patch Set 2 : Release the ARI lock while stopping the sink #Patch Set 3 : Safer fix + rebase #
Total comments: 3
Patch Set 4 : Don't reset audio_clock_ explicitly + remove the DCHECK #Messages
Total messages: 31 (19 generated)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Description was changed from ========== Fix the crash due to AudioRendererImpl::audio_clock_ being null BUG=715191 ========== to ========== Fix the crash due to AudioRendererImpl::audio_clock_ being null BUG=715191 ==========
servolk@chromium.org changed reviewers: + xhwang@chromium.org
Description was changed from ========== Fix the crash due to AudioRendererImpl::audio_clock_ being null BUG=715191 ========== to ========== Fix the crash due to AudioRendererImpl::audio_clock_ being null Previously AudioRendererImpl::Initialize used to be called only once before the renderer was started, so we didn't need a lock in there. But now AudioRenderer::Initialize might get called again when switching audio tracks, so we need to have a lock in there to be safe. The lock will prevent audio_clock_ from being accessed from CurrentMediaTime while the audio renderer is being reinitialized. BUG=715191 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Seems wrong? I thought we chatted about this and decided that the lock was unnecessary because the stream should be in the paused state prior to the track change?
https://codereview.chromium.org/2837303003/diff/40001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2837303003/diff/40001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:355: audio_clock_.reset(); Instead just delete the CHECK below seems like that isn't catching any crashes....
dalecurtis@chromium.org changed reviewers: + chcunningham@chromium.org
+chcunningham who added that check.
On 2017/05/17 00:14:21, DaleCurtis_OOO_May_5_To_May23 wrote: > Seems wrong? I thought we chatted about this and decided that the lock was > unnecessary because the stream should be in the paused state prior to the track > change? Sorry, I don't recall the previous discussion on this (did you mean https://bugs.chromium.org/p/chromium/issues/detail?id=715191#c7? btw, you said there that blocking might be the right solution), but I believe that the lock is necessary here, because even though the audio track is paused, the audio renderer is still used as a time source by the RendererImpl, so the RendererImpl will still call into AudioRenderer::CurrentMediaTime when asked about the current position.
https://codereview.chromium.org/2837303003/diff/40001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2837303003/diff/40001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:355: audio_clock_.reset(); On 2017/05/17 00:15:52, DaleCurtis_OOO_May_5_To_May23 wrote: > Instead just delete the CHECK below seems like that isn't catching any > crashes.... Sure, we can probably do this. But I believe that still need to add a lock in this method. It used to be called only once before the audio renderer was started and we could assume nothing in the audio renderer could change while it's being initialized. But now the Initialize method will be called more than once (during media track switching) and we need to ensure no other thread is changing the audio renderer while it's being initialized.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2837303003/diff/40001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2837303003/diff/40001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:355: audio_clock_.reset(); On 2017/05/17 00:23:31, servolk wrote: > On 2017/05/17 00:15:52, DaleCurtis_OOO_May_5_To_May23 wrote: > > Instead just delete the CHECK below seems like that isn't catching any > > crashes.... > > Sure, we can probably do this. But I believe that still need to add a lock in > this method. It used to be called only once before the audio renderer was > started and we could assume nothing in the audio renderer could change while > it's being initialized. But now the Initialize method will be called more than > once (during media track switching) and we need to ensure no other thread is > changing the audio renderer while it's being initialized. Done (removed the explicit audio_clock_.reset() here and the check below, the clock will be recreated on line 465 below anyway).
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry, there can not be a lock here. If one exists we're doing something wrong. There should be no access into ARI while the track change is occurring, RendererImpl needs to hold all calls until the track change completes.
On 2017/05/17 00:35:43, DaleCurtis_OOO_May_5_To_May23 wrote: > Sorry, there can not be a lock here. If one exists we're doing something wrong. > There should be no access into ARI while the track change is occurring, > RendererImpl needs to hold all calls until the track change completes. TBH it's not clear to me - why is having a lock here unacceptable? What could break? If we want to hold on RendererImpl level, then we might run into other problems. ARI::CurrentMediaTime is called from RendererImpl::GetMediaTime (since time_source_ is audio renderer), but according to the comment at https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=efe... the RendererImpl::GetMediaTime gets called on a different thread, so we probably can't safely access the RendererImpl::restarting_audio_ flag from there, or can we?
On 2017/05/17 at 00:45:14, servolk wrote: > On 2017/05/17 00:35:43, DaleCurtis_OOO_May_5_To_May23 wrote: > > Sorry, there can not be a lock here. If one exists we're doing something wrong. > > There should be no access into ARI while the track change is occurring, > > RendererImpl needs to hold all calls until the track change completes. > > TBH it's not clear to me - why is having a lock here unacceptable? What could break? This lock primarily guards Render() and the audio data structures. No calls should be coming into ARI while we're changing tracks. The lock unnecessarily complicates the design by extending the lock for unintended purposes. It's also papering over only one of the issues here -- see my next comment. > > If we want to hold on RendererImpl level, then we might run into other problems. ARI::CurrentMediaTime is called from RendererImpl::GetMediaTime (since time_source_ is audio renderer), but according to the comment at https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=efe... the RendererImpl::GetMediaTime gets called on a different thread, so we probably can't safely access the RendererImpl::restarting_audio_ flag from there, or can we? As soon as track switches are started, basically no calls should be allowed into the ARI (or VRI for that matter) until the track change completes. Time should freeze and renderer impl should return a cached value while track changes are in progress. We may need a lock there to coordinate this, but putting the lock in ARI initialize is not correct.
On 2017/05/17 00:51:52, DaleCurtis_OOO_May_5_To_May23 wrote: > On 2017/05/17 at 00:45:14, servolk wrote: > > On 2017/05/17 00:35:43, DaleCurtis_OOO_May_5_To_May23 wrote: > > > Sorry, there can not be a lock here. If one exists we're doing something > wrong. > > > There should be no access into ARI while the track change is occurring, > > > RendererImpl needs to hold all calls until the track change completes. > > > > TBH it's not clear to me - why is having a lock here unacceptable? What could > break? > > This lock primarily guards Render() and the audio data structures. No calls > should be coming into ARI while we're changing tracks. The lock unnecessarily > complicates the design by extending the lock for unintended purposes. It's also > papering over only one of the issues here -- see my next comment. > > > > > If we want to hold on RendererImpl level, then we might run into other > problems. ARI::CurrentMediaTime is called from RendererImpl::GetMediaTime (since > time_source_ is audio renderer), but according to the comment at > https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=efe... > the RendererImpl::GetMediaTime gets called on a different thread, so we probably > can't safely access the RendererImpl::restarting_audio_ flag from there, or can > we? > > As soon as track switches are started, basically no calls should be allowed into > the ARI (or VRI for that matter) until the track change completes. Time should > freeze and renderer impl should return a cached value while track changes are in > progress. We may need a lock there to coordinate this, but putting the lock in > ARI initialize is not correct. Well, I guess we could do something like this: https://codereview.chromium.org/2890603004 It does fix the crash in the media/avtrack/track-switching.html layout test, but we'll probably need to add a lock for the RI::restarting_audio_ flag to be really safe? I'll test that CL some more tomorrow.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
On 2017/05/17 01:23:37, servolk wrote: > On 2017/05/17 00:51:52, DaleCurtis_OOO_May_5_To_May23 wrote: > > On 2017/05/17 at 00:45:14, servolk wrote: > > > On 2017/05/17 00:35:43, DaleCurtis_OOO_May_5_To_May23 wrote: > > > > Sorry, there can not be a lock here. If one exists we're doing something > > wrong. > > > > There should be no access into ARI while the track change is occurring, > > > > RendererImpl needs to hold all calls until the track change completes. > > > > > > TBH it's not clear to me - why is having a lock here unacceptable? What > could > > break? > > > > This lock primarily guards Render() and the audio data structures. No calls > > should be coming into ARI while we're changing tracks. The lock unnecessarily > > complicates the design by extending the lock for unintended purposes. It's > also > > papering over only one of the issues here -- see my next comment. > > > > > > > > If we want to hold on RendererImpl level, then we might run into other > > problems. ARI::CurrentMediaTime is called from RendererImpl::GetMediaTime > (since > > time_source_ is audio renderer), but according to the comment at > > > https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=efe... > > the RendererImpl::GetMediaTime gets called on a different thread, so we > probably > > can't safely access the RendererImpl::restarting_audio_ flag from there, or > can > > we? > > > > As soon as track switches are started, basically no calls should be allowed > into > > the ARI (or VRI for that matter) until the track change completes. Time should > > freeze and renderer impl should return a cached value while track changes are > in > > progress. We may need a lock there to coordinate this, but putting the lock in > > ARI initialize is not correct. > > Well, I guess we could do something like this: > https://codereview.chromium.org/2890603004 > It does fix the crash in the media/avtrack/track-switching.html layout test, but > we'll probably need to add a lock for the RI::restarting_audio_ flag to be > really safe? I'll test that CL some more tomorrow. Ok, the https://codereview.chromium.org/2890603004 seems to work well enough, and I realized that we could just use an std::atomic<bool> for the RI::restarting_audio_ flag. So I'll abandon this CL and will land https://codereview.chromium.org/2890603004 instead, PTAL. |
