-
Notifications
You must be signed in to change notification settings - Fork 95
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
Ogg metadata (and a bit more) #197
Conversation
In case you wonder, I could have used libogg and done the scanning with it. But that would mean an extra amount of cpu because then all data would now be copied and stored by libogg to be regurgitated as packets that we don't need because we don't decode them here (our scanning must be done in the streamer, not in the decoder). Of course the decoders unwrap ogg containers but they use the highlevel functions of libvorbifile and never handle ogg bitstream directly, so it's not something I could re-use (I do ogg-direct only in my esp32 implementation to avoid the "big artwork" issue where libogg tries to allocate a packet of 1~2MB) In this implementation, once I have sync, I look at few bytes every page of data and have a quick look at an identifier that reliably enough tells me if a new stream has started and then I only extract the metadata I want, not the whole big packet as libogg would do. If you remember, ogg receives packets of bytes (as decided by the codec, splits them in segments in N segments of 255 bytes or less and put that into pages (of max 255 segments) that provide framing). My method only handles pages, which mean I might not have the whole packet, but that's sufficient for the very limited metadata set that LMS expects. |
Thanks for the updates. I'm guessing the choice to not use libogg was mainly to support squeezeamp builds? |
I can also build a living version with offers if you want and you decide which one you prefer, it's quick as I already have the ogg parser. I chose not to use it for memory and cpu on the esp32 version indeed, but also in general I did not like the idea to unwrap the whole ogg streams all the time just to have the comment header but yes, from a pure esthetic and compatibility, it would probably be better. |
I would prefer a libogg implementation but I don't want you spending more time on this just for my preference. |
No problem, I'll do a libogg implementation, I think it can be fairy easy |
Here you go. A full version that uses libogg instead of home-made parsing. It loads libogg in a compatible way with LINKALL or not. If you don't define USE_LIBOGG, it's the home-made. Feel free to remove fully that part if you prefer. |
That is awesome. I should have some time next week to test this and get it merged. Thank you. |
I'm also going to add OggFlac, so no rush |
Okay. That's great! |
84373bc
to
09fefa2
Compare
It's all done now |
Thank you philippe44. Increase squeezelite revision to 1466.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the (and a bit more)? Unfortunately, It's broken playback of 24bit high sample rate flac files.
I don't use them, CD quality is fine for me, but I've had users complain to me directly via email. I'd like to revert it unless you can help figure out why.
Here's one of the files I use for testing. https://www.dropbox.com/scl/fi/d05jsk005z77p71gz3t1m/11-Ola-Gjeilo-Ola-Gjeilo-Ubi-Caritas-piano-improvisation.flac?rlkey=0bfmbrcjkgz5bfvn11bk0t1r6&dl=1
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to fix that today and if I can't then please revert it. Such an unhappy PR, I'm sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no reason to be sorry, we'll get there. I really appreciate all the enhancements you've provided to squeezelite over the years. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, but sometimes all this feels useless. Anyway, here is what happens: the fix I made is correct (for once..) as the threshold must be expressed in frames, not in bytes. But with such an insane sampling rate, the default buffer size is not enough to store enough to reach threshold. The proper fix should be to increase output buffer by the user but I'll provide a PR with a reduction of threshold and a big fat warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #215
Thank you philippe44. Increase squeezelite revision to 1466. (cherry picked from commit c89faf3)
This PR adds to squeezelite what is on all SqueezeBox2 and SqueezePlay: ogg streams of type vorbis, opus and flac is scanned for metadata (in direct or proxied mode) and forwarded back to LMS to he handled in Slim::Player::Protocols::HTTP::parseMetadata.
This is done for every ogg (webradio/local/streaming) but is useful mainly for webradio as others are scanned if possible by LMS itself. For flac, it could be argued that metadata parsing could be done inside the decoder, not the streamer as OggFlac offers metadata callbacks, but vorbis does not, so it's better to do all in streams, where metadata handling should be done anyway.
There is a bit more in that PR as you can see the output threshold fix in there as well. You can decide to just take and I'll rebase my PR.
Note that OggFlac playback with multiple streams only works if you use updated library here https://github.com/philippe44/flac and the PR to flac is here xiph/flac#667. I don't know if the maintainer will be willing to accept it (in which case more work needs to be done, but for now and our squeezelite needs, these changes are enough)