Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(84)

Issue 1083383005: Connect the new video rendering path to the compositor. (Closed)

Created:
3 years, 3 months ago by DaleCurtis
Modified:
3 years, 3 months ago
CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, cc-bugs_chromium.org, miu
Base URL:
https://chromium.googlesource.com/chromium/src.git@vra
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Connect the new video rendering path to the compositor. Hooks up the new VideoRendererAlgorithm to the VideoRendererImpl and it to the VideoRendererSink (VideoFrameCompositor) which finally talks to the VideoFrameProviderClientImpl! All of this is behind a new flag: --enable-new-video-renderer for right now. We'll slowly ramp it up based on crash and YouTube metrics. It is also available via chrome://flags for our YT friends to test immediately. The playback experience is significantly smoother, dropped frame counts are significantly more accurate, power consumption is much lower in background tabs, and slightly lower in the foreground! Smoothness scores (windows): New: 24: Smoothness: 98.750000, Freezing: 100.000000, ~Dropped: 29 / 749 (3.871829%) 30: Smoothness: 99.330000, Freezing: 100.000000, ~Dropped: 0 / 900 (0.000000%) 60: Smoothness: 99.280000, Freezing: 99.670000, ~Dropped: 6 / 1800 (0.333333%) Old: 24: Smoothness: 92.640000, Freezing: 100.000000, ~Dropped: 29 / 749 (3.871829%) 30: Smoothness: 68.490000, Freezing: 99.440000, ~Dropped: 5 / 900 (0.555556%) 60: Smoothness: 99.280000, Freezing: 99.670000, ~Dropped: 6 / 1800 (0.333333%) See http://xorax.sea/barcode/results_win/ for further details. Power consumption metrics on OSX for 1080p VP9 playback averaged over 10 runs: New: energy_consumption_mwh: 38.994639mWh idle_wakeups_total: 1459.600000count cpu_utilization (browser): 4.752003% cpu_utilization (gpu): 7.791677% cpu_utilization (renderer): 74.691065% Old: energy_consumption_mwh: 39.114139mWh idle_wakeups_total: 1947.900000count cpu_utilization (browser): 5.125488% cpu_utilization (gpu): 8.624919% cpu_utilization (renderer): 84.009566% New Background: energy_consumption_mwh: 31.402967mWh idle_wakeups_total: 195.500000count cpu_utilization (browser): 1.594367% cpu_utilization (gpu): 0.154281% cpu_utilization (renderer): 49.920668% Old Background: energy_consumption_mwh: 42.224069mWh idle_wakeups_total: 522.200000count cpu_utilization (browser): 1.629945% cpu_utilization (gpu): 0.155414% cpu_utilization (renderer): 63.333668% See http://xorax.sea/barcode/results_osx/ for the full run details. I wouldn't trust the absolute values for cpu or energy consumption, but the relative difference should be meaningful. This clamps background canvas updates from video to once every 250ms, but in practice I couldn't tell the difference when tab switching in a debug build. BUG=386551, 396803, 438680, 438766, 439548, 460190 TEST=manual playback, unit tests TBR=asvitkine Committed: https://crrev.com/f28f0e50dd756f424541a734cfa8fc5f8542c57a Cr-Commit-Position: refs/heads/master@{#327889}

Patch Set 1 #

Total comments: 6

Patch Set 2 : Fix and add tests! #

Patch Set 3 : Fix background rendering, test it! #

Patch Set 4 : Remove NullVideoSink #

Total comments: 14

Patch Set 5 : Fix layout tests. #

Total comments: 8

Patch Set 6 : Comments. #

Total comments: 12

Patch Set 7 : Fix comments. Add flags entry. #

Patch Set 8 : Fix only EOS failures. #

Patch Set 9 : Update LoginCustomFlags histogram. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1017 lines, -147 lines) Patch
M cc/layers/video_frame_provider_client_impl.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M cc/layers/video_frame_provider_client_impl.cc View 1 2 3 4 4 chunks +33 lines, -7 lines 0 comments Download
M chrome/app/generated_resources.grd View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M media/base/media_switches.h View 1 2 3 4 6 1 chunk +2 lines, -0 lines 0 comments Download
M media/base/media_switches.cc View 1 2 3 4 6 1 chunk +3 lines, -0 lines 0 comments Download
M media/base/null_video_sink.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M media/base/test_helpers.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M media/base/video_renderer_sink.h View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M media/blink/video_frame_compositor.h View 1 2 3 4 5 6 2 chunks +33 lines, -6 lines 0 comments Download
M media/blink/video_frame_compositor.cc View 1 2 3 4 5 6 6 chunks +85 lines, -27 lines 0 comments Download
M media/blink/video_frame_compositor_unittest.cc View 1 2 3 4 5 6 chunks +216 lines, -22 lines 0 comments Download
M media/renderers/video_renderer_impl.h View 1 2 3 4 5 6 5 chunks +76 lines, -0 lines 0 comments Download
M media/renderers/video_renderer_impl.cc View 1 2 3 4 5 6 7 18 chunks +342 lines, -64 lines 0 comments Download
M media/renderers/video_renderer_impl_unittest.cc View 1 2 3 4 5 6 7 19 chunks +203 lines, -21 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 39 (9 generated)
DaleCurtis
Here's the final piece of the puzzle (modulo a 100% roll out that is)! I ...
3 years, 3 months ago (2015-04-24 21:20:28 UTC) #2
miu
Drive-by comment: https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provider_client_impl.cc File cc/layers/video_frame_provider_client_impl.cc (right): https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provider_client_impl.cc#newcode160 cc/layers/video_frame_provider_client_impl.cc:160: !provider_->UpdateCurrentFrame(args.frame_time + args.interval, Looks like this math ...
3 years, 3 months ago (2015-04-24 22:35:34 UTC) #3
DaleCurtis
https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provider_client_impl.cc File cc/layers/video_frame_provider_client_impl.cc (right): https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provider_client_impl.cc#newcode160 cc/layers/video_frame_provider_client_impl.cc:160: !provider_->UpdateCurrentFrame(args.frame_time + args.interval, On 2015/04/24 22:35:34, miu wrote: > ...
3 years, 3 months ago (2015-04-24 22:43:18 UTC) #4
sunnyps
https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provider_client_impl.cc File cc/layers/video_frame_provider_client_impl.cc (right): https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provider_client_impl.cc#newcode27 cc/layers/video_frame_provider_client_impl.cc:27: stopped_(true) { Let's not use the stopped_ variable for ...
3 years, 3 months ago (2015-04-24 23:18:12 UTC) #5
DaleCurtis
https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provider_client_impl.cc File cc/layers/video_frame_provider_client_impl.cc (right): https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provider_client_impl.cc#newcode160 cc/layers/video_frame_provider_client_impl.cc:160: !provider_->UpdateCurrentFrame(args.frame_time + args.interval, On 2015/04/24 23:18:11, sunnyps wrote: > ...
3 years, 3 months ago (2015-04-24 23:35:01 UTC) #6
sunnyps
On 2015/04/24 23:35:01, DaleCurtis wrote: > https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provider_client_impl.cc > File cc/layers/video_frame_provider_client_impl.cc (right): > > https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provider_client_impl.cc#newcode160 > ...
3 years, 3 months ago (2015-04-24 23:38:21 UTC) #7
DaleCurtis
On 2015/04/24 23:38:21, sunnyps wrote: > On 2015/04/24 23:35:01, DaleCurtis wrote: > > > https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provider_client_impl.cc ...
3 years, 3 months ago (2015-04-24 23:48:10 UTC) #8
DaleCurtis
On 2015/04/24 23:48:10, DaleCurtis wrote: > On 2015/04/24 23:38:21, sunnyps wrote: > > On 2015/04/24 ...
3 years, 3 months ago (2015-04-24 23:49:31 UTC) #9
DaleCurtis
On 2015/04/24 23:49:31, DaleCurtis wrote: > On 2015/04/24 23:48:10, DaleCurtis wrote: > > On 2015/04/24 ...
3 years, 3 months ago (2015-04-25 00:11:12 UTC) #10
DaleCurtis
The latest patch set has all unit tests updated and passing when run with --enable-new-video-renderer. ...
3 years, 3 months ago (2015-04-25 23:10:47 UTC) #11
xhwang
On 2015/04/25 23:10:47, DaleCurtis wrote: > The latest patch set has all unit tests updated ...
3 years, 3 months ago (2015-04-28 18:09:27 UTC) #12
DaleCurtis
Hmm, I'd rather not and can't for most of it; tests will fail without the ...
3 years, 3 months ago (2015-04-28 18:17:58 UTC) #13
xhwang
On 2015/04/28 18:17:58, DaleCurtis wrote: > Hmm, I'd rather not and can't for most of ...
3 years, 3 months ago (2015-04-28 18:30:23 UTC) #14
DaleCurtis
I've removed NullVideoSink at least, https://codereview.chromium.org/1116473002/ The rest I think should stay together.
3 years, 3 months ago (2015-04-28 20:28:56 UTC) #15
DaleCurtis
Latest patch set has all metrics recorded and passes all non-image layout tests. The image ...
3 years, 3 months ago (2015-04-29 04:34:08 UTC) #16
xhwang
I looked at most of VRI and it looks mostly good. But I definitely miss ...
3 years, 3 months ago (2015-04-29 06:51:55 UTC) #17
xhwang
I reviewed the whole CL and it looks mostly good. It's really nice to see ...
3 years, 3 months ago (2015-04-29 22:42:03 UTC) #18
DaleCurtis
This addresses all comments, but doesn't fix image based layout tests; since this is behind ...
3 years, 3 months ago (2015-04-30 03:49:37 UTC) #20
xhwang
lgtm with a few comments. Thanks! https://codereview.chromium.org/1083383005/diff/120001/media/blink/video_frame_compositor.cc File media/blink/video_frame_compositor.cc (right): https://codereview.chromium.org/1083383005/diff/120001/media/blink/video_frame_compositor.cc#newcode122 media/blink/video_frame_compositor.cc:122: LOG(ERROR) << "Got ...
3 years, 3 months ago (2015-04-30 16:39:21 UTC) #21
DaleCurtis
+creis for the chrome://flags and switch propagation changes: chrome/app/generated_resources.grd chrome/browser/about_flags.cc content/browser/renderer_host/render_process_host_impl.cc https://codereview.chromium.org/1083383005/diff/120001/media/blink/video_frame_compositor.cc File media/blink/video_frame_compositor.cc (right): ...
3 years, 3 months ago (2015-04-30 18:06:01 UTC) #23
Charlie Reis
LGTM for: chrome/app/generated_resources.grd chrome/browser/about_flags.cc content/browser/renderer_host/render_process_host_impl.cc
3 years, 3 months ago (2015-04-30 20:18:53 UTC) #24
brianderson
cc lgtm
3 years, 3 months ago (2015-04-30 22:15:32 UTC) #25
DaleCurtis
Thanks for review everyone, I'll CQ after https://codereview.chromium.org/1021943002/ lands later tonight.
3 years, 3 months ago (2015-04-30 23:49:49 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1083383005/160001
3 years, 3 months ago (2015-05-01 02:23:02 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/13335)
3 years, 3 months ago (2015-05-01 03:22:18 UTC) #31
DaleCurtis
tbr=asvitkine for corresponding LoginCustomFlags entry to histograms.xml
3 years, 3 months ago (2015-05-01 06:03:48 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1083383005/180001
3 years, 3 months ago (2015-05-01 06:04:09 UTC) #36
commit-bot: I haz the power
Committed patchset #9 (id:180001)
3 years, 3 months ago (2015-05-01 07:08:36 UTC) #37
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/f28f0e50dd756f424541a734cfa8fc5f8542c57a Cr-Commit-Position: refs/heads/master@{#327889}
3 years, 3 months ago (2015-05-01 07:09:37 UTC) #38
Alexei Svitkine (slow)
3 years, 3 months ago (2015-05-01 15:10:53 UTC) #39
Message was sent while issue was closed.
lgtm

Powered by Google App Engine
This is Rietveld 408576698