CanvasStream + MediaRecorder does not create variable framerate video

RESOLVED FIXED in Firefox 53

Status

()

Core
Audio/Video: Recording
P2
normal
Rank:
22
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: The 8472, Assigned: pehrsons)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla53
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
I have tried the following

1. create a canvas not attached to the DOM
2. capture canvas stream
3. use setTimeout to draw images onto the canvas at variable intervals

i did that in two variants:

a) canvas.captureStream()
b) canvas.captureStream(0) + triggering stream.requestFrame() after each draw


In both cases the result was a 30fps constant framerate webm instead of the expected VFR output.


MDN docs[1] suggest that not explicitly setting a framerate should only capture frames when something is drawn on the canvas.


reading the spec[2], specifically this part:

> If new content has been drawn to the canvas since it was last painted, and if the frameCaptureRequested internal property of track is set, add a new frame to track containing what was painted to the canvas. 

also suggests that setting 0fps on captureStream() should not add any frames unless explicitly requested.


[1] https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/captureStream 
[2] https://w3c.github.io/mediacapture-fromelement/#methods-1

Updated

3 years ago
Status: UNCONFIRMED → NEW
Rank: 22
Ever confirmed: true
Priority: -- → P2
(Assignee)

Updated

2 years ago
Blocks: 1224079
(Assignee)

Comment 1

2 years ago
I want to fix this to be able to get >30fps videos.
Assignee: nobody → pehrson
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8826163 - Flags: review?(bechen)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8826164 [details]
Bug 1231848 - Encode every frame as it comes in to VP8TrackEncoder.

https://reviewboard.mozilla.org/r/104186/#review104912

This patch does what it says basically, and simplifies the code, but I have one big problem with this patch - it assumes the input durations on the videochunks are correct.  

There is no way to know the correct duration *until* the next frame arrives somewhere.  Since we don't want to delay inserting frames into the graph, we choose the mimimum duration requested by MSG in the pull when we initially insert data.  When we get the next NotifyPull(), if a new frame hasn't arrived, we will extend the duration of the current frame (by calling AppendToTrack with the same image, and the additional duration the graph is running for).  

Note that this is *never* a very accurate representation of the total duration of the frame, which would be based on frame capture time of the *next* frame minus the frame capture time of this frame - and of course that can't be known until the next frame arrives.  A reasonable approximation of this would be final duration of a segment when the next frame gets added....

We do notify consumers of each extension of the frame time, by passing the new Chunk with the same image, and the additional delta duration.  If you set up a 1-frame queue before the encoder, you can approximate the duration by accumulating the durations.  That would add it's own complications (and it's closer to the old code).

Also: the old code implicitly handled this "Notify with same image" behavior by grouping into target-encode-duration chunks (though perhaps not optimally).  With the new code, I'm concerned we might encode the same image more than once, with very short durations.  Perhaps we don't (I didn't dig deep enough to be sure, given the other issues), but it's a question we need to answer (and document in comments) before landing this.
Attachment #8826164 - Flags: review?(rjesup) → review-

Comment 5

2 years ago
mozreview-review
Comment on attachment 8826163 [details]
Bug 1231848 - Do not write framerate metadata to webm container in MediaRecorder.

https://reviewboard.mozilla.org/r/104184/#review104928

r? rillian since he knows more about WebM than I do

Patch seems ok, but I'm unsure as to the impact of ommitting the framerate from the WebM stream on players.
Attachment #8826163 - Flags: review?(rjesup) → review+
Comment on attachment 8826163 [details]
Bug 1231848 - Do not write framerate metadata to webm container in MediaRecorder.

Adding rillian from bugzilla interface
Attachment #8826163 - Flags: review?(giles)
(Assignee)

Comment 7

2 years ago
(In reply to Randell Jesup [:jesup] from comment #4)
> Comment on attachment 8826164 [details]
> Bug 1231848 - Encode every frame as it comes in to VP8TrackEncoder.
> 
> https://reviewboard.mozilla.org/r/104186/#review104912
> 
> This patch does what it says basically, and simplifies the code, but I have
> one big problem with this patch - it assumes the input durations on the
> videochunks are correct.  
> 
> There is no way to know the correct duration *until* the next frame arrives
> somewhere.

You are right, but you are also wrong :-)

VideoTrackEncoder makes sure the frames are unique once they get to VP8TrackEncoder: http://searchfox.org/mozilla-central/source/dom/media/encoder/TrackEncoder.cpp#276

If you want I can add an assert to that effect.


(In reply to Randell Jesup [:jesup] from comment #5)
> Comment on attachment 8826163 [details]
> Bug 1231848 - Do not write framerate metadata to webm container in
> MediaRecorder.
> 
> https://reviewboard.mozilla.org/r/104184/#review104928
> 
> r? rillian since he knows more about WebM than I do
> 
> Patch seems ok, but I'm unsure as to the impact of ommitting the framerate
> from the WebM stream on players.

kinetik assured me in Hawaii that it was ok to omit the framerate metadata if you have a variable framerate.
(Assignee)

Comment 8

2 years ago
Comment on attachment 8826164 [details]
Bug 1231848 - Encode every frame as it comes in to VP8TrackEncoder.

See comment 7.

Actually my biggest concern with this patch is that it sets the maximum key frame interval to 60 frames (1 second at 60fps, which is what we can get when driven by the compositor generally), but we have no idea what the frame rate we'll pass in to the encoder is.
Attachment #8826164 - Flags: review- → review?(rjesup)
(Reporter)

Comment 9

2 years ago
This is probably outside the spec, but would it also be possible to pass the timing information directly for each frame, e.g. if the code calling .requestFrame() knows the difference in time relative to the previous frame, even if processing delay makes it submit it a few milliseconds later than intended?
(Assignee)

Comment 10

2 years ago
(In reply to The 8472 from comment #9)
> This is probably outside the spec, but would it also be possible to pass the
> timing information directly for each frame, e.g. if the code calling
> .requestFrame() knows the difference in time relative to the previous frame,
> even if processing delay makes it submit it a few milliseconds later than
> intended?

requestFrame() is async by design. Frame time depends on first render after drawing something. And if two captureStreams requestFrame() at different times to the same canvas (and get the same frame), what do you do?
(Reporter)

Comment 11

2 years ago
Hrm, I guess it provides too little control for my use-case then (synthesizing VFR video with accurate timing for individual frames).

I'll have to look into some emscripten ports of ffmpeg and mkvmerge.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8826164 [details]
Bug 1231848 - Encode every frame as it comes in to VP8TrackEncoder.

https://reviewboard.mozilla.org/r/104186/#review104960

we can get duplicate frames due to bug 1261007, but only once per second.

One issue is that the AppendVideoSegment code above us does NOT truly generate frame durations, it approximates them by taking time-since-last-frame and adding the current chunk's duration (typically ~10ms due to how frames are inserted into MSG).  For real variable-frame-rate sources, this could be mildly wrong to Very wrong.  (For example, a canvas.captureStream source, with say frame ... 100ms ... frame ... 100ms ... frame ... 900ms ... frame ... 100ms ... frame will yield durations of probably around 10ms (should be 100), 100ms, 100ms (should be 900), 900ms (should be 100) etc

I'll r+, but please file a followup to fix the durations by waiting for the next frame (which may mean delaying muxing too, not sure).
Attachment #8826164 - Flags: review?(rjesup) → review+

Comment 13

2 years ago
mozreview-review
Comment on attachment 8826163 [details]
Bug 1231848 - Do not write framerate metadata to webm container in MediaRecorder.

https://reviewboard.mozilla.org/r/104184/#review104982

::: media/libmkv/WebMElement.c:85
(Diff revision 1)
>      if (pixelHeight != displayHeight) {
>        Ebml_SerializeUnsigned(glob, DisplayHeight, displayHeight);
>      }
> +    if (frameRate > 0.0) {
> -    Ebml_SerializeFloat(glob, FrameRate, frameRate);
> +      Ebml_SerializeFloat(glob, FrameRate, frameRate);
> +    }

`FrameRate` is deprecated, so removing this entirely should be fine. I'd suggest removing the argument from `writeVideoTrack()` entirely.

The only way to declare a fixed frame rate in WebM is to use the `DefaultDuration` element and not override it with conflicting `BlockDuration` elements, so AFACT WebM is inherently variable framerate. Leaving those elements out mean the frame durations are interpreted from the timestamps.

So while I would prefer to produce a fixed framerate stream if one is requested through `HTMLCanvasElement::captureStream()` always returning variable framerate stream approximating the requested framerate complies with the spec.
Attachment #8826163 - Flags: review?(giles) → review+
(Assignee)

Comment 14

2 years ago
mozreview-review-reply
Comment on attachment 8826163 [details]
Bug 1231848 - Do not write framerate metadata to webm container in MediaRecorder.

https://reviewboard.mozilla.org/r/104184/#review104982

> `FrameRate` is deprecated, so removing this entirely should be fine. I'd suggest removing the argument from `writeVideoTrack()` entirely.
> 
> The only way to declare a fixed frame rate in WebM is to use the `DefaultDuration` element and not override it with conflicting `BlockDuration` elements, so AFACT WebM is inherently variable framerate. Leaving those elements out mean the frame durations are interpreted from the timestamps.
> 
> So while I would prefer to produce a fixed framerate stream if one is requested through `HTMLCanvasElement::captureStream()` always returning variable framerate stream approximating the requested framerate complies with the spec.

Ah, even better!

Canvas captureStream runs off the refresh driver (so vsync, mostly), and I think if we got timestamps from the driver they'd be accurate for the most part. Of course if you overshoot *some* of your deadlines things might look odd, but we'd at least match the timestamps that were given to requestAnimationFrame().

Comment 15

2 years ago
mozreview-review
Comment on attachment 8826163 [details]
Bug 1231848 - Do not write framerate metadata to webm container in MediaRecorder.

https://reviewboard.mozilla.org/r/104184/#review105104
Attachment #8826163 - Flags: review?(bechen) → review+

Comment 16

2 years ago
mozreview-review
Comment on attachment 8826164 [details]
Bug 1231848 - Encode every frame as it comes in to VP8TrackEncoder.

https://reviewboard.mozilla.org/r/104186/#review105108

::: dom/media/encoder/VP8TrackEncoder.h:21
(Diff revision 1)
>  typedef struct vpx_image vpx_image_t;
>  
>  /**
>   * VP8TrackEncoder implements VideoTrackEncoder by using libvpx library.
> - * We implement a realtime and fixed FPS encoder. In order to achieve that,
> + * We implement a realtime and variable FPS encoder. In order to achieve that,
>   * there is a pick target frame and drop frame encoding policy implemented in

Please remove the "pick target frame" words since we encode evey frames now.
Attachment #8826164 - Flags: review?(bechen) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

2 years ago
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/200e1510a9c1
Do not write framerate metadata to webm container in MediaRecorder. r=bechen,jesup,rillian
https://hg.mozilla.org/integration/autoland/rev/c7bad8774250
Encode every frame as it comes in to VP8TrackEncoder. r=bechen,jesup
(Assignee)

Updated

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

Updated

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

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/200e1510a9c1
https://hg.mozilla.org/mozilla-central/rev/c7bad8774250
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.