1333341 - VideoTrackEncoder is subject to an accumulating rounding error

Status: RESOLVED FIXED

(Core :: Audio/Video: Recording, defect, P1)

Description

[Andreas Pehrson [:pehrsons]]

Bug 1330918 made VideoTrackEncoder base the encoded frames on the incoming frames' timestamps rather than their duration in track rate units.

This was done by converting timestamp diffs to duration in track rate units, alas introducing a rounding error. These were then passed to existing code for encoding - which would base the start of frame N on the duration of frame N-1, alas the rounding error is accumulating.

I have a gtest to test this, and a fix in the works.

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1333341 - Add gtest for per-frame rounding error.

Review commit: https://reviewboard.mozilla.org/r/106812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106812/

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1333341 - Fix accumulating rounding error in VideoTrackEncoder.

Review commit: https://reviewboard.mozilla.org/r/106814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106814/

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1333341 - Let VideoUtils functions that return CheckedInt take a CheckedInt.

To simplify when the callsite chains several CheckedInt operations.

Review commit: https://reviewboard.mozilla.org/r/106816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106816/

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1333341 - Fix accumulating rounding error in VP8TrackEncoder.

Review commit: https://reviewboard.mozilla.org/r/106818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106818/

Comment 5

[Andreas Pehrson [:pehrsons]]

Hmm, I did remove all reviewers before publishing, but apparently that didn't work. For now I just want to run this on try.

Comment 6

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8829822 [details]
Bug 1333341 - Fix accumulating rounding error in VideoTrackEncoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106814/diff/1-2/

Comment 7

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8829824 [details]
Bug 1333341 - Fix accumulating rounding error in VP8TrackEncoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106818/diff/1-2/

Comment 8

[Andreas Pehrson [:pehrsons]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51ab1aeb2f17abdf12772666a35aa12466d35c1c

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8829822 [details]
Bug 1333341 - Fix accumulating rounding error in VideoTrackEncoder.

https://reviewboard.mozilla.org/r/106814/#review107868

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8829824 [details]
Bug 1333341 - Fix accumulating rounding error in VP8TrackEncoder.

https://reviewboard.mozilla.org/r/106818/#review107872

::: dom/media/encoder/VP8TrackEncoder.cpp:573
(Diff revision 2)
> -      GetEncodedPartitions(aData);
> +      rv = GetEncodedPartitions(aData);
> +      NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

still no fan of NS_ENSURE_SUCCESS()... hidden returns are not good.  But not a big deal given other uses.  If this is the first use here, maybe avoid it.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8829821 [details]
Bug 1333341 - Add gtest for per-frame rounding error.

https://reviewboard.mozilla.org/r/106812/#review107874

Comment 12

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8829824 [details]
Bug 1333341 - Fix accumulating rounding error in VP8TrackEncoder.

https://reviewboard.mozilla.org/r/106818/#review107872

> still no fan of NS_ENSURE_SUCCESS()... hidden returns are not good.  But not a big deal given other uses.  If this is the first use here, maybe avoid it.

Second use :/
First one is original.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33824207f7a3
Add gtest for per-frame rounding error. r=jesup
https://hg.mozilla.org/integration/autoland/rev/25a00c4aa2a6
Fix accumulating rounding error in VideoTrackEncoder. r=jesup
https://hg.mozilla.org/integration/autoland/rev/28765570b293
Fix accumulating rounding error in VP8TrackEncoder. r=jesup

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/33824207f7a3
https://hg.mozilla.org/mozilla-central/rev/25a00c4aa2a6
https://hg.mozilla.org/mozilla-central/rev/28765570b293

Comment 15

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8829822 [details]
Bug 1333341 - Fix accumulating rounding error in VideoTrackEncoder.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1330918
[User impact if declined]: A recorded video track might be too short, getting out of sync with the audio track, if available.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Changes are not trivial, but test coverage is good.
[String changes made/needed]: None

This uplift request applies to all patches on this bug.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8829822 [details]
Bug 1333341 - Fix accumulating rounding error in VideoTrackEncoder.

Regression with 53, let's uplift this for aurora

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c142accc4070
https://hg.mozilla.org/releases/mozilla-aurora/rev/74c77cc41d32
https://hg.mozilla.org/releases/mozilla-aurora/rev/b55972079c35