search

w3c / mediacapture-record

MediaStream Recording
https://w3c.github.io/mediacapture-record/
Other
104 stars 21 forks source link

Defer deciding codec until "onstart" fires #190

Closed henbos closed 4 years ago

henbos commented 4 years ago

Fixes #139.

This allows the User Agent to avoid the cost of re-encoding.

henbos commented 4 years ago

Please take a look @Pehrsons and @jan-ivar

henbos commented 4 years ago

I'm thinking it'd be simpler and clearer to just move the step of Modify |extendedMimeType| by adding media type, subtype and codecs parameter... to when firing "start". I.e., to not even set the mimeType attribute synchronously in start().

The spec change for setting it synchronously in start() was recent enough that no-one is implementing it yet. I have implemented it in Firefox but those patches are still under review.

It would allow you to avoid duplicating language about extending and setting the mime type according to the configuration of the recorder. And most of all it avoids forcing applications to handle two paths (mimetype present vs not present) where one is so rare (mimetype not preset) it's going to be easily missed.

Oh that's great, it's what I would have preferred so that mimeType is always only updated in one place, that's less surprises for developers. I'll update the PR again.

henbos commented 4 years ago

Ok. If we can't solve this I'd rather see us skip the mimetype deferral and only add a note.

Let's not throw everything out over a difference of opinion about whether or not to wait for data to become available, I'm happy to not make that change, as long as we don't force the UA to set the mimeType prior to starting the recording. See PR, I hope this will make both sides happy?

Pehrsons commented 4 years ago

Ok. If we can't solve this I'd rather see us skip the mimetype deferral and only add a note.

Let's not throw everything out over a difference of opinion about whether or not to wait for data to become available, I'm happy to not make that change, as long as we don't force the UA to set the mimeType prior to starting the recording. See PR, I hope this will make both sides happy?

Primarily I want to avoid a situation where we set the mimeType at different times depending on the UA's intentions (sync in start() if transcoding, vs async when start() later fires "start" when avoiding transcoding). Your PR achieves that.

Secondarily I want to avoid shutting the door on something that would allow an application to dynamically handle track set changes (FWIW issue #4 covered this and is one of the most discussed issues on the spec, but was closed without resolution). Your PR does not achieve that. I could accept shutting this door if the WG discusses this and finds consensus for shutting it.

I see two ways out that avoid both situations above:

I'll review the details of the PR if we can move forward with one of the three options mentioned above.

henbos commented 4 years ago

Secondarily I want to avoid shutting the door on something that would allow an application to dynamically handle track set changes (FWIW issue #4 covered this and is one of the most discussed issues on the spec, but was closed without resolution). Your PR does not achieve that. I could accept shutting this door if the WG discusses this and finds consensus for shutting it.

My PR seems largely irrelevant from that issue. Prior to my PR it said "Decide mimeType synchronously, then in parallel start encoding." After my PR it says "In parallel, decide mimeType and start encoding."

I have not changed when encoding starts. The only change to be spec-compliant before vs now is to not surface the mimeType synchronously. If anything this adds flexibility, it doesn't remove it. If you think this is a problem, please explain what you mean.

I see two ways out that avoid both situations above:

  • Letting the UA start encoding data synchronously in start() although it won't fire "start" (or reveal its mimeType) yet.

If you want to change when encoding starts I think that should be discussed in a separate issue and PR. This is already something that happens in parallel. And considering encoding is something that happens in parallel, I don't think it makes sense to say to "start encoding synchronously" unless you intend to move encoding to the main thread. The best you can get is "in parallel, start encoding as soon as possible". If you want to keep track of order of starting and stopping and being able to control things synchronously for the purpose of modifying track sets I think you can do that with or without this PR. Again, that seems like a separate issue?

  • Letting the UA guess (I imagine there's a bunch of heuristics the UA could use to guess right) mimeType synchronously in start() and avoiding transcoding iff the guessed codec and the actual codec match.

I'll review the details of the PR if we can move forward with one of the three options mentioned above.

Why does the application - or implementation for that matter - need to know mimeType synchronously at start() when encoding, and all events related to encoding (onstart, onstop, ondataavailable, onpause, onresume, onerror) happen later?

henbos commented 4 years ago

@jan-ivar Do you have input?

Pehrsons commented 4 years ago

@henbos I must have misread your previous comment. Apologies.

This approach looks fine. I'll see if I have any detailed comments to put in a review.

Pehrsons commented 4 years ago

I'm updating WPT in https://bugzilla.mozilla.org/show_bug.cgi?id=1590997. See https://github.com/web-platform-tests/wpt/pull/20171.