Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1021943002:
Introduce cadence based VideoRendererAlgorithm. (Closed)
|
Created:
5 years, 9 months ago by DaleCurtis Modified:
5 years, 7 months ago 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. |
DescriptionIntroduce 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. #
Messages
Total messages: 53 (14 generated)
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #3 (id:100001) has been deleted
dalecurtis@chromium.org changed reviewers: + sandersd@chromium.org, xhwang@chromium.org
PTAL! I'm still considering what other tests to add, but this is in fine enough shape for review. Dan mentioned some passing interest so now he gets a to review a bunch of code :)
dalecurtis@chromium.org changed reviewers: + miu@chromium.org
+miu who has a lot of experience in this area.
https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... File media/filters/video_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm_unittest.cc:17: Notes to self: Still need tests for AccountForMissingIntervals(), OnFrameDropped(), EffectiveFramesQueued(), and CalculateDriftForFrame().
I only reviewed the (mostly public) API and here are my first round of comments. Maybe we should chat offline to understand more about how things work. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:27: // drift from the interval. This class is not trivial. Can you explain more about how this class should be used? For example, when should RemoveExpiredFrames() be called etc... https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:31: using TimeConverterCB = base::Callback<base::TimeTicks(base::TimeDelta)>; add include for base::Callback https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:33: VideoRendererAlgorithm(const TimeConverterCB& time_converter_cb); explicit https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:49: base::TimeTicks deadline_max, Bike shedding: 1. We have this deadline_min and deadline_max pattern in many places (in this CL and the previous CL), which make me think why don't we have a class for it? Something like TimeInterval or WallClockTimeInterval, where you can get min/max (or begin/end), and duration etc. 2. A deadline interval is a bit odd. By definition, a deadline should be a time (TimeTicks), not a duration/interval. Here it sounds more like a rendering window, or a target interval. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:59: int RemoveExpiredFrames(base::TimeTicks deadline_min); Wondering shouldn't this be handled internally in this class? https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:72: // Removes all frames from the |frame_queue_| and clears predictors. Just to double check, does this set |this| to a clean state? https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:91: void disable_cadence_hysteresis_for_testing() { Move to private since you have the test class as a friend? https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:98: enum { why enum instead of just a static const? http://stackoverflow.com/questions/204983/static-const-member-value-vs-member... https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:104: // it allows 29.9fps and 59.94fps in 60Hz and vice versa. I still don't follow how 8 seconds is calculated; can you elaborate more? https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:124: // 29.5fps in 60Hz, ~17ms max drift => exhausted in ~1 second. Is 29.5 the perfect cadence, and 60 the clamped cadence? May need to be more explicit in the comment. Also, what is |fractional| here? https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:240: base::TimeDelta render_interval_; How about |last_interval_|? If you have a class for the interval, you can merge this and the |last_deadline_max_|.
https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:27: // drift from the interval. On 2015/04/15 18:05:28, xhwang wrote: > This class is not trivial. Can you explain more about how this class should be > used? For example, when should RemoveExpiredFrames() be called etc... Good idea. I'll add more here. Essentially a class should EnqueueFrames() as it receives them and call Render() on a periodic basis. If Render() callbacks are expected to stop for any reason clients can manage the recency of the queue via RemoveExpiredFrames() and EnqueueFrames(). https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:49: base::TimeTicks deadline_max, On 2015/04/15 18:05:28, xhwang wrote: > Bike shedding: > > 1. We have this deadline_min and deadline_max pattern in many places (in this CL > and the previous CL), which make me think why don't we have a class for it? > Something like TimeInterval or WallClockTimeInterval, where you can get min/max > (or begin/end), and duration etc. > > 2. A deadline interval is a bit odd. By definition, a deadline should be a time > (TimeTicks), not a duration/interval. Here it sounds more like a rendering > window, or a target interval. Hmm, I'll have to think about adding a class for this; it feels like it would complicate the interface and harm readability, but that may just be my resistance to helper classes like that. At the compositor level this is called "frame_time" so we could call it frame_begin and frame_end or something similar. If we went with a class it could be a FrameInterval. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:59: int RemoveExpiredFrames(base::TimeTicks deadline_min); On 2015/04/15 18:05:28, xhwang wrote: > Wondering shouldn't this be handled internally in this class? The VRI will use this to expire frames via a countdown timer when Render() callbacks stop. I don't think we want to call this implicitly via EnqueueFrames() or we won't count those frames as dropped. I was thinking VRI's countdown timer would first expire old frames, set a background_rendering flag, pump the decode loop (which would continue until frame limits are met again), and then reset its countdown timer. This keeps memory usage under control and maintains enough frames that switching back to the tab won't be too far behind. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:72: // Removes all frames from the |frame_queue_| and clears predictors. On 2015/04/15 18:05:28, xhwang wrote: > Just to double check, does this set |this| to a clean state? Yes. I use it extensively in the tests :) https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:98: enum { On 2015/04/15 18:05:28, xhwang wrote: > why enum instead of just a static const? > > http://stackoverflow.com/questions/204983/static-const-member-value-vs-member... Hmm, I was under the impression this would result in each includer of the header getting its own copy of the value. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:104: // it allows 29.9fps and 59.94fps in 60Hz and vice versa. On 2015/04/15 18:05:28, xhwang wrote: > I still don't follow how 8 seconds is calculated; can you elaborate more? Essentially I played some videos and looked at how noticeable the glitches were in relation to common rates which would benefit from cadence. I also compared a couple of desktop players which use similar algorithms. The gist is we want to ensure forcing cadence is relatively stable. This means that we won't repeat or drop a frame for at least 8 seconds; versus without cadence (which may be much shorter in cases where the render interval has jitter). I originally had this at 18 seconds based on estimated short term memory limits, but that proved too high for common frame rates. The cadence approach is superior in terms of glitch counts to coverage, so it should be used as much as possible. 8 seconds may even be too high. We may want to allow down to 4 seconds. Consider that the coverage based approach in this case would generally model the cadence approach, but may drift away faster based on jitter in the vsync interval. See this diagram from the compositor folk for more details on how jitter can affect the selection process (of note is my comment at the end of BestFrameByCoverage too): https://docs.google.com/drawings/d/19babNb3eGdZiz36dBE0NCbIVn_PUFguXAC_RPlqyO... https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:124: // 29.5fps in 60Hz, ~17ms max drift => exhausted in ~1 second. On 2015/04/15 18:05:28, xhwang wrote: > Is 29.5 the perfect cadence, and 60 the clamped cadence? May need to be more > explicit in the comment. Also, what is |fractional| here? 29.5fps is the video frame rate, 60Hz is the display rate, the clamped cadence would be 2 (round(60/29.5)). Fractional cadence would be zero in this case. In the 120fps in 59.9Hz case, ideal_cadence would be "1" and the fractional_cadence would also be 1 (round(120/59.9) - 1); which tells us that the frame we select should be displayed once, and then we should always drop the next frame.
https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:42: // of cadence detection. Can you elaborate on this as well? By "adjacent", do you mean they can be non-contiguous (allowing gaps)? https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:49: base::TimeTicks deadline_max, On 2015/04/15 19:10:53, DaleCurtis wrote: > On 2015/04/15 18:05:28, xhwang wrote: > > Bike shedding: > > > > 1. We have this deadline_min and deadline_max pattern in many places (in this > CL > > and the previous CL), which make me think why don't we have a class for it? > > Something like TimeInterval or WallClockTimeInterval, where you can get > min/max > > (or begin/end), and duration etc. > > > > 2. A deadline interval is a bit odd. By definition, a deadline should be a > time > > (TimeTicks), not a duration/interval. Here it sounds more like a rendering > > window, or a target interval. > > Hmm, I'll have to think about adding a class for this; it feels like it would > complicate the interface and harm readability, but that may just be my > resistance to helper classes like that. > > At the compositor level this is called "frame_time" so we could call it > frame_begin and frame_end or something similar. If we went with a class it could > be a FrameInterval. FrameInterval can be confused with frame duration... Since this class is pretty generic: it's really a TimeDelta with a fixed starting TimeTicks. The class name should be generic as well. That's why I was proposing TimeInterval or WallClockTimeInterval, which has nothing to do with rendering, frame etc. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:97: Maybe have a summary here on how the internal algorithm works. Explain cadence, clamp, fraction, drift, glitches, coverage etc. That will make it much easier to read. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:98: enum { On 2015/04/15 19:10:53, DaleCurtis wrote: > On 2015/04/15 18:05:28, xhwang wrote: > > why enum instead of just a static const? > > > > > http://stackoverflow.com/questions/204983/static-const-member-value-vs-member... > > Hmm, I was under the impression this would result in each includer of the header > getting its own copy of the value. Since this is an integer type, the value will be evaluated at compile time. I don't think you'll have any extra copy of it. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:124: // 29.5fps in 60Hz, ~17ms max drift => exhausted in ~1 second. On 2015/04/15 19:10:53, DaleCurtis wrote: > On 2015/04/15 18:05:28, xhwang wrote: > > Is 29.5 the perfect cadence, and 60 the clamped cadence? May need to be more > > explicit in the comment. Also, what is |fractional| here? > > 29.5fps is the video frame rate, 60Hz is the display rate, the clamped cadence > would be 2 (round(60/29.5)). Fractional cadence would be zero in this case. In > the 120fps in 59.9Hz case, ideal_cadence would be "1" and the fractional_cadence > would also be 1 (round(120/59.9) - 1); which tells us that the frame we select > should be displayed once, and then we should always drop the next frame. Thanks for the explanation. We definitely need more docs on these parameters...
https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:31: using TimeConverterCB = base::Callback<base::TimeTicks(base::TimeDelta)>; On 2015/04/15 18:05:28, xhwang wrote: > add include for base::Callback Done. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:33: VideoRendererAlgorithm(const TimeConverterCB& time_converter_cb); On 2015/04/15 18:05:28, xhwang wrote: > explicit Done. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:42: // of cadence detection. On 2015/04/15 20:51:39, xhwang wrote: > Can you elaborate on this as well? By "adjacent", do you mean they can be > non-contiguous (allowing gaps)? Yes, hence the "Gaps which exceed..." - do you still need more clarity? https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:91: void disable_cadence_hysteresis_for_testing() { On 2015/04/15 18:05:28, xhwang wrote: > Move to private since you have the test class as a friend? Removed, it's a relic of when I didn't have a bool for disabling hysteresis. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:97: On 2015/04/15 20:51:39, xhwang wrote: > Maybe have a summary here on how the internal algorithm works. Explain cadence, > clamp, fraction, drift, glitches, coverage etc. That will make it much easier to > read. I've given a high level summary at the top of the class, let me know if you want more information there. Otherwise I feel the summary attached to each function is sufficient. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:98: enum { On 2015/04/15 20:51:39, xhwang wrote: > On 2015/04/15 19:10:53, DaleCurtis wrote: > > On 2015/04/15 18:05:28, xhwang wrote: > > > why enum instead of just a static const? > > > > > > > > > http://stackoverflow.com/questions/204983/static-const-member-value-vs-member... > > > > Hmm, I was under the impression this would result in each includer of the > header > > getting its own copy of the value. > > Since this is an integer type, the value will be evaluated at compile time. I > don't think you'll have any extra copy of it. http://stackoverflow.com/questions/92546/variable-declarations-in-header-file... https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:124: // 29.5fps in 60Hz, ~17ms max drift => exhausted in ~1 second. On 2015/04/15 20:51:39, xhwang wrote: > On 2015/04/15 19:10:53, DaleCurtis wrote: > > On 2015/04/15 18:05:28, xhwang wrote: > > > Is 29.5 the perfect cadence, and 60 the clamped cadence? May need to be more > > > explicit in the comment. Also, what is |fractional| here? > > > > 29.5fps is the video frame rate, 60Hz is the display rate, the clamped cadence > > would be 2 (round(60/29.5)). Fractional cadence would be zero in this case. In > > the 120fps in 59.9Hz case, ideal_cadence would be "1" and the > fractional_cadence > > would also be 1 (round(120/59.9) - 1); which tells us that the frame we select > > should be displayed once, and then we should always drop the next frame. > > Thanks for the explanation. We definitely need more docs on these parameters... Done. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:240: base::TimeDelta render_interval_; On 2015/04/15 18:05:28, xhwang wrote: > How about |last_interval_|? If you have a class for the interval, you can merge > this and the |last_deadline_max_|. Still thinking about this; it seems reasonable, but I didn't have time to play with the code today.
cc:sunnyps,briananderson who were interested in this.
Sorry for the delay. Will review tomorrow.
Some extra nits/questions. I started to look at the implementation but I think I need more time on that. Sorry for the delay. https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:98: enum { On 2015/04/16 01:48:01, DaleCurtis wrote: > On 2015/04/15 20:51:39, xhwang wrote: > > On 2015/04/15 19:10:53, DaleCurtis wrote: > > > On 2015/04/15 18:05:28, xhwang wrote: > > > > why enum instead of just a static const? > > > > > > > > > > > > > > http://stackoverflow.com/questions/204983/static-const-member-value-vs-member... > > > > > > Hmm, I was under the impression this would result in each includer of the > > header > > > getting its own copy of the value. > > > > Since this is an integer type, the value will be evaluated at compile time. I > > don't think you'll have any extra copy of it. > > http://stackoverflow.com/questions/92546/variable-declarations-in-header-file... [I am just learning on this :) Would be happy to get the bottom of it no matter which one is true.] I don't think that applies to static const class member. Typically if you have a non-integral static const class member, you cannot define/initialize it in the header file. You'll need to define it in a cc file. So you'll only have one definition of it in the entire program. The fact that you can initialize a static const "integral" class memeber in the header file is just a shortcut, but it shouldn't change the fact that we only have one definition of it: http://stackoverflow.com/questions/4547660/c-static-member-variable-and-its-i... https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:20: // frames from with the goal of providing a smooth playback experience. I.e., s/from// https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:76: // The deadline interval provided to a Render() call should be adjacent to the tiny nit: Since this is part of the API, it would be nicer to make it clear that the intervals will not overlap, could have gaps and will monotonically increase. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:82: // were removed from |frame_queue_|, during this call, which were never Does this include the frame dropped during EnqueueFrame()? From this comment it shouldn't. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:86: base::TimeTicks deadline_max, We can iterate on this in a later CL if you prefer. There are other places we need update anyways. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:95: // then. Returns the number of frames expired. nit: s/expired/removed https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:96: int RemoveExpiredFrames(base::TimeTicks deadline_min); nit: How about just return a size_t to be consistent with EffectiveFramesQueued() and avoid a static cast in the implementation? (Not sure how this will be called though, where we might need to cast anyways.) https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:116: // frames given to EnqueueFrame(). What about a super old frame was enqueued and therefore was dropped even no cadence pattern has been detected? https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:119: // which have a non-zero ideal render count. What is "non-zero ideal render count"? https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:120: size_t EffectiveFramesQueued() const; nit: Add an empty line here. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:141: kMinimumAcceptableTimeBetweenGlitchesSecs = 8 See comment in the previous PS. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:155: // nearest integer value to |perfect_cadence|. When computing a fractional In this case, do we need to pass in |clamped_cadence| at all? If we already know perfect_cadence, calculating clamped_cadence should be trivial? https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:157: // the rendered and actual frame durations are computed correctly. So when |fractional| is true, we must have |perfect_cadence < 1|? Is this correct? Is the reverse correct as well (I guess not)? https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:274: base::TimeDelta frame_duration_; nit: s/frame_duration_/average_frame_duration/
brianderson@chromium.org changed reviewers: + brianderson@chromium.org
https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:281: std::abs((deadline_min - previous_deadline_max) / render_interval_); Is the abs here to round render_cycle_count down to 0 when the deadline ticks backwards slightly? Would this fail in weird ways if we gave you a tick that went backwards too much and caused render_cycle_count to be >=1? Might be safer to use std::max(0, ...) and DCHECK that the deadlines_min/max's are "sufficiently" monotonic. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:291: if (frame_queue_[last_frame_index_].render_count) Does this need to be a loop for the case were render_count + render_cycle_count > ideal_render_count? https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:384: // See if the clamped cadence fits acceptable thresholds for exhausting drift. Ok. I understand now how this block makes sure 29.97fps video will have a 2 frame cadence, glitch once when the drift is bad enough, and then enter the 2 frame cadence again avoid the problem I was concerned about in https://docs.google.com/drawings/d/19babNb3eGdZiz36dBE0NCbIVn_PUFguXAC_RPlqyO... . https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:552: // TODO(dalecurtis): We may want to make a better decision about what to do Thanks for adding this TODO. Fixing this shouldn't block landing of this patch, but we should definitely get some numbers to see if this is a real problem. For ~24fps video, some runs will be immune to jitter naturally if the frame phases don't align with the vsync phase, so the problem may be intermittent.
(Just comments) https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:281: std::abs((deadline_min - previous_deadline_max) / render_interval_); On 2015/04/16 22:57:56, brianderson wrote: > Is the abs here to round render_cycle_count down to 0 when the deadline ticks > backwards slightly? > > Would this fail in weird ways if we gave you a tick that went backwards too much > and caused render_cycle_count to be >=1? > > Might be safer to use std::max(0, ...) and DCHECK that the deadlines_min/max's > are "sufficiently" monotonic. Yes, fractional values should be zero. I see your point, I'll ensure we only run this calculation when deadline_min > previous_deadline_max. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:291: if (frame_queue_[last_frame_index_].render_count) On 2015/04/16 22:57:56, brianderson wrote: > Does this need to be a loop for the case were render_count + render_cycle_count > > ideal_render_count? No, the FindBestFrameByCadence() will handle overages. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:76: // The deadline interval provided to a Render() call should be adjacent to the On 2015/04/16 22:30:45, xhwang wrote: > tiny nit: Since this is part of the API, it would be nicer to make it clear that > the intervals will not overlap, could have gaps and will monotonically increase. Actually, I forgot Brian told me the other day that the end of the frame interval is estimated, so the next start could overlap slightly. I'll update the comment to reflect this. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:82: // were removed from |frame_queue_|, during this call, which were never On 2015/04/16 22:30:45, xhwang wrote: > Does this include the frame dropped during EnqueueFrame()? From this comment it > shouldn't. There are no frames dropped during EnqueueFrame. Do you mean during RemoveExpiredFrames()? It doesn't include those. It only includes frames which were dropped in the normal course of events (no background expiry). https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:116: // frames given to EnqueueFrame(). On 2015/04/16 22:30:45, xhwang wrote: > What about a super old frame was enqueued and therefore was dropped even no > cadence pattern has been detected? It does not count, in the implementation you can see we start counting from |last_frame_index_| which a super old frame would be added before. I think you might be mixing up when frames are dropped too. Again, frames aren't dropped during EnqueueFrame(). They're dropped by Render() or removed by RemoveExpiredFrames(). EnqueueFrames() always adds frames. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:119: // which have a non-zero ideal render count. On 2015/04/16 22:30:45, xhwang wrote: > What is "non-zero ideal render count"? A positive render count :) I guess I need to explain fractional cadence in the introduction too instead of eliding over it. If you look at |fractional_cadence_| below it has some details. Essentially when rendering 120fps/60hz we'd render the first frame (ideal render count == 1) and drop the second frame (ideal render count == 0) of each pair of frames. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:155: // nearest integer value to |perfect_cadence|. When computing a fractional On 2015/04/16 22:30:45, xhwang wrote: > In this case, do we need to pass in |clamped_cadence| at all? If we already know > perfect_cadence, calculating clamped_cadence should be trivial? This function is first passed |frame_duration| / |render_interval| to see if it can find a normal integer cadence, it is then given |render_interval| / |frame_duration| to see if a fractional cadence fits. In this case |fractional| is then set to true. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:157: // the rendered and actual frame durations are computed correctly. On 2015/04/16 22:30:45, xhwang wrote: > So when |fractional| is true, we must have |perfect_cadence < 1|? Is this > correct? Is the reverse correct as well (I guess not)? No, this isn't true, this function powers two calculations; see above comment.
https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:82: // were removed from |frame_queue_|, during this call, which were never On 2015/04/17 02:38:12, DaleCurtis wrote: > On 2015/04/16 22:30:45, xhwang wrote: > > Does this include the frame dropped during EnqueueFrame()? From this comment > it > > shouldn't. > > There are no frames dropped during EnqueueFrame. Do you mean during > RemoveExpiredFrames()? It doesn't include those. It only includes frames which > were dropped in the normal course of events (no background expiry). I was referring to the comment at l.104-105: "Frames inserted prior to the last rendered frame will be dropped."
https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:82: // were removed from |frame_queue_|, during this call, which were never On 2015/04/17 04:02:14, xhwang wrote: > On 2015/04/17 02:38:12, DaleCurtis wrote: > > On 2015/04/16 22:30:45, xhwang wrote: > > > Does this include the frame dropped during EnqueueFrame()? From this comment > > it > > > shouldn't. > > > > There are no frames dropped during EnqueueFrame. Do you mean during > > RemoveExpiredFrames()? It doesn't include those. It only includes frames which > > were dropped in the normal course of events (no background expiry). > > I was referring to the comment at l.104-105: > "Frames inserted prior to the last rendered frame will be dropped." Ah, my bad! That is confusing. They are counted here, not in EnqueueFrames. I'll reword this to be less confusing.
https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:69: FrameSelector frame_selector = NONE; This is set but not used for anything? https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:119: // which have a non-zero ideal render count. On 2015/04/17 02:38:12, DaleCurtis wrote: > On 2015/04/16 22:30:45, xhwang wrote: > > What is "non-zero ideal render count"? > > A positive render count :) I guess I need to explain fractional cadence in the > introduction too instead of eliding over it. If you look at > |fractional_cadence_| below it has some details. Essentially when rendering > 120fps/60hz we'd render the first frame (ideal render count == 1) and drop the > second frame (ideal render count == 0) of each pair of frames. Thinking wildly... I wonder why we need to treat fractional_cadence_ as a special case. It seems the cadence detection can be generalized to support any perfect cadence if we define cadence as an array (frame display pattern), instead of one integer number. For example: fps Hz PerfectCadence DetectedCadence (each element is the number of times a frame should be rendered) 30 60 2 2 24 50 25/12 2 2 2 2 2 2 2 2 2 2 2 3 24 60 5/2 2 3 25 60 12/5 2 2 3 2 3 29.5 60 120/59 2...2 (29 2s) 3 2...2 (27 2s) 3 29.9 60 600/299 (this is will be crazily long...) 60 30 1/2 1 0 60 50 5/6 1 1 1 1 1 0 120 30 1/4 1 0 0 0 BTW, I assume 2 2 3 2 3 isn't treated as "glitch" because it's really the best we can do: if we don't use cadence and fallback to coverage based, it can only be worse, and could be subject to render interval errors. In theory, the DetectedCadence can have infinite precision, which isn't realistic because: 1) It can result is a super long cadence 2) The PerfectCadence is already an estimation because we don't know the exact frame rate. But we can still use some approximation. For example, we can limit the length (L) of DetectedCadence. For example, L <= 16. Note that "clamped cadence" in the current implementation is a special case where L = 1. Some extra notes: - The TimeUntilGlitch will be much longer for most common combinations, e.g. 24fps at 50Hz. - Fractional cadence is naturally supported and we don't need to have any special treatment for it. - Overtime we'll still have drifts. We can still add or remove one frame from time to time (using the existing code), but that should be much less frequent because our cadence is more accurate. That said, I am not sure how easy/hard it'll be to implement this. Cadence estimation is a pure math problem and should be easy to tackle. Then for each queued frame, we need to use the estimated cadence (an array) to decide how many times a frame should be rendered... I am writing this down because I feel it has a potential to simplify some implementation, and/or improve performance because we can use cadence in more cases. But I am pretty sure I am over simplifying a lot of things. Feel free to ignore if it's unnecessarily complex and/or overkill.
https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:119: // which have a non-zero ideal render count. On 2015/04/17 06:18:32, xhwang wrote: > On 2015/04/17 02:38:12, DaleCurtis wrote: > > On 2015/04/16 22:30:45, xhwang wrote: > > > What is "non-zero ideal render count"? > > > > A positive render count :) I guess I need to explain fractional cadence in the > > introduction too instead of eliding over it. If you look at > > |fractional_cadence_| below it has some details. Essentially when rendering > > 120fps/60hz we'd render the first frame (ideal render count == 1) and drop the > > second frame (ideal render count == 0) of each pair of frames. > > Thinking wildly... > > I wonder why we need to treat fractional_cadence_ as a special case. It seems > the cadence detection can be generalized to support any perfect cadence if we > define cadence as an array (frame display pattern), instead of one integer > number. For example: > > fps Hz PerfectCadence DetectedCadence (each element is the number of times a > frame should be rendered) > 30 60 2 2 > 24 50 25/12 2 2 2 2 2 2 2 2 2 2 2 3 > 24 60 5/2 2 3 > 25 60 12/5 2 2 3 2 3 > 29.5 60 120/59 2...2 (29 2s) 3 2...2 (27 2s) 3 > 29.9 60 600/299 (this is will be crazily long...) > 60 30 1/2 1 0 > 60 50 5/6 1 1 1 1 1 0 > 120 30 1/4 1 0 0 0 > > BTW, I assume 2 2 3 2 3 isn't treated as "glitch" because it's really the best > we can do: if we don't use cadence and fallback to coverage based, it can only > be worse, and could be subject to render interval errors. > > In theory, the DetectedCadence can have infinite precision, which isn't > realistic because: > 1) It can result is a super long cadence > 2) The PerfectCadence is already an estimation because we don't know the exact > frame rate. > But we can still use some approximation. For example, we can limit the length > (L) of DetectedCadence. For example, L <= 16. Note that "clamped cadence" in the > current implementation is a special case where L = 1. > > Some extra notes: > - The TimeUntilGlitch will be much longer for most common combinations, e.g. > 24fps at 50Hz. > - Fractional cadence is naturally supported and we don't need to have any > special treatment for it. > - Overtime we'll still have drifts. We can still add or remove one frame from > time to time (using the existing code), but that should be much less frequent > because our cadence is more accurate. > > That said, I am not sure how easy/hard it'll be to implement this. Cadence > estimation is a pure math problem and should be easy to tackle. Then for each > queued frame, we need to use the estimated cadence (an array) to decide how many > times a frame should be rendered... > > I am writing this down because I feel it has a potential to simplify some > implementation, and/or improve performance because we can use cadence in more > cases. But I am pretty sure I am over simplifying a lot of things. Feel free to > ignore if it's unnecessarily complex and/or overkill. Very astute observation! You're correct and that's the reason I designed the fractional cadence support the way I did (ideal+actual count per frame vs global counts). Maybe one day we can extend it for arbitrary cadence sequences. The problem (I think) is that the math problem is an optimization problem which can get hairy fast; but I haven't given it deep thought. I do know it is subject to noise and thus may fluctuate similarly to BestFrameByCoverage; most importantly in ways that the simple time based hysteresis can't account for. We'd need to use a schmitt trigger or something similar as briananderson@ originally suggested. Before diving in on that complexity I wanted to make sure it's necessary, which for 3:2 hasn't been the case, see my comments on l.408 in UpdateFrameStatistics. I may be overestimating the difficulty of solving this problem though, so I'd love to chat and hear your thoughts :) I think it's a v2.0 type thing though as I'm pretty sure it can only add complexity. The reason fractional_cadence_ is it's own variable is so that EnqueueFrames() can extend the cadence to new frames appropriately (and thus callers can use EffectiveFramesQueued() immediately after).
Patchset #6 (id:200001) has been deleted
Patchset #6 (id:220001) has been deleted
Patchset #6 (id:240001) has been deleted
https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm.h:98: enum { On 2015/04/16 22:30:45, xhwang wrote: > On 2015/04/16 01:48:01, DaleCurtis wrote: > > On 2015/04/15 20:51:39, xhwang wrote: > > > On 2015/04/15 19:10:53, DaleCurtis wrote: > > > > On 2015/04/15 18:05:28, xhwang wrote: > > > > > why enum instead of just a static const? > > > > > > > > > > > > > > > > > > > > http://stackoverflow.com/questions/204983/static-const-member-value-vs-member... > > > > > > > > Hmm, I was under the impression this would result in each includer of the > > > header > > > > getting its own copy of the value. > > > > > > Since this is an integer type, the value will be evaluated at compile time. > I > > > don't think you'll have any extra copy of it. > > > > > http://stackoverflow.com/questions/92546/variable-declarations-in-header-file... > > [I am just learning on this :) Would be happy to get the bottom of it no matter > which one is true.] > > I don't think that applies to static const class member. Typically if you have a > non-integral static const class member, you cannot define/initialize it in the > header file. You'll need to define it in a cc file. So you'll only have one > definition of it in the entire program. The fact that you can initialize a > static const "integral" class memeber in the header file is just a shortcut, but > it shouldn't change the fact that we only have one definition of it: > > http://stackoverflow.com/questions/4547660/c-static-member-variable-and-its-i... Looked into this a bit more, it seems okay for integral types: https://randomascii.wordpress.com/2014/06/26/please-calculate-this-circles-ci... https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... File media/filters/video_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/1021943002/diff/160001/media/filters/video_re... media/filters/video_renderer_algorithm_unittest.cc:17: On 2015/04/15 02:20:20, DaleCurtis wrote: > Notes to self: Still need tests for AccountForMissingIntervals(), > OnFrameDropped(), EffectiveFramesQueued(), and CalculateDriftForFrame(). All tests added! https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:69: FrameSelector frame_selector = NONE; On 2015/04/17 06:18:32, xhwang wrote: > This is set but not used for anything? Was used for debug logs, removed. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:281: std::abs((deadline_min - previous_deadline_max) / render_interval_); On 2015/04/17 02:38:12, DaleCurtis wrote: > On 2015/04/16 22:57:56, brianderson wrote: > > Is the abs here to round render_cycle_count down to 0 when the deadline ticks > > backwards slightly? > > > > Would this fail in weird ways if we gave you a tick that went backwards too > much > > and caused render_cycle_count to be >=1? > > > > Might be safer to use std::max(0, ...) and DCHECK that the deadlines_min/max's > > are "sufficiently" monotonic. > > Yes, fractional values should be zero. I see your point, I'll ensure we only run > this calculation when deadline_min > previous_deadline_max. Done. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:20: // frames from with the goal of providing a smooth playback experience. I.e., On 2015/04/16 22:30:45, xhwang wrote: > s/from// Done. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:76: // The deadline interval provided to a Render() call should be adjacent to the On 2015/04/17 02:38:12, DaleCurtis wrote: > On 2015/04/16 22:30:45, xhwang wrote: > > tiny nit: Since this is part of the API, it would be nicer to make it clear > that > > the intervals will not overlap, could have gaps and will monotonically > increase. > > Actually, I forgot Brian told me the other day that the end of the frame > interval is estimated, so the next start could overlap slightly. I'll update the > comment to reflect this. Done. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:82: // were removed from |frame_queue_|, during this call, which were never On 2015/04/17 04:15:55, DaleCurtis wrote: > On 2015/04/17 04:02:14, xhwang wrote: > > On 2015/04/17 02:38:12, DaleCurtis wrote: > > > On 2015/04/16 22:30:45, xhwang wrote: > > > > Does this include the frame dropped during EnqueueFrame()? From this > comment > > > it > > > > shouldn't. > > > > > > There are no frames dropped during EnqueueFrame. Do you mean during > > > RemoveExpiredFrames()? It doesn't include those. It only includes frames > which > > > were dropped in the normal course of events (no background expiry). > > > > I was referring to the comment at l.104-105: > > "Frames inserted prior to the last rendered frame will be dropped." > > Ah, my bad! That is confusing. They are counted here, not in EnqueueFrames. I'll > reword this to be less confusing. Done. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:86: base::TimeTicks deadline_max, On 2015/04/16 22:30:45, xhwang wrote: > We can iterate on this in a later CL if you prefer. There are other places we > need update anyways. Yeah, lets handle this in another CL. I'm starting to like the idea in terms of the number of member variables and calculations it condenses. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:95: // then. Returns the number of frames expired. On 2015/04/16 22:30:45, xhwang wrote: > nit: s/expired/removed Done. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:96: int RemoveExpiredFrames(base::TimeTicks deadline_min); On 2015/04/16 22:30:45, xhwang wrote: > nit: How about just return a size_t to be consistent with > EffectiveFramesQueued() and avoid a static cast in the implementation? (Not sure > how this will be called though, where we might need to cast anyways.) This spirals out into a couple of of size_t conversions since -1 is used to signal no frame found, but since these are object counts, I think it makes sense to do this. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:120: size_t EffectiveFramesQueued() const; On 2015/04/16 22:30:45, xhwang wrote: > nit: Add an empty line here. Done. https://codereview.chromium.org/1021943002/diff/180001/media/filters/video_re... media/filters/video_renderer_algorithm.h:274: base::TimeDelta frame_duration_; On 2015/04/16 22:30:45, xhwang wrote: > nit: s/frame_duration_/average_frame_duration/ Done.
General approach looks solid. Nice, clean code and very well tested! :) Mostly minor things. On the whole, try to avoid floating-point math where it doesn't add value (pun time is fun time). BTW--Have you considered what should happen with variable-rate content playback? I see you wrote a test (VariableFrameRateCadence), but what if the variable frame rate is irregular? I'm not sure how common such video files/streams are, other than playing back screen captures. But, something to think about. Maybe the |frame_queue_| generally doesn't grow very large, so the |average_frame_duration_| is allowed to fluctuate wildly as well in these situations? https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:31: Reset(); nit: DCHECK(!time_converter_cb_.is_null()); https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:62: if (!UpdateFrameStatistics()) { It feels expensive to update the wall clock timestamps for all frames and recompute all the stats each time Render() is called. Is there a reason we expect the values returned by |time_converter_cb_| to skew or otherwise not be stateless? If so, maybe that should be documented in the header file comments. Consider updating frame statistics using a more on-line algorithm and from EnqueueFrame() instead of Render(). https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:110: frame_to_render = FindBestFrameByDrift(deadline_min); Minor optimization: frame_to_render = FindBestFrameByDrift(deadline_min, &selected_frame_drift); ...to avoid calling CalculateDriftForFrame() again. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:287: for (size_t i = 0; i < frame_queue_.size() - 1; ++i) { Consider surrounding this loop with: #ifndef NDEBUG ... #endif https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:287: for (size_t i = 0; i < frame_queue_.size() - 1; ++i) { ditto: Consider #ifndef NDEBUG ... #endif around this loop. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:316: double clamped_cadence, The type of the |clamped_cadence| arg should be int, not double, right? https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:337: if (rendered_vs_actual_duration_delta < The type of |rendered_frame_duration|, |actual_frame_duration|, and |rendered_vs_actual_duration_delta| could be base::TimeDelta. Then, you can compare less-than-or-equal to zero here rather than deal with epsilon. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:342: const double frames_rendered_before_drift_exhausted = Should be of type int, not double. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:360: if (frame_info.wall_clock_time.is_null()) bikeshedding: Is it possible for frame timestamps to be negative? If so, it's possible for wall_clock_time to be validly set to a zero value. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:367: return true; Should this return false here, since average_frame_duration_ == base::TimeDelta() at this point? https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:369: // Update |average_frame_duration_|, weighted towards the existing duration. nit: extra space after comma https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:510: *adjusted_ideal_render_count = nit/consistency: This statement becomes one line if you use the reference variable: *adjusted_ideal_render_count = current_frame.ideal_render_count; https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:558: continue; Should this be a break statement instead of a continue statement? Or, can wall_clock_time values be out-of-order even when the media timestamps are in-order? Perhaps this assumption should be DCHECK'ed where the wall_clock_time values are computed/assigned? https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:561: const base::TimeTicks next_frame_time = naming: For clarity, this is really frame_end_time, not next_frame_time. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:577: coverage[i] = duration.InSecondsF() / render_interval_.InSecondsF(); To simplify, how about just: coverage[i] = duration; Meaning, there's no need to divide by render_interval_. You get the same result without the extra float conversions and floating-point division operations. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:599: const double kAllowableJitter = 500.0 / render_interval_.InMicroseconds(); Side note: If you take my advice above, more floating-point math is avoided, since this simplifies to: const int kAllowableJitterUsec = 500; if (*second_best >= base::TimeDelta() && best_frame_by_coverage > *second_best && (best_coverage - coverage[*second_best]).magnitude().InMicroseconds() <= kAllowableJitterUsec) { std::swap(...); } https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:641: if (frame.wall_clock_time < deadline_min && It the LHS of the logical-and expression needed? Seems like the LHS is always true if the RHS is true (but the compiler optimizer won't know that). https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:643: return deadline_min - (frame.wall_clock_time + average_frame_duration_); This could all be simplified as: const base::TimeDelta before_deadline = deadline_min - (frame.wall_clock_time + average_frame_duration_); if (before_deadline > base::TimeDelta()) return before_deadline; https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:194: // under cadence or exactly on cadence. Returns -1 if not enough frames are nit: The first sentence of this comment is incomplete. Given the rest of the comment, maybe it should just be deleted? https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:201: // rendered frame's ideal render count in the case over selection, optionally nit: s/in the case over/in the case of over/ https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:227: base::TimeDelta CalculateDriftForFrame(base::TimeTicks deadline_min, Since the values returned are absolute drift (i.e., they hide whether the frame is before or after a point-in-time), should this be called CalculateAbsoluteDriftForFrame()? https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:230: struct ReadyFrame { style nit: Types should be defined at the top of the "private:" section before methods. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:236: base::TimeDelta media_timestamp; Please remove this duplicate field. It doesn't seem to be mutated after construction, so all code can reference frame->timestamp() instead, right? https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:280: TimeConverterCB time_converter_cb_; nit: Type should be "const TimeConverterCB" since it's only set from the ctor initializer list. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:301: bool last_render_had_glitch_; This doesn't need to be a member variable. It can be local to the Render() method. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... File media/filters/video_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm_unittest.cc:20: #define NTSC(fps) fps / 1.001 nit: Add more parens: #define NTSC(fps) ((fps) / 1.001) If it wasn't used in static initializers, I'd just suggest making it a function; but, alas, we cannot use constexpr types yet. ;-) https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm_unittest.cc:31: base::TimeDelta interval(int tick_count) { nit: This and some of the other methods (and some private members) should be const. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm_unittest.cc:141: template <typename OnRenderCallback> Instead of templating, consider passing the 4th argument as: const std::function<...>& render_test_func
As discussed offline, we'll have something like a CadenceEstimator to split the Cadence estimation and frame render count calculation out. The tests are extensive and cover a lot of cases. I am really impressed by that. The only thing I can think of is that we are probably testing too much about the internal implementation versus testing against the public interface. The public interfaces is pretty simple, the input is frames enqueued, and the vsync render intervals, the output is the rendered frame count of each frame. I wonder whether we could simply have a lot of input/output combinations to cover the algorithm. But I don't have any concrete suggestions at this point. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:119: } You could use DVLOG_IF https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:150: if (!frame_queue_[i].render_count) { nit: how about for (const auto& frame : frame_queue_) https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:272: } If you store frame_queue_ as a map<media_time, ReadyFrame>, and keep |last_frame_index_| as the iterator to the last frame, then - you don't need to calculate the lower_bound on l.262. - you don't need to update last frame index when you insert/erase frames. The drawback is that when you iterator through all frames, you'll need i->second to access the ReadyFrame. But that seems not too bad to me. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:543: int VideoRendererAlgorithm::FindBestFrameByCoverage( Wondering can we use media::Ranges to help simplify this function? https://code.google.com/p/chromium/codesearch#chromium/src/media/base/ranges.... https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:175: // 120fps in 59.9Hz, ~8.3ms max drift => exhausted in ~8.2 seconds. Can we add UMA for the real fps/vsync combinations in a later CL? https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:242: bool operator<(const ReadyFrame& other) const; style nit: move to before member variables? https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... File media/filters/video_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm_unittest.cc:141: template <typename OnRenderCallback> On 2015/04/21 07:04:22, miu wrote: > Instead of templating, consider passing the 4th argument as: const > std::function<...>& render_test_func C++11 library features are not allowed yet: https://chromium-cpp.appspot.com/ https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm_unittest.cc:318: // Now calling AccountForMissingIntervals with the an interval which overlaps s/the// https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm_unittest.cc:818: current_frame = frame; style nit: "Keep unnamed lambdas short. If a lambda body is more than maybe five lines long, prefer to give the lambda a name, or to use a named function instead of a lambda." https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Formatting_L...
As discussed offline, we'll have something like a CadenceEstimator to split the Cadence estimation and frame render count calculation out. The tests are extensive and cover a lot of cases. I am really impressed by that. The only thing I can think of is that we are probably testing too much about the internal implementation versus testing against the public interface. The public interfaces is pretty simple, the input is frames enqueued, and the vsync render intervals, the output is the rendered frame count of each frame. I wonder whether we could simply have a lot of input/output combinations to cover the algorithm. But I don't have any concrete suggestions at this point.
OOC. Currently we are estimating the render/vsync interval by render_interval_ = deadline_max - deadline_min. I wonder shouldn't the CC code know the ideal/exact vsync interval (e.g. 1/60) even though in reality each render interval can have some minor jitter for some reason? If we know the ideal/exact vsync interval, our cadence estimation would be more accurate.
These arguments are available to us: https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/begin_fr... Per sunnyps@ (a couple weeks ago) the call to Render we want is the following: Render(args.frame_time + args.interval, args.frame_time + 2 * args.interval). Which was surprising to me, I expected (frame_time, frame_time + interval). In either case though we have the compositor's best guess of the vsync interval length.
https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:272: } On 2015/04/22 06:11:38, xhwang wrote: > If you store frame_queue_ as a map<media_time, ReadyFrame>, and keep > |last_frame_index_| as the iterator to the last frame, then I disagree. The deque is much more memory efficient, and using the map for the reasons you state doesn't actually gain anything: > - you don't need to calculate the lower_bound on l.262. std::lower_bound() is implemented as a binary search on an ordered array and is O(lg n). The look-up for any element in a binary tree is also O(lg n). > - you don't need to update last frame index when you insert/erase frames. It always has to be checked/updated anyway in case you insert the first frame, or erase the only frame, in the frame_queue_. If there wasn't a need to iterate frames in-order for the various analyses, a min-heap would probably be the best data structure option.
Regarding variable frame rate: The hysteresis in the cadence detector should cause coverage based rendering to be used as the frame rate oscillated in, out, and between cadences. Regarding testing: Yes, I am testing some of the internal methods heavily, but only in supplementation to the public interface tests (which I feel are quite significant already). PTAL! All comments addressed. There's a new class VideoCadenceEstimator which rips out the cadence estimation part and includes miu@'s comments. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:31: Reset(); On 2015/04/21 07:04:22, miu wrote: > nit: DCHECK(!time_converter_cb_.is_null()); Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:62: if (!UpdateFrameStatistics()) { On 2015/04/21 07:04:21, miu wrote: > It feels expensive to update the wall clock timestamps for all frames and > recompute all the stats each time Render() is called. Is there a reason we > expect the values returned by |time_converter_cb_| to skew or otherwise not be > stateless? If so, maybe that should be documented in the header file comments. > Consider updating frame statistics using a more on-line algorithm and from > EnqueueFrame() instead of Render(). These values are directly calculated based on rendered audio frames in the ideal case and predicted if they haven't been written to the audio device yet. agree it's not ideal to recalculate every time, but early in playback and especially in the case of playback rate changes, these values can skew significantly. I played with this a lot and didn't find a good solution :( I think to do better we'd have to have some sort of registration callback on the timesource which registered a timestamp and timeticks pointer which could be updated as timestamps change. In the other direction, once all this is wired up I can do some more extensive testing to see just how accurate we need these values to be. I've been overly cautious thus far. For cadence based rendering, we just need a frame duration which should be stable even if the wall clock time jitters (since all times would jitter the same for a given cycle). So long as we had a mechanism to detect playback rate changes at Render() time, we could recompute only when necessary. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:110: frame_to_render = FindBestFrameByDrift(deadline_min); On 2015/04/21 07:04:22, miu wrote: > Minor optimization: > > frame_to_render = FindBestFrameByDrift(deadline_min, &selected_frame_drift); > > ...to avoid calling CalculateDriftForFrame() again. Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:119: } On 2015/04/22 06:11:38, xhwang wrote: > You could use DVLOG_IF Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:150: if (!frame_queue_[i].render_count) { On 2015/04/22 06:11:38, xhwang wrote: > nit: how about for (const auto& frame : frame_queue_) We're only iterating up to |frame_to_render| here, so that doesn't work here. I've added a local variable to avoid all the lookups though. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:272: } On 2015/04/23 18:23:00, miu wrote: > On 2015/04/22 06:11:38, xhwang wrote: > > If you store frame_queue_ as a map<media_time, ReadyFrame>, and keep > > |last_frame_index_| as the iterator to the last frame, then > > I disagree. The deque is much more memory efficient, and using the map for the > reasons you state doesn't actually gain anything: > > > - you don't need to calculate the lower_bound on l.262. > > std::lower_bound() is implemented as a binary search on an ordered array and is > O(lg n). The look-up for any element in a binary tree is also O(lg n). > > > - you don't need to update last frame index when you insert/erase frames. > > It always has to be checked/updated anyway in case you insert the first frame, > or erase the only frame, in the frame_queue_. > > If there wasn't a need to iterate frames in-order for the various analyses, a > min-heap would probably be the best data structure option. Yeah, I prefer the deque here for the constant iteration needs. In the vast majority of cases the lower_bound should complete without any internal iterations since timestamps are generally in order. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:287: for (size_t i = 0; i < frame_queue_.size() - 1; ++i) { On 2015/04/21 07:04:21, miu wrote: > ditto: Consider #ifndef NDEBUG ... #endif around this loop. Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:316: double clamped_cadence, On 2015/04/21 07:04:22, miu wrote: > The type of the |clamped_cadence| arg should be int, not double, right? Removed, now just TimeDelta is passed around in VideoCadenceEstimator. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:337: if (rendered_vs_actual_duration_delta < On 2015/04/21 07:04:22, miu wrote: > The type of |rendered_frame_duration|, |actual_frame_duration|, and > |rendered_vs_actual_duration_delta| could be base::TimeDelta. Then, you can > compare less-than-or-equal to zero here rather than deal with epsilon. Done, mostly, but kept the duration delta as a double to avoid a static_cast in the ceil() computation. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:342: const double frames_rendered_before_drift_exhausted = On 2015/04/21 07:04:22, miu wrote: > Should be of type int, not double. Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:360: if (frame_info.wall_clock_time.is_null()) On 2015/04/21 07:04:21, miu wrote: > bikeshedding: Is it possible for frame timestamps to be negative? If so, it's > possible for wall_clock_time to be validly set to a zero value. They should never be negative, we have DCHECKS in FFmpegDemuxer for this. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:367: return true; On 2015/04/21 07:04:22, miu wrote: > Should this return false here, since average_frame_duration_ == > base::TimeDelta() at this point? It shouldn't have before, but you're right that it's a bit odd that the rest of the calculations are attempted when the duration is zero. I've short circuited this and changed the code to return false (which failed unit tests until I repaired the render count). https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:369: // Update |average_frame_duration_|, weighted towards the existing duration. On 2015/04/21 07:04:22, miu wrote: > nit: extra space after comma Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:510: *adjusted_ideal_render_count = On 2015/04/21 07:04:22, miu wrote: > nit/consistency: This statement becomes one line if you use the reference > variable: > > *adjusted_ideal_render_count = current_frame.ideal_render_count; Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:543: int VideoRendererAlgorithm::FindBestFrameByCoverage( On 2015/04/22 06:11:37, xhwang wrote: > Wondering can we use media::Ranges to help simplify this function? > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/ranges.... I think it'd be just as much work to get translate the frame data into ranges and then transform the results into a double. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:558: continue; On 2015/04/21 07:04:22, miu wrote: > Should this be a break statement instead of a continue statement? Or, can > wall_clock_time values be out-of-order even when the media timestamps are > in-order? Perhaps this assumption should be DCHECK'ed where the wall_clock_time > values are computed/assigned? Good point, it should be a break, but it's possible that these are out of order if we get an unlucky playback rate change with our calls into the time converter (which is potentially running on a different thread). For now I've added a CHECK() to UpdateFrameStatistics to see how frequently this happens... We may need either a capped retry mechanism for time calculations or the ability to give all of our timestamps to the TimeConverterCB at once. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:561: const base::TimeTicks next_frame_time = On 2015/04/21 07:04:22, miu wrote: > naming: For clarity, this is really frame_end_time, not next_frame_time. Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:577: coverage[i] = duration.InSecondsF() / render_interval_.InSecondsF(); On 2015/04/21 07:04:22, miu wrote: > To simplify, how about just: > > coverage[i] = duration; > > Meaning, there's no need to divide by render_interval_. You get the same result > without the extra float conversions and floating-point division operations. Done, great suggestion. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:599: const double kAllowableJitter = 500.0 / render_interval_.InMicroseconds(); On 2015/04/21 07:04:22, miu wrote: > Side note: If you take my advice above, more floating-point math is avoided, > since this simplifies to: > > const int kAllowableJitterUsec = 500; > if (*second_best >= base::TimeDelta() && best_frame_by_coverage > *second_best > && > (best_coverage - coverage[*second_best]).magnitude().InMicroseconds() <= > kAllowableJitterUsec) { > std::swap(...); > } Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:641: if (frame.wall_clock_time < deadline_min && On 2015/04/21 07:04:21, miu wrote: > It the LHS of the logical-and expression needed? Seems like the LHS is always > true if the RHS is true (but the compiler optimizer won't know that). Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:643: return deadline_min - (frame.wall_clock_time + average_frame_duration_); On 2015/04/21 07:04:21, miu wrote: > This could all be simplified as: > > const base::TimeDelta before_deadline = deadline_min - (frame.wall_clock_time > + average_frame_duration_); > if (before_deadline > base::TimeDelta()) > return before_deadline; I like the readability better as is, I did simplify a bit by bringing in frame_end_time for the above function. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:175: // 120fps in 59.9Hz, ~8.3ms max drift => exhausted in ~8.2 seconds. On 2015/04/22 06:11:38, xhwang wrote: > Can we add UMA for the real fps/vsync combinations in a later CL? Absolutely. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:194: // under cadence or exactly on cadence. Returns -1 if not enough frames are On 2015/04/21 07:04:22, miu wrote: > nit: The first sentence of this comment is incomplete. Given the rest of the > comment, maybe it should just be deleted? Edited. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:201: // rendered frame's ideal render count in the case over selection, optionally On 2015/04/21 07:04:22, miu wrote: > nit: s/in the case over/in the case of over/ Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:227: base::TimeDelta CalculateDriftForFrame(base::TimeTicks deadline_min, On 2015/04/21 07:04:22, miu wrote: > Since the values returned are absolute drift (i.e., they hide whether the frame > is before or after a point-in-time), should this be called > CalculateAbsoluteDriftForFrame()? Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:230: struct ReadyFrame { On 2015/04/21 07:04:22, miu wrote: > style nit: Types should be defined at the top of the "private:" section before > methods. Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:236: base::TimeDelta media_timestamp; On 2015/04/21 07:04:22, miu wrote: > Please remove this duplicate field. It doesn't seem to be mutated after > construction, so all code can reference frame->timestamp() instead, right? Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:242: bool operator<(const ReadyFrame& other) const; On 2015/04/22 06:11:38, xhwang wrote: > style nit: move to before member variables? Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:280: TimeConverterCB time_converter_cb_; On 2015/04/21 07:04:22, miu wrote: > nit: Type should be "const TimeConverterCB" since it's only set from the ctor > initializer list. Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm.h:301: bool last_render_had_glitch_; On 2015/04/21 07:04:22, miu wrote: > This doesn't need to be a member variable. It can be local to the Render() > method. It does if I don't want to pollute the Render() interface and expose it to tests. Am I missing something? https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... File media/filters/video_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm_unittest.cc:20: #define NTSC(fps) fps / 1.001 On 2015/04/21 07:04:22, miu wrote: > nit: Add more parens: > > #define NTSC(fps) ((fps) / 1.001) > > If it wasn't used in static initializers, I'd just suggest making it a function; > but, alas, we cannot use constexpr types yet. ;-) Done, as a function now, I only needed this for global statics, function level statics can use a function. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm_unittest.cc:31: base::TimeDelta interval(int tick_count) { On 2015/04/21 07:04:22, miu wrote: > nit: This and some of the other methods (and some private members) should be > const. Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm_unittest.cc:318: // Now calling AccountForMissingIntervals with the an interval which overlaps On 2015/04/22 06:11:38, xhwang wrote: > s/the// Done. https://codereview.chromium.org/1021943002/diff/260001/media/filters/video_re... media/filters/video_renderer_algorithm_unittest.cc:818: current_frame = frame; On 2015/04/22 06:11:38, xhwang wrote: > style nit: > > "Keep unnamed lambdas short. If a lambda body is more than maybe five lines > long, prefer to give the lambda a name, or to use a named function instead of a > lambda." > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Formatting_L... - whitespace and comments it's only 8 lines... close enough? :) I feel like consistency with the other tests is more important here.
Hmm, just got everything hooked up and files with alternating timestamps deltas like [17000, 16000, 17000, 16000, 17000, 16000] are problematic with the current cadence detection approach. The average ends up oscillating too for cadence detection based on how which delta is more dominant for a given period of time. Still pondering what to do about it. Ideas welcome :) Interval: 16666, Duration: 0 Interval: 16666, Duration: 16666 Interval: 16666, Duration: 16666 Interval: 16666, Duration: 16666 Interval: 16666, Duration: 16888 Interval: 16666, Duration: 16722 Interval: 16666, Duration: 16680 Interval: 16666, Duration: 16670 Interval: 16666, Duration: 16667 Interval: 16666, Duration: 16666 Interval: 16666, Duration: 16666 Interval: 16666, Duration: 16666 Cadence switch: (0, 0) -> (1, 0) Interval: 16666, Duration: 16666 glitch time: 2.08325s Interval: 16666, Duration: 16733 glitch time: 1.16662s Interval: 16666, Duration: 16546 glitch time: 3.24987s Interval: 16666, Duration: 16709 glitch time: 1.86659s Interval: 16666, Duration: 16741 glitch time: 1.18329s Interval: 16666, Duration: 16548 glitch time: 4.79981s Interval: 16666, Duration: 16637 glitch time: 0.99996s Cadence switch: (1, 0) -> (0, 0)
On 2015/04/24 01:54:06, DaleCurtis wrote: > Hmm, just got everything hooked up and files with alternating timestamps deltas > like [17000, 16000, 17000, 16000, 17000, 16000] are problematic with the current > cadence detection approach. > > The average ends up oscillating too for cadence detection based on how which > delta is more dominant for a given period of time. > > Still pondering what to do about it. Ideas welcome :) > > Interval: 16666, Duration: 0 > Interval: 16666, Duration: 16666 > Interval: 16666, Duration: 16666 > Interval: 16666, Duration: 16666 > Interval: 16666, Duration: 16888 > Interval: 16666, Duration: 16722 > Interval: 16666, Duration: 16680 > Interval: 16666, Duration: 16670 > Interval: 16666, Duration: 16667 > Interval: 16666, Duration: 16666 > Interval: 16666, Duration: 16666 > Interval: 16666, Duration: 16666 > Cadence switch: (0, 0) -> (1, 0) > Interval: 16666, Duration: 16666 > glitch time: 2.08325s > Interval: 16666, Duration: 16733 > glitch time: 1.16662s > Interval: 16666, Duration: 16546 > glitch time: 3.24987s > Interval: 16666, Duration: 16709 > glitch time: 1.86659s > Interval: 16666, Duration: 16741 > glitch time: 1.18329s > Interval: 16666, Duration: 16548 > glitch time: 4.79981s > Interval: 16666, Duration: 16637 > glitch time: 0.99996s > Cadence switch: (1, 0) -> (0, 0) What's the "Intervel" and "Duration" in this table? Are you saying alternating frame duration could cause cadence switch? Can we use long term average frame duration, which I assume shouldn't change over time in most cases. That'll give us a better cadence estimate. But I may be missing the context here.
On 2015/04/24 05:25:01, xhwang wrote: > On 2015/04/24 01:54:06, DaleCurtis wrote: > > Hmm, just got everything hooked up and files with alternating timestamps > deltas > > like [17000, 16000, 17000, 16000, 17000, 16000] are problematic with the > current > > cadence detection approach. > > > > The average ends up oscillating too for cadence detection based on how which > > delta is more dominant for a given period of time. > > > > Still pondering what to do about it. Ideas welcome :) > > > > Interval: 16666, Duration: 0 > > Interval: 16666, Duration: 16666 > > Interval: 16666, Duration: 16666 > > Interval: 16666, Duration: 16666 > > Interval: 16666, Duration: 16888 > > Interval: 16666, Duration: 16722 > > Interval: 16666, Duration: 16680 > > Interval: 16666, Duration: 16670 > > Interval: 16666, Duration: 16667 > > Interval: 16666, Duration: 16666 > > Interval: 16666, Duration: 16666 > > Interval: 16666, Duration: 16666 > > Cadence switch: (0, 0) -> (1, 0) > > Interval: 16666, Duration: 16666 > > glitch time: 2.08325s > > Interval: 16666, Duration: 16733 > > glitch time: 1.16662s > > Interval: 16666, Duration: 16546 > > glitch time: 3.24987s > > Interval: 16666, Duration: 16709 > > glitch time: 1.86659s > > Interval: 16666, Duration: 16741 > > glitch time: 1.18329s > > Interval: 16666, Duration: 16548 > > glitch time: 4.79981s > > Interval: 16666, Duration: 16637 > > glitch time: 0.99996s > > Cadence switch: (1, 0) -> (0, 0) > > What's the "Intervel" and "Duration" in this table? Are you saying alternating > frame duration could cause cadence switch? Can we use long term average frame > duration, which I assume shouldn't change over time in most cases. That'll give > us a better cadence estimate. But I may be missing the context here. Interval is render interval, duration is average_frame_duration_. Cadence values are (cadence, fractional cadence) -> new values. Yeah, a moving average of sufficient N is what I've been thinking too.
On 2015/04/24 05:49:05, DaleCurtis wrote: > On 2015/04/24 05:25:01, xhwang wrote: > > On 2015/04/24 01:54:06, DaleCurtis wrote: > > > Hmm, just got everything hooked up and files with alternating timestamps > > deltas > > > like [17000, 16000, 17000, 16000, 17000, 16000] are problematic with the > > current > > > cadence detection approach. Ah. This is why I absolutely loathe code that computes media timestamps in integer numbers of milliseconds. They just don't provide sufficient precision. > > > Cadence switch: (0, 0) -> (1, 0) > > > Interval: 16666, Duration: 16666 > > > glitch time: 2.08325s > > > Interval: 16666, Duration: 16733 > > > glitch time: 1.16662s > > > Interval: 16666, Duration: 16546 > > > glitch time: 3.24987s > > > Interval: 16666, Duration: 16709 > > > glitch time: 1.86659s > > > Interval: 16666, Duration: 16741 > > > glitch time: 1.18329s > > > Interval: 16666, Duration: 16548 > > > glitch time: 4.79981s > > > Interval: 16666, Duration: 16637 > > > glitch time: 0.99996s > > > Cadence switch: (1, 0) -> (0, 0) > > > Interval is render interval, duration is average_frame_duration_. Cadence values > are (cadence, fractional cadence) -> new values. Yeah, a moving average of > sufficient N is what I've been thinking too. Wait. Is this how the current code behaves? If so, it looks like the average_frame_duration_ is very close to the ideal value of 16667. Is the problem that the glitch time is so short? One solution might be to compute the standard deviation of the recent frame durations, and allow for a shorter glitch time for higher variance. So, a well-behaved media file that provides 16667 for all frame durations would have a standard deviation of zero and therefore we'd want to make sure the glitch time was very long. Likewise, a poor media file that produces 16000, 17000, 17000, ...; would have a standard deviation of around 500, and this may indicate a lower glitch time is acceptable.
On 2015/04/24 20:49:40, miu wrote: > On 2015/04/24 05:49:05, DaleCurtis wrote: > > On 2015/04/24 05:25:01, xhwang wrote: > > > On 2015/04/24 01:54:06, DaleCurtis wrote: > > > > Hmm, just got everything hooked up and files with alternating timestamps > > > deltas > > > > like [17000, 16000, 17000, 16000, 17000, 16000] are problematic with the > > > current > > > > cadence detection approach. > > Ah. This is why I absolutely loathe code that computes media timestamps in > integer numbers of milliseconds. They just don't provide sufficient precision. > > > > > Cadence switch: (0, 0) -> (1, 0) > > > > Interval: 16666, Duration: 16666 > > > > glitch time: 2.08325s > > > > Interval: 16666, Duration: 16733 > > > > glitch time: 1.16662s > > > > Interval: 16666, Duration: 16546 > > > > glitch time: 3.24987s > > > > Interval: 16666, Duration: 16709 > > > > glitch time: 1.86659s > > > > Interval: 16666, Duration: 16741 > > > > glitch time: 1.18329s > > > > Interval: 16666, Duration: 16548 > > > > glitch time: 4.79981s > > > > Interval: 16666, Duration: 16637 > > > > glitch time: 0.99996s > > > > Cadence switch: (1, 0) -> (0, 0) > > > > > Interval is render interval, duration is average_frame_duration_. Cadence > values > > are (cadence, fractional cadence) -> new values. Yeah, a moving average of > > sufficient N is what I've been thinking too. > > Wait. Is this how the current code behaves? If so, it looks like the > average_frame_duration_ is very close to the ideal value of 16667. A few hundred microseconds adds up really fast to overwhelm a few milliseconds of allowed drift. I.e. frame_duration/2 (~8ms) is exhausted in the "glitch time" values you see above. > > Is the problem that the glitch time is so short? One solution might be to > compute the standard deviation of the recent frame durations, and allow for a > shorter glitch time for higher variance. So, a well-behaved media file that > provides 16667 for all frame durations would have a standard deviation of zero > and therefore we'd want to make sure the glitch time was very long. Likewise, a > poor media file that produces 16000, 17000, 17000, ...; would have a standard > deviation of around 500, and this may indicate a lower glitch time is > acceptable. The moving average approach feels simpler than variable thresholds. PTAL. I've updated the code to use moving average of size 25.
Looking pretty good. I just have a few more comments. https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_aver... File media/base/moving_average.cc (right): https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_aver... media/base/moving_average.cc:24: } If you initialize |samples_| with all zeros (which is the default value), we don't need this "if" block. https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_aver... media/base/moving_average.cc:33: return total_ / std::min(depth_, count_); nit: If we always choose a |depth_| of a power of 2 (e.g. 32, which should be close enough to 25), then in most cases we can use bit shift instead of division. But it's a bit tricky since we are dealing with TimeDelta instead of integer here. https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_aver... File media/base/moving_average.h (right): https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_aver... media/base/moving_average.h:21: // Add a new sample to the average; replaces the oldest sample if |depth_| has nit: s/Add/Adds https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_aver... media/base/moving_average.h:38: scoped_ptr<base::TimeDelta[]> samples_; Will a vector work? https://codereview.chromium.org/1021943002/diff/300001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/300001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:14: static const int kMovingAverageSamples = 25; nit: no need to use "static" here. https://codereview.chromium.org/1021943002/diff/320001/media/filters/video_ca... File media/filters/video_cadence_estimator.h (right): https://codereview.chromium.org/1021943002/diff/320001/media/filters/video_ca... media/filters/video_cadence_estimator.h:22: // Forced integer cadence means we round the actual cadence (~2.0029 in the nit: s/Forced/Clamped? https://codereview.chromium.org/1021943002/diff/320001/media/filters/video_ca... media/filters/video_cadence_estimator.h:29: // Obviously forcing cadence like that leads to drift over time of the actual nit: s/forcing/clamping https://codereview.chromium.org/1021943002/diff/320001/media/filters/video_ca... media/filters/video_cadence_estimator.h:91: int get_cadence_for_testing() const { When you get something like "2", how do you know whether it's cadence, or fractional cadence? https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:36: base::TimeDelta time_until_fractional_cadence_glitch; It seems these two are only for logging? Can we just have one variable, e.g. time_until_cadence_glitch? https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:41: false, &new_cadence, &time_until_cadence_glitch)) { DCHECK(new_cadence)? https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:52: render_intervals_cadence_held_ = 0; The comment says 132 // ... |render_intervals_cadence_held_| is 133 // incremented for each UpdateCadenceEstimate() where the cadence remains the 134 // same. But here we are reset it to zero... https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:64: if (new_cadence == previous_cadence_ && |previous_cadence| is really a |pending_cadence_| or a |pending_new_cadence_|. It hasn't been used yet, so |previous| is a bit misleading. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:66: if (++render_intervals_cadence_held_ * render_interval >= note: render_interval can be different every time so this is an approximate. But I guess it's good enough in real life. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:81: << time_until_fractional_cadence_glitch << ")"; nit: just return false here, so that you don't need "else" on l.83? https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:86: render_intervals_cadence_held_ = 1; If in the extreme case where cadence estimation oscillates randomly (e.g. for the 2:3 case when the ideal cadence is 2.5, and the estimate could be either 2 or 3), we could have |previous_cadence_| changing pretty often so that it can take very long to reach kMinimumCadenceDurationMs because we are resetting |render_intervals_cadence_held_| here. Is that expected? https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:100: *time_until_glitch = base::TimeDelta(); DCHECK instead of set? https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:128: // rendered after accounting for those |clamped_cadence| frames. We are checking "fractional ?" in 3 places in this function. Maybe we should just split this function into CalculateCalmpedCadence() and CalculateFractionalCadence()... Then we can just do the estimation in a straightforward way using simple math... https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... File media/filters/video_cadence_estimator.h (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.h:100: // Determines an ideal integer cadence for the given |render_interval| and "ideal integer cadence" is not defined anywhere else. Does that mean clamped or fractional cadence? I guess "ideal" means it satisfies |minimum_time_until_glitch_|? https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.h:104: // ideal integer cadence. Otherwise, do we still set |cadence|? https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... File media/filters/video_cadence_estimator_unittest.cc (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator_unittest.cc:13: static const int kMinimumAcceptableTimeBetweenGlitchesSecs = 8; nit: "static" not needed. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator_unittest.cc:127: estimator->disable_cadence_hysteresis_for_testing(); Can you also test with the hysteresis enabled? https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:212: last_deadline_max_ = deadline_min; It's odd that deadline_max_ = deadline_min. Maybe s/deadline_min/deadline? https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:260: // we have enough frames to base the the maximum on frame duration. drop one "the" https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:457: frame_queue_[best_frame].ideal_render_count = new_ideal_render_count; Can FindBestFrameByCadenceInternal() update the ideal_render_count directly? Then we don't need l.470 and l.487 can be simplified. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... media/filters/video_renderer_algorithm.h:209: // |deadline_min|. There's always a best frame by drift, so the return value Is there any reason not to use the drift from (min + max) / 2? By using |min| we are preferring frames before the current interval.
https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_aver... File media/base/moving_average.cc (right): https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_aver... media/base/moving_average.cc:24: } On 2015/04/28 16:01:07, xhwang wrote: > If you initialize |samples_| with all zeros (which is the default value), we > don't need this "if" block. Done. https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_aver... media/base/moving_average.cc:33: return total_ / std::min(depth_, count_); On 2015/04/28 16:01:07, xhwang wrote: > nit: If we always choose a |depth_| of a power of 2 (e.g. 32, which should be > close enough to 25), then in most cases we can use bit shift instead of > division. But it's a bit tricky since we are dealing with TimeDelta instead of > integer here. Hmm, I feel that's a bit of premature optimization :) I've added a TODO() to do some testing to see if this is a hot spot. https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_aver... File media/base/moving_average.h (right): https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_aver... media/base/moving_average.h:21: // Add a new sample to the average; replaces the oldest sample if |depth_| has On 2015/04/28 16:01:07, xhwang wrote: > nit: s/Add/Adds Done. https://codereview.chromium.org/1021943002/diff/300001/media/base/moving_aver... media/base/moving_average.h:38: scoped_ptr<base::TimeDelta[]> samples_; On 2015/04/28 16:01:07, xhwang wrote: > Will a vector work? Done. https://codereview.chromium.org/1021943002/diff/300001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/300001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:14: static const int kMovingAverageSamples = 25; On 2015/04/28 16:01:07, xhwang wrote: > nit: no need to use "static" here. Done. https://codereview.chromium.org/1021943002/diff/320001/media/filters/video_ca... File media/filters/video_cadence_estimator.h (right): https://codereview.chromium.org/1021943002/diff/320001/media/filters/video_ca... media/filters/video_cadence_estimator.h:22: // Forced integer cadence means we round the actual cadence (~2.0029 in the On 2015/04/28 16:01:08, xhwang wrote: > nit: s/Forced/Clamped? Done. https://codereview.chromium.org/1021943002/diff/320001/media/filters/video_ca... media/filters/video_cadence_estimator.h:29: // Obviously forcing cadence like that leads to drift over time of the actual On 2015/04/28 16:01:08, xhwang wrote: > nit: s/forcing/clamping Done. https://codereview.chromium.org/1021943002/diff/320001/media/filters/video_ca... media/filters/video_cadence_estimator.h:91: int get_cadence_for_testing() const { On 2015/04/28 16:01:08, xhwang wrote: > When you get something like "2", how do you know whether it's cadence, or > fractional cadence? Tests can figure this out by calling GetCadenceForFrame(1), it simplifies many conditionals to have this collapsed into a single function. If you feel strongly I can change it. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:36: base::TimeDelta time_until_fractional_cadence_glitch; On 2015/04/28 16:01:08, xhwang wrote: > It seems these two are only for logging? Can we just have one variable, e.g. > time_until_cadence_glitch? No, because we need both values for the log message if both calculations fail. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:41: false, &new_cadence, &time_until_cadence_glitch)) { On 2015/04/28 16:01:08, xhwang wrote: > DCHECK(new_cadence)? Done. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:52: render_intervals_cadence_held_ = 0; On 2015/04/28 16:01:08, xhwang wrote: > The comment says > > 132 // ... |render_intervals_cadence_held_| is > 133 // incremented for each UpdateCadenceEstimate() where the cadence remains > the > 134 // same. > > But here we are reset it to zero... > This prevents the previous cadence values from accumulating incorrectly. I've clarified the comments. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:64: if (new_cadence == previous_cadence_ && On 2015/04/28 16:01:08, xhwang wrote: > |previous_cadence| is really a |pending_cadence_| or a |pending_new_cadence_|. > It hasn't been used yet, so |previous| is a bit misleading. Done. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:66: if (++render_intervals_cadence_held_ * render_interval >= On 2015/04/28 16:01:08, xhwang wrote: > note: render_interval can be different every time so this is an approximate. But > I guess it's good enough in real life. Yup. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:81: << time_until_fractional_cadence_glitch << ")"; On 2015/04/28 16:01:08, xhwang wrote: > nit: just return false here, so that you don't need "else" on l.83? Done. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:86: render_intervals_cadence_held_ = 1; On 2015/04/28 16:01:08, xhwang wrote: > If in the extreme case where cadence estimation oscillates randomly (e.g. for > the 2:3 case when the ideal cadence is 2.5, and the estimate could be either 2 > or 3), we could have |previous_cadence_| changing pretty often so that it can > take very long to reach kMinimumCadenceDurationMs because we are resetting > |render_intervals_cadence_held_| here. Is that expected? 2.5 is pretty unlikely to clamp to 2 or 3, it exhausts the allowable drift in a single frame. It should almost always clamp to zero. Even if it does clamp once or twice, I doubt it would clamp for 100ms. Though we'll want some UMA statistics added to this later on for seeing common frame durations. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:100: *time_until_glitch = base::TimeDelta(); On 2015/04/28 16:01:08, xhwang wrote: > DCHECK instead of set? No, I want to clear them every time. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:128: // rendered after accounting for those |clamped_cadence| frames. On 2015/04/28 16:01:08, xhwang wrote: > We are checking "fractional ?" in 3 places in this function. Maybe we should > just split this function into CalculateCalmpedCadence() and > CalculateFractionalCadence()... Then we can just do the estimation in a > straightforward way using simple math... The math is already tricky, I'd rather not duplicate it in two places. Splitting the functions just adds to the complexity because I'll need a third helper function for the last bit of calculations. It also doesn't simplify the math in any way as far as I can tell, can you elaborate on why you think it would simplify the math? I feel it's more readable done within a single function too as it's easier to figure out what's going on when you see how fractional calculations only change the inputs slightly. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... File media/filters/video_cadence_estimator.h (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.h:100: // Determines an ideal integer cadence for the given |render_interval| and On 2015/04/28 16:01:08, xhwang wrote: > "ideal integer cadence" is not defined anywhere else. Does that mean clamped or > fractional cadence? I guess "ideal" means it satisfies > |minimum_time_until_glitch_|? Ideal is misleading here, so I changed the wording to just be "clamped cadence" since a clamped cadence is always present, whether it's good or not is explained by the subsequent text. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.h:104: // ideal integer cadence. On 2015/04/28 16:01:08, xhwang wrote: > Otherwise, do we still set |cadence|? Yes, always set. Clarified. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... File media/filters/video_cadence_estimator_unittest.cc (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator_unittest.cc:13: static const int kMinimumAcceptableTimeBetweenGlitchesSecs = 8; On 2015/04/28 16:01:08, xhwang wrote: > nit: "static" not needed. Done. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator_unittest.cc:127: estimator->disable_cadence_hysteresis_for_testing(); On 2015/04/28 16:01:08, xhwang wrote: > Can you also test with the hysteresis enabled? Whoops, forgot about that. Done. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:212: last_deadline_max_ = deadline_min; On 2015/04/28 16:01:08, xhwang wrote: > It's odd that deadline_max_ = deadline_min. Maybe s/deadline_min/deadline? Done. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:260: // we have enough frames to base the the maximum on frame duration. On 2015/04/28 16:01:08, xhwang wrote: > drop one "the" Done. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:457: frame_queue_[best_frame].ideal_render_count = new_ideal_render_count; On 2015/04/28 16:01:08, xhwang wrote: > Can FindBestFrameByCadenceInternal() update the ideal_render_count directly? > Then we don't need l.470 and l.487 can be simplified. It can't since it's a const function, which is required for it to be used in EffectiveFramesQueued() (where we don't want to change the ideal render count). https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... media/filters/video_renderer_algorithm.h:209: // |deadline_min|. There's always a best frame by drift, so the return value On 2015/04/28 16:01:08, xhwang wrote: > Is there any reason not to use the drift from (min + max) / 2? By using |min| we > are preferring frames before the current interval. Good question, I forgot the reason and had to work it out again. In practice it doesn't matter because frames are treated as contiguous. So if there's a frame that has a drift of -10ms (it ends 10ms before deadline_min) there's always another frame is considered to start at deadline_min - 10ms, that would overlap deadline and subsequently have a drift of zero. I've added some clarifying text: // Note: Drift calculations assume contiguous frames in the time domain, so // it's not possible to have a case where a frame is -10ms from |deadline_min| // and another frame which is at some time after |deadline_min|. The second // frame would be considered to start at -10ms before |deadline_min| and would // overlap |deadline_min|, so its drift would be zero.
Thanks for the patience! LGTM with two last comments (where I don't have strong opinions). https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:36: base::TimeDelta time_until_fractional_cadence_glitch; On 2015/04/28 21:45:24, DaleCurtis wrote: > On 2015/04/28 16:01:08, xhwang wrote: > > It seems these two are only for logging? Can we just have one variable, e.g. > > time_until_cadence_glitch? > > No, because we need both values for the log message if both calculations fail. Acknowledged. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:86: render_intervals_cadence_held_ = 1; On 2015/04/28 21:45:24, DaleCurtis wrote: > On 2015/04/28 16:01:08, xhwang wrote: > > If in the extreme case where cadence estimation oscillates randomly (e.g. for > > the 2:3 case when the ideal cadence is 2.5, and the estimate could be either 2 > > or 3), we could have |previous_cadence_| changing pretty often so that it can > > take very long to reach kMinimumCadenceDurationMs because we are resetting > > |render_intervals_cadence_held_| here. Is that expected? > > 2.5 is pretty unlikely to clamp to 2 or 3, it exhausts the allowable drift in a > single frame. It should almost always clamp to zero. Even if it does clamp once > or twice, I doubt it would clamp for 100ms. Though we'll want some UMA > statistics added to this later on for seeing common frame durations. Acknowledged. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:100: *time_until_glitch = base::TimeDelta(); On 2015/04/28 21:45:24, DaleCurtis wrote: > On 2015/04/28 16:01:08, xhwang wrote: > > DCHECK instead of set? > > No, I want to clear them every time. hmm, wondering why? The caller already initializes them. This could hide errors in the caller side. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:128: // rendered after accounting for those |clamped_cadence| frames. On 2015/04/28 21:45:24, DaleCurtis wrote: > On 2015/04/28 16:01:08, xhwang wrote: > > We are checking "fractional ?" in 3 places in this function. Maybe we should > > just split this function into CalculateCalmpedCadence() and > > CalculateFractionalCadence()... Then we can just do the estimation in a > > straightforward way using simple math... > > The math is already tricky, I'd rather not duplicate it in two places. Splitting > the functions just adds to the complexity because I'll need a third helper > function for the last bit of calculations. It also doesn't simplify the math in > any way as far as I can tell, can you elaborate on why you think it would > simplify the math? > > I feel it's more readable done within a single function too as it's easier to > figure out what's going on when you see how fractional calculations only change > the inputs slightly. For me I have to read this function twice, one with fractional being true, one being false to understand how things work. Since I already read through this, it's not a problem now. But I do feel readability could improve by splitting it to two functions, where you don't need to introduce new concepts like rendered_frame_duration and actual_frame_duration. You may end up doing the division twice but that'll be only a few lines. Maybe it's only me though :) If other reviewers (+miu) feel it's okay, I have no problem with the current code. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... media/filters/video_renderer_algorithm.cc:457: frame_queue_[best_frame].ideal_render_count = new_ideal_render_count; On 2015/04/28 21:45:24, DaleCurtis wrote: > On 2015/04/28 16:01:08, xhwang wrote: > > Can FindBestFrameByCadenceInternal() update the ideal_render_count directly? > > Then we don't need l.470 and l.487 can be simplified. > > It can't since it's a const function, which is required for it to be used in > EffectiveFramesQueued() (where we don't want to change the ideal render count). Acknowledged. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... File media/filters/video_renderer_algorithm.h (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_re... media/filters/video_renderer_algorithm.h:209: // |deadline_min|. There's always a best frame by drift, so the return value On 2015/04/28 21:45:24, DaleCurtis wrote: > On 2015/04/28 16:01:08, xhwang wrote: > > Is there any reason not to use the drift from (min + max) / 2? By using |min| > we > > are preferring frames before the current interval. > > Good question, I forgot the reason and had to work it out again. In practice it > doesn't matter because frames are treated as contiguous. So if there's a frame > that has a drift of -10ms (it ends 10ms before deadline_min) there's always > another frame is considered to start at deadline_min - 10ms, that would overlap > deadline and subsequently have a drift of zero. > > I've added some clarifying text: > > // Note: Drift calculations assume contiguous frames in the time domain, so > // it's not possible to have a case where a frame is -10ms from |deadline_min| > // and another frame which is at some time after |deadline_min|. The second > // frame would be considered to start at -10ms before |deadline_min| and would > // overlap |deadline_min|, so its drift would be zero. Acknowledged. https://codereview.chromium.org/1021943002/diff/360001/media/base/moving_aver... File media/base/moving_average.cc (right): https://codereview.chromium.org/1021943002/diff/360001/media/base/moving_aver... media/base/moving_average.cc:12: : depth_(depth), count_(0), samples_(depth_, base::TimeDelta()) { nit: base::TimeDelta() is not needed as it's the default value.
sandersd, briananderson, miu: PTAL or remove yourself from the reviewers list if you're happy to let others review. Thanks for your review! Sorry it's so much code :) https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:100: *time_until_glitch = base::TimeDelta(); On 2015/04/28 22:17:39, xhwang wrote: > On 2015/04/28 21:45:24, DaleCurtis wrote: > > On 2015/04/28 16:01:08, xhwang wrote: > > > DCHECK instead of set? > > > > No, I want to clear them every time. > > hmm, wondering why? The caller already initializes them. This could hide errors > in the caller side. Actually, they don't need to be initialized here anymore; that was older code that I ditched. Removed. https://codereview.chromium.org/1021943002/diff/340001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:128: // rendered after accounting for those |clamped_cadence| frames. On 2015/04/28 22:17:39, xhwang wrote: > On 2015/04/28 21:45:24, DaleCurtis wrote: > > On 2015/04/28 16:01:08, xhwang wrote: > > > We are checking "fractional ?" in 3 places in this function. Maybe we should > > > just split this function into CalculateCalmpedCadence() and > > > CalculateFractionalCadence()... Then we can just do the estimation in a > > > straightforward way using simple math... > > > > The math is already tricky, I'd rather not duplicate it in two places. > Splitting > > the functions just adds to the complexity because I'll need a third helper > > function for the last bit of calculations. It also doesn't simplify the math > in > > any way as far as I can tell, can you elaborate on why you think it would > > simplify the math? > > > > I feel it's more readable done within a single function too as it's easier to > > figure out what's going on when you see how fractional calculations only > change > > the inputs slightly. > > For me I have to read this function twice, one with fractional being true, one > being false to understand how things work. Since I already read through this, > it's not a problem now. But I do feel readability could improve by splitting it > to two functions, where you don't need to introduce new concepts like > rendered_frame_duration and actual_frame_duration. You may end up doing the > division twice but that'll be only a few lines. > > Maybe it's only me though :) If other reviewers (+miu) feel it's okay, I have no > problem with the current code. l.136, l.148 would end up in a third function with a bunch of variables passed in to avoid duplicating that code; two of those variables would be "actual_frame_duration and "rendered_frame_duration", so they wouldn't go away :) https://codereview.chromium.org/1021943002/diff/360001/media/base/moving_aver... File media/base/moving_average.cc (right): https://codereview.chromium.org/1021943002/diff/360001/media/base/moving_aver... media/base/moving_average.cc:12: : depth_(depth), count_(0), samples_(depth_, base::TimeDelta()) { On 2015/04/28 22:17:39, xhwang wrote: > nit: base::TimeDelta() is not needed as it's the default value. Done.
dalecurtis@chromium.org changed reviewers: - brianderson@chromium.org, sandersd@chromium.org
sandersd, brianderson -> cc per offline discussion. miu@ PTAL or let me know if you won't have time, since this is behind a flag post-land is fine if you don't see anything glaringly wrong.
lgtm https://codereview.chromium.org/1021943002/diff/400001/media/filters/video_ca... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1021943002/diff/400001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:141: const double duration_delta = std::abs( nit: Instead of std::abs((expr).InMicroseconds()), use: (expr).magnitude().InMicroseconds()
\o/ thanks for review everyone :) https://codereview.chromium.org/1021943002/diff/400001/media/filters/video_ca... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1021943002/diff/400001/media/filters/video_ca... media/filters/video_cadence_estimator.cc:141: const double duration_delta = std::abs( On 2015/04/30 21:57:07, miu wrote: > nit: Instead of std::abs((expr).InMicroseconds()), use: > > (expr).magnitude().InMicroseconds() Done.
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/1021943002/#ps440001 (title: "Fix evaluation order.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1021943002/440001
Message was sent while issue was closed.
Committed patchset #15 (id:440001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/3c36fc4302b32f1db2b86a4b91729faeefe44898 Cr-Commit-Position: refs/heads/master@{#327864} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
