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

Allow opus read size to be set by user #9

Closed
wants to merge 1 commit into from

Conversation

jastrauckas
Copy link

In the same vein as xiph/vorbis#36

The default 2kB read size incurs a lot of overhead when the read callbacks are making HTTP requests. Does this approach seem sensible?

@tterribe
Copy link

tterribe commented Jan 8, 2018

Please see my comments in xiph/vorbis#36. The same applies here: I don't think this is necessary, and actually using large read sizes incurs a significant latency overhead.

@jastrauckas
Copy link
Author

@tterribe there are reasons we can't or don't want to request the whole resource at once. We could definitely still decouple the vorbis/opus read size from the actual HTTP requests with an extra layer of buffering, but like you said it will be fairly involved to implement and using larger read sizes is working nicely for our use case at the moment. Do you think there's any harm in exposing the read size like this? I realize it's probably not ideal if its only purpose is to support our lack of buffering, but seems like it might be nice for other performance/resource management reasons?

@tterribe
Copy link

tterribe commented Jan 8, 2018

So, referring again to src/http.c, we do no buffering at all. We simply read from the socket at most the number of bytes requested by the current read callback (the OS buffers for us, of course [1]).

Handling multiple simultaneous connections and dynamically sizing your requests is more complicated, but the basic idea of decoupling request size from read size should be straightforward, even if you don't want to request the whole resource. You need to do two things: (1) when you first issue a request, only read the required number of bytes from the request body, and (2) remember where you are in that body, and on the next read, simply read more bytes from the same body if there are still some left and there hasn't been a seek to a new location. You do have to keep track of the requests state between callbacks and clean it up on close, but this shouldn't be too involved.

I think there's harm in exposing the read size only to the extent that someone actually changes it. There's a reason that it's much smaller than OP_CHUNK_SIZE. That reason is latency, as I said above: files take substantially longer to start playing with a larger read size, and seeking becomes much slower, even if read size is decoupled from request size as described above.

Incidentally, what is the reason you can't use the HTTP support built into libopusfile directly?

[1] This is actually a better idea than doing your own buffering: the OS will tune the buffer size based on network conditions, and the feedback produced by dropping packets when that buffer fills up is what keeps the sender from just blasting the whole file at you as fast as your connection can handle (e.g., if you're playing back the file in real time).

@rillian
Copy link
Contributor

rillian commented May 2, 2020

There's been no further discussion. Closing as addressed.

@rillian rillian closed this May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants