Force flac to operate exclusively in the 'C' locale

[mw9]

This proposed change patches the flac command line tool to force it to work exclusively within the 'C' locale. This ensures that LMS's use of flac's --skip and --until options will work properly in all locales. LMS uses these options when transcoding to seek within a flac format audio file.

The patch applies cleanly to the version of flac 1.32+ adopted by LMS in September 2018, which is taken from the flac tree headed by commit reference faafa4.

Suitable adaptation may be required if another version of flac is used, e.g. the (earlier) published flac version 1.32.

The reasoning behind this patch is discussed in issue #277 filed against Logitech Media Server: "flac - seeking within files doesn't function in many locales, and breaks playback" https://github.com/Logitech/slimserver/issues/277

The patch has already been tested on macOS, Debian linux, and at least one version of Windows, and appears to work as expected.

I have not been able to test all of the build script modifications, as I don't use the platforms, but I can't see what would go wrong here.

I did fall over one point during some earlier testing. If the patch should fail in any of these build scripts, they will just carry on regardless and produce a binary. That was quite puzzling for a while, because I hadn't noticed the failure. Perhaps set -e or something similar should be placed in the scripts to force a bail out on failure. I don't use these scripts regularly, perhaps someone who does might comment.

[mherger]

Heh... thought I'd give this a try on Windows first, as one of the reporters was using Windows. Just to figure out that I had no clue how to do it on Windows 🤔... I'll need some more time. Thanks very much already!

@ralph-irving - did you provider the binaries I committed last year? Would you have a recipe/script?

[ralph-irving]

Yes I did. I don't have a script or runbook, it's all very manual and I'm still using visual studio 2008. I'm currently working on a new build VM with vs2017 but it's a bit of nightmare. High level steps. Install nasm then add as an external tool to visual studio. Change libogg static vs2008 project to release and build, Copy ogg headers and library to flac source tree. flac-1.3.2/include/ogg flac-1.3.2/objs/release/lib/libogg_static.lib Change static flac-vs2005 project to release and build. My github repo https://github.com/ralph-irving/flac-lmswin has the source tree I used from slimserver-vendor to build the current flac.exe I've applied this proposed patch and built a windows binary https://sourceforge.net/projects/lmsclients/files/utility/flac/flac-pr72-win32.zip/download

[fsbruva]

Good morning-

I see two options for the build recipe:

1. Create a separate, master VS solution to capture the dependencies of the two pieces of software. To facilitate this, the existing tarballs for flac and ogg would be replaced by a git subtree. This will allow users to use a particular commit from the two libraries, and pulling the slimserver-vendor repo will pull in the correct version of flac and ogg (as the code gets replicated into our repo by subtree).
2. Use MSYS2/MinGW-W64.

Option 1 seems a lot like work. For Option 2, I created a stub wrapper, buildme-mingw.sh, which contained one meaningful line: LDFLAGS="-fPIC" ./buildme-linux.sh This then used the buildme-linux.sh script (from the PR) to successfully build the exe.

Thoughts?

[ralph-irving]

Binaries built with Option 2 must be tested with windows UNC paths that contain unicode characters in both a directory and the filename. Previous attempts to use Cygwin and MSYS/MinGW for flac.exe in LMS caused file not found errors when unicode characters were present.

[fsbruva]

Understood - didn't know the history. Is there a write up of the problem someplace I can look at (tool chain versions, error messages, etc)? Did the flac.exe also malfunction, or was it some peculiarity with the interaction with LMS?

[fsbruva]

Also - what was the version of flac used? If it was before https://git.xiph.org/?p=flac.git;a=commitdiff;h=5705b4d7b2c3c5311138e9f4b66658c51f3cc22b which was around 1.3.0 or so, then there's a possibility that the issue is fixed.

[ralph-irving]

5705b4d7b2c3c5311138e9f4b66658c51f3cc22b Commit faafa4c82c31e5aed7bc7c0e87a379825372c6ac which is between 1.3.2 and 1.3.3

[fsbruva]

Sorry - I meant: What was the version of flac built with Cygwin that had problems with Windows UNC paths with Unicode characters in filename and directory?

[fsbruva]

I just installed the latest LMS (7.7.6 r1521467459), and the flac that came with it (1.2.0).

I am able to transcode and play a file with Unicode in the filename and directory from a network share, using both the bundled flac and the one I built (1.3.2, as patched in this PR) using MinGW-W64. Is there a particular sample file that was problematic or character sets? I did a mix of German (umlauts) and Cyrillic.

Interestingly, when I use the bundled flac.exe file from a Windows CMD prompt, it cannot perform file analysis, because it cannot write to the network location. When I used my flac from the same prompt and same file, it is able to complete the analysis and write the .ana file to the network location.

[fsbruva]

It's not all roses - I decided to expand the test set. I think the file I used previously worked fine because it was hosted locally, and wasn't actually a UNC path. The new testing involves files across the network from the server.

My MinGW flac finds/plays the same files as the LMS bundled flac, which finds/plays the same files as the binary built using Visual Studio as described above. However, all three of these still have problems with some filenames.

I used two different folders, to test the path aspect: \\server\music\fooҒӪձγ\ \\server\music\bar\

I used four files, duplicated each and added Unicode characters to the filename: baz_1.flac baz_1æ.flac baz_2.flac baz_2áʢϢ.flac baz_3.flac baz_3ώȭǹŤ.flac baz_4.flac baz_4кëﭚ.flac

The versions of baz_2, baz_3 and baz_4 with abnormal characters were not found by any of the three flac variants. Is there a better problem set of characters that you'd recommend I check and test? By all accounts right now, the MinGW built flac conforms to the behavior of the "known good" bundled with LMS, as well conforms to the version built with Visual Studio.

[ralph-irving]

Thank you for taking the time to test the various UNC path scenarios. The original discussion was via email and from what you've discribed much more extensive than previous.

The 1.2.0 flac.exe included with 7.7.6 is very old and not part of the scope of this PR. Is this the binary you used as 'known good'?

Can you try the same test using the current binary included in LMS 7.9.2 https://github.com/Logitech/slimserver/raw/public/7.9/Bin/MSWin32-x86-multi-thread/flac.exe and the build that includes the proposed patch in this PR. https://sourceforge.net/projects/lmsclients/files/utility/flac/flac-pr72-win32.zip/download

[fsbruva]

The 1.2.0 flac.exe included with 7.7.6 is very old and not part of the scope of this PR. Is this the binary you used as 'known good'?

Yes. It represents a baseline that our performance should be at least equal to.

I tested with the 7.9.2 and the build hosted as pr72 (pr72 used the same patched sourcecode that my MinGW build did). Both of the versions you asked about exhibited the same behavior as the other three copies - they only found 10 of the 16 files. Without a copy of the "problem child" Cygwin compiled flac binary, I/we have no way to verify that the current MinGW toolchain avoids or exhibits the previously observed erroneous behavior.

If you can dredge up the filename(s) from the email, we can better replicate that scenario. For now, however, I don't have a clue why some Unicode characters are okay, but others are not.

I looked into the flac-1.3.2 AM stuff, and if the build system or target is mingw, then it does a few things. First, it prepends -D__MSVCRT_VERSION__=0x0601 to the CPPFLAGS used. Second, it adds the windows_unicode_filesnames.c file to the libFLAC sources list.

[mw9]

I have no experience of building on Windows, or the associated UNC issues.

Do they prevent us from proceeding with the subject of this PR ?

I am getting an impression, perhaps wrongly, that they may be taking it off track. Happy to be put right.

[mherger]

Thanks guy! @mw9 I guess you're right: we shouldn't get side-tracked by the Windows build question.

@ralph-irving would you be fine if I included your build as is? I built on macOS and can do Linux later today or tomorrow.

[ralph-irving]

@mherger Yes of course you can include my windows build. It appears that the UNC unicode character issues are inherent to the flac sources and not a result of what tools are used to build it. Need to test the latest official 1.3.3 release win32 build to confirm.

[mw9]

Well, that sounds like a very worthwhile discovery.

[fsbruva]

Using my testbed setup with the previously mentioned files, the latest official flac binary (1.3.2) exhibits the same behavior. During the scan operation, the Slim::Utils::Scanner::Local::new (790) Handling new audio track occurs for the same files as with the other variants of the flac binary. I concur with @ralph-irving : flac UNC unicode character issues on Windows are build tool agnostic.

[fsbruva]

In another interesting development, server v. 7.7.6 had no issues with the Unicode folder name (regardless of flac used), but still couldn't manage the unicode versions of baz_2, 3 or 4. However, 7.9.2 additionally mangles the folder "fooҒӪձγ" into "F3R12Z~N". This behavior is observed when accessing the server via browser, and via Squeezeplay. Likely points to a problem with server?

[michaelherger]

Should this be followed up with in a different issue? If it's not a regression with this PR?

[michaelherger]

Hmm... building flac on Rasbpian (Pi 3) or an oldish Debian 8 (x86_64) results in binaries 60-90% larger than the previous ones. Any idea what might be wrong? I'm simply running the build-linux.sh script from the flac sub-folder.

[mw9]

I see similar building on Debian 10, armel. File size increases from 372k to 495k. That 33%, not 60-90%.

I don't know the answer, but I observe that the previous flac appears to include a library link to libgcc_s.so.1, while the new one doesn't. Is it possible that the new flac has some stuff from libgcc_s.so.1 embedded in it that the previous one didn't ?

I've no idea, really. Would need to see how the previous build went.

martin@LMS-Build-Buster:~/slimserver/slimserver-vendor/flac$ ls -al flac.*
-rwxr-xr-x 1 martin martin 495420 Nov 19 13:19 flac.new
-rwxr-xr-x 1 martin martin 372356 Nov 19 13:47 flac.original
martin@LMS-Build-Buster:~/slimserver/slimserver-vendor/flac$ ldd flac.new
    libm.so.6 => /lib/arm-linux-gnueabi/libm.so.6 (0xb6bbe000)
    libc.so.6 => /lib/arm-linux-gnueabi/libc.so.6 (0xb6a69000)
    /lib/ld-linux.so.3 (0xb6fbd000)
martin@LMS-Build-Buster:~/slimserver/slimserver-vendor/flac$ ldd flac.original
    libm.so.6 => /lib/arm-linux-gnueabi/libm.so.6 (0xb6e27000)
    libgcc_s.so.1 => /lib/arm-linux-gnueabi/libgcc_s.so.1 (0xb6df8000)
    libc.so.6 => /lib/arm-linux-gnueabi/libc.so.6 (0xb6ca3000)
    /lib/ld-linux.so.3 (0xb6edc000)

[mw9]

I repeated the build, on Debian 8, armel. (gcc version 4.9.2).

File size increases from 372k to 525k, that's 41%. And, again, libgcc_s.so.1 is not included in the library links.

Acting on a hunch, I tried LDFLAGS='-shared-libgcc' ./buildme-linux.sh.

While we do now get libgcc_s.so.1 in the library links, the file size still increases significantly, to 511k.

Also, tried with gcc version 4.8.4. Same story.

So, I am no wiser. Perhaps the previous build logs might provide a clue.

[fsbruva]

On windows, the official binary from Xiph.org is 722K, whereas the previous binary (1.3.2) bundled with 7.9.2 was 603K. When I build it on MinGW for x86_64, the patched version in the PR is 680K.

I think @mw9 is on the right track - it's likely a static library being linked in. Maybe something changed in gcc in the last year?

[mw9]

Well, a possible answer to @mherger 's question came to me in the shower this morning.

On Debian 10, armel, we have:

./buildme-linux.sh yields a flac of size 495k.

CFLAGS='-O1' ./buildme-linux.sh yields a flac of size 373k.

CFLAGS='-O2' ./buildme-linux.sh yields a flac of size 352k.

The latter compare well with the previously distributed armel flac of size 372k. So my guess is that the answer may well lie with the particular compilation flags used.

None of these newly built binaries includes a library link to libgcc_s.so.1, the GCC support library. I don't know whether that might be a concern, or simply reflects better/different optimization by the newer compiler. The previous flac did include this link.

The previous flac seems to have been compiled on a Debian system with gcc version 4.4.5, (Debian 4.4.5-8). This version of gcc was included with Debian 6 (Squeeze). ( From readelf -p .comment flac.original).

I spun up a Debian 6 armel system, and did a couple of compiles, but couldn't get a really close match, size-wise. But the theme seems to be the same. Accurate knowledge of compiler/linker flags used might get us there, if needed.

[michaelherger]

Could this flag be part of the script?

[mw9]

Well, a flag or flags certainly could be put into the script, but might not be optimal. Somewhat beyond my pay grade, as I know nothing about this optimization stuff.

Compiling for armel (armv5) on Debian 10 (Buster), gcc 8.3.0, I did some simple speed tests on flac's decode operation. Some results surprised me.

In summary:

Compiler flags tried were in the form CFLAGS='-On' CXXFLAGS='-On', with n being each of 0,1,2,3,s, in turn, together with a no flags baseline.

No flags produced the best result, with O2 and O3 vying for second place.

The O0 optimization flags just killed it. Given that the gcc manual states that O0 optimization is the default, I expected to see no difference between an O0 binary and a no flags binary. But, in reality, there is a very significant difference, so I have much to learn.

The results I got may be peculiar to armel, and/or to the particular armel platform (sheeva), and/or to gcc 8.3.0. They may well carry across to a Pi, certainly the older models. I guess that x86 boxes probably wouldn't be so bothered about micro-managing performance.

The test was a simple decode of an 84M flac encoded file, with output to /dev/null. Timed using bash's time builtin. Each test was repeated ten times. time ./flac -s -d -c source.flac >/dev/null;

I don't think I've made a mistake, but it may be worth trying out a few other source files, gcc versions, and platforms. I have a Pi zero knocking about somewhere, but I don't have a Linux based x86 box, which would otherwise have been my next experiment.

Herewith a table of results:

Flags     Seconds    Flac exec size
No flags     9.4         495k
  -O0       33.2         536k
  -O1       10.8         373k 
  -O2        9.7         352k 
  -O3        9.8         401k 
  -Os       10.8         311k
Unknown     10.5         372k, this is the current LMS
Unknown     14.0         459k, the old LMS 1.2.1

[mw9]

@mherger You asked:

Could this flag be part of the script?

I've followed up my earlier flac decoder tests on:

• arm - Sheeva armv5tel - Debian 10 - gcc 8.3
• Old raspberry pi - model B armv6l - Debian 10 - gcc 8.3
• An existing Amazon cloud i386 instance - Ubuntu 18.04 - gcc 7.4

I tested both decoding and also encoding (a wav file). The test audio files were reasonably large, representing about 12 minutes of music.

In each case, compiling without defining additional CFLAGS or CXXFLAGS consistently produced the fastest flac, both for encoding and decoding.

But the speed improvement over the next best contenders, CFLAGS='-O2' CXXFLAGS='-O2' ./buildme-linux.sh or CFLAGS='-O3' CXXFLAGS='-O3' ./buildme-linux.sh was really quite small, perhaps 1% when encoding. Which is the harder task.

So I'd say, either one of those sets of flags would be appropriate if you wish to go that way, for example to reduce the size of the generated binaries.

Of the two, CFLAGS='-O2' CXXFLAGS='-O2' would give the most effective size reduction. It could be easily put into the script, and I see that the buildme-pcp.sh script does indeed do that.

[ralph-irving]

I've added Linux armel, armhf, x86_64 and MacOS 10.7+ i386/x86_64 builds to https://sourceforge.net/projects/lmsclients/files/utility/flac/ All were compiled using -O2 and I removed the ppc arch, added x86_64, newer SDK and 10.7 as the min target to the osx build script.

Hopefully this helps with testing. I've been using the patched i386 and win32 flac binaries since I built them last November.

[mw9]

All were compiled using -O2

Thank you, @ralph-irving. I have been intending to amend the PR with the -O2 flag for some time, but other matters have been getting in my way. Will attend to that shortly, and suggest appropriate change to the pCP script as well. (Although I believe this platform may use the 'C' locale anyway).

Mentioning @Moonbase59, @philippe44, and @gorman42 because they have each expressed interest against the related Slimserver issue.

[michaelherger]

I will commit @ralph-irving's build to LMS8. What about this PR? Is this what the binaries are built from? Is it worth being merged?

[mw9]

Is it worth being merged?

I'd suggest yes if this is to be the 'official' source for the LMS bundled flac, but would defer to @ralph-irving as he has provided the builds.

[ralph-irving]

Yes, I've always used this as the official lms flac source. All the binaries were built using this repo.

The change to buildme-osx.sh overwrites the CFLAGS defined early in the file removing the MacOS SDK settings. Attached is a patch to fix it and is the build script I used for the osx flac. If you would apply it in the PR then we should be good to merge it.

buildme-osx.patch.txt

[mw9]

Apologies for OSX goof, now pushed revised commit that takes up your amendment. Should now be ok...

[ralph-irving]

I will commit @ralph-irving's build to LMS8. What about this PR? Is this what the binaries are built from? Is it worth being merged?

@michaelherger Yes, please merge this pull request. I've confirmed these changes match what I used to build the flac binaries already commited to the slimserver 8.0 branch.