chained streams #667
chained streams #667
Conversation
|
⬆️ ⬇️ ? |
|
I am currently not able to review this and I probably won't for quite a while. I am not very familiar with the ogg code and as you say it is rather complicated. |
|
Understood, no worries. What I'll do then is add the "boiler plate" code so that is complete while it's still fresh in my mind and you can review it at your leisure. |
| @@ -1007,8 +1037,16 @@ FLAC_API FLAC__bool FLAC__stream_decoder_process_single(FLAC__StreamDecoder *dec | |||
| else | |||
| return true; | |||
| case FLAC__STREAM_DECODER_SEARCH_FOR_FRAME_SYNC: | |||
| if(!frame_sync_(decoder)) | |||
| return true; /* above function sets the status for us */ | |||
| if(!frame_sync_(decoder)) { | |||
There was a problem hiding this comment.
Do you think there might be a way to replace this (and similar) bits of code with more code in ogg_decoder_aspect.c? The current approach 'catches' end of stream in the generic code, but I think it would be nice if the ogg_decoder_aspect code could handle that. More specifically, it would be great if as much handling as possible would be in the function FLAC__ogg_decoder_aspect_read_callback_wrapper
|
Hi @philippe44, sorry it took quite a while to review this. I think the code looks nice, but it would be even nicer if as much of it as possible would be in Also, can you explain to me what the last few commits' back-and-forth is supposed to do? |
|
No worries, I understand what it is to be a maintainer and thank you for doing it, this is hard as the recent xz issue reminded us. The latest back and forth was a miserable failure to try to deal with some backlash I had from people using project that required (optionally) my changes and that had to deal with existing libflac for static and dynamic linking. GCC and LLVC offer ways to deal with that at run time but not MSVC and not all versions of GCC. At the end I could not find a good solution so I just added a #define, parking to see what you'd think if/when you had time to review it. I'll review the rest of your comments soon. |
|
Any chance this gets merged into the main branch soon? |
|
No. However, you can help along by explaining your use case? The main problem I currently have is that I want to implement this in a way that covers as much usage scenarios as possible, and I don't know how people want the API to function |
|
I have been pointed here by the following Debian bug report: |
|
gentoo user here, this certainly affects more than just debian. i've been (im)patiently waiting and watching for a fix as i cannot listen to approx 20% of the streaming radio stations that i enjoy. seems silly that software as good and as popular as flac has such a brutal bug going unfixed for so long. @philippe44 i tried your fork of flac thinking your fix would be in it...it didnt work. am i missising something? |
|
How did you try it? I've been running it for many month now and it works for me. |
|
@philippe44 i simply cloned your git master and built it. can you confirm that this stream plays for you with metadata and continues after a song change? http://radio3.radio-calico.com:8080/calico ..thanks! |
This is no bug, this is a missing feature, and a rather complex one too. Sure, this looks like 'just keep playing' but to me this is a feature that either should be implemented properly (with seeking, metadata handling etc.) or not at all. And such an implementation means breaking the API, which also means we can't 'just fix it' in the next release either.
Apparently no-one cares enough about FLAC to take a role regularly adding new features after the creator left the project 14 years ago. |
|
I think I've done most of it, except seeking which is really an order of magnitude more difficult. But in general, need it's not an easy feature, despite what if seems. At least it was not an easy addition for me to do :-) |
|
I think I've done most of it except the seeking which is an order of magnitude more complex. But it was indeed not an easy update, despite what it seems and I thought at the beginning. At least it was not easy for me :-) Hence I pushed for a review as my memory of what I did fades away ... |
|
i'm not even sure how or why seeking is accomplished in a live stream, which is where i've found oggchaining 100% of the time.. |
|
Not sure I understand - it work on all platforms (my PR) |
|
@philippe44 again this morning i switched to your flac fork and rebuilt mpd. the behaviour is unchanged from the xiph version. playback immediately dies at metadata update which occurs in conjunction with a song change. this is what happens with all my ogg-encapsulated-flac streams. i believe this is what distinguishes them from other flac streams which appear to be native flac streams. those ones do, and always have continued to work fine. the example i gave previously is one such stream..does it continue to play for you? edit: am i doing something wrong? should i be cloning the master in your github repository? (thats what i'm doing) |
|
Are you sure that mpd uses the right library? You probably know, but you can use a static flac library linked with your application (mpd, but not that I have no experience rebuilding it) or the shared library but you have to make sure that the newly built .so is system-installed or in the LD_LIBRARY of the application you are running. In addition, and it is coming back to me as while I'm writing that, per @ktmf01 asked, I built this extension of the api with a requirement to explicitly enable chained stream. The reasonable requirement is to have NO change compared to existing behavior. You can either change mpd or change my flac mod so that chained are enabled by default. |
|
i'm fairly certain mpd is using the system flac...which i'm building from https://github.com/philippe44/flac.git.. to be clear i'm not doing any streaming, i just have a playlist of maybe 30 radio stations in mpd that i switch between. " built this extension of the api with a requirement to explicitly enable chained stream. " -do you mean disable? |
No, by default my PR has chained disabled to be compatible with existing applications
No, you need to make a api call to enable it or tweak the code so that it's enabled by default. |
|
@philippe44 i was not aware that chaining was disabled by default! i have patched your source to force-enable it ...a configure option would be preferable here, but it works! thanks so much for improving my life! i hope xiph/flac can integrate this sometime soon for all the stream listeners out there |
|
Glad it worked! I know it was a pain for you but to some extend showing that it is not enabled by default demonstrates that the PR is not modifying current behavior 😃 I understand why this is so important for @ktmf01 because I did not realize that the original dev has left and there is not a lot of maintainer. Knowing how popular flac is makes any change scary. As a side note, it reminds me how funny it is to see companies confortable using foss even when there is basically nobody to maintain it vs being super shy to start an internal dev unless you have a army of own dev+qa |
|
@philippe44 I took a few hours to study your approach in-depth and I think it is rather nice. However, I would like to make some changes. Normally I would make a few changes right in PR itself (for which you have given permission to do) but it seems that tree is being used by people wanting this functionality. Do you mind if I 'duplicate' this PR, squash your commits and make a few changes? Most importantly, I would like to rename all instances of 'chaining' (variables, API functions, command line options) to 'decode_chained_stream' which I think is more descriptive. |
|
Yes, please do as you want. |
|
@ktmf01 i'm happy to test, please ping me when you've committed something as i'm not syncing with xiph/flac |
|
Implemented by #735 |
This is probably not yet an acceptable PR but getting closer and I wanted to know if this goes into the right direction for you.
Beyond the simple, dirty hack that is to reset serial number on page EoS which works but creates some sync issue for new metadata headers that are missed, this PR tries to properly handle chained stream.
I realized that the difference of data acquisition where flac pulls and ogg pushes creates a real challenge and I wanted to minimize modification to code. What happens is that when EoS is detected in the ogg decoder, we have no idea when that packet (audio frame) will be processed by the flac decoder. When it pulls 8kB of data, it could contain 2 remaining audio frames, then the streaminfo/vorbis/.. and audio frame for the next stream, alltogether. The ogg decoder has detected EoS but for the flac decoder, we are far from being there.
So it's easy to stop the ogg decoder when it has sent all packets of the EoS page but we still cannot instruct yet the flac decoder to self-reset because we don't know how many frames are in its internal buffer, although the read callback has sent everything. The only solution I found is to block ogg decoder once it has sent the last packet of the EoS page and fully "simulate" a FLAC_END_OF_STREAM to the flac decoder. Then we have to wait for flac to re-authorize ogg decoder to grab data once it has detected that end of stream. It's fully hidden to client, even when they decode frame-by-frame. They will not see a return from
FLAC__stream_decoder_process_singleuntil we have certainty. Once re-authorized to grab data, a "real" end of stream will be generated if we are really at the end of the stream. Otherwise, the decoder will reset, grab streaminfo and all other metadata headers and start decoding the new stream.I've now added the boilerplate code to enabme/disable it as well as a
--chainingoption in flac application. When it is disabled, I don't think a single extra line of code is executed that was not before, so I'm hopeful there is no regression.