search

web-platform-tests / interop

web-platform-tests Interop project
https://wpt.fyi/interop
278 stars 28 forks source link

`video-encoder-flush.https.any.js` doesn't match the spec #614

Closed padenot closed 5 months ago

padenot commented 8 months ago

Test List

https://github.com/web-platform-tests/wpt/blob/master/webcodecs/video-encoder-flush.https.any.js

Rationale

Rationale + proposed fix (but see below) in this Firefox patch, not yet merged: https://phabricator.services.mozilla.com/D195715#C7171670NL57. A way to reproduce this is to switch the codec used from vp8 to any h264 (I don't think the profile matters) when everything is the default, on (at least) macOS with the VideoToolbox encoder (I think I had it in hardware encoding mode, but it might not matter). But it's not codec specific, it so happens that I was testing various permutation and found this, it could have been triggered with a different vp8 encoder or a different implementation.

With vp8 it's for us 1-in 1-out, so this wouldn't be triggered.

That said, the spec is a bit imprecise, the algorithm is synchronous here: https://w3c.github.io/webcodecs/#output-encodedvideochunks and doesn't check that e.g. the config is valid, but the intro to the method says that it "reset [...] all pending callbacks.".

The change in its current state is closer to what the algorithm says, but we can change the algorithm to check that there's still a valid config each loop iteration, and make it so only the first callback is called.

padenot commented 8 months ago

@Djuffin @youennf fyi

sandersdan commented 8 months ago

I don't expect an encoder to call output() while the JS main thread is running. This follows from the output callbacks being queued tasks ("Running a control message to encode the frame" step 4.5).

padenot commented 8 months ago

They aren't. There is a single queued task, and then a loop inside the task. This for loop can run more than once, executing js at each loop iteration when synchronously calling the callback.

https://w3c.github.io/webcodecs/#output-encodedvideochunks

youennf commented 8 months ago

FWIW, in WebKit, each chunk is outputted in its own event task.

sandersdan commented 8 months ago

This for loop can run more than once, executing js at each loop iteration when synchronously calling the callback.

The linked changes use outputCallbackInBetweenEncode to detect a situation which is impossible per the spec.

It is true that the spec allows multiple outputs to be output together in specific cases (maybe we shouldn't allow that), but they would still need to be the root of the call stack.

foolip commented 5 months ago

Interop 2023 is now closed, but more importantly the test in question wasn't on the official test list: https://wpt.fyi/results/webcodecs?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=interop&q=label%3Ainterop-2023-webcodecs

I'm not sure if it was labeled and then unlabeled, but we've now frozen the 2023 dashboard and this test isn't part of it, so Ill close this issue.