1231848 - CanvasStream + MediaRecorder does not create variable framerate video

Status: RESOLVED FIXED

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

Description

[The 8472]

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

Comment 1

[Andreas Pehrson [:pehrsons]]

I want to fix this to be able to get >30fps videos.

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1231848 - Do not write framerate metadata to webm container in MediaRecorder.

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

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1231848 - Encode every frame as it comes in to VP8TrackEncoder.

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

Comment 4

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

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.

Comment 5

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

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.

Comment 6

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

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

Adding rillian from bugzilla interface

Comment 7

[Andreas Pehrson [:pehrsons]]

(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.

Comment 8

[Andreas Pehrson [:pehrsons]]

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.

Comment 9

[The 8472]

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?

Comment 10

[Andreas Pehrson [:pehrsons]]

(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?

Comment 11

[The 8472]

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

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

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).

Comment 13

[Ralph Giles (:rillian)]

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.

Comment 14

[Andreas Pehrson [:pehrsons]]

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

[Benjamin Chen [:bechen]]

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

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

Comment 16

[Benjamin Chen [:bechen]]

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.

Comment 17

[Andreas Pehrson [:pehrsons]]

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

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

Comment 18

[Andreas Pehrson [:pehrsons]]

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

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/200e1510a9c1
https://hg.mozilla.org/mozilla-central/rev/c7bad8774250