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

Issue 1021943002: Introduce cadence based VideoRendererAlgorithm. (Closed)

Created:
4 years, 7 months ago by DaleCurtis
Modified:
4 years, 6 months ago
Reviewers:
xhwang, miu
CC:
chromium-reviews, feature-media-reviews_chromium.org, sunnyps, brianderson, sandersd (OOO until July 31)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Introduce cadence based VideoRendererAlgorithm. This is the engine which will power the new video rendering pipeline! VideoRendererImpl will use this to manage frame selection (as audio uses a similarly named AudioRendererAlgorithm). It attempts to first find a cadence match for a set of frames (i.e. 30fps in 60Hz would have each frame displayed for two render intervals). If it can not find a cadence match, it attempts to find the frame which covers the vsync interval the most. If no frame covers the interval it then chooses the frame which is closest to the deadline. Cadence fitting is based on a calculation of how long it would take for a frame to be repeated or dropped if the cadence is forced from say X.# to just X.0. It handles both integer (30fps in 60Hz) and fractional cadences (120fps in 60Hz). It uses a hysteresis of 100ms to avoid oscillation of the cadence. Clients may use have a secondary pumping mechanism which can expire frames which fall too far behind the current media time; this is to handle cases where the compositor has stopped providing callbacks. Testing shows it's significantly smoother than the existing rendering pipeline! \o/ Profiling notes over 45 minutes of simulated time: - 83ms, UpdateFrameStatistics() most expensive. - 73ms, for removal of frames from the deque. - 20ms, for CalculateCadence(). Overall nothing is much of a hot spot, 45 minutes of simulated time takes ~400ms to run. BUG=439548 TEST=new unittest. Committed: https://crrev.com/3c36fc4302b32f1db2b86a4b91729faeefe44898 Cr-Commit-Position: refs/heads/master@{#327864}

Patch Set 1 #

Patch Set 2 : Complete! Tests. #

Patch Set 3 : Justify limits. Fixes. Tests. #

Patch Set 4 : Moar tests! #

Total comments: 35

Patch Set 5 : Comments. #

Total comments: 40

Patch Set 6 : Even moar tests. Bug fixes. #

Total comments: 72

Patch Set 7 : Comments addressed. #

Patch Set 8 : Switch to MovingAverage. #

Total comments: 10

Patch Set 9 : Improve EffectiveFramesQueued. #

Total comments: 6

Patch Set 10 : Fix frame expiration when Render() never called. #

Total comments: 42

Patch Set 11 : Comments. #

Total comments: 2

Patch Set 12 : Comments. #

Patch Set 13 : Fix frame expiry; add test. #

Total comments: 2

Patch Set 14 : Use magnitude instead of abs. #

Patch Set 15 : Fix evaluation order. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2637 lines, -0 lines) Patch
M media/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +6 lines, -0 lines 0 comments Download
M media/base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -0 lines 0 comments Download
A media/base/moving_average.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +47 lines, -0 lines 0 comments Download
A media/base/moving_average.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +41 lines, -0 lines 0 comments Download
A media/base/moving_average_unittest.cc View 1 2 3 4 5 6 7 1 chunk +36 lines, -0 lines 0 comments Download
A media/filters/video_cadence_estimator.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +154 lines, -0 lines 0 comments Download
A media/filters/video_cadence_estimator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +167 lines, -0 lines 0 comments Download
A media/filters/video_cadence_estimator_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +171 lines, -0 lines 0 comments Download
A media/filters/video_renderer_algorithm.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +285 lines, -0 lines 0 comments Download
A media/filters/video_renderer_algorithm.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +627 lines, -0 lines 0 comments Download
A media/filters/video_renderer_algorithm_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1091 lines, -0 lines 0 comments Download
M media/media.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +9 lines, -0 lines 0 comments Download

Messages

Total messages: 53 (14 generated)
DaleCurtis
PTAL! I'm still considering what other tests to add, but this is in fine enough ...
4 years, 7 months ago (2015-04-15 01:52:25 UTC) #7
DaleCurtis
+miu who has a lot of experience in this area.
4 years, 7 months ago (2015-04-15 02:15:35 UTC) #9
DaleCurtis
https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_renderer_algorithm_unittest.cc File media/filters/video_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_renderer_algorithm_unittest.cc#newcode17 media/filters/video_renderer_algorithm_unittest.cc:17: Notes to self: Still need tests for AccountForMissingIntervals(), OnFrameDropped(), ...
4 years, 7 months ago (2015-04-15 02:20:20 UTC) #10
xhwang
I only reviewed the (mostly public) API and here are my first round of comments. ...
4 years, 7 months ago (2015-04-15 18:05:29 UTC) #11
DaleCurtis
https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_renderer_algorithm.h File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_renderer_algorithm.h#newcode27 media/filters/video_renderer_algorithm.h:27: // drift from the interval. On 2015/04/15 18:05:28, xhwang ...
4 years, 7 months ago (2015-04-15 19:10:53 UTC) #12
xhwang
https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_renderer_algorithm.h File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_renderer_algorithm.h#newcode42 media/filters/video_renderer_algorithm.h:42: // of cadence detection. Can you elaborate on this ...
4 years, 7 months ago (2015-04-15 20:51:39 UTC) #13
DaleCurtis
https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_renderer_algorithm.h File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_renderer_algorithm.h#newcode31 media/filters/video_renderer_algorithm.h:31: using TimeConverterCB = base::Callback<base::TimeTicks(base::TimeDelta)>; On 2015/04/15 18:05:28, xhwang wrote: ...
4 years, 7 months ago (2015-04-16 01:48:01 UTC) #14
DaleCurtis
cc:sunnyps,briananderson who were interested in this.
4 years, 7 months ago (2015-04-16 01:49:05 UTC) #15
miu
Sorry for the delay. Will review tomorrow.
4 years, 7 months ago (2015-04-16 04:56:03 UTC) #16
xhwang
Some extra nits/questions. I started to look at the implementation but I think I need ...
4 years, 7 months ago (2015-04-16 22:30:46 UTC) #17
brianderson
https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_renderer_algorithm.cc File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_renderer_algorithm.cc#newcode281 media/filters/video_renderer_algorithm.cc:281: std::abs((deadline_min - previous_deadline_max) / render_interval_); Is the abs here ...
4 years, 7 months ago (2015-04-16 22:57:56 UTC) #19
DaleCurtis
(Just comments) https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_renderer_algorithm.cc File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_renderer_algorithm.cc#newcode281 media/filters/video_renderer_algorithm.cc:281: std::abs((deadline_min - previous_deadline_max) / render_interval_); On 2015/04/16 ...
4 years, 7 months ago (2015-04-17 02:38:12 UTC) #20
xhwang
https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_renderer_algorithm.h File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_renderer_algorithm.h#newcode82 media/filters/video_renderer_algorithm.h:82: // were removed from |frame_queue_|, during this call, which ...
4 years, 7 months ago (2015-04-17 04:02:14 UTC) #21
DaleCurtis
https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_renderer_algorithm.h File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_renderer_algorithm.h#newcode82 media/filters/video_renderer_algorithm.h:82: // were removed from |frame_queue_|, during this call, which ...
4 years, 7 months ago (2015-04-17 04:15:55 UTC) #22
xhwang
https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_renderer_algorithm.cc File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_renderer_algorithm.cc#newcode69 media/filters/video_renderer_algorithm.cc:69: FrameSelector frame_selector = NONE; This is set but not ...
4 years, 7 months ago (2015-04-17 06:18:32 UTC) #23
DaleCurtis
https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_renderer_algorithm.h File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_renderer_algorithm.h#newcode119 media/filters/video_renderer_algorithm.h:119: // which have a non-zero ideal render count. On ...
4 years, 6 months ago (2015-04-17 16:47:31 UTC) #24
DaleCurtis
https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_renderer_algorithm.h File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_renderer_algorithm.h#newcode98 media/filters/video_renderer_algorithm.h:98: enum { On 2015/04/16 22:30:45, xhwang wrote: > On ...
4 years, 6 months ago (2015-04-18 01:29:21 UTC) #28
miu
General approach looks solid. Nice, clean code and very well tested! :) Mostly minor things. ...
4 years, 6 months ago (2015-04-21 07:04:23 UTC) #29
xhwang
As discussed offline, we'll have something like a CadenceEstimator to split the Cadence estimation and ...
4 years, 6 months ago (2015-04-22 06:11:38 UTC) #30
xhwang
As discussed offline, we'll have something like a CadenceEstimator to split the Cadence estimation and ...
4 years, 6 months ago (2015-04-22 06:11:39 UTC) #31
xhwang
OOC. Currently we are estimating the render/vsync interval by render_interval_ = deadline_max - deadline_min. I ...
4 years, 6 months ago (2015-04-23 17:13:51 UTC) #32
DaleCurtis
These arguments are available to us: https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/begin_frame_args.h&l=22 Per sunnyps@ (a couple weeks ago) the call ...
4 years, 6 months ago (2015-04-23 17:25:30 UTC) #33
miu
https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_renderer_algorithm.cc File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_renderer_algorithm.cc#newcode272 media/filters/video_renderer_algorithm.cc:272: } On 2015/04/22 06:11:38, xhwang wrote: > If you ...
4 years, 6 months ago (2015-04-23 18:23:00 UTC) #34
DaleCurtis
Regarding variable frame rate: The hysteresis in the cadence detector should cause coverage based rendering ...
4 years, 6 months ago (2015-04-23 21:45:41 UTC) #35
DaleCurtis
Hmm, just got everything hooked up and files with alternating timestamps deltas like [17000, 16000, ...
4 years, 6 months ago (2015-04-24 01:54:06 UTC) #36
xhwang
On 2015/04/24 01:54:06, DaleCurtis wrote: > Hmm, just got everything hooked up and files with ...
4 years, 6 months ago (2015-04-24 05:25:01 UTC) #37
DaleCurtis
On 2015/04/24 05:25:01, xhwang wrote: > On 2015/04/24 01:54:06, DaleCurtis wrote: > > Hmm, just ...
4 years, 6 months ago (2015-04-24 05:49:05 UTC) #38
miu
On 2015/04/24 05:49:05, DaleCurtis wrote: > On 2015/04/24 05:25:01, xhwang wrote: > > On 2015/04/24 ...
4 years, 6 months ago (2015-04-24 20:49:40 UTC) #39
DaleCurtis
On 2015/04/24 20:49:40, miu wrote: > On 2015/04/24 05:49:05, DaleCurtis wrote: > > On 2015/04/24 ...
4 years, 6 months ago (2015-04-24 23:54:11 UTC) #40
xhwang
Looking pretty good. I just have a few more comments. https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_average.cc File media/base/moving_average.cc (right): https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_average.cc#newcode24 ...
4 years, 6 months ago (2015-04-28 16:01:08 UTC) #41
DaleCurtis
https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_average.cc File media/base/moving_average.cc (right): https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_average.cc#newcode24 media/base/moving_average.cc:24: } On 2015/04/28 16:01:07, xhwang wrote: > If you ...
4 years, 6 months ago (2015-04-28 21:45:24 UTC) #42
xhwang
Thanks for the patience! LGTM with two last comments (where I don't have strong opinions). ...
4 years, 6 months ago (2015-04-28 22:17:39 UTC) #43
DaleCurtis
sandersd, briananderson, miu: PTAL or remove yourself from the reviewers list if you're happy to ...
4 years, 6 months ago (2015-04-29 00:11:12 UTC) #44
DaleCurtis
sandersd, brianderson -> cc per offline discussion. miu@ PTAL or let me know if you ...
4 years, 6 months ago (2015-04-30 21:35:53 UTC) #46
miu
lgtm https://codereview.chromium.org/1021943002/diff/400001/media/filters/video_cadence_estimator.cc File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1021943002/diff/400001/media/filters/video_cadence_estimator.cc#newcode141 media/filters/video_cadence_estimator.cc:141: const double duration_delta = std::abs( nit: Instead of ...
4 years, 6 months ago (2015-04-30 21:57:07 UTC) #47
DaleCurtis
\o/ thanks for review everyone :) https://codereview.chromium.org/1021943002/diff/400001/media/filters/video_cadence_estimator.cc File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1021943002/diff/400001/media/filters/video_cadence_estimator.cc#newcode141 media/filters/video_cadence_estimator.cc:141: const double duration_delta ...
4 years, 6 months ago (2015-04-30 22:03:52 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1021943002/440001
4 years, 6 months ago (2015-04-30 23:36:16 UTC) #51
commit-bot: I haz the power
Committed patchset #15 (id:440001)
4 years, 6 months ago (2015-05-01 01:48:13 UTC) #52
commit-bot: I haz the power
4 years, 6 months ago (2015-05-01 01:49:24 UTC) #53
Message was sent while issue was closed.
Patchset 15 (id:??) landed as
https://crrev.com/3c36fc4302b32f1db2b86a4b91729faeefe44898
Cr-Commit-Position: refs/heads/master@{#327864}

Powered by Google App Engine
This is Rietveld 408576698