Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify reset() behaviour when multiple things are being output #755

Open
padenot opened this issue Dec 13, 2023 · 3 comments
Open

Clarify reset() behaviour when multiple things are being output #755

padenot opened this issue Dec 13, 2023 · 3 comments
Labels
CR Blocking Needs to resolved for Candidate Recommendation

Comments

@padenot
Copy link
Collaborator

padenot commented Dec 13, 2023

See web-platform-tests/interop#614

Let's say I have an encoder that has quite a long latency, e.g. 16 frames.

If we enqueue 15 frames for encoding, call flush() and in output callback, call reset() on the encoder after e.g. the 8th callback, what should happen?

Firefox outputs the 15 frames, the flush() succeeds, and then the encoder is reset, because there's no prose to check that the encoder has been reset in the algorithm that calls the callback. But the description of the reset() method says that callbacks are not to continue (paraphrasing).

The intent might have been to not call the callbacks after a reset, and we should make it explicit if that's the case.

@youennf
Copy link
Contributor

youennf commented Dec 13, 2023

Spec is indeed not very precise. It makes sense to me for reset() to cancel all previous tasks.
According https://wpt.fyi/results/webcodecs/video-encoder-flush.https.any.html?label=master&label=experimental&aligned, it seems Chrome and Safari agree here.

@Djuffin
Copy link
Contributor

Djuffin commented Dec 13, 2023

To me it looks like the spec does say things about callbacks and promises, but we can probably make it more explicit in the callback algo.


reset()

Immediately resets all state including configuration, control messages in the control message queue, and all pending callbacks.
When invoked, run the Reset VideoEncoder algorithm with an AbortError DOMException.

Reset VideoEncoder (with exception)

Run these steps:

  1. If [[state]] is "closed", throw an InvalidStateError.

  2. Set [[state]] to "unconfigured".

  3. Set [[active encoder config]] to null.

  4. Set [[active output config]] to null.

  5. Signal [[codec implementation]] to cease producing output for the previous configuration.

  6. Remove all control messages from the [[control message queue]].

  7. If [[encodeQueueSize]] is greater than zero:

    1. Set [[encodeQueueSize]] to zero.

    2. Run the Schedule Dequeue Event algorithm.

  8. For each promise in [[pending flush promises]]:

    1. Reject promise with exception.

    2. Remove promise from [[pending flush promises]].

@padenot
Copy link
Collaborator Author

padenot commented Dec 14, 2023

Yes but the point is that depending on the encoder being used, reset() hasn't been called when flush() is called, details are in the proposed change, there are comments.

@aboba aboba added the CR Blocking Needs to resolved for Candidate Recommendation label Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR Blocking Needs to resolved for Candidate Recommendation
Projects
None yet
Development

No branches or pull requests

4 participants