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

Decode chained ogg streams, attempt #3 #735

Merged
merged 18 commits into from
Sep 27, 2024
Merged

Decode chained ogg streams, attempt #3 #735

merged 18 commits into from
Sep 27, 2024

Conversation

ktmf01
Copy link
Collaborator

@ktmf01 ktmf01 commented Aug 28, 2024

Here's my attempt to implement decoding of chained ogg streams, based on #667. This still needs work squashing bugs, but as far as I know all features are present. This includes additions to the test suite, MD5 verification and seeking.

@ktmf01
Copy link
Collaborator Author

ktmf01 commented Aug 28, 2024

@philippe44 @whatamesss @ziplantil @jprjr as you all showed interest in this feature in some form, at some point, could you perhaps take a look at this?

@whatamesss
Copy link

hello ktmf01! well, i've tried your chaining3 branch but it seems the flac stream once again dies after the current song..
not sure if i missed something but the behaviour is back to how it is with the master

i've got a patch to manually set aspect->decode_chained_stream = true; ..question: will this always be necessary?

@whatamesss
Copy link

i just noticed some odd behaviour...while mpd dies after the current song, mpv keeps playing but retains stale metadata

@ktmf01
Copy link
Collaborator Author

ktmf01 commented Aug 28, 2024

Decoding of chained streams is turned off by default. This is because users of libFLAC (programs to play music with for example) haven't been equipped to deal with this new behaviour yet. This might explain the odd behaviour you are seeing.

So, this PR equips libFLAC to handle chained streams. However, the programs need to implement this too, and that is why this is turned off by default.

@whatamesss
Copy link

it would be nice if this was a config time option at least, that way on gentoo it could be set by a USE flag..users needing to patch the source would seriously limit the reach of this feature..

as for the behaviour, i dont think it's my player (mpd) as it works with phlippe44's code

@ziplantil
Copy link
Contributor

I do not think it should be a compile-time flag. Applications should have to explicitly enable the feature when they're confident they can support it.

@ktmf01
Copy link
Collaborator Author

ktmf01 commented Aug 28, 2024

@whatamesss I introduced a new decoder state and added a few new functions. These are (of course) not yet handled and used by mpd. That is why an application needs to enable this specifically, and needs to be tested with this feature. mpv seems to use ffmpeg and not libFLAC.

@whatamesss
Copy link

whatamesss commented Aug 30, 2024

hello again. using this updated PR, i've noticed that chained streams continue to play if they have no metadata.

streams that have metadata (artist, song title, etc) still die a few seconds after song change.

@ktmf01
Copy link
Collaborator Author

ktmf01 commented Aug 30, 2024

@whatamesss I assume this is mpd? As said before, players need to be slightly modified to work with chained streams. libFLAC does the heavy lifting, but this PR needs some 'cooperation' so to say. So, you can't expect to plug it in and work correctly without any changes.

@whatamesss
Copy link

hello @ktmf01 oddly, gstreamer works fine here...can we get input from @MaxKellermann? i'm not sure how to get this support into mpd as this branch is not merged..

@ktmf01
Copy link
Collaborator Author

ktmf01 commented Sep 11, 2024

I just found out that seeking in chained multiplexed streams is terribly broken, and needs to be fixed. Note to self: the current way to determine the location in the bitstream needs to be replaced with something better. Perhaps a combination of whatever the tell callback returns, information about what is left in the buffer, how much of the current page has been used etc.

philippe44 and others added 13 commits September 26, 2024 15:36
Co-authored-by: Martijn van Beurden <mvanb1@gmail.com>
When two ogg streams were made with two different calls to `flac`
within the same second, their serial numbers would be the same.
This caused problems with seeking in the test suite. While in
theory, libFLAC should be able to seek in a chained stream with
two equal serial numbers without fail, in practice seeking in
chained streams is hard enough. So, this commit makes sure the
chained test files have links with unequal serial numbers.

Concerning the use of specific casts, shifts and masks: For some
reason, on some platforms, seemingly, saturating arithmetic was
used where overflow was expected, in effect always seeding with
the same number, UINT32_MAX.
This commit adds the capability to find the end of a link through
seeking instead of by reading forward, which should be much more
efficient. It also exposes this capability through the new API
function FLAC__stream_decoder_skip_single_link

This commit includes additions to the test suite and fuzzers
ogg_sync_check is not properly exported by libogg, causing problems
compiling on win32. It can be removed though, as that check is
included already through ogg_sync_buffer.
@ktmf01
Copy link
Collaborator Author

ktmf01 commented Sep 26, 2024

Hi all,

I'm planning to merge this very soon, probably tomorrow if nothing comes up. If you have objections, please let me know.

@whatamesss
Copy link

this is excellent news! congrats @ktmf01

@ktmf01 ktmf01 merged commit d367d6f into xiph:master Sep 27, 2024
15 checks passed
This was referenced Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants