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

OggFlac can't decode a file or radio stream that has an updated serial number + Ogg headers #666

Open
philippe44 opened this issue Jan 15, 2024 · 7 comments

Comments

@philippe44
Copy link

philippe44 commented Jan 15, 2024

When trying to decode a webradio that uses OggFlac (https://maggie.torontocast.com:8076/flac) with metadata, I realized that the decoding stops as soon there is an update in the stream. The flac callback continues requesting data to read and never outputs anything.

That file/webradio seems properly ogg-encoded to me: a eos at last page of 1st stream, then and a bos, page counts restarts, with the proper OggFlac headers. and then audio. I have a standalone ogg-unwrapper that gets everything. Now, of course, the serial number changes.

Lookign at ogg_decoder_aspects.c, function FLAC__ogg_decoder_aspect_read_callback_wrapper, the explanation and the code tells me that it does not handle change of serial number. It does not seem to care about eos and bos flags or serial number, unless after reset. Then of course, when serial number changes, ogg_stream_pagein will not return a page unless you have used ogg_stream_reset_serialno.

In other words, in my experience of using libogg, when you have a ogg_sync_pageout, you must check the ogg_page_bos for a beginning of stream and then read the serial number and set into your stream_state if you want ogg_stream_pagein to return pages for that stream, and that's clearly no what FLAC__ogg_decoder_aspect_read_callback_wrapper does. It gets the serial number once and that's it. It waits for a flac reset to be called which of course does not happen.

I think the fix is pretty trivial, but I don't know what the maintainer want to do.

Is this a known issue or shall I try to investigate more? Or is this on purpose and by design?

NB: Any libFlac-based decoder fails. Either the flac executable itself or apps in LogitechMediaServer's SqueezeLite or my own applications that use libFlac

@ktmf01
Copy link
Collaborator

ktmf01 commented Jan 15, 2024

This is a known problem, and the fix is all but trivial.

See #254, #261 and #568

Handling of seeking and multiple instances of metadata make a fix for this problem rather complicated. I have several possible solutions, but all require an overhaul of the API.

@philippe44
Copy link
Author

What I mean by trivial was to simply detect eos of the current stream and reset the flag for need to acquire a serial number so the decoding would continue.

I understand that might not fit an expectation where decoding must be made into different files, one per serial number for example. That would indeed require changes in cmd line too options and api.

Here I was just looking for continua decoding of the same format, all what was required by webradio is the possibility to add headers with a vorbis comment

@ktmf01
Copy link
Collaborator

ktmf01 commented Jan 15, 2024

I am very hesitant to make that change, because it would involve skipping over metadata, because returning metadata (of the second stream) after returning audio (of the first stream) is bound to break applications. Therefore, I would like to address this by changing the API, adding a function with which the application can select behaviour: default is current behaviour (only decode first stream), one option is continue playback without returning metadata, one option is to continue playback with returning metadata. Seeking behaviour should also be selected: absolute seeking in the whole file (which involves scanning all chains before starting decoding) or relative seeking (jumping chains).

Currently I am not able to work on this.

@philippe44
Copy link
Author

philippe44 commented Jan 15, 2024

I've read the different issues and PR, my uneducated opinion would be that 2 steps could be very easy

  • 1 when reaching eos of current stream, just treat that as end of decode. Consistent and that will not lock webradio players for ever.

  • 2 a bit more advanced, just accept metadata changes, reset the flac decoder and continue decoding if rate, size, channels are the same.

option 1 at least stops a deadlock and options 2 is what I think 100% of webradio streams do.

@philippe44
Copy link
Author

I am very hesitant to make that change, because it would involve skipping over metadata, because returning metadata (of the second stream) after returning audio (of the first stream) is bound to break applications. Therefore, I would like to address this by changing the API, adding a function with which the application can select behaviour: default is current behaviour (only decode first stream), one option is continue playback without returning metadata, one option is to continue playback with returning metadata. Seeking behaviour should also be selected: absolute seeking in the whole file (which involves scanning all chains before starting decoding) or relative seeking (jumping chains).

Currently I am not able to work on this.

Understood, taking care of all these use cases is a significant amount of work. But back to my previous message don't you think that at least ending decoding when eos is reached is the right thing to do with no impact?

For my own needs, I'll probably do a fork that continues decoding if key parameters are identical. I don't need the metadata from the flac decoder because there is another separate layer before that extracts metadata on all ogg containers (vorbis, flac, opus)

@philippe44
Copy link
Author

BTW, I can try to propose a complete PR as well but I'd need to know exactly what you want in term of features.

@philippe44
Copy link
Author

See #667

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

No branches or pull requests

2 participants