Closed philippe44 closed 9 months ago
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 was trying to build this PR yesterday for win32 and the old vc2008 compiler only supports variable declarations at the beginning of a code block, not within for statements, and some declarations need to be at the top of the function before any code. I'm going to build win32 exes without this PR, then merge it. It's probably time to stop creating a squeezelite that still runs on windows versions back to XP anyway.
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. I'll let you decide.
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!
It's all done now
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 https://github.com/xiph/flac/pull/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)