AudioRendererSinkCacheImpl::GetSink() should never return sinks in a bad state. |
||||||
Issue descriptionToday a cache hit may include a sink which encountered an error since the error may occur when no mixer is connected. AudioRendererMixerManager::GetMixer() returns nullptr in this case, but it shouldn't as that will result is no-audio/broken playback. It should delete the old sink and try returning a fresh one. If the device status is still bad then the GetMixer() code may be return null. Specifically this block of code: https://cs.chromium.org/chromium/src/content/renderer/media/audio_renderer_sink_cache_impl.cc?l=160 Should loop over the cache and delete any sinks which are in an invalid state. If none remain a new sink should be created for the requested parameters. It's possible this is contributing to our above normal audio error rates.
,
Nov 25 2016
I think in this case there is no point in caching unused sinks in invalid state.
,
Nov 28 2016
Both issues can occur; but in either case the cache should never vend a sink in a known-bad state. Vending unknown is okay, i.e. direct result of creation, but it should never cache one that is bad, and never serve one from the cache which has gone bad.
,
Dec 2 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3e07bf0ac5733215f2b670b42d4cbf90037ea673 commit 3e07bf0ac5733215f2b670b42d4cbf90037ea673 Author: olka <olka@chromium.org> Date: Fri Dec 02 15:20:16 2016 AudioRendererSinKCache: Do not use unhealthy sinks Cache an unused sink only if it is healthy (has an OK status); Get output parameters only from healthy cached sinks. Note that we still can have used sinks in the cache those become unhealthy. BUG= 668250 ,663546 Review-Url: https://codereview.chromium.org/2533443003 Cr-Commit-Position: refs/heads/master@{#435937} [modify] https://crrev.com/3e07bf0ac5733215f2b670b42d4cbf90037ea673/content/renderer/media/audio_renderer_sink_cache_impl.cc [modify] https://crrev.com/3e07bf0ac5733215f2b670b42d4cbf90037ea673/content/renderer/media/audio_renderer_sink_cache_impl.h [modify] https://crrev.com/3e07bf0ac5733215f2b670b42d4cbf90037ea673/content/renderer/media/audio_renderer_sink_cache_unittest.cc
,
Dec 7 2016
Thanks for the fix, I hope it has a positive impact. Can we still add some method of tracking how often a null sink is used, or a mechanism of turning it off? I fear this will become a P0 when this hits stable; our track record of picking up these problems from Beta feedback isn't great.
,
Dec 7 2016
Yes, it landed here: https://codereview.chromium.org/2533443002/ Media.WebAudioSourceProvider.SinkStatus is a UMA stat to track.
,
Dec 7 2016
Linky version: https://uma.googleplex.com/histograms/?endDate=latest&dayCount=1&histograms=Media.WebAudioSourceProvider.SinkStatus&fixupData=true&showMax=true&filters=isofficial%2Ceq%2CTrue&implicitFilters=isofficial 100% okay so far (only 70 samples; probably all bots)
,
Dec 7 2016
This fix should be merged to m56 too.
,
Dec 7 2016
Ditto for the one in c#6 once it's soaked a bit more.
,
Dec 7 2016
Your change meets the bar and is auto-approved for M56 (branch: 2924)
,
Dec 8 2016
For Wednesday we have 0.1% of timed out authorizations for WASP and 0.5% for AudioOutputDevice (now that we don't cache timed out sinks, % of timeouts for AOD may grow). Percentage is about the same on Mac and Win (which smells) https://uma.googleplex.com/histograms/?endDate=20161207&dayCount=1&histograms=Media.Audio.Render.OutputDeviceStatus%2CMedia.WebAudioSourceProvider.SinkStatus&fixupData=true&showMax=true&filters=isofficial%2Ceq%2CTrue&implicitFilters=isofficial
,
Dec 9 2016
***BULK EDIT*** Your change has been approved for M56. Please ensure to verify the fix and merge ASAP so that we could take it for next Beta Release. If the change is already merged and no pending work please remove Merge-Approved-56 label and add merge-merged-2924.
,
Dec 10 2016
It's diverged a fair bit now, .32% on OSX, still ~0.09% on Windows. Can you merge this fix olka@? We probably also want to merge https://codereview.chromium.org/2533443002/
,
Dec 10 2016
How much do we trust unique user grouping? The results suggest that this isn't necessarily specific to a hardware configuration, e.g. 3.7% of Win10 and 2.2% of OSX unique users hit this condition. It's not quite as high as what we would expect if the event had no per-device correlation, but it's still higher than what would happen if the event was deterministic or at least strongly coupled to some hardware.
,
Dec 10 2016
Those numbers are higher than I'd expect, but it's hard to say yet, the change is still only on canary. It'll be more interesting on dev next week.
,
Dec 12 2016
,
Dec 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/18152911205ff73d2df9be6798dd252b82d3a25d commit 18152911205ff73d2df9be6798dd252b82d3a25d Author: olka <olka@chromium.org> Date: Mon Dec 12 12:07:17 2016 AudioRendererSinKCache: Do not use unhealthy sinks Cache an unused sink only if it is healthy (has an OK status); Get output parameters only from healthy cached sinks. Note that we still can have used sinks in the cache those become unhealthy. BUG= 668250 ,663546 Review-Url: https://codereview.chromium.org/2533443003 Cr-Commit-Position: refs/heads/master@{#435937} (cherry picked from commit 3e07bf0ac5733215f2b670b42d4cbf90037ea673) TBR=dalecurtis@chromium.org NOTRY=true NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2566113002 Cr-Commit-Position: refs/branch-heads/2924@{#453} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/18152911205ff73d2df9be6798dd252b82d3a25d/content/renderer/media/audio_renderer_sink_cache_impl.cc [modify] https://crrev.com/18152911205ff73d2df9be6798dd252b82d3a25d/content/renderer/media/audio_renderer_sink_cache_impl.h [modify] https://crrev.com/18152911205ff73d2df9be6798dd252b82d3a25d/content/renderer/media/audio_renderer_sink_cache_unittest.cc
,
Dec 12 2016
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by olka@chromium.org
, Nov 24 2016