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 why the writable stream backpressure should be disabled #188

Closed
saschanaz opened this issue Jun 22, 2023 · 3 comments · Fixed by #195
Closed

Clarify why the writable stream backpressure should be disabled #188

saschanaz opened this issue Jun 22, 2023 · 3 comments · Fixed by #195

Comments

@saschanaz
Copy link
Member

https://w3c.github.io/webrtc-encoded-transform/#stream-creation

Set up this.[[writable]] with its writeAlgorithm set to writeEncodedData given this as parameter and its sizeAlgorithm to an algorithm that returns 0.

Chunk size is set to 0 to explictly disable streams backpressure on the write side.

#108 disabled it and added some notes, but the intention is not clear why it should be disabled. The note only says it's disables by letting sizeAlgorithm to return 0 (which I think it should set highWaterMark to Infinity instead).

@youennf
Copy link
Collaborator

youennf commented Jun 23, 2023

I think it should set highWaterMark to Infinity instead

We can do this.

As of why it is disabled, the principle is that the pipeline is a realtime pipeline where buffering should be very limited.
Backpressure should be handled either at input or at output by the UA.
For instance, if the writer is pushing data to the network, packets might be dropped if overshooting network, or the encoder will reduce the size of next frames to limit throughput.
In any case, backpressure should not be used to let the web applications delay its transform processing.

youennf added a commit to youennf/webrtc-insertable-streams that referenced this issue Jul 5, 2023
…hm returning zero.

This makes it clearer backpressure is disabled.
Mention reducing buffering as the reason behind this choice.

Fixes w3c#188.
youennf added a commit to youennf/webrtc-insertable-streams that referenced this issue Jul 5, 2023
…hm returning zero.

This makes it clearer backpressure is disabled.
Mention reducing buffering as the reason behind this choice.

Fixes w3c#188.
youennf added a commit to youennf/webrtc-insertable-streams that referenced this issue Jul 5, 2023
…hm returning zero.

This makes it clearer backpressure is disabled.
Mention reducing buffering as the reason behind this choice.

Fixes w3c#188.
@alvestrand
Copy link
Contributor

alvestrand commented Jul 18, 2023

This is the wrong direction to go in.
Depending on the UA knowing how the input of the transform connects to the output of the transform is the wrong model, and needs to be abandoned; for one thing, it does not permit one-ended use cases.

The right way of approaching this is to make the feedback channel explicit as outlined in https://github.com/alvestrand/hackathon-encoded-media

EDIT: It seems that we have already disabled backpressure in the specified API, so we have no defense against buffer overruns. This seems dangerous, but it is a previously accepted danger.

@dontcallmedom-bot
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants