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 Vorbis Read Size to be set by user #36

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

8W9aG
Copy link

@8W9aG 8W9aG commented Nov 21, 2017

  • Hooking up Vorbis read callbacks to an HTTP data
    provider currently yields a lot of overhead due to
    many read requests at 2kB
  • Allowing the user to set the vorbis read size
    lets backend reads become more efficient by
    reading greater chunks at any given time

* Hooking up Vorbis read callbacks to an HTTP data
provider currently yields a lot of overhead due to
many read requests at 2kB
* Allowing the user to set the vorbis read size
lets backend reads become more efficient by
reading greater chunks at any given time
@tdaede
Copy link
Contributor

tdaede commented Nov 22, 2017

Looks okay, but adding API surface will probably need a soname minor bump. Also, it might be wise to sanity check the read size.

@erikd
Copy link
Member

erikd commented Nov 23, 2017

Yeah, really need sanity checking on that. Pretty sure really bad things would happen with negative numbers and very large positive numbers.

@8W9aG
Copy link
Author

8W9aG commented Nov 27, 2017

Thanks for the feedback!

  • Will add some sanity checks that prevent <= 0 and > 64kB
  • Will bump the minor version number (@tdaede do you know how to do this?)
  • Will add a getter

* Does not allow read sizes < 1
* Does not allow read sizes above chunk sizes
@8W9aG
Copy link
Author

8W9aG commented Nov 27, 2017

I think the version bump is stuffing up running the CI tests, can someone advise how to do this properly?

@tdaede
Copy link
Contributor

tdaede commented Dec 5, 2017

I can just pull without the minor commit number for now and do that separately.

@8W9aG
Copy link
Author

8W9aG commented Dec 5, 2017

@tdaede Thanks i'll reset this branch to 3ef0b16

* Does not work on windows so well
@tterribe
Copy link

tterribe commented Jan 8, 2018

Just FYI, but doing a separate HTTP request for every read operation is going to have significant latency and overhead, regardless of the read size. A much better approach is to request the entire resource, and just close the connection if the next read doesn't pick up where the previous one left off. That means you don't need to change the read size Vorbis uses at all. You run the risk of buffering some extra data in the OS's socket buffers, but this is much cheaper than all of the round-trips required to issue a new request in the common case.

You can use a more sophisticated strategy that maintains multiple connections and issues smaller requests when seeking (gradually building back up to larger ones as streaming resumes). This is substantially more complicated to implement, though. See https://git.xiph.org/?p=opusfile.git;a=blob;f=src/http.c;hb=HEAD for an example implementation for libopusfile (using largely the same callback API as libvorbisfile). However, even in this case, the size of each HTTP request is completely independent of the amount you read off the socket in each callback.

@8W9aG
Copy link
Author

8W9aG commented May 21, 2018

@tterribe We can't really request the entire resource at once, it adds massive latency to the start of our program. We also want to explicitly support seeking which means we cannot close the connection if the next read doesn't pick up where the previous one left off. The overhead produced by HTTP when using large payloads for the ogg read size is insignificant, and shrinks as the payload size is increased. Since we are developing a cross platform application that takes advantage of mobile and has features such as OAuth header injection, we cannot easily use the http module provided by vorbis, therefore I am still of the opinion that this is the best option, since it gives the user of the library more control and doesn't force them to write complicated wrappers specifically for dealing with this use case.

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.

4 participants