Closed Bug 1332619 Opened 3 years ago Closed 3 years ago

VP8TrackEncoder shortens the encoded track's duration when skipping frames

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

Details

Attachments

(4 files)

When we skip a frame, we add the skipped frame's duration to the last one that was encoded.

Problem: Encoded frames are in microseconds and raw frames are in the TrackRate from the MediaStreamGraph, which is significantly lower than microseconds.

This seems to stem from the initial version of VP8TrackEncoder, http://searchfox.org/mozilla-central/rev/8179f228c64f5448b595122cca834c379f251e93/content/media/encoder/VP8TrackEncoder.cpp#194-204,427-430
Rank: 17
Priority: -- → P1
Comment on attachment 8828816 [details]
Bug 1332619 - Fix missed duration when skipping a frame in VP8TrackEncoder.

https://reviewboard.mozilla.org/r/106114/#review107066
Attachment #8828816 - Flags: review?(rjesup) → review+
Comment on attachment 8828815 [details]
Bug 1332619 - Add gtest for skipped frames.

https://reviewboard.mozilla.org/r/106112/#review107068
Attachment #8828815 - Flags: review?(rjesup) → review+
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/123099d23df5
Add gtest for skipped frames. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea8e4f2edae
Fix missed duration when skipping a frame in VP8TrackEncoder. r=jesup
Seems like TimeStamp on Win8 doesn't have enough resolution to handle 0.1ms frames without rounding errors (some random number of frames got duration of 8 instead of 9). I'm trying with 1ms instead, should hopefully be long enough to still skip frames.
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a406da1d258c
Add gtest for skipped frames. r=jesup
https://hg.mozilla.org/integration/autoland/rev/08d0dccfb2c3
Fix missed duration when skipping a frame in VP8TrackEncoder. r=jesup
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f9573970a445
Backed out 2 changesets by request.
Bad merge that screwed the test on my part. The try was ok, but not the revs in mozreview. New patches coming.
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a1c0fbce74c9
Add gtest for skipped frames. r=jesup
https://hg.mozilla.org/integration/autoland/rev/c0f841d38400
Fix missed duration when skipping a frame in VP8TrackEncoder. r=jesup
https://hg.mozilla.org/mozilla-central/rev/a1c0fbce74c9
https://hg.mozilla.org/mozilla-central/rev/c0f841d38400
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8828816 [details]
Bug 1332619 - Fix missed duration when skipping a frame in VP8TrackEncoder.

Approval Request Comment
[Feature/Bug causing the regression]: This is a longstanding bug, from when VP8TrackEncoder was first introduced.
[User impact if declined]: A recorded video track might be shorter than expected, thus getting out of sync with the audio track, if present.
[Is this code covered by automated tests?]: Yes, a gtest is added.
[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?]: No.
[Why is the change risky/not risky?]: We're just adding a conversion of an existing value.
[String changes made/needed]: None.
Attachment #8828816 - Flags: approval-mozilla-beta?
Attachment #8828816 - Flags: approval-mozilla-aurora?
Comment on attachment 8828816 [details]
Bug 1332619 - Fix missed duration when skipping a frame in VP8TrackEncoder.

fix track duration when skipping vp8 frames, aurora53+, beta52+
Attachment #8828816 - Flags: approval-mozilla-beta?
Attachment #8828816 - Flags: approval-mozilla-beta+
Attachment #8828816 - Flags: approval-mozilla-aurora?
Attachment #8828816 - Flags: approval-mozilla-aurora+
Needs rebasing for Beta uplift.
Flags: needinfo?(pehrson)
Rebased for beta. Carries forward r+.
Flags: needinfo?(pehrson)
Attachment #8830332 - Flags: review+
Rebased for beta. Carries forward r+.
Attachment #8830333 - Flags: review+
Whiteboard: [checkin-needed-beta]
You need to log in before you can comment on or make changes to this bug.