Closed Bug 1330918 Opened 7 years ago Closed 7 years ago

Make MediaRecorder use timestamps for video

Categories

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

Product:
Core
Component:
Audio/Video: Recording
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1330918 - Use timestamps for frames in VideoTrackEncoder.
59 bytes, text/x-review-board-request
bechen
: review+
jesup
: review+
Details
Bug 1330918 - Add VideoTrackEncoder::AppendVideoSegment logs.
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1330918 - Set timestamp in VP8TrackEncoder GTest.
59 bytes, text/x-review-board-request
jesup
: review+
Details 
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.
See Also: → 1330919
Rank: 25
Priority: -- → P2
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 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 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+
Attachment #8828120 - Attachment is obsolete: true
Attachment #8828120 - Flags: review?(rjesup)
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 on attachment 8828121 [details]
Bug 1330918 - Add VideoTrackEncoder::AppendVideoSegment logs.

https://reviewboard.mozilla.org/r/105574/#review106442
Attachment #8828121 - Flags: review?(rjesup) → 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+
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
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)
Ah, the gtest took another path to Init(), so mLastChunk.mDuration was not initialized!
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+
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
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
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1333341
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: