Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1459923003:
Fix Bug: Video with Variable Frame Rate plays at incorrect speed. (Closed)
|
Created:
5 years, 1 month ago by qiangchen Modified:
5 years ago Reviewers:
DaleCurtis 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. |
DescriptionFix Bug: Video with Variable Frame Rate plays at incorrect speed.
The root cause is that Cadence method essentially takes
local average of frame durations, and then in Variable
Frame Rate case, long frame would be renderered shorter
than it should be, and short frame would be renderered
longer than it should be.
In this CL, we add a variable frame rate detection
mechanism, and disable Cadence if variable frame rate gets
detected.
BUG=555917
Committed: https://crrev.com/501ce040bbd2a213d62b41750dee0749c1f1149c
Cr-Commit-Position: refs/heads/master@{#362737}
Patch Set 1 : #
Total comments: 16
Patch Set 2 : Empty Cadence Stylex #Patch Set 3 : Estimator Test #
Total comments: 2
Patch Set 4 : VideoRendererAlgorithm Test Case #
Total comments: 2
Patch Set 5 : MovingAverage Test Case #Patch Set 6 : Rebase #
Messages
Total messages: 31 (11 generated)
Description was changed from ========== Fix Bug: Video with Variable Frame Rate plays at incorrect speed. The root cause is that Cadence method essentially takes local average of frame durations, and then in Variable Frame Rate case, long frame would be renderered shorter than it should be, and short frame would be renderered longer than it should be. In this CL, we add a variable frame rate detection mechanism, and disable Cadence if variable frame rate gets detected. BUG=555917 ========== to ========== Fix Bug: Video with Variable Frame Rate plays at incorrect speed. The root cause is that Cadence method essentially takes local average of frame durations, and then in Variable Frame Rate case, long frame would be renderered shorter than it should be, and short frame would be renderered longer than it should be. In this CL, we add a variable frame rate detection mechanism, and disable Cadence if variable frame rate gets detected. BUG=555917 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
qiangchen@chromium.org changed reviewers: + dalecurtis@chromium.org
My suggested solution to variable FPS issue. I've tested the youtube, the constant FPS video has deviation-frame-duration ratio as 1%, apprtc has deviation ratio as 5%. So I set ratio<7% as criterion to determine a video is const FPS, ratio>10% as criterion to determine a video is variable FPS. To tune the criterion constants, I think it is better to have more test examples, both const FPS and variable FPS. Any idea on this?
Awesome! What was the deviation for the video in the bug? How does deviation vary as you scroll videos on and off the page / put them in background tabs? Does it recover quickly?
On 2015/11/20 19:08:42, DaleCurtis wrote: > Awesome! What was the deviation for the video in the bug? How does deviation > vary as you scroll videos on and off the page / put them in background tabs? > Does it recover quickly? On the order of 70%. So that is unquestionably a variable FPS video.
On 2015/11/20 19:12:28, qiangchenC wrote: > On 2015/11/20 19:08:42, DaleCurtis wrote: > > Awesome! What was the deviation for the video in the bug? How does deviation > > vary as you scroll videos on and off the page / put them in background tabs? > > Does it recover quickly? > > On the order of 70%. So that is unquestionably a variable FPS video. For that video sample, the deviation changes little, because our moving window can contain 32 frames, but the video itself has only 18 frames.
On 2015/11/20 19:14:02, qiangchenC wrote: > On 2015/11/20 19:12:28, qiangchenC wrote: > > On 2015/11/20 19:08:42, DaleCurtis wrote: > > > Awesome! What was the deviation for the video in the bug? How does deviation > > > vary as you scroll videos on and off the page / put them in background tabs? > > > > Does it recover quickly? > > > > On the order of 70%. So that is unquestionably a variable FPS video. > > For that video sample, the deviation changes little, because our moving window > can contain 32 frames, but the video itself has only 18 frames. That's why I said we needed more realistic samples for tuning.
Sorry, I wasn't clear, I was asking what the deviation looked like on YouTube videos in the background/offscreen.
On 2015/11/20 19:19:45, DaleCurtis wrote: > Sorry, I wasn't clear, I was asking what the deviation looked like on YouTube > videos in the background/offscreen. Ah, I tested background rendering, changing playing point, the ratio order does not change. For background rendering, though render() call frequency is lower, we calculate the target render instants as usual, so the statistics should not be affected. For playing point change, or playing rate change, we reset the MovingAverage, so we never mixed the pre-operation data with post-operation data resulting in weird statistics.
Can you add a test for this behavior? https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... File media/base/moving_average.cc (right): https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... media/base/moving_average.cc:22: square_sum_ms_ += sample.InMillisecondsF() * sample.InMillisecondsF() - Why work in float here? Keep these as timedelta*timedelta - timedelta*timedelta https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... media/base/moving_average.cc:41: double sqr_deviation_ms = square_sum_ms_ / size - average_ms * average_ms; cast size to double and keep the rest as TimeDelta and you won't lose any precision here by making everything TimeDelta. https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... File media/base/moving_average.h (right): https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... media/base/moving_average.h:46: double square_sum_ms_; Store as base::TimeDelta https://codereview.chromium.org/1459923003/diff/60001/media/filters/video_cad... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1459923003/diff/60001/media/filters/video_cad... media/filters/video_cadence_estimator.cc:27: const double kVariableFPSFactor = 0.1; Your variable frame rate clip was 0.7? If so it seems like this should be around that value as well. https://codereview.chromium.org/1459923003/diff/60001/media/filters/video_cad... media/filters/video_cadence_estimator.cc:112: if (cadence_ != Cadence()) { if !cadence_.empty() cadence_.clear()
Fixed one minor issue. Replied about the square unit issue. I'm still trying to figure out a way to write a test. I'm trying to figure out a theoretical way to determine the threshold. https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... File media/base/moving_average.cc (right): https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... media/base/moving_average.cc:22: square_sum_ms_ += sample.InMillisecondsF() * sample.InMillisecondsF() - On 2015/11/20 23:50:46, DaleCurtis wrote: > Why work in float here? Keep these as timedelta*timedelta - timedelta*timedelta I do not think there is a multiplication defined in that way. And it is not natural to define one, as the result's unit is time square, not time. https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... media/base/moving_average.cc:41: double sqr_deviation_ms = square_sum_ms_ / size - average_ms * average_ms; On 2015/11/20 23:50:46, DaleCurtis wrote: > cast size to double and keep the rest as TimeDelta and you won't lose any > precision here by making everything TimeDelta. Perhaps, we can use int64 to store the square_sum_us, and then no precision lost at all. https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... File media/base/moving_average.h (right): https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... media/base/moving_average.h:46: double square_sum_ms_; On 2015/11/20 23:50:46, DaleCurtis wrote: > Store as base::TimeDelta Probably may not be a good idea. As the unit is essentially second^2, not second. And from programming perspective, there isn't multiplication and square root defined for base::TimeDelta, and thus, even if we store the data in base::TimeDelta, we will have to take out the raw value for computation. https://codereview.chromium.org/1459923003/diff/60001/media/filters/video_cad... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1459923003/diff/60001/media/filters/video_cad... media/filters/video_cadence_estimator.cc:27: const double kVariableFPSFactor = 0.1; On 2015/11/20 23:50:46, DaleCurtis wrote: > Your variable frame rate clip was 0.7? If so it seems like this should be around > that value as well. Simply excluding that extreme case may not be optimal. Then one can make another variable FPS video with variation a little below the threshold and then the issue still exists obviously. I'll try to use a more theoretical way to determine the threshold. That is near the threshold, Cadence method would result in almost same render length error with that of drift method. https://codereview.chromium.org/1459923003/diff/60001/media/filters/video_cad... media/filters/video_cadence_estimator.cc:112: if (cadence_ != Cadence()) { On 2015/11/20 23:50:46, DaleCurtis wrote: > if !cadence_.empty() cadence_.clear() Done.
https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... File media/base/moving_average.cc (right): https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... media/base/moving_average.cc:22: square_sum_ms_ += sample.InMillisecondsF() * sample.InMillisecondsF() - On 2015/11/25 18:06:33, qiangchenC wrote: > On 2015/11/20 23:50:46, DaleCurtis wrote: > > Why work in float here? Keep these as timedelta*timedelta - > timedelta*timedelta > > I do not think there is a multiplication defined in that way. > > And it is not natural to define one, as the result's unit is time square, not > time. They are defined, which is why I suggested it :) It'll include some checked numeric code to prevent overflows as well. If you don't want to store as TimeDelta storing as int64_t is fine with me. I.e., square_sum_us_ = (sample*sample - oldest*oldest).InMicroseconds() https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... media/base/moving_average.cc:41: double sqr_deviation_ms = square_sum_ms_ / size - average_ms * average_ms; On 2015/11/25 18:06:33, qiangchenC wrote: > On 2015/11/20 23:50:46, DaleCurtis wrote: > > cast size to double and keep the rest as TimeDelta and you won't lose any > > precision here by making everything TimeDelta. > > Perhaps, we can use int64 to store the square_sum_us, and then no precision lost > at all. sgtm https://codereview.chromium.org/1459923003/diff/60001/media/filters/video_cad... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1459923003/diff/60001/media/filters/video_cad... media/filters/video_cadence_estimator.cc:27: const double kVariableFPSFactor = 0.1; On 2015/11/25 18:06:33, qiangchenC wrote: > On 2015/11/20 23:50:46, DaleCurtis wrote: > > Your variable frame rate clip was 0.7? If so it seems like this should be > around > > that value as well. > > Simply excluding that extreme case may not be optimal. Then one can make another > variable FPS video with variation a little below the threshold and then the > issue still exists obviously. > > I'll try to use a more theoretical way to determine the threshold. > > That is near the threshold, Cadence method would result in almost same render > length error with that of drift method. Sure, but I think we want to err on the side of caution here, lets just exclude things that are *absolutely* variable frame rate material until we have more data points.
Patchset #3 (id:100001) has been deleted
Write a Estimator Unittest. Still working on how to write a VideoRendererAlgorithm Unittest. https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... File media/base/moving_average.cc (right): https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... media/base/moving_average.cc:22: square_sum_ms_ += sample.InMillisecondsF() * sample.InMillisecondsF() - On 2015/11/25 21:29:24, DaleCurtis wrote: > On 2015/11/25 18:06:33, qiangchenC wrote: > > On 2015/11/20 23:50:46, DaleCurtis wrote: > > > Why work in float here? Keep these as timedelta*timedelta - > > timedelta*timedelta > > > > I do not think there is a multiplication defined in that way. > > > > And it is not natural to define one, as the result's unit is time square, not > > time. > > They are defined, which is why I suggested it :) It'll include some checked > numeric code to prevent overflows as well. If you don't want to store as > TimeDelta storing as int64_t is fine with me. I.e., square_sum_us_ = > (sample*sample - oldest*oldest).InMicroseconds() There is multiplication defined between numeric (int, float) and TimeDelta, there is not a multiplication defined between two TimeDeltas. https://codereview.chromium.org/1459923003/diff/60001/media/base/moving_avera... media/base/moving_average.cc:41: double sqr_deviation_ms = square_sum_ms_ / size - average_ms * average_ms; On 2015/11/25 21:29:23, DaleCurtis wrote: > On 2015/11/25 18:06:33, qiangchenC wrote: > > On 2015/11/20 23:50:46, DaleCurtis wrote: > > > cast size to double and keep the rest as TimeDelta and you won't lose any > > > precision here by making everything TimeDelta. > > > > Perhaps, we can use int64 to store the square_sum_us, and then no precision > lost > > at all. > > sgtm Done. https://codereview.chromium.org/1459923003/diff/60001/media/filters/video_cad... File media/filters/video_cadence_estimator.cc (right): https://codereview.chromium.org/1459923003/diff/60001/media/filters/video_cad... media/filters/video_cadence_estimator.cc:27: const double kVariableFPSFactor = 0.1; On 2015/11/25 21:29:24, DaleCurtis wrote: > On 2015/11/25 18:06:33, qiangchenC wrote: > > On 2015/11/20 23:50:46, DaleCurtis wrote: > > > Your variable frame rate clip was 0.7? If so it seems like this should be > > around > > > that value as well. > > > > Simply excluding that extreme case may not be optimal. Then one can make > another > > variable FPS video with variation a little below the threshold and then the > > issue still exists obviously. > > > > I'll try to use a more theoretical way to determine the threshold. > > > > That is near the threshold, Cadence method would result in almost same render > > length error with that of drift method. > > Sure, but I think we want to err on the side of caution here, lets just exclude > things that are *absolutely* variable frame rate material until we have more > data points. Done.
Not sure a VRA test is needed, but can you add a MovingAverage test? lgtm % nit + test. https://codereview.chromium.org/1459923003/diff/120001/media/base/moving_aver... File media/base/moving_average.cc (right): https://codereview.chromium.org/1459923003/diff/120001/media/base/moving_aver... media/base/moving_average.cc:42: square_sum_us_ / size - average_us * average_us; Cast upper or lower to double and store as double? The multiplies won't lose precision unless there's an overflow, but the divide + sqrt might lose something meaningful.
Patchset #4 (id:120002) has been deleted
Added VRA test case. https://codereview.chromium.org/1459923003/diff/120001/media/base/moving_aver... File media/base/moving_average.cc (right): https://codereview.chromium.org/1459923003/diff/120001/media/base/moving_aver... media/base/moving_average.cc:42: square_sum_us_ / size - average_us * average_us; On 2015/12/01 20:11:28, DaleCurtis wrote: > Cast upper or lower to double and store as double? The multiplies won't lose > precision unless there's an overflow, but the divide + sqrt might lose something > meaningful. Done. Not sure are you fine with implicit cast. Anyway, in float computation, No DCHECK_GE can be applied, because theoretically average square sum is larger than or equal to square average. In float computation it is possible to have a value like -1e-14, in the case that all frames have exactly same length.
https://codereview.chromium.org/1459923003/diff/150001/media/base/moving_aver... File media/base/moving_average.cc (right): https://codereview.chromium.org/1459923003/diff/150001/media/base/moving_aver... media/base/moving_average.cc:42: (square_sum_us_ - total_us / size * total_us) / size; I think your old code w/ size as a double is better, pedantically this uses two divides instead of a single one.
https://codereview.chromium.org/1459923003/diff/150001/media/base/moving_aver... File media/base/moving_average.cc (right): https://codereview.chromium.org/1459923003/diff/150001/media/base/moving_aver... media/base/moving_average.cc:42: (square_sum_us_ - total_us / size * total_us) / size; On 2015/12/01 22:42:19, DaleCurtis wrote: > I think your old code w/ size as a double is better, pedantically this uses two > divides instead of a single one. Done.
lgtm
The CQ bit was checked by qiangchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1459923003/#ps190001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1459923003/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1459923003/190001
Message was sent while issue was closed.
Description was changed from ========== Fix Bug: Video with Variable Frame Rate plays at incorrect speed. The root cause is that Cadence method essentially takes local average of frame durations, and then in Variable Frame Rate case, long frame would be renderered shorter than it should be, and short frame would be renderered longer than it should be. In this CL, we add a variable frame rate detection mechanism, and disable Cadence if variable frame rate gets detected. BUG=555917 ========== to ========== Fix Bug: Video with Variable Frame Rate plays at incorrect speed. The root cause is that Cadence method essentially takes local average of frame durations, and then in Variable Frame Rate case, long frame would be renderered shorter than it should be, and short frame would be renderered longer than it should be. In this CL, we add a variable frame rate detection mechanism, and disable Cadence if variable frame rate gets detected. BUG=555917 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== Fix Bug: Video with Variable Frame Rate plays at incorrect speed. The root cause is that Cadence method essentially takes local average of frame durations, and then in Variable Frame Rate case, long frame would be renderered shorter than it should be, and short frame would be renderered longer than it should be. In this CL, we add a variable frame rate detection mechanism, and disable Cadence if variable frame rate gets detected. BUG=555917 ========== to ========== Fix Bug: Video with Variable Frame Rate plays at incorrect speed. The root cause is that Cadence method essentially takes local average of frame durations, and then in Variable Frame Rate case, long frame would be renderered shorter than it should be, and short frame would be renderered longer than it should be. In this CL, we add a variable frame rate detection mechanism, and disable Cadence if variable frame rate gets detected. BUG=555917 Committed: https://crrev.com/501ce040bbd2a213d62b41750dee0749c1f1149c Cr-Commit-Position: refs/heads/master@{#362737} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/501ce040bbd2a213d62b41750dee0749c1f1149c Cr-Commit-Position: refs/heads/master@{#362737}
Message was sent while issue was closed.
Thanks Qiang! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
