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

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

Created:
4 years, 1 month ago by DaleCurtis
Modified:
4 years 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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: > ...
4 years, 1 month 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 ...
4 years, 1 month 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: > ...
4 years, 1 month 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 > ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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. ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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.
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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): ...
4 years 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
4 years ago (2015-04-30 20:18:53 UTC) #24
brianderson
cc lgtm
4 years 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.
4 years 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
4 years 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)
4 years ago (2015-05-01 03:22:18 UTC) #31
DaleCurtis
tbr=asvitkine for corresponding LoginCustomFlags entry to histograms.xml
4 years 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
4 years ago (2015-05-01 06:04:09 UTC) #36
commit-bot: I haz the power
Committed patchset #9 (id:180001)
4 years 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}
4 years ago (2015-05-01 07:09:37 UTC) #38
Alexei Svitkine (slow)
4 years 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