flac - seeking within files doesn't function in many locales, and breaks playback

[mw9]

Many transcoding rules make use of flac's ability play out a subsection of a file, as if it were a single track. For example, LMS' handling of 'wav files with cue sheets' relies on this ability.

Sadly, this is currently broken in many locales, and playback will simply fail.

The flac command line that handles playback will include --skip and --until time offset options, in this form: --skip=mm:ss.ss. mm represents the minutes part of the file offset, ss.ss represents the seconds part, and can include a decimal fractional part.

This time offset specification is generated by Slim::Utils::DateTime::fracSecToMinSec, which generates a '.' as the decimal separator, without regard to the current locale in force.

flac, on the other hand, interprets these options in a locale dependent manner. In a comma using locale, the version of flac that currently ships with LMS (1.3.2) will simply fail with this output: ERROR: invalid value for --skip.

The shipped flac was updated to version 1.3.2 during the summer of 2018.

The earlier version (1.2.1) would 'fail' in a more gracious manner, in that it would simply discard the fractional part of the time specification before continuing.

A change is required to restore functionality, and this might be undertaken either within LMS itself, or by suitably patching the shipped version of flac.

So, which way forward ? Any thoughts ?

Here are my, probably incomplete, thoughts:

• Possible changes within LMS

1. Create a replacement Slim::Utils::DateTime::fracSecToMinSec that is locale aware.

The wvunpack utility that ships with LMS uses the same --skip and --until options. However it is not locale aware, so a new, locale aware, conversion function would be required to avoid breaking wvunpack.

2. Modify convert.conf

Prefacing each relevant flac command line with, for example, LC_NUMERIC=C should be effective on all Unix-like systems, but it appears not to be supported on a Windows system. Unless someone knows differently.

• Possible changes within flac

1. Patch flac to work exclusively within the 'C' locale.

This appears to be straightforward, effective, and probably resilient against future changes to flac. A proof of concept patch has already been tested on macOS, Debian linux, and at least one version of Windows, and appears to work as expected.

2. Patch flac's time specification parser to accept full stop, '.', as separator regardless of locale.

Again, proof of concept patches have been prepared and appear to work as expected.

• Other matters

I've raised this issue on this slimserver repo in first instance. If patching flac is seen as the better option, then perhaps it should be moved to the slimserver-vendor repo.

There is a recent and long thread on the forum which has been instrumental in exposing the problem: Playing cue files with LMS. Thanks are due to all participants for their patient investigation and support !

I'll mention @bpa-code, @paul-1, and @ralph-irving directly, as they have all contributed to and/or followed the forum thread, and may have a direct interest in the outcome.

Apologies for length of post.

[mherger]

Thanks for this summary! I tried to follow the linke discussion... but quickly was lost :-)

Sadly, this is currently broken in many locales, and playback will simply fail.

Are there more known "real-world" cases than the one discussed in that thread? I know that you're able to reproduce the issue. And I'm not trying to play down the problem. I just want to know about the risk of breaking something for users without the problem vs. solving it for a few.

I tend to vote for patching flac for a few reasons, but mostly for keeping the change as isolated as possible:

• we did not update all included copies of flac. By changing LMS side code we risk to break the experience for these users

• one problem about changing code you outline yourself. It can have side-effects on other parts of the system re-using the same code.

• let's fix what is broken (if possible).

[bpa-code]

"real world " examples of where "skip" parameter is used - anytime flac is transcoded and/or resampled. If user seeks (i.e. ffwd or rew to a time spot) when

• playing a 96kHz flac file on an SB3 or receiver (flac -> flac)
• player has bit rate limit and plays a flac file (i.e. flac -> mp3)

[mw9]

Are there more known "real-world" cases than the one discussed in that thread?

A number of comments from Anglophones expressed surprise that the problem had not been noticed before. Forum member bpa suggested:

Flac.exe is not used to skip time when seeking (i.e. jump to a specific time) a flac track - LMS does it internally. Also anybody with a collection of WAV file often have WAV->Flac conversion turned off and I'm guessing this could include many "cue" users.

So the user base which might encounter this problem is small.

I have a few (off-air) jazz sets, each recorded as one audio file, and I use cue sheets to break the set into individual track segments to give me a nice 'Album view' of the set. But, they are all mp2/mp3 format, and flac is not used in this instance.

Forum member JohnB presented an off air classical music recording organized in the same manner. This did break, because he provided a flac encoded file. But I get the impression that this was probably converted from an m4a/aac source, which would normally use faad, not flac, if it uses anything.

I don’t know of any more real world cases, but my experience is not very broad. Perhaps some non-Anglophones do, but have remained silent[1].

I suppose that anyone who habitually rips a CD into an ’entire’ wav/flac file would be impacted, but perhaps learns to split the rip up into tracks. I don’t have enough experience of the transcoding methodologies to narrow down specific use cases. Input from others might be helpful.

Patching flac

I think that patching flac can be entirely risk free if we limit ourselves to patching flac's time specification parser, on the try/retry on fail principle.

Forcing flac into the ‘C’ locale should also be risk free, provided there are no unintended consequences elsewhere within flac. Example: parsing of file names.

But I’d be guided by those who will be implementing and maintaining the patch. I’m a tinkerer, not a long serving, grey bearded, developer.

[1] I believe that you would not have remained silent !

[mw9]

I've cloned flac and published the proof of concept patches should anyone wish to review in context.

Operate flac exclusively in 'C' locale
mw9/flac@8f43e76

Time specification parser - Simple try/retry between full stop and comma
mw9/flac@1ed0758

Time specification parser - Try in 'C' locale first, retry 'normally' if fails
mw9/flac@a9af8c6

[michaelherger]

Hi @mw9 - I'm a bit confused: are these three approaches at addressing the same issue? It would probably indeed be better to continue the discussion in https://github.com/Logitech/slimserver-vendor. Thanks a lot!

[mw9]

@mherger - so that was a fail, then !

"Yes" to your question. I've been away for a few days, and will be for a few more. I shall continue the discussion as suggested, on my return.

[mherger]

@mw did the discussion continue elsewhere? Would you know whether this behaviour could have been triggered by an update to the flac binary? Whether flac changed the parsing?

[mherger]

Ok, skimmed the flac changelog and found this line for flac 1.3.0:

Fix bug with fractional seconds on some locales (SF #1815517, SF #1858012).

What they've fixed breaks things on our end. We'll have to implement locale aware time string rendering, too. Unfortunately we haven't updated all platforms to flac >= 1.3.0. Which means that we'll have to make this decision based on the flac version used...

[mherger]

Another idea: what if you prefixed all lines in convert.conf running flac with LOCALE=C?

LOCALE=C [flac] -dcs $START$ $END$ -- $FILE$ | [sox] -q -t wav - -t flac -C 0 $RESAMPLE$ - 

I tried to understand what it would require to have the timestamp localized in LMS, and it looks like a lot of code. or code too specific to one file format in an otherwise totally format agnostic environment. The biggest issue here is that it's not just "flac requires localized timestamps", but it's only flac from a certain version.

[bpa-code]

IIRC This is not a Linux only issue. As noted in post 1 LOCALE prefix won't works on Windows.
Why the user have this problem ? Are they not using a LMS supplied Flac ?

[mherger]

We did update flac for some (but not all) platforms a while back. That new version would respect the locale, but not the old one.

[bpa-code]

We did a lot of testing of the new flac with different LOCALE settings. I think we need to double check what version the user has installed and the LOCALE Settings to understand (i) did we miss something or (ii) is user somehow using a standard (i.e. not LMS) version of Flac.

[michaelherger]

We did a lot of testing of the new flac with different LOCALE settings.

I thought that too, but can't find the outcome of the testing. Where did it happen?

[ralph-irving]

I thought that too, but can't find the outcome of the testing. Where did it happen?

Most of the testing results are in the Playing cue files with LMS thread starting around page 19. https://forums.slimdevices.com/showthread.php?110871-Playing-cue-files-with-LMS/page19

[mherger]

Thanks @ralph-irving! I skimmed about 10+ pages, but didn't see a conclusion - other than that we should create a patch for flac. That's probably when I asked @mw9 to continue over in the slimserver-vendor repository... oh my brain...

[ralph-irving]

The flac test patch discussion continues on pages 34-35 as well. Did you get that far? It's very hard to follow in the thread. @bpa concluded that patch3 works if applied to the flac sources from slimserver-vendor git but not the latest flac sources available at testing time. https://forums.slimdevices.com/showthread.php?110871-Playing-cue-files-with-LMS&p=948702&viewfull=1#post948702

[mherger]

@mw9 - could you please provide a pull request for in slimserver-vendor with what is referred to as the "patch3"? Thanks!

[mw9]

Apologies to all, I've been very distracted for the last couple of months, so didn't follow up on my promise to raise on slimserver-vendor.

@mherger - will do. I agree that "patch3" seems to be the most platform agnostic/trouble free option, and readily maintainable.

[Moonbase59]

Re [1]: This should hit me, as we have a decimal comma in Germany, only I use very few CUE files (prefer single titles and gapless playback).

In general, I’d opt for staying away from modifying flac, since with LMS we have so many versions (and users have even different ones on their systems). Would be ideal to convince the flac programmers to add a "locale-neutral" option switch, I think. But this wouldn’t have effect for older versions, so maybe switch to "C" locale before we invoke flac? (Don’t know if this would work for Windows.)

[mw9]

so maybe switch to "C" locale before we invoke flac? (Don’t know if this would work for Windows.)

Windows is the problem here, as it, or they, apparently don't accomodate prepending the command line with something like LOCALE=C , as can be done with a Unix based system.

The proposed flac modification seems to have got stuck, I can't remember why. Need to revisit.

[philippe44]

Got hit by this one while I was doing my header & transcoding rules patches. Any idea how to solve it?

[gorman42]

This still in need of a fix. https://forums.slimdevices.com/showthread.php?112497-Using-Sox-to-parametrically-equalizing-I-m-losing-seek-functionality-Could-it-work/

[paul-1]

@ralph-irving what was the change made to the source? Can you update the source or add the patch in slimserver-vendor? I would like to update the aarch64 binaries.

[mw9]

@ralph-irving what was the change made to the source? Can you update the source or add the patch in slimserver-vendor? I would like to update the aarch64 binaries.

Here is a link to the PR that I created:
LMS-Community/slimserver-vendor#72

@ralph-irving highlighted an error that I had made, that I have since corrected. Hopefully the PR as it now exists aligns with the change that @ralph-irving actually took up, but only he can confirm that for certain.

[ralph-irving]

@paul-1 I used the flac-C-locale patch file and script changes from PR#72 mw9 linked above.

[mw9]

@paul-1 I believe that modified aarch64 binaries have not yet been taken up in LMS. The other platform binaries were taken up in this commit: b13f61d.

I was intending to close this issue - but perhaps should wait on the updated aarch64 binaries.

[paul-1]

The binaries that I supply with pCP are patched, I typically do not submit my binaries to this repo, as pCP is always a very recent glibc, and that causes general compatibility issues with other distros. For example pCP7 is using glibc2.31, whereas RaspiOS64 is using glibc2.28.

[mw9]

@paul-1 Aplogies, I had assumed that you were the go-to source for aarch64.

[ralph-irving]

I have RaspiOS64 running on an rpi4. I can build a patched aarch64 flac binary.

[ralph-irving]

I've build flac, faad, sox and wvunpack for aarch64 using glibc 2.28. They can be download from https://sourceforge.net/projects/lmsclients/files/utility/aarch64/ However, I suspect we're going to have issues with glibc as it seems ever flavour of aarch64 is using a different version. So before I create a PR to lms8 we really need to have users test them.

[michaelherger]

That would mostly be NAS users, right? Anybody in this thread here actually using an aarch64 based system?

I'd prefer to defer including these in LMS8 today, as I plan to release 8.0.0 really soon.

[ralph-irving]

The 64bit piCorePlayer 7.0 upcoming release is aarch64, which I'm using. My glibc concern is based on getting a single working squeezelite binary for ubuntu and debian aarch64 (arm64) as well as raspos64. Using the older 2.28 glibc allows the binaries to run on systems with newer versions but not older. I would expect the newer binaries I built not to work on a NAS with a glibc version older than 2.28, but don't have one to confirm.

[mw9]

Closing because updated flac binaries now prepared for all supported systems.