#113 failure to return error code for bogus header

1.9.0
open
nobody
mpg123 (103)
5
2012-12-12
2009-09-15
No

Hiya,

We use some bogus 'mp3' files to test error handling in libraries we use and in our own code. Prior versions of mpg123 (1.7.0...) did not notice the problem, but returned 0 for the samplerate from mpg123_getformat(). Now, in version 1.9.0, error messages get logged to stderr, and mpg123_getformat() and mpg123_info() report the same, non-zero samplerate, but MPG123_OK is returned from all functions, and the information from mpg123_info() isn't self consistent.

Once an error has been noticed with a file-based handle, mpg123 should never return MPG123_OK from functions that are handed the bogus handle.

Here's the stderr messages:
Note: Illegal Audio-MPEG-Header 0x00060009 at offset 0xda3.
Note: Trying to resync...
Note: Skipped 129 bytes in input.

The file in question is too big to attach to this defect report. You can find it here:
http://hltcoe.hodain.net/problem.mp3

-Hugh

Discussion

  • Thomas Orgis
    Thomas Orgis
    2009-09-15

    No time yet to deep-investigate, but there is room for interpretation on how some garbage in between is to be handled.
    mpg123's stance is that when the resync process suceeds, MPG123_OK is justified.

    But, I guess I need to have a closer look to determine if there is any success ... did you actually put a "good" frame in there or only distorted information?

     
  • Heh, the thing that brought this to my attention is that the mpg123 behavior vis-a-vis the reported samplerate changed from 1.7 to 1.9..., so now our tests fail for one of these versions.

    I didn't understand the resync behavior.... Having the API expose clear, and perhaps parametrically controlled, semantics for what it does with bogus headers would be a nice thing. At the very least, it would let me use mpg123 to discover which files have bogus headers/frames.

    I don't have the provenance of this file; it won't play in QuickTime which I believe is what brought it to my attention -- heh heh, which gets back to the point that it would be nice to be able to use mpg123 to dissect it enough to answer your question about a "good" frame or distorted info!

    -Hugh

     
  • Thomas Orgis
    Thomas Orgis
    2009-09-16

    Well, the default of mpg123 has always been to try to work with damaged files ("damage" could always mean some funky additions like strange tags, album art, RIFF headers...). Again, I need to find some time (probably not during the week) to investigate the file closer.
    What I can see now is that mplayer seems to play it (mplayer uses some derived mpg123 code for that)... and I suspect the addition of free format support....

    OK, I got suspicious when mplayer played the file... as plain uncompressed pcm!
    What you have there is a RIFF wave file with a rather exotic MPEG header prepended. MPlayer for some reason does what mpg123 does in attempt to find valid MPEG data, in reverse: It skips the first bytes before the RIFF header.

    Per default, mpg123 tries hard to find MPEG data, in fact, also by skipping a leading RIFF header (since that apparently happened in the past, too). OK, so now we got the reversed case. If you want to check for error-free MPEG files, I suggest using MPG123_NO_RESYNC (the --no-resync command line flag).

    All clear now?

     
  • When I change our use of the library to set the parameter flag MPG123_NO_RESYNC, the message that's logged to stderr changes, but the mpg123 functions still all return MPG123_OK. Here's the new error message that goes to stderr:
    Note: Illegal Audio-MPEG-Header 0x00060009 at offset 0xda3.
    [parse.c:658] error: not attempting to resync...

    I believe it is a defect. But if not, what can I do in order to have the mpg123 library let my library know that the file is bogus? (Parsing stderr is not an option.)

     
  • Thomas Orgis
    Thomas Orgis
    2009-09-18

    Hm, OK. You get MPG123_DONE, right? Seems like we need to define another flag that should treat any stream error as hard error. Currently, we have things in mind like a trailing BMP album art (seen that!) that should not trigger failure of the decoder. Mpg123 is coded to be tolerant and giving...
    But I see, that for stream checking a strict mode might be good, that one should also include missing bit reservoir parts, anything that's wrong... right?

     
  • Thomas Orgis
    Thomas Orgis
    2012-03-07

    Just wanna note that I stumbled over this again. We didn't really come to a conclusion... I got another idea: Just store a stream error counter in the handle. As mpg123 is forgiving, it could still try hard to find valid data and ignore junk, resulting in MPG123_OK, but one could query afterwards if there were errors in the stream that got glossed over.

    Would that work for you? (over two years later...)