headers for remote streams (including volatile)

[philippe44]

Summary

for users

Streaming AIF and WAV works, including seeking. No need to force proxy stream. Streaming MP4 works (in most cases), including seeking. Exception is when audio encoding is ALAC and player does not support that format (all SB native devices AFAIK). This is because the default transcoding rules do not work streams, only with files.

for tinkerers of conversion rules

When a mp4 with aac inside is found, the codec format is set to 'aac', so look for 'aac xxx' transcoding rules, not 'mp4 xxx'. If you want MP4/alac streams to be played as well, then you need to replace the 'mp4 flc' (e.g.) rule to make it use ffmpeg and add stdin (I) flag (or you need the "multiple transcoding" patch below).

There is also a new rule flag 'H' which forces LMS to strip out header and pass directly audio body to the rule below. Be careful that any header adjustment is disabled in that case. All you get is the audio body data. In case LMS cannot parse the header of a stream, then streaming starts at the beginning (or at the offset defined by seeking)

for developers

This patch aims to do two things:
1- Provide a framework to dynamically build headers for remote streams when seeking. It is as an extension of existing Slim::Format:::XXX which was used for files
2- Provide a mechanism to intercept streaming HTTP audio to potentially alter data (e.g. demux mp4 streams or find flac synchronization on-the-fly)

framework to parse remote headers

In every Slim::Format::XXX format you want to add dynamic header for, the getInitialAudioBlock might need to be modified, but in most cases the current version works. In addition, you should define a 'parseStream' method. It is used to parse the stream on-the-fly, acquire track information (samplerate, size, ...) but also to store the track header so that it maybe be adjusted later when seeking. This is where you can set parameters for intercepting audio data (see below). This method is aimed to be called everytime new chunks are received, until a header is successfully parsed. It receives a class, a reference to new received data chunk, a stash to keep context and an optional array reference of acceptable output formats (3 letters content-type). It shall return one of:

• 0: stop, can't parse any more
• -1: need more bytes
• value > 0: please jump to that offset in the stream
• hash: contains parsed information: 'samplerate', 'channels' and many other fields returned by Audio::Scan. In addition, it contains a temp file handler ('fh') to the actual header raw content and optionally an 'audio_initiate' that contains a code reference to initialize (bootstrap) audio interception.

That 'parseStream' method is usually used from Scan::Utils::Scanner::Remote::readRemoteHeaders to then set all relevant information in a $track object. Especially, the following Slim::Schema::RemoteTrack new fields are expecting to be set from there. There are critical if you need header re-building when seeking.

• audio_offset: offset of audio data in the remote stream
• audio_size: size of full audio (not reduced by any seek offset)
• initial_block_fh: a handle to a temp file containing the remote stream header. I will also be passed to Slim::Format::XXX::getInitialAudioBlock and Slim::Format::XXX::findFrameBoundaries to build an actual header and calculate seek offset (leveraging Audio::Scan)
• initial_block_type = undef ,0, 1, 2. It defines how the Slim::Format::getIntitialAudioBlock is called and if the stream is proxied or not.
  undef: well, LMS acts as usual, none of these considerations below apply (including audio interception). Direct or indirect streaming follows LMS's rules.
  0: stream is direct when not seeking, otherwise stream is proxied and Slim::Format::XXX::getInitialAudioBlock is called only once, then the returned header is used for any further $song restart.
  1: stream is direct when not seeking, otherwise stream is proxied and Slim::Format::getInitialAudioBlock is called everytime to obtain a new header.
  2: stream is never direct and Slim::Format::getInitialAudioBlock is always called.
• audio_initiate: an optional reference to a function that is used to initialize HTTP audio stream interception (see below)

It seems that a lot of plugins don't want the 'scanUrl' method to be called automatically as they don't want all tracks returned by a search to be scanned. Unfortunately, this also means dynamic headers re-building will not work. If the streaming service supports scanning the url (it's a non-volatile), then I recommend calling the Slim::Utils::Scanner::Remote:parseXXXHeader method for the format of your track, typically when you get a track in the "getNextTrack". You need to start a http request for that url before and pass that as a parameter to parseXXXHeader, you can look at the WIMP plugin for an example, it's very simple.

If it really cannot be done or if the url is a kind whose content changes at each HTTP session (volatile) and you need header management and/or audio interception then you can try the live header handling below.

Remember that in most case, we don't need header management or audio interception, so we are just fine with what we have. TBH, all this is really important for mp4 or flac frame alignement for legacy SB devices (Boom, SB3). I've made that a general framework in case there is more needed in the future and because I dislike ad-hoc software hacks that step by step ruin a good construction by de-architecturing it. But honestly, a lot of that could have been hacked "more simply" to just do mp4.

on-the-fly parsing / interception setting

If there is a need for header management or some audio processing (e.g. mp4 to adts) but the url cannot be scanned at all (it changes) then you can use the Slim::Player::HTTP::setLiveHeader method. It takes the following parameters

• the streaming socket
• the 3-letters input format (aac, wav, flc ...)
• an optional array reference of accepted output format

The stream will be scanned on the fly looking for a header using Slim::Format::XXX::parseStream (XXX is selected from the input format). If the header is found and the parsing function finds an accepted format, then the modified header (if any) is sent and streaming interception (if set by parseStream, see above) is put in place, then audio data streaming continues. If no (acceptable) header is found received data is forwarded, so if failure happens at the first chunk, not data is lost

This is really a last resort option as it has many caveats. I really think that plugin that don't want to allow scanUrl should try to use Slim::Utils::Scan::Remote::ParseXXXHeader because with this live parsing:

• Proxied streaming must be forced before calling setLiveHeader otherwise LMS might go direct
• It's too late to change $track's content_type and this define transcoding rules, so it must be set before to match the expected result of parseStream
• When header parsing fails or can't find a compatible option, streaming is likely to fail
• Seeking can't work

audio streaming interception

It might be useful to intercept audio data streamed by the HTTP class and process it before it is piped to the player/transcoder. For example, it works to demux mp4(aac) stream and outputs ADTS frames. A "hook" can be set to be called for every audio chunk received (no header). That hooks receives a stash (see below), audio chunk and max bytes writable in place in the audio buffer. The returned value is the amount of non-processed input audio (this allows LMS to not get further bytes from the remote stream until 'audio_process' has emptied its buffer, avoiding data built-up, as the hook likely cannot process all input chunk at once or has an expansion factor).

To setup that interception system, the Slim::RemoteTrack::audio_initiate must be set with a reference to a method that will be called when a new stream begins (a bootstrap). This method receives a reference to the header build by Slim::Format::XXX::getInitialAudioBlock and must return a two items in a list

• a reference to the method called to process audio (the hook)
• a stash that will be given to that hook.

That method only happens of course when the stream is being proxied, so the $track->initial_block_type must be set to either INITIAL_BLOCK_ALWAYS or INITIAL_BLOCK_ONSEEK. Otherwise, LMS could decide to stream direct and so no interception will happen

Note that this initializing method can update the header or even empty it. When de-muxing mp4(aac) to ADTS, the whole mp4 header is useless and must be removed. The bootstrap potentially works in combination with parseStream to find the right bootstrapper to propose depending on the accepted audio format (parseStream parameter). Even the bootstrapper could send different 'audio_process' method depending on the actual InitialAudioBlock it receives.

Today, it's only for Slim::Format::Movie and there is one bootstrapper (setADTSProcess) and one processor (extractADTS). Nevertheless, this system is not targeted to replace the external transcoding LMS framework, which allows a much better flexibility

NB: when the transcoding rule requires header bypass ('H' flag set or '-' rule with pcm output), the stream may be proxied or not, but none of the mechanism above applies (including audio interception). It's all bypassed except the live header parsing.

multiple transcoding rules patch (tinkerers)

This allows more than one transcoding rule for for the same format pair. For example, for 'aac pcm', you can use faad on files (F) and ffmpeg on stdin (I). As in general, the first rule with a perfect match (all options available) or the first partial rule with the highest matching score wins.
This solves the case of mp4/alac mentionned above by having a 'aac flc' with flag 'I' using ffmpeg while keeping use of faad for files and mp4/aac

Background:

That patch started with the objective to fix streaming remote wav/aif stream where LMS was not assessing properly the samplerate, size and channels because the stream headers were not parsed. Then I realized that the rules to handle aif files was 'questionnable' as it claims to output an aif file, but in fact it's a pcm without any header. Knowing that squeezelite now supports streams with wav and aif real headers, I thought it would be a good idea to fix the aif rule, add proper 'aif pcm' and 'wav wav' ones to be able to send real wav or aif files when player claims to support these.

While doing this, I had to introduce the notion of "stripping header" for "wav pcm" and "aif pcm" rules, so I decided to open it to users by adding a 'H' tag in the convert.conf that when set, forces the header to be stripped, it might be convenient one day. Just to be clear, stripping header for "wav/aif pcm" pair and "-" rules is automatically done, no need to add the "H" tag. Of course, if the transcoder is not "-", the header is not stripped unless explicitly requested by 'H', this is in case the converter program wants the header. For legacy compatibility, aif header is stripped for "aif aif" pair and '-' rules for legacy SB. All that happens in TranscodingHelper::getConvertCommand2

Then I also realized that mp4 was suffering from the same issues so I've extended the idea to mp4 and made it more general so that at some point, others format wanting to parse headers or even alter audio can. This was needed as remote mp4 cannot be transcoded by faad because it does not support stdin on mp4 file. Only options was to use ffmpeg but it's not by default. So there is as well a mp4 extractor that builds raw aac (ADTS) data in Perl now. This is not a general method to replace external transcoding of course, just a convenience when some obvious things can/shall be done by LMS directly.

When mp4 codec is supported directly by a player and there is no seek, then direct streaming is done (if user authorized). When there is a seek, then same logic as waif/aif is used, the header calculation is just a bit more complicated.

Unfortunately, there is one extra difficulty with mp4 when streams have the 'moov' atom at the bottom of the file, so I had to do a first HTTP request to seek for it and if 'mdat' (audio) is reached before 'moov', then I launch another HTTP request starting at the end of the audio to grab the 'moov'. Then this 'moov' is stitched to the already acquired header to form a complete one which is sent to the player. Of course, then you can't do direct streaming because the header always needs to be adjusted and I had to find a way to tell LMS to prevent direct streaming and the only solution I found was to add HTTP::canDirectStreamSong and use a different value (2) for RemoteTrack::initial_block_type. This method is called by Squeezebox2::canDirectStream, but it seems that some plugins call SUPER::canDirectStream in their canDirectStreamSong and not the SUPER::canDirectStreamSong, I have to think if this is an issue or not.

Finally, for this to work fully for players that do not support mp4, the "mp4 flc" rule has to be applicable, and today it uses faad and as faad does not support stdin for mp4 files, the rule is an 'F' only and fails if we add an 'I'. I talked to @ralph-irving and I think he agrees to try to modify faad to accept stdin for mp4 as long as there is no -e/-j options (seek) which make sense because the seeking is done by LMS now, so as far as faad is concerned, it's a regular mp4 to be played from the beginning. This is still uncertain, so I've added an mp4 extractor so that LMS can spit raw ADTS frames directly from the mp4 stream. Then the "aac xxx" rules can be applied and faad is happy there using stdin. Now, when mp4 audio encoding is alac, there is still a need to have an alac to xxx transcoder and faad still cannot do the job. So there would be a need for ffmpeg here (and my other "multiple identical rules" patch). I'm not sure a lot of people are streaming alac, though :-)

NB1: keep in mind that this patch is also against another previously accepted one for wav/aif headers and I've been able to simplify things, so some changes (e.g. StreamingController) are simply returning to previous code.

NB2: I still don't handle 'moof' type of mp4 files but we simply might use ffmpeg with the multiple rules patch. This patch will not do any header work if 'moov' is not found and will simply call the original callback, leaving normal LMS process to happen

[philippe44]

The wav/aif/mp4 re-insertion now works in direct and proxied mode. In mp4, can't do yet seeking. One benefit is that I'm now able to play remote mp4/alac streams. The stitching of moov headers that are at the end of stream works, but result don't play yet. Enough for today :-)

[philippe44]

Well ... it's starting to work include for podcast like "Open Jazz" so, stitching & re-creating 'moov' is up and running. My helixaac does not like the result, but Squeezelite/faad & SB Radio do. More to come

[michaelherger]

We'll need all eyez on you(r code)!

[philippe44]

We'll need all eyez on you(r code)!

That would be welcome :-) b/c some items like handling seek value upon player's reconnect gave me real heartburn.

[philippe44]

all right - so it works now, including seeking in files with moov at the end. Direct streaming will be selected if authorized and possible. I've continued to isolate Player::Protocols::HTTP.pm from knowing format idiosyncrasies so when a header needs to be adjusted for seeking, Remote.pm signals it at url parsing and get called back when needed at song opening.

Few things remains to be done, the easy ones first

1- AIFF header is not adjusted when seeking in remote and local
2- WAV header is not adjusted when seeking in local

More difficult

3- Unfortunately, faad does accept '-' for stdinput, so we still can't use the 'mp4 flc' rule for players that don't support mp4/aac. I've asked his opinion @Ralphy and I hope we can do a fix to accept stdin as long as there is no seeking required and 'moov' and 'mdat' are ordered - fingers crossed

I'm a bit annoyed by the fact that my helixaac decoder does not like stitched moov ... but at least I can step by step into it and see what's wrong. I hope I won't have to modify too many things either in Audio::Scan or helixaac.

Maybe more difficult, @bpa-code I've not given thought how to handle 'moof' and 'mfra' type of files. Maybe we can just do 'moof' and ignore 'mfra' as repositionning only between fragments (not withina fragment) could be acceptable.

Finally, I hope that what I've done is generic enough so that if others feel like extending remote file headers management, it will be easy by just adding to this framework. There is no knowledge of the formats specifics anywhere except in Remote.pm. Which reminds me that maybe Remote.pm shall be split like File.pm is into a core and a set of format processors, but that's luxury at that point.

Enough for tonight

[philippe44]

So, it's fully functional now. I've even been able to find the issue with my helixaac codec (was on my side of course) so that's great, one less bug in squeezeesp32.

All headers are adjusted, file & streams, including AIF & WAV. Remaining part is faad handling of stdin and mp4. So at this point, I think I should wait for feedback and opinion on mp4 handling, including do we need to think about 'moof' & al

[michaelherger]

Phew... quite a change to digest... Could you please update the initial comment with a.) what user-facing problem you're solving, and b.) how you achieved it? We can then ask the right people to join the review. Thank you so much!

[philippe44]

Will do - l still need to make some changes anyway, so bear with me a bit more :-)

[bpa-code]

It is a lot of interconnected changes and so deserves time to review - however at the moment easing of restrictions means less time for LMS. I'll make comments as soon as I can. I think a working use case may help show what problem is being solved. Is it possible to get MP4 podcast using faad ? This Jazz feed has MP4 podcasts http://radiofrance-podcast.net/podcast09/rss_12283.xml

[philippe44]

No worries, I'll make changes for better consistency and I'll rewrite the first post to better explain the use case and how I think I solved these, so take your time and I'll signal when it's ready for review (my earier post was a bit optimistic :-).

Currently, that podcast you mention plays with any player that accepts mp4 directly. I'm seeking in the remote stream and stitching the 'moov' atom back at the beginning. As of now, faad is an issue because it does not accept mp4 from stdin, so for players like Boom it does not work yet, but @Ralphy thinks he can have faad accept stdin as long as there is no -j or -e option, which would be the case because the seek is handled by LMS now.

[bpa-code]

You've answered the two points I wanted confirmed about my understanding about the end functionality of the mod:

• streamed MP4 skipping for "native" players.
• streamed MP4 transcoded using faad without "-j" & "-e"

[philippe44]

I've edited the 1st post with a looong explanation, hope it helps. I also hope that this version is the right one wrt aif,wav,mp4(moov+mdat or mdat+moov),direct,proxy,http,https.
Many permutation and combinations although the number of modified lines is modest

[philippe44]

As it seems that using stdin for mp4/faad does not work (although @ralph-irving is trying, thanks!), I've put together quickly a Perl mp4 to adts extractor https://github.com/philippe44/slimserver/tree/built-in-MP4-to-ADTS. It works with players that can do aac/adts natively (Radio, Squeezelite) but for some reason it plays a couple of seconds and then fails when proxying through faad... grrr!

[philippe44]

Just continuing ... so, I've found the issue with pipeline and I've updated my version (not the PR). It's working nicely, I'm able to stream & reposition mp4 (including the ones with trailing mdat) to my Boom using faad as it is.

I don't know if you'll be willing that mp4-adts demux in LMS or not (depends if we can fix faad and mp4 on stdin). Now, with the transcoder patch, it gives plenty of solutions for @michaelherger's tidal updates + seeking / playing mp4 in general.

Still, if this becomes integrated, I think we have to find a better place as that code does really not belong to Utils::Scanner:Remote.pm IMHO, but we would need to add some extension of Format::RemoteStream.pm like File.pm is extended with a set of readers (Aiff.pm, Wav.pm). Maybe just putting it in these files (here Movie.pm) is the right place, I don't know.

[philippe44]

Finally, I hope you don't mind but I've decided to put together the MP4 to AAC and Remote Headers patches as they are really complementary and it will be easier (=complete) for your review.

I've refactored the audio & header handling to where I think they belong more naturally, i.e. in their respective Slim::Formats. With that, Slim::Utils::Scanner::Remote really takes care of what it should, which is scanning headers and prepping Song to be able to build Headers with or w/o seeking offsets. The knowledge of building the header has been sent to Slim::Formats as it was already for Files.pm. I feel it's reasonably clean like that.

This is now ready for reviews & critics :-)

[michaelherger]

Wow... never have I seen a PR description like this. Quite some information to digest. Thank you very much!

One first question I had was about how Slim::Utils::Scanner::Remote->scanURL() is being called. AFAICT it's only done in Slim::Player::Protocols::HTTP->scanUrl() - which is being overwritten in most protocol handlers to prevent scanning (eg. Deezer). Wouldn't all of your work then mostly be ignored? Do they need an update, too?

You mention:

... so I had to do a first HTTP request to seek for it and if 'mdat' (audio) is reached before 'moov', then I launch another HTTP request starting...

IIRC some streaming services provide single-use URLs. In that case we'd have a problem, wouldn't we? I'll try to test your change with TIDAL.

More later.

[philippe44]

Wow... never have I seen a PR description like this. Quite some information to digest. Thank you very much!

Thanks :-) That also helped me to verify my code and was proven useful ...

One first question I had was about how Slim::Utils::Scanner::Remote->scanURL() is being called. AFAICT it's only done in Slim::Player::Protocols::HTTP->scanUrl() - which is being overwritten in most protocol handlers to prevent scanning (eg. Deezer). Wouldn't all of your work then mostly be ignored? Do they need an update, too?

argh ... that's a very good point. But that also means that any header management we have done for ogg, ogf is void as well, no? Maybe it does not matter for these formats but if we want mp4, then it's a problem.

You mention:

... so I had to do a first HTTP request to seek for it and if 'mdat' (audio) is reached before 'moov', then I launch another HTTP request starting...

IIRC some streaming services provide single-use URLs. In that case we'd have a problem, wouldn't we? I'll try to test your change with TIDAL.

If they do, then it cannot be a stream with the 'moov' at the end otherwise it would never be usable. I assume that url is like for Deezer Flow (AFAIR), the ProtocolHandler is in charge of retrieving the real url where the audio is, no? Or is this like every time you open the same url, you have a complete mp4 file, but different?

More later.

If the latter is correct, then there is definitively much more to come :-)

[bpa-code]

OK - I'll look into it and comment on #371 .

[philippe44]

I've used this version (including the "reliable" plugin) and did not have issues so far, but my usage is far from being exhaustive.

What keeps bothering me is that I don't know how to track where references to the $track objects are so it's pretty unclear to me when they are destroyed. They definitively are when you set the cache to a low value, but the logic espaces me. That can be a bit annoying if you build a very large playlist with ton of remote streams and a plugin that allows scanUrl. Not so many AFAIK

[philippe44]

BTW, this PR can be "decomposed" for review in 3 different steps

• parsing & rebuilding remote headers as part of scanUrl ( Remote.pm; File.pm; AIFF.pm, Wav.pm, Movie.pm, HTTP.pm, Song.pm). Ignore the "audio_process", "_parseStreamHeader" and "setLiveHeaders" references in HTTP.pm and Movie.pm
• option to process received audio data, precisely used for mp4 to aac (Remote.pm, Movie.pm and HTTP.pm, more precisely setADTSProcess and extractADTS)
• option to parse & rebuild remote headers live while streaming, in case scanUrl cannot be used, directly or indirectly (see example in WIMP plugin for what I call indirect use). This one is probably the most difficult to follow, hence I would suggest to keep the review of HTTP::_parseStreamHeader and HTTP::setLiveHeaders for the end. But this explains some of separation between Remote.pm and Formats:XXX.pm, so that Formats:XXX.pm function can be used without a track object and the function to set $track objects only happens in Remote.pm. Without that perspective, this separation seems a bit artificial

If it helps, I'm happy to provide 3 "nested" PR to facilitate the review (although I'd like to keep the merge candidate as a single PR)

[michaelherger]

If it helps, I'm happy to provide 3 "nested" PR to facilitate the review (although I'd like to keep the merge candidate as a single PR)

I was about to ask this... Overall I'd have to trust you with this change. I doubt I'll be able to get an understanding of all the changes in a reasonable time. But I hope I could at least get parts of it if I could concentrate on parts of the change.

[philippe44]

Ok, I tried something (I'm not very good with git, I failed trying 3 branches initially). The "r-header" PR is not aimed to be functional, but it's just the 3 steps described above. Let me know if it helps.

PS: I just saw that there was a bit too much in the initial commit but that's not material, so if you don't mind, I'll keep it like that. Please don't hesitate to ask question, it's the best help for me to verify what I did

[michaelherger]

@bpa-code Did you find time to further review this PR? I'll be off for the rest of the month. I can merge this today, or it'll have some more time to ripen while I'm out :-).

[bpa-code]

I've done a paper review. I just wasted time trying to get up to speed with smartgit and so haven't been able to actually test whether the changes break anything of mine.

I think the changes are OK. If you merge before vacation and something is broken, user can rollback and just wait until your return. Alternatively there is no rush for the changes and they can wait but then they'll be on your desk after vacation. WIMP is the main candidate for using the features of the PR and I think with these changes, it should be much simpler. MP4 Podcast will be second.

I have a few niggles. I can't see an example of setLiveHeader in use and has references to 3 letter types when in fact there are many 4 letter types (e.g. wmal, wmap) - AFAICT length of 3 is not in code. I don't know why a 2nd key to "content_type" in addition to 'ct' is necessary in %localTagMapping - may make harder to find all reference of use in future. The change of EWOULDBLOCK to EINTR may break some plugin but since I think they are all Triode's unlikely to be significant. The need to use a "localFh" in getInitialAudioBlock seems like something is not quite right somwhere.

I think the changes may help sort out the "mdat" issue which created the need for alcx and mp4x types - needs to be checked and tested later.

I think (maybe not part of this PR) there is no right answer for the GUI for multiple rules in Transcoding but there are instances where current approach produces results that may confuse users. Need to keep an eye out for forum posts on this topic.

[philippe44]

I think the changes are OK. If you merge before vacation and something is broken, user can rollback and just wait until your return. Alternatively there is no rush for the changes and they can wait but then they'll be on your desk after vacation. WIMP is the main candidate for using the features of the PR and I think with these changes, it should be much simpler. MP4 Podcast will be second.

About timing, the only thing on my side is how fresh this is in my mind. One month later will be more difficult for me to get back up to speed. But I understand the risk of having unhappy users if I broke something

I have a few niggles. I can't see an example of setLiveHeader in use and has references to 3 letter types when in fact there are many 4 letter types (e.g. wmal, wmap) - AFAICT length of 3 is not in code.

I'm not following the reference to 3 letters. The setLiveHeader is not in use today, that's correct and it's because it's more a plugin option in case you cannot do a scanURL or do what I did in WIMP, so you have no option to scan the header "off line" but you have to scan it while you get the actual live stream. It might happen if a stream has a fixed url with changing content but you need header management. Candidly, I think and I hope it's a corner case. It started from @mherger comment about scanUrl and how plugin usually bypass that, so I did implement this, but I found a better option in WIMP

I don't know why a 2nd key to "content_type" in addition to 'ct' is necessary in %localTagMapping - may make harder to find all reference of use in future.

The reason is that I need to know what was the original stream's format. When the mp4 to aac demux happens, the content_type is changed to 'aac' and I've lost track of the fact it was mp4. But the HTTP::request needs to know that it was originally an mp4 file to call the right header builder. There are ambiguity there as aac can come from multiple source formats. I could have hard-coded that of course, but I thought it's not the right thing to do, so I kept trace of the original source format. It's invisible to all other application because, to access it, you need to call the accessor original_content_type

The change of EWOULDBLOCK to EINTR may break some plugin but since I think they are all Triode's unlikely to be significant.

That I agree can be removed, but I was a bit horrified to see how hard sysread was it when EWOULDBLOCK was hammered when waiting for negotiation where in LMS code EINTR is nicely handled as a "wait a bit an come back later". FYI, in the RP plugin modification that has been around for a while now, I use EINTR.

The need to use a "localFh" in getInitialAudioBlock seems like something is not quite right somwhere.

My comment is misleading. I did not myself see a need to do that and it's not that it would not work without it (no hidden blackmagic) but I saw that Fomat::Wav.pm was doing that and I was not sure why, maybe there was a reason I would not understand, like we modify the file position and it creates an issue. I really do not think it's the case, but as it's a zero-cost thing, I did not want to take the risk, so I did the "monkeys and the banana" https://sixsigmadsi.com/10-hungry-monkeys-a-story-of-cultural-training/#:~:text=%E2%80%9C10%20Hungry%20Monkeys%E2%80%9D%2C%20A%20Story%20of%20Cultural%20Training&text=As%20a%20sociological%20experiment%2C%2010,were%20a%20bunch%20of%20bananas.&text=The%20monkeys%20immediately%20spot%20the,begins%20to%20climb%20the%20ladder.

I think the changes may help sort out the "mdat" issue which created the need for alcx and mp4x types - needs to be checked and tested later.

Yes it does, you probably don't need them anymore

I think (maybe not part of this PR) there is no right answer for the GUI for multiple rules in Transcoding but there are instances where current approach produces results that may confuse users. Need to keep an eye out for forum posts on this topic.

Agreed

[bpa-code]

Timing - I understand but there'll be early bugs and there'll be bugs found in months/years time.

Still don't quite understand the 'content_type' but probably need more time to study rationale.

EINTR / EWOULDBLOCK may be related to an old schedule loop bug Michael only found a few years ago which messed up Triode's initial implementation of HLS.

Again I understand about "localFh" - I had one like this and only a long time later I found root cause was a closure issue.

Good news about alcx & mp4x - never liked it, it was a quick hack to tackle a frequent issue.

[philippe44]

Timing - I understand but there'll be early bugs and there'll be bugs found in months/years time.

Yes, I'm not expecting that to be bug and corner cases free, it's just super-fresh in my mind now :-)

Still don't quite understand the 'content_type' but probably need more time to study rationale.

When header is being parsed in scanUrl (or equivalent) and stored in the track object for later use, I much change the "content_type" when the mp4 to ADTS demux is activated (it changes from mp4 to aac). That allows the right transcoding rules to be used if needed as LMS needs to know that now it will receive an aac stream. But when the song actually start playing, I still need to build a mp4 header (including handling early mdat and seeking) then this header is sent to the demuxer (each $song instance must have a different header, as a $track can serve two players). If I don't have access to the original content_type, I can't know what is the format so I can't select the right header building function.

EINTR / EWOULDBLOCK may be related to an old schedule loop bug Michael only found a few years ago which messed up Triode's initial implementation of HLS.

I would have to different to @michaelherger there

Again I understand about "localFh" - I had one like this and only a long time later I found root cause was a closure issue.

I'll try to see if I can identify a reason

Good news about alcx & mp4x - never liked it, it was a quick hack to tackle a frequent issue.

We still have to understand what needs to be done for "moof" type mp4...

[michaelherger]

I don't know why a 2nd key to "content_type" in addition to 'ct' is necessary in %localTagMapping - may make harder to find all reference of use in future.

Would it help if that additional field was named differently, giving the name a hint about its purpose (original CT)? And leave a comment in there?

[philippe44]

I don't know why a 2nd key to "content_type" in addition to 'ct' is necessary in %localTagMapping - may make harder to find all reference of use in future.

Would it help if that additional field was named differently, giving the name a hint about its purpose (original CT)? And leave a comment in there?

It already has a different name (original_content_length). Previously, the content_length was a field of RemoteTrack and accessible through an accessor. Now, what I wanted is that when "one of us" writes RemoteTrack::content_length('xxx') then, the field "original_content_length" is set but never changed with further calls to the "content_length" accessor.

So, I created a method named "content_length" (instead of just an accessor field) and renamed the existing accessor field _content_length. That method write once into the new accessor field "original_content_length". Then, I can access that accessor field "original_content_length" to know what was set initially (whoever set it) and at the same time none of the existing code had to change as it still use 'content_length" which is now a RemoteTrack method.

I also had to map "content_length" to "_content_length" in the "localTagMapping" so that some DB that access directly content_length (not as an accessor, but directly as a key of the track hash) still work - that one bit me. That's the whole story

[philippe44]

@bpa-code; any chance you can have another look? It's just that I've moved back to write some code for the squeezelite-esp32 project and my brain is being heavily rewired to embedded C :-) so I start to have more difficulties to explain these LMS/Perl patches. Per our previous discussion, I know some bugs will remains for a while, it's just that it's easier to explain key choices now that it's fresh. But I fully understand you are busy as well. I just value your input a lot.

[bpa-code]

Sorry, didn't notice recent updates. Had email problem and didn't get any email since 21st.

[michaelherger]

Just when I thought I'd suggest merging this I broke the PR by committing a fix introduced earlier... but then that's probably a good thing: your previous change to the header handling has caused an issue with "volatile" tracks, eg. files read from disk, but not stored in the database. These are in some places treated like remote tracks and need some special love (as you can see from my latest commit). Could you think of other places in this PR where this could be relevant?

[michaelherger]

@philippe44 Shall we give this a try?

[philippe44]

Sorry I missed that, I was busy with SqueezeliteESP32... I now need to rewire my brain from C, counting bytes and cycles to Perl & LMS :-)

But, I looked at that and I really don't think I introduced the issue. I've tested on a 7.9.2 and I have the same result on VolatileURL, so I think it has been there forever like the WAV initial streaming issue.

If you remember, I started working on this broader topic because one user had problems with streaming small mp3 that I fixed and then he had "chmpmunk" issue with streaming wav that were not 44.1. It was due to the fact that LMS did not analyze wav header for remote streams and was always assuming 44.1. So I embarked into analyzing WAV headers for remote streams to be able to pass the correct sampling rate/size/channels to the player.

At the same time, I realized that for remote wav files, LMS was allowing seeking but in many case, it would not work as LMS was simply doing a GET with a range and the range was a byte value calculated from the time offset - it was not taking into account the header, sample size & channels, so you could end up anywhere and indeed, you get white noise of you seek in a remote URL with 7.9.2

So, I added to that first PR a hack to also prevent the "wav pcm" identity rule to be used on remote streams, but that was a "bonus" to the PR, not a correction of a problem it brought.

What I forgot to add as well is that VolatileURL seems to behave the same, i.e. we don't have the file header available, so LMS does a blind "jump" of bytes in the file. Works for many streamable formats, but not for wav/aif.

The proposed patch fixes that by also ruling out the identity rule. My current PR proposal is different as it now very allows seeking even on remote streams because it keeps track of the header and re-insert it on demand. So I'll see how to correct this VolatileURL issue as well with the new PR

Sorry for the long post, but I also needed to start remembering the details of that and writing helped :-)

[philippe44]

and the reason is that $track->block_alignement is not set when doing a volatileurl. I still have to find why the $track->audio_offset is set but not $track->block_alignement...

[mherger]

@philippe44 Could you please merge 8.0 into this branch to get rid of the conflict (it's whitespace only, fwict)?

[philippe44]

Yes, it's whitespace only and I think I screwed up again with tabs (did that oneline...). Well, whenever you feel ready if you have mp4 for tidal, we can try a merge. I've been using that version for a few weeks now but my usage is very far from being exhaustive

[mherger]

Ok, let's give this a try!

@philippe44 would you mind doing a very short announcement in the forums to let users know there are important changes coming their way, and what to look for? Not as long and elaborate as here, but stuff like "AAC transcoding", podcast issues or whatever? Thank you VERY much!

[mherger]

Sure enough I missed this change... what happened here? Why did you remove this part of the code?

[philippe44]

euh ... because you asked me :-)

[mherger]

Oh, because it's unused code? Oh my... Which means I actually did not miss this part. My memory...

[philippe44]

Yes, and I always do what the Boss says

[philippe44]

And about memory, I think I'm in the same boat, just rougher waters... That's why I was begging for the PR to be reviewed and accepted so that when arrows flows, at least it's fresh in my head. It was pretty difficult to go back for the volatile part already :-)