Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(559)

Issue 1135143002: Partially revert change to not post AttemptRead() every Render(). (Closed)

Created:
5 years, 7 months ago by DaleCurtis
Modified:
5 years, 7 months ago
Reviewers:
xhwang
CC:
chromium-reviews, feature-media-reviews_chromium.org
Base URL:
http://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Partially revert change to not post AttemptRead() every Render(). http://crrev.com/328649 changed VideoRendererImpl so that it only posted AttemptRead() when there was space in the queue. Unfortunately this causes more dropped frames for demanding content (H.264 4K in particular) than always posting since the AttemptRead() call may be delayed for some time: AttemptRead() TimeFromPostUntilExecuted: 0.076 ms AttemptRead() TimeFromPostUntilExecuted: 4.613 ms AttemptRead() TimeFromPostUntilExecuted: 56.158 ms AttemptRead() TimeFromPostUntilExecuted: 56.197 ms AttemptRead() TimeFromPostUntilExecuted: 52.214 ms AttemptRead() TimeFromPostUntilExecuted: 35.555 ms AttemptRead() TimeFromPostUntilExecuted: 18.917 ms At the same time, the performance improvements I saw locally on my MacBook are not reflected by the performance bots. I further see the dropped frame regression locally on Windows, so let's revert. I considered a couple other solutions to avoid always posting, but none were very glamorous: - Release lock before calling AttemptRead(), fixes cases where Render() was blocked on the lock in FrameReady(). - Checking the end time / ideal render count for the current frame and posting on when we're almost expired... In both cases, neither resolved all problem cases and both add more complexity than just posting. The task will abort trivially if there is no work to do, so just revert. BUG=439548 TEST=telemetry dropped frame count is lower. Committed: https://crrev.com/95a43e334414c7da299dd16f95d7b8433483f86e Cr-Commit-Position: refs/heads/master@{#329281}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -20 lines) Patch
M media/renderers/video_renderer_impl.h View 1 chunk +1 line, -4 lines 0 comments Download
M media/renderers/video_renderer_impl.cc View 2 chunks +9 lines, -16 lines 0 comments Download

Messages

Total messages: 7 (2 generated)
DaleCurtis
5 years, 7 months ago (2015-05-11 21:06:12 UTC) #2
xhwang
lgtm
5 years, 7 months ago (2015-05-11 22:41:30 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135143002/1
5 years, 7 months ago (2015-05-11 22:46:25 UTC) #5
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 7 months ago (2015-05-11 23:53:08 UTC) #6
commit-bot: I haz the power
5 years, 7 months ago (2015-05-11 23:53:53 UTC) #7
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/95a43e334414c7da299dd16f95d7b8433483f86e
Cr-Commit-Position: refs/heads/master@{#329281}

Powered by Google App Engine
This is Rietveld 408576698