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

VideoEncoder configure does not return a promise #242

Closed
youennf opened this issue May 11, 2021 · 10 comments
Closed

VideoEncoder configure does not return a promise #242

youennf opened this issue May 11, 2021 · 10 comments
Labels
extension Interface changes that extend without breaking.

Comments

@youennf
Copy link
Contributor

youennf commented May 11, 2021

VideoEncoder configure can succeed or fail but it is unclear when the web page will know for sure that it succeeded or failed.
It seems that returning a Promise would help.
One example where it could potentially fail is if H264 is used with a low level but a very high image resolution, or hardware requirements.

Another small issue here is that configure can be called and frames can be enqueued as soon as configure is called, even though in practice, configure might fail. If configure actually fails, the web application would need to either store the frames, which is bad, or agree to forget about the frames.

It also seems that, if an encoder is successfully configured, configure can be called again. If so, it could stick with the old configuration if the second configure fails instead of going to an error state.
It also seems that, if the encoder is in an error state, it cannot recover even with configure. Is there a reason for that design?

This might also apply to decoders or audio encoder.

@sandersdan
Copy link
Contributor

sandersdan commented May 11, 2021

There are a lot of competing goals in the design. One of the primary goals was to allow implementations to defer applying configurations until they also have a frame, either to conserve resources that may never be used, or to resolve ambiguities that a configuration alone doesn't answer. In this case a Promise may force eager resource allocation since apps are likely to be waiting for promise resolution.

There are platforms where it is difficult to determine whether a configuration is acceptable without also decoding a frame, and in this case the only reasonable feedback mechanism is the error() callback. Although this shape isn't the simplest possible, it is simpler than requiring apps to support multiple possible feedback mechanisms. I don't know if similar issues apply to encode, but I also don't see a strong reason to break symmetry in this regard.

In general we have also strived to remove Promises from the API because they have led to ordering ambiguity and confusing state changes. Pure callbacks operate synchronously with the state of the encoder/decoder and we can make straightforward guarantees without as much risk of triggering edge-case behavior in apps. (Eg. an apparently successful configure arriving to JS after an error might trigger an app to fetch and queue data even though the state is now closed.)

flush() survives out of necessity. It does present some edge-case behavior during errors and resets, but it seemed excessive in this case to add a synchronous callback just for flush done, and there does seem to be less risk of apps behaving strangely in these cases.

The second prototype in Chrome was Promise-based (first was Streams-based), and anecdotally removing Promises has made it easier to write demo-level JS code, and state handling in the implementation was substantially simplified.

@chcunningham
Copy link
Collaborator

chcunningham commented May 11, 2021

Here is some documentation of the design discussions that took place around this in October of last year
#98 (comment)

In reply to these points:

One example where it could potentially fail is if H264 is used with a low level but a very high image resolution, or hardware requirements.

Another small issue here is that configure can be called and frames can be enqueued as soon as configure is called, even though in practice, configure might fail. If configure actually fails, the web application would need to either store the frames, which is bad, or agree to forget about the frames.

Apps should detect this by first querying IsConfigSupported(). The spec documents that best practice in every configure(). In this way, apps can know whats supported even before chunks arrive. Its smoother than holding up the pipeline to see how your configure() promise resolved/rejected.

There will be very very narrow cases where configure() may still fail (say your stream has some quirk that your hardware doesn't like), and those will trigger the error callback as Dan points out. Using a promise doesn't make those (rare) edges any softer.

It also seems that, if an encoder is successfully configured, configure can be called again. If so, it could stick with the old configuration if the second configure fails instead of going to an error state.
It also seems that, if the encoder is in an error state, it cannot recover even with configure. Is there a reason for that design?

This is another topic discussed at length last year. We arrived at the current design for similar reasons of massively simplifying the usage pattern. Making errors fatal removes any ambiguity about what state the control message queue is in, which decodes were accepted vs aborted, which configure() was most recently applied, etc., etc... The answer is instead very simple: everything is dropped, please make a new codec.

Also, I want to add emphasis to Dan's comment:

In general we have also strived to remove Promises from the API because they have led to ordering ambiguity and confusing state changes.

removing Promises has made it easier to write demo-level JS code, and state handling in the implementation was substantially simplified.

+1000. Removing promises from these APIs was a hugely simplifying for apps.

@chcunningham chcunningham added the extension Interface changes that extend without breaking. label May 12, 2021
@chcunningham
Copy link
Collaborator

Triage notes: marking 'extension' as the request is to change the return type from one that is currently undefined (void).

Note: My stance on actually taking up this extension (opposed) is given in the prior comment.

@Djuffin
Copy link
Contributor

Djuffin commented May 25, 2021

If need be API users can call flush() after configure() to achieve a similar result.

@youennf
Copy link
Contributor Author

youennf commented Jun 3, 2021

Making errors fatal removes any ambiguity about what state the control message queue is in, which decodes were accepted vs aborted, which configure() was most recently applied, etc., etc... The answer is instead very simple: everything is dropped, please make a new codec.

I understand this is a simplification from an implementation point of view, but it seems that this is at odds with the desire to build a generic purpose encoder/decoder API.
FWIW, it is difficult to validate the design of the API given there is no document describing use cases/requirements and underneath assumptions.

@chcunningham
Copy link
Collaborator

I understand this is a simplification from an implementation point of view, but it seems that this is at odds with the desire to build a generic purpose encoder/decoder API.

Sorry, I'm not able to connect the dots. Why is this at odds?

FWIW, it is difficult to validate the design of the API given there is no document describing use cases/requirements and underneath assumptions.

I think the explainer covers this pretty well. I'm not sure what assumptions you mean.

@youennf
Copy link
Contributor Author

youennf commented Jun 4, 2021

Sorry, I'm not able to connect the dots. Why is this at odds?

Codecs may have non fatal errors. Dropping a frame is for instance a non fatal error. Ditto for trying to decode bad data, or decode data that is missing reference frames. By making them fatal errors, the web application can no longer have specific error handling for those cases.

I think the explainer covers this pretty well. I'm not sure what assumptions you mean.

I was confused by the fact that supporting realtime mode (with the ability for encoders to drop frames) was not totally part of the API design, so I was assuming not part of the requirements. According the explainer and #240, it seems there is rough consensus to have that support in V1.

@Djuffin
Copy link
Contributor

Djuffin commented Jun 4, 2021

Codecs may have non fatal errors

For encoders, it is hard to think of a non fatal error, except for the most trivial ones, like a frame format is not supported by the encoder's. Such trivial errors are easy to avoid for API users, they usually indicate bugs in the code.

Dropping a frame is for instance a non fatal error.

If an encoder drops a frame because it can't meet bitrate constraints, it is not an error. It is a completely normal situation.

@aboba
Copy link
Collaborator

aboba commented Jun 11, 2021

@Djuffin said:

"If an encoder drops a frame because it can't meet bitrate constraints, it is not an error. It is a completely normal situation."

[BA] That makes sense to me, but in Issue 240, it is noted that dropping if disallowed (even with a bitrate constraint??) would be considered an implementation bug. Seems to me like it shouldn't be permitted to simultaneously disallow frame drops and set a bitrate constraint.

@chcunningham
Copy link
Collaborator

Ditto for trying to decode bad data, or decode data that is missing reference frames.

Bad data could be a fatal error for some impls.

By making them fatal errors, the web application can no longer have specific error handling for those cases.

The error callback can provide specific errors. For instance, bad data would be EncodingError.

[BA] That makes sense to me, but in Issue 240, it is noted that dropping if disallowed (even with a bitrate constraint??) would be considered an implementation bug. Seems to me like it shouldn't be permitted to simultaneously disallow frame drops and set a bitrate constraint.

Lets move this discussion to #269. We should consider these points there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension Interface changes that extend without breaking.
Projects
None yet
Development

No branches or pull requests

5 participants