ralph-irving / squeezelite

Lightweight headless squeezebox player for Lyrion Media Server
https://sourceforge.net/projects/lmsclients/files/squeezelite/
Other
391 stars 98 forks source link

Decoding m4a files over-runs music data atom #184

Open jtbr opened 1 year ago

jtbr commented 1 year ago

In an m4a (aac) stream, the mdat atom/box contains the music data.

In faad.c, the length of the atom is read at line 119. Once the mdat atom is detected, playback is started on the data within that atom. However the length of the mdat atom is ignored (line 246). It would seem that, rather than terminating playback at the end of the atom, all further data in the file/stream is sent to the NeAACDecDecode function (lines 406-419). The atom length is not used.

If I understand correctly, there is no constraint on the ordering of atoms in an mpeg-4 file. The MDAT may come before other data, all of which the current implementation would attempt to decode as if it were audio. In particular I have read that it is possible that in a streaming application, there may be multiple MOOV atoms with associated separate MDAT atoms (each fairly short), to allow real-time streaming where the full runtime is not known in advance. It would seem to be important to terminate decoding at the end of the mdat atom, and ideally to also allow detection and decoding of further atoms (particularly including mdats) in the stream. I am not sure how best to solve this however, or I would have tried to make a patch myself.

I came across this issue while attempting to uncover why I was getting the error:

Slim::Player::Squeezebox2::statHandler (156) Error: 9c:b6:d0:ec:ab:cc: Decoder does not support file format, code 0​

in LMS when playing a large number of my .m4a files. It turned out that squeezelite was showing getting the following error:

[00:41:19.933032] faad_decode:427 error: 21 Unexpected channel configuration change
[00:41:19.933063] faad_decode:466 unable to decode further

(both of these point to faad.c, although it seems the channel configuration error is emitted by libfaad2 upon trying to decode past the end of the stream).

They happen because (for reasons unknown to me) these m4a files have a second, empty mdat atom at the end of the file, after the first mdat which contains the actual music. The encoder bonks on the 8 bytes in this atom (\0x00\0x00\0x00\0x08mdat). In squeezelite, the error seems to have no consequence in this case. It's at the end of the file, and playback continues with the next file in the playlist. But downstream of you, in Philippe44's LMS-Cast (LMS to chromecast) plugin, this causes playback to become confused and stop soon after (some discussion here).

I found info about the m4a file format here, if that is of any use. Of note: if the atom size is 0, that means the atom extends to the end of the file. If it's 1, the actual (64-bit) size is found in the first 8 bytes following the type field.

Many thanks for your work on LMS and related apps, they're really great.

jtbr commented 1 year ago

Here is an example m4a file that exhibits the problem

example_mp4.zip

ralph-irving commented 1 year ago

Using the latest LMS 8.4, the example m4a file plays to completion each time in a 3 track playlist of the same file with squeezelite 1.9.9-1428 and faad library 2.7 git 8cde9ff from https://github.com/ralph-irving/faad2

Squeezelite does emits a different decoder error but plays the entire track.

faad_decode:427 error: 1 Gain control not yet implemented
faad_decode:466 unable to decode further

What OS and CPU type and versions of lms, squeezelite and faad2 library are you using?

jtbr commented 1 year ago

I'm running linux mint 21.1 (ubuntu 22.04) x86_64 on both the LMS box and my laptop where I tested squeezelite. LMS v8.3.1.

Usually I only use squeezelite indirectly via LMS-cast (castbridge), but I installed the debian package to test. That appears to be version 1.9.9-1395. The faad2 library would be whatever version squeezelite built with (similarly for castbridge 2.1.16), not sure how to verify this.

Your behavior is like mine. Playback succeeds; the decoding error occurs at the end of the file, after all the audio content has ended. In squeezelite it continues normally to the next track after the error. But in castbridge the error has greater consequences. It loses its position in the song/playlist and the playlist stops soon after.

The error message you see is a little different, might be due to a newer faad2 version. The code I looked at however is your latest in github (similarly to that in Philippe44/LMS-Cast).

More generally, it seems the decoding would continue arbitrarily beyond where it should stop, so it might (?) accidentally decode unrelated data as sound, which could be unpleasant. Or it might give an error and fail to recognize further music in the stream (if there is more than one mdat atom in the file).

jtbr commented 1 year ago

As an aside, if I build the latest faad2 executable from your repo, it is able to decode the file to a wav without emitting any warnings.

But I believe the code concerned is that which calls faad2, not faad2 itself.

ralph-irving commented 1 year ago

The debian squeezelite package is very old and uses the system installed libfaad2, whereas my builds include all the decoder libraries within the binary. I don't troubleshoot issues with it for that reason.

A current squeezelite linux x86_64 binary is available for you to try at https://sourceforge.net/projects/lmsclients/files/squeezelite/linux/squeezelite-1.9.9.1428-x86_64.tar.gz/download

If you run the system squeezelite with -d all=debug you can confirm the use of the faad2 system library from these two lines of output;

load_faad:633 loaded libfaad.so.2
register_faad:663 using faad to decode aac
ralph-irving commented 1 year ago

Looking at Philippe44/LMS-Cast if you're using the non-static build of the squeeze2cast helper then it too is using the system installed libfaad2. It's unclear what version of libfaad2 squeeze2cast incorporates into the static build? Do you see the same behaviour with both versions?

philippe44 commented 1 year ago

No in fact since version 2.x I always include codecs. I had too many issues with different install

ralph-irving commented 1 year ago

Thanks for the correction. What version of libfaad2 do you use in squeeze2cast?

jtbr commented 1 year ago

I use the static version of squeeze2cast in any case (I think ubuntu has a too-new version of libssl for shared libs to work)

Using the build you linked to, I get:

[19:02:14.856375] faad_decode:427 error: 21 Unexpected channel configuration change
[19:02:14.856431] faad_decode:466 unable to decode further
[19:02:14.856446] decode_thread:100 decode error

I see register_faad:663 using faad to decode aac but no load_faad. I guess it's the static version.

Here is another example file, thought it might have been why you see a different error (I normally have tested with this one), but I get the same error either way.

philippe44 commented 1 year ago

Thanks for the correction. What version of libfaad2 do you use in squeeze2cast?

Still the 2.7 version. Where do you take it from these days? here?

ralph-irving commented 1 year ago

No. I only use my faad2 repository https://github.com/ralph-irving/faad2.git