Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1125893002:
Implement support for 2-pattern cadence. (Closed)
|
Created:
5 years, 7 months ago by DaleCurtis Modified:
5 years, 7 months ago Reviewers:
xhwang CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement support for 2-pattern cadence.
An N pattern cadence is simply figuring out if N times the
frame duration has an integer cadence (M) relative to the
render interval, then figuring out how to distribute N
values such that sum(x1 + x2 + x3 ... xN) == M.
I haven't dug through our discussion on how to compute this
for N > 2, but for N == 2, it's simple to just use
x1 == ceil(M/2), x2 == floor(M/2)
This change refactors the main function in VideoCadenceEstimator
into smaller pieces (an original review request :) in order to
reuse them for detecting 2-pattern cadence.
BUG=439548
TEST=all unit tests still pass, 3:2 cadence tests pass.
Committed: https://crrev.com/b3a547e78d4d07186d114128934a2931e55880b5
Cr-Commit-Position: refs/heads/master@{#329813}
Patch Set 1 #Patch Set 2 : Rebase. Fix tests. #
Total comments: 1
Patch Set 3 : Remove bad code. #Patch Set 4 : Rebase ontop of overage fix. #
Total comments: 31
Patch Set 5 : Comments. #
Total comments: 1
Patch Set 6 : Refactor CalculateCadence. #
Total comments: 10
Patch Set 7 : Fix comments. #
Messages
Total messages: 17 (2 generated)
dalecurtis@chromium.org changed reviewers: + xhwang@chromium.org
Not for review yet, but this is based on the discussions we had for figuring out n-pattern cadence! :) Take a look if you want / have time, if I get everything else landed in time, I might see about landing this too.
Whoops, typoed the email, but the cl description is correct now.
Ready for review now! https://codereview.chromium.org/1125893002/diff/20001/media/filters/video_cad... File media/filters/video_cadence_estimator.h (right): https://codereview.chromium.org/1125893002/diff/20001/media/filters/video_cad... media/filters/video_cadence_estimator.h:91: int GetCadenceOffsetForFrame(int frame_cadence) const; I'm not super happy with this, so if you have ideas to make this better let me know :)
https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:58: std::vector<int> new_cadence; s/std::vector<int>/Cadence https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:79: new_cadence.push_back(new_cadence[0] - 1); You can hide the block 59-79 into one CalculateCadence call now. Then clamp/fractional/two_pattern are all implementation detail within CalculateCadence(). Then you don't need 3 time_until* variables. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:142: return i; Even though we are only using 2-cadence now, it'll be nice if our code is generic (so that it can handle N cadence easily). For example, what if we have a cadence of [3 2 3 2 2], then with a |frame_cadence| of 3, you need to be able to tell which 3 in the cadence it is. Looking at the code, I *think* the problem comes from the fact that |last_frame_index_| is the index in the queue, not the true frame index. By "true frame index" I mean the index from the first frame in the stream. For example, if you have 10000 frames in the stream, the last frame's index is 10000 - 1. If you can somehow maintain the "true frame index" then you can always use it to get the cadence of a frame, without worrying about CadenceOffsetForFrame. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:156: const Cadence& b) const { Since they are just vectors, can't you just do a == b? Then you don't need a function... https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:238: std::ostream_iterator<int>(os, ":")); s/:/,/ ? In the N cadence case, it would make more sense to have [3, 2, 3, 2, 2] than [3:2:3:2:2] IMHO https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... File media/filters/video_cadence_estimator.h (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.h:19: // Cadence is the ratio of the frame duration to render interval length. I.e. Please update the comments before landing. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.h:91: int GetCadenceOffsetForFrame(int frame_cadence) const; See comment below about whether we really need this call (which is hard to name and reason about). https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.h:111: bool CompareCadence(const Cadence& a, const Cadence& b) const; See comment below. You can just do a == b and don't need this function. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.h:149: base::TimeDelta* time_until_glitch) const; It's odd that the function IsAcceptableCadence() doesn't take a cadence at all :) How about IsAcceptableCadence(double perfect_cadence, const Cadence& cadence, base::TimeDelta render_interval, ...)? https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.h:149: base::TimeDelta* time_until_glitch) const; naming nit: You have max_acceptable_drift and time_until_glitch, but it seems like glitch is equivalent to "max_drift". Can we be consistent? For example |max_drift| and |time_until_max_drift|? https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... File media/filters/video_cadence_estimator_unittest.cc (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator_unittest.cc:72: VerifyCadenceVector(&estimator, 24, 60, CreateTwoCadence(3, 2)); To be fancy, you can have VerifyCadenceVector() take a string like "[3, 2]" as the 3rd parameter (where [] will be the empty cadence). Then this code would be more readable, and you don't need those Create*Cadence() functions. It's also consistent with GetCadenceForTesting(). https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_ren... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_ren... media/filters/video_renderer_algorithm.cc:214: deadline_min - frame_queue_.front().start_time >= render_interval_ / -2) { Not knowing what we are doing here (from a previous CL?), but "/ -2" looks odd ;) https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_ren... File media/filters/video_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_ren... media/filters/video_renderer_algorithm_unittest.cc:953: } Aren't these covered by VideoCadenceEstimatorTest::CadenceCalculations test?
https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:58: std::vector<int> new_cadence; On 2015/05/12 06:04:46, xhwang wrote: > s/std::vector<int>/Cadence Done. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:79: new_cadence.push_back(new_cadence[0] - 1); On 2015/05/12 06:04:46, xhwang wrote: > You can hide the block 59-79 into one CalculateCadence call now. Then > clamp/fractional/two_pattern are all implementation detail within > CalculateCadence(). Then you don't need 3 time_until* variables. I don't see how this is true? We still want to print all three values if no cadence is found. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:142: return i; On 2015/05/12 06:04:46, xhwang wrote: > Even though we are only using 2-cadence now, it'll be nice if our code is > generic (so that it can handle N cadence easily). > > For example, what if we have a cadence of [3 2 3 2 2], then with a > |frame_cadence| of 3, you need to be able to tell which 3 in the cadence it is. > > Looking at the code, I *think* the problem comes from the fact that > |last_frame_index_| is the index in the queue, not the true frame index. By > "true frame index" I mean the index from the first frame in the stream. For > example, if you have 10000 frames in the stream, the last frame's index is 10000 > - 1. > > If you can somehow maintain the "true frame index" then you can always use it to > get the cadence of a frame, without worrying about CadenceOffsetForFrame. Yeah this is the approach I wanted to go with, but I wasn't convinced of its feasibility. After some testing though, it seems to work correctly. What I'm doing now is as follows: - |cadence_frame_counter_| incremented for every frame removed, or expired which should have been using cadence. - |cadence_frame_counter_| reset to zero when a new cadence is detected or when the cadence selected frame is overridden. This seems reasonable and seems to work in practice. I'm still looking at adding a couple tests more though. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:156: const Cadence& b) const { On 2015/05/12 06:04:46, xhwang wrote: > Since they are just vectors, can't you just do a == b? Then you don't need a > function... Hmm, I didn't think we had an operator== defined for vectors. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:238: std::ostream_iterator<int>(os, ":")); On 2015/05/12 06:04:46, xhwang wrote: > s/:/,/ ? > > In the N cadence case, it would make more sense to have [3, 2, 3, 2, 2] than > [3:2:3:2:2] IMHO Industry standard is with :, http://en.wikipedia.org/wiki/Telecine https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... File media/filters/video_cadence_estimator.h (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.h:19: // Cadence is the ratio of the frame duration to render interval length. I.e. On 2015/05/12 06:04:46, xhwang wrote: > Please update the comments before landing. Done. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.h:91: int GetCadenceOffsetForFrame(int frame_cadence) const; On 2015/05/12 06:04:46, xhwang wrote: > See comment below about whether we really need this call (which is hard to name > and reason about). Agreed, removed. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.h:111: bool CompareCadence(const Cadence& a, const Cadence& b) const; On 2015/05/12 06:04:46, xhwang wrote: > See comment below. You can just do a == b and don't need this function. Done. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.h:149: base::TimeDelta* time_until_glitch) const; On 2015/05/12 06:04:46, xhwang wrote: > naming nit: You have max_acceptable_drift and time_until_glitch, but it seems > like glitch is equivalent to "max_drift". Can we be consistent? For example > |max_drift| and |time_until_max_drift|? Done. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.h:149: base::TimeDelta* time_until_glitch) const; On 2015/05/12 06:04:46, xhwang wrote: > It's odd that the function IsAcceptableCadence() doesn't take a cadence at all > :) > > How about > IsAcceptableCadence(double perfect_cadence, const Cadence& cadence, > base::TimeDelta render_interval, ...)? > Hmm, currently the function is cut such that CalculateCadence and CalculateFractionalCadence contain the parts that are different. Doing as you say will add complexity and arguments which are unused. I'd rather just rename the function unless you have some ideas on how to better organize the split of these functions. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... File media/filters/video_cadence_estimator_unittest.cc (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator_unittest.cc:72: VerifyCadenceVector(&estimator, 24, 60, CreateTwoCadence(3, 2)); On 2015/05/12 06:04:46, xhwang wrote: > To be fancy, you can have VerifyCadenceVector() take a string like "[3, 2]" as > the 3rd parameter (where [] will be the empty cadence). Then this code would be > more readable, and you don't need those Create*Cadence() functions. > > It's also consistent with GetCadenceForTesting(). Done. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_ren... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_ren... media/filters/video_renderer_algorithm.cc:214: deadline_min - frame_queue_.front().start_time >= render_interval_ / -2) { On 2015/05/12 06:04:46, xhwang wrote: > Not knowing what we are doing here (from a previous CL?), but "/ -2" looks odd > ;) Whoops, yeah this was from https://codereview.chromium.org/1124333012/ -- rebased. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_ren... File media/filters/video_renderer_algorithm_unittest.cc (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_ren... media/filters/video_renderer_algorithm_unittest.cc:953: } On 2015/05/12 06:04:46, xhwang wrote: > Aren't these covered by VideoCadenceEstimatorTest::CadenceCalculations test? In a way, this forces them to be detected via EnqueueFrames / Render cycles though, so it's a double check. https://codereview.chromium.org/1125893002/diff/70001/media/filters/video_ren... File media/filters/video_renderer_algorithm.cc (right): https://codereview.chromium.org/1125893002/diff/70001/media/filters/video_ren... media/filters/video_renderer_algorithm.cc:261: size_t frames_to_expire = last_frame_index_; This prevents overflow of the arithmetic below without extra conditionals.
Comments on old comments. Not reviewing new code yet. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:79: new_cadence.push_back(new_cadence[0] - 1); On 2015/05/12 20:22:43, DaleCurtis wrote: > On 2015/05/12 06:04:46, xhwang wrote: > > You can hide the block 59-79 into one CalculateCadence call now. Then > > clamp/fractional/two_pattern are all implementation detail within > > CalculateCadence(). Then you don't need 3 time_until* variables. > > I don't see how this is true? We still want to print all three values if no > cadence is found. So we are doing this only for logging purposes? Hmm, I wonder what's the value of logging all three values? Imagine in a N-cadence world, you only find one optimal N-cadence, and have one minimum time_until_glitch. You would just log that one value. It won't be interesting to log the values of suboptimal cadences. Even though we are not there yet, I feel it still makes sense from an API's perspective. https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:238: std::ostream_iterator<int>(os, ":")); On 2015/05/12 20:22:43, DaleCurtis wrote: > On 2015/05/12 06:04:46, xhwang wrote: > > s/:/,/ ? > > > > In the N cadence case, it would make more sense to have [3, 2, 3, 2, 2] than > > [3:2:3:2:2] IMHO > > Industry standard is with :, http://en.wikipedia.org/wiki/Telecine Cool! TIL!
https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:79: new_cadence.push_back(new_cadence[0] - 1); On 2015/05/12 20:33:39, xhwang wrote: > On 2015/05/12 20:22:43, DaleCurtis wrote: > > On 2015/05/12 06:04:46, xhwang wrote: > > > You can hide the block 59-79 into one CalculateCadence call now. Then > > > clamp/fractional/two_pattern are all implementation detail within > > > CalculateCadence(). Then you don't need 3 time_until* variables. > > > > I don't see how this is true? We still want to print all three values if no > > cadence is found. > > So we are doing this only for logging purposes? Hmm, I wonder what's the value > of logging all three values? Imagine in a N-cadence world, you only find one > optimal N-cadence, and have one minimum time_until_glitch. You would just log > that one value. It won't be interesting to log the values of suboptimal > cadences. Even though we are not there yet, I feel it still makes sense from an > API's perspective. I think even in an N-cadence world you'd want to know how close you were if no cadence is detected or hysteresis bounces the cadence (which is the point of these logs today). We'd probably just want a different display mechanism.
https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:79: new_cadence.push_back(new_cadence[0] - 1); On 2015/05/12 20:53:18, DaleCurtis wrote: > On 2015/05/12 20:33:39, xhwang wrote: > > On 2015/05/12 20:22:43, DaleCurtis wrote: > > > On 2015/05/12 06:04:46, xhwang wrote: > > > > You can hide the block 59-79 into one CalculateCadence call now. Then > > > > clamp/fractional/two_pattern are all implementation detail within > > > > CalculateCadence(). Then you don't need 3 time_until* variables. > > > > > > I don't see how this is true? We still want to print all three values if no > > > cadence is found. > > > > So we are doing this only for logging purposes? Hmm, I wonder what's the value > > of logging all three values? Imagine in a N-cadence world, you only find one > > optimal N-cadence, and have one minimum time_until_glitch. You would just log > > that one value. It won't be interesting to log the values of suboptimal > > cadences. Even though we are not there yet, I feel it still makes sense from > an > > API's perspective. > > I think even in an N-cadence world you'd want to know how close you were if no > cadence is detected or hysteresis bounces the cadence (which is the point of > these logs today). We'd probably just want a different display mechanism. You'll always have an optimal cadence (under constraints, eg. vector length N), and a time-to-glitch for this optimal cadence. If the time-to-glitch is too large (not acceptable) then this "optimal cadence" is still too bad and no cadence is detected. So every time you call CalculateCadence() you'll always have a time-to-glitch. Then it's the caller's responsibility to decide when to report this time-to-glitch, e.g. when hysteresis is passed, or no cadence is found... My point is that we don't really need to know time-to-glitch for suboptimal solutions, so we don't need 3 time-until-glitch variables. Then we can have one call instead of 3...
https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1125893002/diff/40002/media/filters/video_cad... media/filters/video_cadence_estimator.cc:79: new_cadence.push_back(new_cadence[0] - 1); On 2015/05/12 21:02:54, xhwang wrote: > On 2015/05/12 20:53:18, DaleCurtis wrote: > > On 2015/05/12 20:33:39, xhwang wrote: > > > On 2015/05/12 20:22:43, DaleCurtis wrote: > > > > On 2015/05/12 06:04:46, xhwang wrote: > > > > > You can hide the block 59-79 into one CalculateCadence call now. Then > > > > > clamp/fractional/two_pattern are all implementation detail within > > > > > CalculateCadence(). Then you don't need 3 time_until* variables. > > > > > > > > I don't see how this is true? We still want to print all three values if > no > > > > cadence is found. > > > > > > So we are doing this only for logging purposes? Hmm, I wonder what's the > value > > > of logging all three values? Imagine in a N-cadence world, you only find one > > > optimal N-cadence, and have one minimum time_until_glitch. You would just > log > > > that one value. It won't be interesting to log the values of suboptimal > > > cadences. Even though we are not there yet, I feel it still makes sense from > > an > > > API's perspective. > > > > I think even in an N-cadence world you'd want to know how close you were if no > > cadence is detected or hysteresis bounces the cadence (which is the point of > > these logs today). We'd probably just want a different display mechanism. > > You'll always have an optimal cadence (under constraints, eg. vector length N), > and a time-to-glitch for this optimal cadence. If the time-to-glitch is too > large (not acceptable) then this "optimal cadence" is still too bad and no > cadence is detected. > > So every time you call CalculateCadence() you'll always have a time-to-glitch. > Then it's the caller's responsibility to decide when to report this > time-to-glitch, e.g. when hysteresis is passed, or no cadence is found... > > My point is that we don't really need to know time-to-glitch for suboptimal > solutions, so we don't need 3 time-until-glitch variables. Then we can have one > call instead of 3... You seem to feel strongly about this and I really don't care outside of using these logs for debugging :) So I've gone ahead and removed them and cleaned up the calculation functions in vein.
Looking pretty good. I only have a few comments about the comments. It seems a lot of comments are not up-to-date now. https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... File media/filters/video_cadence_estimator.h (right): https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... media/filters/video_cadence_estimator.h:27: // ~2.0029. This is the perfect cadence, which is a real number. All N-cadences [a_1, a_2, ... a_N] where a_k is an integer are an approximation of the perfect cadence. When N=1, we have a 1-frame cadence, which is the clamped integer cadence you have below. When N=2, you have a 2-frame cadence. When N > 1 and you have a 1:0:0... pattern, you have a fractional cadence. https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... media/filters/video_cadence_estimator.h:44: // the ratio of twice the frame duration to render interval length. I.e. for This (twice the frame duration) is an implementation detail. It might be easier if you just define the general N-cadence: the average of a_1, a_2, ..., a_k should be as close to the perfect cadence as possible. Then 1-frame, 2-frame and n-frame fractional cadences are just some special cases of the general N-cadence. https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... media/filters/video_cadence_estimator.h:102: // Given a frame |index|, where zero is the most recently rendered frame, s/index/frame_number https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... media/filters/video_cadence_estimator.h:112: // frame number is now 3. The VideoCadenceEstimator has no concept of rendered/removed/dropped frames. The |frame_number| is really just the index of frames after a cadence is detected. Maybe I am missing something, we can chat on this offline. https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... media/filters/video_cadence_estimator.h:182: // |fractional_cadence_| is the number of frames per render interval; the Comments needs to be updated. For example, you don't have a |fractional_cadence_| any more.
https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... File media/filters/video_cadence_estimator.h (right): https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... media/filters/video_cadence_estimator.h:27: // ~2.0029. On 2015/05/13 06:09:55, xhwang wrote: > This is the perfect cadence, which is a real number. All N-cadences [a_1, a_2, > ... a_N] where a_k is an integer are an approximation of the perfect cadence. > When N=1, we have a 1-frame cadence, which is the clamped integer cadence you > have below. When N=2, you have a 2-frame cadence. When N > 1 and you have a > 1:0:0... pattern, you have a fractional cadence. Done. https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... media/filters/video_cadence_estimator.h:102: // Given a frame |index|, where zero is the most recently rendered frame, On 2015/05/13 06:09:55, xhwang wrote: > s/index/frame_number Done. https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... media/filters/video_cadence_estimator.h:112: // frame number is now 3. On 2015/05/13 06:09:55, xhwang wrote: > The VideoCadenceEstimator has no concept of rendered/removed/dropped frames. The > |frame_number| is really just the index of frames after a cadence is detected. > Maybe I am missing something, we can chat on this offline. It definitely knows about rendered, we're asking for a render_interval in UpdateCadenceEstimate :) Removed is generic and does not necessarily indicate dropped. I've removed the example though since it does explicitly mention dropped and doesn't really add much here. https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... media/filters/video_cadence_estimator.h:182: // |fractional_cadence_| is the number of frames per render interval; the On 2015/05/13 06:09:55, xhwang wrote: > Comments needs to be updated. For example, you don't have a > |fractional_cadence_| any more. Done.
lgtm https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... File media/filters/video_cadence_estimator.h (right): https://codereview.chromium.org/1125893002/diff/90001/media/filters/video_cad... media/filters/video_cadence_estimator.h:112: // frame number is now 3. On 2015/05/13 17:12:42, DaleCurtis wrote: > On 2015/05/13 06:09:55, xhwang wrote: > > The VideoCadenceEstimator has no concept of rendered/removed/dropped frames. > The > > |frame_number| is really just the index of frames after a cadence is detected. > > Maybe I am missing something, we can chat on this offline. > > It definitely knows about rendered, we're asking for a render_interval in > UpdateCadenceEstimate :) Removed is generic and does not necessarily indicate > dropped. I've removed the example though since it does explicitly mention > dropped and doesn't really add much here. Acknowledged.
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1125893002/110001
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b3a547e78d4d07186d114128934a2931e55880b5 Cr-Commit-Position: refs/heads/master@{#329813} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
