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

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

Open
mw9 opened this issue Aug 23, 2019 · 6 comments

Comments

@mw9
Copy link
Contributor

commented Aug 23, 2019

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.

  1. 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.

  1. 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

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

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

This comment has been minimized.

Copy link

commented Aug 26, 2019

"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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

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

This comment has been minimized.

Copy link

commented Aug 28, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.