Make MediaRecorder use timestamps for video

RESOLVED FIXED in Firefox 53

Status

()

P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pehrsons, Assigned: pehrsons)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
With bug 1231848 we'll be encoding frames as they come in from the MediaStreamGraph. But this is based on chunk durations, which are a multiple of a MediaStreamGraph iteration intervals, typically 10ms.

We have timestamps on video chunks and we should switch to them for better accuracy.
(Assignee)

Updated

2 years ago
See Also: → bug 1330919
(Assignee)

Updated

2 years ago
Rank: 25
Priority: -- → P2
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8826568 [details]
Bug 1330918 - Use timestamps for frames in VideoTrackEncoder.

https://reviewboard.mozilla.org/r/104516/#review105174

::: dom/media/encoder/TrackEncoder.cpp:306
(Diff revision 1)
>  
> -      // Because we may get chunks with a null image (due to input blocking),
> -      // accumulate duration and give it to the next frame that arrives.
> -      // Canonically incorrect - the duration should go to the previous frame
> -      // - but that would require delaying until the next frame arrives.
> -      // Best would be to do like OMXEncoder and pass an effective timestamp
> +    RefPtr<layers::Image> lastImage = mLastChunk.mFrame.GetImage();
> +    mRawSegment.AppendFrame(lastImage.forget(),
> +                            RateConvertTicksRoundUp(
> +                                mTrackRate,
> +                                1000 * 1000 /* microseconds per second */,

PR_US_PER_SEC

::: dom/media/encoder/TrackEncoder.cpp:318
(Diff revision 1)
> +      // If we get a null image as the next frame, we continue sending the last
> +      // one. Since it was already passed, we just update its timestamp.
> +      mLastChunk.mTimeStamp = chunk.mTimeStamp;

IIUC the timestamp should remain the same - a null frame isn't a new frame, it's the lack of a new frame.  The time the frame was sampled doesn't change.  

I would update the timestamp if we resumbit a frame due to the 1 second thing (and if we don't,  we'd get continuous sending of the frame -- which I think is a bug in the patch as it stands)
Attachment #8826568 - Flags: review?(rjesup) → review-
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8826568 [details]
Bug 1330918 - Use timestamps for frames in VideoTrackEncoder.

https://reviewboard.mozilla.org/r/104516/#review105888
Attachment #8826568 - Flags: review?(bechen) → review+

Comment 5

2 years ago
mozreview-review
Comment on attachment 8826568 [details]
Bug 1330918 - Use timestamps for frames in VideoTrackEncoder.

https://reviewboard.mozilla.org/r/104516/#review105972
Attachment #8826568 - Flags: review?(rjesup) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8828120 - Attachment is obsolete: true
Attachment #8828120 - Flags: review?(rjesup)
(Assignee)

Comment 11

2 years ago
Comment on attachment 8826568 [details]
Bug 1330918 - Use timestamps for frames in VideoTrackEncoder.

I had to make some changes to make it pass on try. Please check the diff between revs 2 and 4. Thanks!
Attachment #8826568 - Flags: review+ → review?(rjesup)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8828121 [details]
Bug 1330918 - Add VideoTrackEncoder::AppendVideoSegment logs.

https://reviewboard.mozilla.org/r/105574/#review106442
Attachment #8828121 - Flags: review?(rjesup) → review+

Comment 13

2 years ago
mozreview-review
Comment on attachment 8826568 [details]
Bug 1330918 - Use timestamps for frames in VideoTrackEncoder.

https://reviewboard.mozilla.org/r/104516/#review106446

r+ for updates
Attachment #8826568 - Flags: review?(rjesup) → review+

Comment 14

2 years ago
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b2d8a93a50a8
Use timestamps for frames in VideoTrackEncoder. r=bechen,jesup
https://hg.mozilla.org/integration/autoland/rev/aedd9a68f2c0
Add VideoTrackEncoder::AppendVideoSegment logs. r=jesup
I had to back this out for frequent gtest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=70154610&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/951b80d391e6
Flags: needinfo?(pehrson)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
Comment on attachment 8828275 [details]
Bug 1330918 - Set timestamp in VP8TrackEncoder GTest.

Still some issues with the gtest that I don't understand yet.
Flags: needinfo?(pehrson)
Attachment #8828275 - Flags: review?(rjesup)
(Assignee)

Comment 18

2 years ago
Ah, the gtest took another path to Init(), so mLastChunk.mDuration was not initialized!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8828275 [details]
Bug 1330918 - Set timestamp in VP8TrackEncoder GTest.

https://reviewboard.mozilla.org/r/105748/#review106694
Attachment #8828275 - Flags: review?(rjesup) → review+

Comment 23

2 years ago
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a20498d3c1a2
Use timestamps for frames in VideoTrackEncoder. r=bechen,jesup
https://hg.mozilla.org/integration/autoland/rev/49a053732072
Add VideoTrackEncoder::AppendVideoSegment logs. r=jesup
https://hg.mozilla.org/integration/autoland/rev/a3538d0e7314
Set timestamp in VP8TrackEncoder GTest. r=jesup

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a20498d3c1a2
https://hg.mozilla.org/mozilla-central/rev/49a053732072
https://hg.mozilla.org/mozilla-central/rev/a3538d0e7314
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Updated

2 years ago
Depends on: 1333341
You need to log in before you can comment on or make changes to this bug.