Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2533443003:
AudioRendererSinkCache: Do not use unhealthy sinks (Closed)
|
Created:
4 years ago by o1ka Modified:
4 years ago CC:
asvitkine+watch_chromium.org, 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. |
DescriptionAudioRendererSinKCache: 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
Committed: https://crrev.com/3e07bf0ac5733215f2b670b42d4cbf90037ea673
Cr-Commit-Position: refs/heads/master@{#435937}
Patch Set 1 #
Total comments: 11
Patch Set 2 : "review comments addressed" #
Total comments: 6
Patch Set 3 : Dale's comments addressed #
Messages
Total messages: 28 (16 generated)
The CQ bit was checked by olka@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...
olka@chromium.org changed reviewers: + guidou@chromium.org
Could you PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
looks good, I just have a couple of questions. https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... File content/renderer/media/audio_renderer_sink_cache_impl.cc (right): https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_sink_cache_impl.cc:89: scoped_refptr<media::AudioRendererSink> sink(nullptr); nit: use default constructor? (up to you) https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_sink_cache_impl.cc:109: auto cache_iter = FindHealthyCacheEntry_Locked(source_render_frame_id, nit: if only healthy entries are cached there is no need to change the name of the function. Otherwise the name suggests that it might be possible to find unhealthy entries. https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_sink_cache_impl.cc:131: //|sink| is ref-counted, so it's ok if it is grabage-collected before we get typo: grabage -> garbage. I think the comment is wrong. How can |sink| be garbage collected if |sink| still holds a reference to it? Perhaps you mean evicted from the cache? https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_sink_cache_impl.cc:251: return SinkIsHealthy(val.sink.get()); Can a healthy sink become unhealthy? If not, why check? https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_sink_cache_impl.cc:255: SinkIsHealthy(val.sink.get()); ditto
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Hmm, were we caching unhealthy sinks in the old path? That could be a large part of the error rate if so. https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... File content/renderer/media/audio_renderer_sink_cache_impl.cc (right): https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_sink_cache_impl.cc:251: return SinkIsHealthy(val.sink.get()); On 2016/11/25 at 22:13:17, Guido Urdaneta wrote: > Can a healthy sink become unhealthy? If not, why check? Yes, OnError() may be fired by the browser side.
PTAL https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... File content/renderer/media/audio_renderer_sink_cache_impl.cc (right): https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_sink_cache_impl.cc:89: scoped_refptr<media::AudioRendererSink> sink(nullptr); On 2016/11/25 22:13:17, Guido Urdaneta wrote: > nit: use default constructor? (up to you) Done. https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_sink_cache_impl.cc:109: auto cache_iter = FindHealthyCacheEntry_Locked(source_render_frame_id, On 2016/11/25 22:13:17, Guido Urdaneta wrote: > nit: if only healthy entries are cached there is no need to change the name of > the function. Otherwise the name suggests that it might be possible to find > unhealthy entries. Changed so that the cached sinks are always healthy. https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_sink_cache_impl.cc:131: //|sink| is ref-counted, so it's ok if it is grabage-collected before we get On 2016/11/25 22:13:17, Guido Urdaneta wrote: > typo: grabage -> garbage. > I think the comment is wrong. How can |sink| be garbage collected if |sink| > still holds a reference to it? > Perhaps you mean evicted from the cache? Done. https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_sink_cache_impl.cc:251: return SinkIsHealthy(val.sink.get()); On 2016/11/28 19:00:37, DaleCurtis wrote: > On 2016/11/25 at 22:13:17, Guido Urdaneta wrote: > > Can a healthy sink become unhealthy? If not, why check? > > Yes, OnError() may be fired by the browser side. It does not change device status though. We don't do anything about a sink on error, just call an error callback: https://cs.chromium.org/chromium/src/media/audio/audio_output_device.cc?q=Aud.... https://codereview.chromium.org/2533443003/diff/1/content/renderer/media/audi... content/renderer/media/audio_renderer_sink_cache_impl.cc:255: SinkIsHealthy(val.sink.get()); On 2016/11/25 22:13:17, Guido Urdaneta wrote: > ditto Done.
On 2016/11/28 19:00:37, DaleCurtis wrote: > Hmm, were we caching unhealthy sinks in the old path? That could be a large part > of the error rate if so. > Yes, we did (it used to be a rare case when a sink was unhealthy - before we introduced authorization timeout).
Description was changed from ========== 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 ========== to ========== 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 ==========
lgtm https://codereview.chromium.org/2533443003/diff/20001/content/renderer/media/... File content/renderer/media/audio_renderer_sink_cache_impl.cc (right): https://codereview.chromium.org/2533443003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_sink_cache_impl.cc:105: } else { Seems you could early return here to avoid the big nested if, that also avoids the C-style sink variable. https://codereview.chromium.org/2533443003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_sink_cache_impl.cc:164: bool cache_it = SinkIsHealthy(cache_entry.sink.get()); _it naming seems incorrect for a bool. https://codereview.chromium.org/2533443003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_sink_cache_impl.cc:168: UMA_HISTOGRAM_BOOLEAN("Media.Audio.Render.SinkCache.HealthyUsedSinkCached", Do we need these new metrics? With your fixes we'll see drops in the existing metrics.
The CQ bit was checked by olka@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...
guidou@ - PTAL? https://codereview.chromium.org/2533443003/diff/20001/content/renderer/media/... File content/renderer/media/audio_renderer_sink_cache_impl.cc (right): https://codereview.chromium.org/2533443003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_sink_cache_impl.cc:105: } else { On 2016/11/30 22:00:05, DaleCurtis wrote: > Seems you could early return here to avoid the big nested if, that also avoids > the C-style sink variable. Done. https://codereview.chromium.org/2533443003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_sink_cache_impl.cc:164: bool cache_it = SinkIsHealthy(cache_entry.sink.get()); On 2016/11/30 22:00:05, DaleCurtis wrote: > _it naming seems incorrect for a bool. Done. https://codereview.chromium.org/2533443003/diff/20001/content/renderer/media/... content/renderer/media/audio_renderer_sink_cache_impl.cc:168: UMA_HISTOGRAM_BOOLEAN("Media.Audio.Render.SinkCache.HealthyUsedSinkCached", On 2016/11/30 22:00:05, DaleCurtis wrote: > Do we need these new metrics? With your fixes we'll see drops in the existing > metrics. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
The CQ bit was checked by olka@chromium.org
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/2533443003/#ps40001 (title: "Dale's comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480691777246840,
"parent_rev": "c6cdae588a003f3bffc9c3fece589ac74d23abfc", "commit_rev":
"e392ee61a35f26c02c82b472bd9f6ac355430797"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/3e07bf0ac5733215f2b670b42d4cbf90037ea673 Cr-Commit-Position: refs/heads/master@{#435937} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3e07bf0ac5733215f2b670b42d4cbf90037ea673 Cr-Commit-Position: refs/heads/master@{#435937} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
