Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2365723003:
Break out WebAudio suspension code into new class. Add tests. (Closed)
|
Created:
4 years, 2 months ago by DaleCurtis Modified:
4 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBreak out WebAudio suspension code into new class. Add tests.
We've had enough problems with this code over the past year that it needs
to be extracted into something less burdensome on the WebAudio code. It
also needs some better testing.
This creates a new class called SilentSinkSuspender which is only used on
Android today and adds tests for it. It also changes the implementation
of suspension from directly calling into the sink to instead post tasks
to a provided task runner (render thread).
Notably, fake render callbacks are now driven on the media thread instead
of the render thread since WebAudio and Oilpan have a plethora of checks
around what is and isn't run on the main thread. This also means more than
one Render() may occur during transition. Too many of these can disrupt
the WebAudio clock, but given the 30 second timeout this should be pretty
rare in actual WebAudio usage situations.
BUG=649018, 614978
TEST=new unittests, no more deadlock upon idle timeout.
Committed: https://crrev.com/b770266d26fc09ab8fb746562d008c5d436e4784
Cr-Commit-Position: refs/heads/master@{#421108}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Add lock. Move to media. #Patch Set 3 : Fix namespace. #Patch Set 4 : Fix non-exported base. #
Total comments: 15
Patch Set 5 : Address comments. #
Messages
Total messages: 42 (23 generated)
dalecurtis@chromium.org changed reviewers: + hongchan@chromium.org, nick@chromium.org, olka@chromium.org
olka: PTAL at media/ code since you've complained about the odd calls in the code previously. hongchan: PTAL from WebAudio perspective if you want. nick: PTAL at BUILD.gn files (probably we should update content/OWNERS from *gypi* to *gn*).
The CQ bit was checked by dalecurtis@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
There's a minor test error due to fake changes, but this is otherwise good to review
> Too many of these can disrupt the WebAudio clock, but given the 30 second timeout this should be pretty rare in actual WebAudio usage situations. Sorry for basic questions, but where can I find the information about '30 second timeout'? IIUC, when an Android device produces 30 seconds of silence, the output path will be switched to a fake audio sink by WebAudioSuspender. This fake audio worker runs on the main thread where the callback is not isochronous. So if a WebAudio app intentionally produces 31 seconds of silence, the app goes into the world of the 'irregular render callback'. That's fine by me since this is already a far-fetched corner case, but I want to have an opinion from rtoy@ here. lgtm from WebAudio's viewpoint.
Description was changed from ========== Break out WebAudio suspension code into new class. Add tests. We've had enough problems with this code over the past year that it needs to be extracted into something less burdensome on the WebAudio code. It also needs some better testing. This creates a new class called WebAudioSuspender which is only used on Android today and adds tests for it. It also changes the implementation of suspension from directly calling into the sink to instead post tasks to a provided task runner (render thread). Notably, fake render callbacks are now driven on the media thread instead of the render thread since WebAudio and Oilpan have a plethora of checks around what is and isn't run on the main thread. This also means more than one Render() may occur during transition. Too many of these can disrupt the WebAudio clock, but given the 30 second timeout this should be pretty rare in actual WebAudio usage situations. BUG=649018 TEST=new unittests, no more deadlock upon idle timeout. ========== to ========== Break out WebAudio suspension code into new class. Add tests. We've had enough problems with this code over the past year that it needs to be extracted into something less burdensome on the WebAudio code. It also needs some better testing. This creates a new class called WebAudioSuspender which is only used on Android today and adds tests for it. It also changes the implementation of suspension from directly calling into the sink to instead post tasks to a provided task runner (render thread). Notably, fake render callbacks are now driven on the media thread instead of the render thread since WebAudio and Oilpan have a plethora of checks around what is and isn't run on the main thread. This also means more than one Render() may occur during transition. Too many of these can disrupt the WebAudio clock, but given the 30 second timeout this should be pretty rare in actual WebAudio usage situations. BUG=649018 TEST=new unittests, no more deadlock upon idle timeout. ==========
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
Right, I was complaining about races here https://bugs.chromium.org/p/chromium/issues/detail?id=614978. But if I understand the change correctly, here the race is more probable. https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... File content/renderer/media/webaudio_suspender.cc (right): https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.cc:76: if (now - first_silence_time_ > silent_timeout_) { can be "else if" https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.cc:101: // Play() since no sink is actively calling into Render() during that time. ARS interface does not guarantee that Pause() is synchronous. AOD does it asynchronously (https://cs.chromium.org/chromium/src/media/audio/audio_output_device.cc?q=Aud...), and we still use AOD (Vanellope is under finch, going for beta only) https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.cc:105: is_using_fake_sink_ = true; The above means that these two flags may be concurrently modified here on |task_runner_| thread and accessed on audio rendering thread at the same time, I suspect. https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... File content/renderer/media/webaudio_suspender.h (right): https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.h:50: void set_silent_timeout_for_testing(base::TimeDelta silent_timeout) { (side note: I'm not a fan of those "set for testing" methods when a value can be passed as a constructor parameter... but from our previous interactions I assume your view is just the opposite :) )
https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... File content/renderer/media/webaudio_suspender.h (right): https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2016 yo https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.h:31: class CONTENT_EXPORT WebAudioSuspender If this is WebAudioSuspender, the filename ought to be web_audio_suspender.
hongchan@ there's not really any documentation on it other than what's written here and documented in attached bugs. We end up in cases where ads / ignorant pages leave an active AudioContext around on Android and this kills the battery -- so we chose an unlikely timeout and shutdown the sink in this case. https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... File content/renderer/media/webaudio_suspender.cc (right): https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.cc:76: if (now - first_silence_time_ > silent_timeout_) { On 2016/09/23 at 10:34:35, o1ka wrote: > can be "else if" Not if silent timeout is 0 or negative (testing). https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.cc:101: // Play() since no sink is actively calling into Render() during that time. On 2016/09/23 at 10:34:35, o1ka wrote: > ARS interface does not guarantee that Pause() is synchronous. AOD does it asynchronously (https://cs.chromium.org/chromium/src/media/audio/audio_output_device.cc?q=Aud...), and we still use AOD (Vanellope is under finch, going for beta only) Ah thanks for pointing that out. I'd forgotten. Added a lock and rejiggered things a bit. https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.cc:105: is_using_fake_sink_ = true; On 2016/09/23 at 10:34:34, o1ka wrote: > The above means that these two flags may be concurrently modified here on |task_runner_| thread and accessed on audio rendering thread at the same time, I suspect. Locked! https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... File content/renderer/media/webaudio_suspender.h (right): https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/09/23 at 16:55:11, ncarter wrote: > 2016 yo Done. https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.h:31: class CONTENT_EXPORT WebAudioSuspender On 2016/09/23 at 16:55:11, ncarter wrote: > If this is WebAudioSuspender, the filename ought to be web_audio_suspender. True, but I named it this way since all the other webaudio classes are named this way. "renderer_webaudio..." and "webaudio_media_stream...". In any case I've renamed it and moved it out of content/ since it has no content dependencies. https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.h:50: void set_silent_timeout_for_testing(base::TimeDelta silent_timeout) { On 2016/09/23 at 10:34:35, o1ka wrote: > (side note: I'm not a fan of those "set for testing" methods when a value can be passed as a constructor parameter... but from our previous interactions I assume your view is just the opposite :) ) In this case I agree with you, removed the setter and made the class more generic so it can live in media/ instead of content/
Description was changed from ========== Break out WebAudio suspension code into new class. Add tests. We've had enough problems with this code over the past year that it needs to be extracted into something less burdensome on the WebAudio code. It also needs some better testing. This creates a new class called WebAudioSuspender which is only used on Android today and adds tests for it. It also changes the implementation of suspension from directly calling into the sink to instead post tasks to a provided task runner (render thread). Notably, fake render callbacks are now driven on the media thread instead of the render thread since WebAudio and Oilpan have a plethora of checks around what is and isn't run on the main thread. This also means more than one Render() may occur during transition. Too many of these can disrupt the WebAudio clock, but given the 30 second timeout this should be pretty rare in actual WebAudio usage situations. BUG=649018 TEST=new unittests, no more deadlock upon idle timeout. ========== to ========== Break out WebAudio suspension code into new class. Add tests. We've had enough problems with this code over the past year that it needs to be extracted into something less burdensome on the WebAudio code. It also needs some better testing. This creates a new class called SilentSinkSuspender which is only used on Android today and adds tests for it. It also changes the implementation of suspension from directly calling into the sink to instead post tasks to a provided task runner (render thread). Notably, fake render callbacks are now driven on the media thread instead of the render thread since WebAudio and Oilpan have a plethora of checks around what is and isn't run on the main thread. This also means more than one Render() may occur during transition. Too many of these can disrupt the WebAudio clock, but given the 30 second timeout this should be pretty rare in actual WebAudio usage situations. BUG=649018 TEST=new unittests, no more deadlock upon idle timeout. ==========
The CQ bit was checked by dalecurtis@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by dalecurtis@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm % |transition_lock_| should be held everywhere. I love how RendererWebAudioDeviceImpl is cleaned up - thank you! https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... File content/renderer/media/webaudio_suspender.cc (right): https://codereview.chromium.org/2365723003/diff/1/content/renderer/media/weba... content/renderer/media/webaudio_suspender.cc:76: if (now - first_silence_time_ > silent_timeout_) { On 2016/09/23 20:24:19, DaleCurtis wrote: > On 2016/09/23 at 10:34:35, o1ka wrote: > > can be "else if" > > Not if silent timeout is 0 or negative (testing). Acknowledged. https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... File media/base/silent_sink_suspender.cc (right): https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:43: base::AutoLock al(transition_lock_); It would be good to mention in the header that transition to a fake sink/backward is not entirely smooth and Render() calls may come in uneven intervals during the switch. https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:65: // Drain any non-silent transitional buffers before queuing more audio data. Maybe log here? https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:68: DCHECK(!is_using_fake_sink_); We may want to DCHECK it even if buffers_after_silence_.empty(). Probably just a general DCHECK_EQ(is_using_fake_sink, !dest) around l.50? And make l.53 "if (is_using_fake_sink_)"? https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:74: // Pass-through to client and request rendering. Could you add "In case of a fake sink we render to a last element of |buffers_after_silence_|"? Though it follows from the code above, it would simplify reading a bit. https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:104: void SilentSinkSuspender::TransitionSinks(bool use_fake_sink) { Maybe some logging? It could help to rule-out this code as a cause in "no audio" cases. https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:112: return; the two can be (use_fake_sink == is_using_fake_sink_) (looks more verbal to me) https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:131: // Stop() is synchronous for the FakeWorker, so no need to hold a lock here. When using a short timeout (for testing, for example, or if SilentSinkSuspender is used somewhere else), it may be that the real sink has not paused yet and its Render() call is in progress (same may happen if there is some issue causing sink pausing to be delayed). I think it's safer to hold a lock here, and it's more future-prove as well. (And we gain almost nothing by not holding it)
Description was changed from ========== Break out WebAudio suspension code into new class. Add tests. We've had enough problems with this code over the past year that it needs to be extracted into something less burdensome on the WebAudio code. It also needs some better testing. This creates a new class called SilentSinkSuspender which is only used on Android today and adds tests for it. It also changes the implementation of suspension from directly calling into the sink to instead post tasks to a provided task runner (render thread). Notably, fake render callbacks are now driven on the media thread instead of the render thread since WebAudio and Oilpan have a plethora of checks around what is and isn't run on the main thread. This also means more than one Render() may occur during transition. Too many of these can disrupt the WebAudio clock, but given the 30 second timeout this should be pretty rare in actual WebAudio usage situations. BUG=649018 TEST=new unittests, no more deadlock upon idle timeout. ========== to ========== Break out WebAudio suspension code into new class. Add tests. We've had enough problems with this code over the past year that it needs to be extracted into something less burdensome on the WebAudio code. It also needs some better testing. This creates a new class called SilentSinkSuspender which is only used on Android today and adds tests for it. It also changes the implementation of suspension from directly calling into the sink to instead post tasks to a provided task runner (render thread). Notably, fake render callbacks are now driven on the media thread instead of the render thread since WebAudio and Oilpan have a plethora of checks around what is and isn't run on the main thread. This also means more than one Render() may occur during transition. Too many of these can disrupt the WebAudio clock, but given the 30 second timeout this should be pretty rare in actual WebAudio usage situations. BUG=649018, 614978 TEST=new unittests, no more deadlock upon idle timeout. ==========
Added 614978 to BUG=, since this change fixes it as well.
(just comment) https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... File media/base/silent_sink_suspender.cc (right): https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:131: // Stop() is synchronous for the FakeWorker, so no need to hold a lock here. On 2016/09/26 at 12:21:55, o1ka wrote: > When using a short timeout (for testing, for example, or if SilentSinkSuspender is used somewhere else), it may be that the real sink has not paused yet and its Render() call is in progress (same may happen if there is some issue causing sink pausing to be delayed). I think it's safer to hold a lock here, and it's more future-prove as well. (And we gain almost nothing by not holding it) What do you mean? FakeWorker::Stop() will try to grab a lock which prevents concurrent execution and clear the internal callback so that no future call can happen. I think the code is clearer without it. Otherwise I need to write a comment like "hold the lock, just in case." :)
content/ lgtm though I don't think you need it anymore
On 2016/09/26 16:54:27, DaleCurtis wrote: > (just comment) > > https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... > File media/base/silent_sink_suspender.cc (right): > > https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... > media/base/silent_sink_suspender.cc:131: // Stop() is synchronous for the > FakeWorker, so no need to hold a lock here. > On 2016/09/26 at 12:21:55, o1ka wrote: > > When using a short timeout (for testing, for example, or if > SilentSinkSuspender is used somewhere else), it may be that the real sink has > not paused yet and its Render() call is in progress (same may happen if there is > some issue causing sink pausing to be delayed). I think it's safer to hold a > lock here, and it's more future-prove as well. (And we gain almost nothing by > not holding it) > > What do you mean? FakeWorker::Stop() will try to grab a lock which prevents > concurrent execution and clear the internal callback so that no future call can > happen. I think the code is clearer without it. Otherwise I need to write a > comment like "hold the lock, just in case." :) Now I'm not sure what lock you mean :) And I mean that sink_ (not a fake one) may have not been paused yet if timeout is 1 ms - because Pause is asynchronous. (This is a quick reply from the phone, sorry if I'm misunderstanding something)
On 2016/09/26 18:59:13, o1ka wrote: > On 2016/09/26 16:54:27, DaleCurtis wrote: > > (just comment) > > > > > https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... > > File media/base/silent_sink_suspender.cc (right): > > > > > https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... > > media/base/silent_sink_suspender.cc:131: // Stop() is synchronous for the > > FakeWorker, so no need to hold a lock here. > > On 2016/09/26 at 12:21:55, o1ka wrote: > > > When using a short timeout (for testing, for example, or if > > SilentSinkSuspender is used somewhere else), it may be that the real sink has > > not paused yet and its Render() call is in progress (same may happen if there > is > > some issue causing sink pausing to be delayed). I think it's safer to hold a > > lock here, and it's more future-prove as well. (And we gain almost nothing by > > not holding it) > > > > What do you mean? FakeWorker::Stop() will try to grab a lock which prevents > > concurrent execution and clear the internal callback so that no future call > can > > happen. I think the code is clearer without it. Otherwise I need to write a > > comment like "hold the lock, just in case." :) > > Now I'm not sure what lock you mean :) And I mean that sink_ (not a fake one) > may have not been paused yet if timeout is 1 ms - because Pause is asynchronous. > (This is a quick reply from the phone, sorry if I'm misunderstanding something) Or in fact it does not depend on timeouts value? What if transition back happens right after transition to a fake one, before the real sink is paused asynchronously?
Oh I see, you're worried about the case where we have real -> fake -> real happen while render() from the first real is being called. That seems possible though unlikely, so i'll add a comment and hold the lock here. Thanks!
The CQ bit was checked by dalecurtis@chromium.org
https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... File media/base/silent_sink_suspender.cc (right): https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:43: base::AutoLock al(transition_lock_); On 2016/09/26 at 12:21:55, o1ka wrote: > It would be good to mention in the header that transition to a fake sink/backward is not entirely smooth and Render() calls may come in uneven intervals during the switch. Done. https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:65: // Drain any non-silent transitional buffers before queuing more audio data. On 2016/09/26 at 12:21:55, o1ka wrote: > Maybe log here? Don't want any logs on the render thread since they can be spammy. https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:68: DCHECK(!is_using_fake_sink_); On 2016/09/26 at 12:21:55, o1ka wrote: > We may want to DCHECK it even if buffers_after_silence_.empty(). > Probably just a general DCHECK_EQ(is_using_fake_sink, !dest) around l.50? > And make l.53 "if (is_using_fake_sink_)"? I think the existing pair of DCHECKs covers the appropriate cases and reads nicer to me. https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:74: // Pass-through to client and request rendering. On 2016/09/26 at 12:21:55, o1ka wrote: > Could you add "In case of a fake sink we render to a last element of |buffers_after_silence_|"? Though it follows from the code above, it would simplify reading a bit. I think that's covered by the comment in l.65 pretty well. https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:104: void SilentSinkSuspender::TransitionSinks(bool use_fake_sink) { On 2016/09/26 at 12:21:55, o1ka wrote: > Maybe some logging? It could help to rule-out this code as a cause in "no audio" cases. Was going to add some LOG(INFO) calls, but it's actually blocked by the presubmit and typically frowned upon. So didn't add these. https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:112: return; On 2016/09/26 at 12:21:55, o1ka wrote: > the two can be (use_fake_sink == is_using_fake_sink_) > (looks more verbal to me) Done. https://codereview.chromium.org/2365723003/diff/60001/media/base/silent_sink_... media/base/silent_sink_suspender.cc:131: // Stop() is synchronous for the FakeWorker, so no need to hold a lock here. On 2016/09/26 at 16:54:27, DaleCurtis wrote: > On 2016/09/26 at 12:21:55, o1ka wrote: > > When using a short timeout (for testing, for example, or if SilentSinkSuspender is used somewhere else), it may be that the real sink has not paused yet and its Render() call is in progress (same may happen if there is some issue causing sink pausing to be delayed). I think it's safer to hold a lock here, and it's more future-prove as well. (And we gain almost nothing by not holding it) > > What do you mean? FakeWorker::Stop() will try to grab a lock which prevents concurrent execution and clear the internal callback so that no future call can happen. I think the code is clearer without it. Otherwise I need to write a comment like "hold the lock, just in case." :) Done.
The patchset sent to the CQ was uploaded after l-g-t-m from hongchan@chromium.org, nick@chromium.org, olka@chromium.org Link to the patchset: https://codereview.chromium.org/2365723003/#ps80001 (title: "Address comments.")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Break out WebAudio suspension code into new class. Add tests. We've had enough problems with this code over the past year that it needs to be extracted into something less burdensome on the WebAudio code. It also needs some better testing. This creates a new class called SilentSinkSuspender which is only used on Android today and adds tests for it. It also changes the implementation of suspension from directly calling into the sink to instead post tasks to a provided task runner (render thread). Notably, fake render callbacks are now driven on the media thread instead of the render thread since WebAudio and Oilpan have a plethora of checks around what is and isn't run on the main thread. This also means more than one Render() may occur during transition. Too many of these can disrupt the WebAudio clock, but given the 30 second timeout this should be pretty rare in actual WebAudio usage situations. BUG=649018, 614978 TEST=new unittests, no more deadlock upon idle timeout. ========== to ========== Break out WebAudio suspension code into new class. Add tests. We've had enough problems with this code over the past year that it needs to be extracted into something less burdensome on the WebAudio code. It also needs some better testing. This creates a new class called SilentSinkSuspender which is only used on Android today and adds tests for it. It also changes the implementation of suspension from directly calling into the sink to instead post tasks to a provided task runner (render thread). Notably, fake render callbacks are now driven on the media thread instead of the render thread since WebAudio and Oilpan have a plethora of checks around what is and isn't run on the main thread. This also means more than one Render() may occur during transition. Too many of these can disrupt the WebAudio clock, but given the 30 second timeout this should be pretty rare in actual WebAudio usage situations. BUG=649018, 614978 TEST=new unittests, no more deadlock upon idle timeout. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Break out WebAudio suspension code into new class. Add tests. We've had enough problems with this code over the past year that it needs to be extracted into something less burdensome on the WebAudio code. It also needs some better testing. This creates a new class called SilentSinkSuspender which is only used on Android today and adds tests for it. It also changes the implementation of suspension from directly calling into the sink to instead post tasks to a provided task runner (render thread). Notably, fake render callbacks are now driven on the media thread instead of the render thread since WebAudio and Oilpan have a plethora of checks around what is and isn't run on the main thread. This also means more than one Render() may occur during transition. Too many of these can disrupt the WebAudio clock, but given the 30 second timeout this should be pretty rare in actual WebAudio usage situations. BUG=649018, 614978 TEST=new unittests, no more deadlock upon idle timeout. ========== to ========== Break out WebAudio suspension code into new class. Add tests. We've had enough problems with this code over the past year that it needs to be extracted into something less burdensome on the WebAudio code. It also needs some better testing. This creates a new class called SilentSinkSuspender which is only used on Android today and adds tests for it. It also changes the implementation of suspension from directly calling into the sink to instead post tasks to a provided task runner (render thread). Notably, fake render callbacks are now driven on the media thread instead of the render thread since WebAudio and Oilpan have a plethora of checks around what is and isn't run on the main thread. This also means more than one Render() may occur during transition. Too many of these can disrupt the WebAudio clock, but given the 30 second timeout this should be pretty rare in actual WebAudio usage situations. BUG=649018, 614978 TEST=new unittests, no more deadlock upon idle timeout. Committed: https://crrev.com/b770266d26fc09ab8fb746562d008c5d436e4784 Cr-Commit-Position: refs/heads/master@{#421108} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b770266d26fc09ab8fb746562d008c5d436e4784 Cr-Commit-Position: refs/heads/master@{#421108} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
