Re: [linux-minidisc] Implementing downloading in QHiMDTransfer

[Michael Karcher <Michael.Karcher@fu-berlin.de>]

On Sat, 2012-10-13 at 00:35 +0200, Kevin Chabowski wrote:
> I am currently trying to implement downloading tracks to HiMD for  
> QHiMDTransfer, I have some questions regarding this:
Thanks you very much for trying to implement it.

> Is there a way to tell if a file is actually an MP3 file or not, using  
> libmad (could not find anything in their "documentation", perhaps someone  
> here knows better)?
This is a technical problem insofar as there is no real MP3 file header
at all. A file that contains MP3 music consists of concatenated MPEG
audio data frames, and those indeed do have a header. There is no need
for the MP3 file to start with a valid MP3 frame (and indeed, if the
file is tagged with an ID3v2 tag, it does not do so), nor there is a
need with the last MP3 frame ending at the end of the file. (and again,
if an APE and/or ID3v1 tag is contained in that file, it does not do
so).

> I looked at the code of himdcli and could not find a  
> file type checking, so I just tried to transfer something else and it  
> "succeeded".
The error handling of himdcli currently in fact does not exceed the
works-for-me level of quality. If the current position of the stream
does not contain a valid MPEG audio frame header, mad_header_decode does
return an error code. This might be fine at the beginning of the file,
as having ID3v2 tags there is quite common. An error is also quite
common at the end of the file, as there might be an MP3 tag. As MPEG
audio is designed, it is mostly self-synchronizing, i.e. even if garbage
data is read from the stream, or some part of the stream is missing, the
decoder is able to find the next header with high reliability by just
guessing. So "loosing synchronization", as libmad calls it, i.e. having
no header at an expected position, is a *recoverable* error. This kind
of error is generally ignored.
What you end up by trying to upload a non-MP3-file is getting the parts
of that file that remotely look like valid MPEG data frames uploaded to
the player, and all other (i.e. most) parts of that file skipped. You
can easily find out the mess when you dump the mp3 track using himdcli
again.
Of course you are not the first person to have this request. A quick
google query resulted for example in this thread: 
  http://www.mars.org/pipermail/mad-dev/2004-April/001021.html

The basic idea of MP3 file validation in my oppinion is to try to sync
on the first frame (for which one might use code that is able to skip
well-known tags, but you don't have to), and then check that you don't
lose synchronisation for a certain number of frames. Be sure to not
consider a single call of mad_header_decode that succeeds to indicate
synchronisation - the header found might be spurious. At least require
two succeeding frames to be successfully read before concluding that
there the decoder locked.

> If there is no way, should I just check, if the file ends with '.mp3'?  
> Would not be a really good solution, but better than nothing IMHO.
In the GUI, I would not consider to offer the user to upload any file
not having a supported extension, i.e. only MP3 at the time being. A
basic MP3 sanity check as discussed above would be a good idea.

> I would need to recycle some code from himdcli. Simply copying and pasting  
> is obviously a terrible solution. Should I add higher level functions to  
> libhimd (seems to be low-level orientated) or introduce a new library for  
> that?
As long as you keep to plain basic C, as it is used in libhimd (and
himdcli), please feel free to move the code to libhimd. It is what
should be done in my oppinion anyway. The code might need some clean-up
before, though. As we have an himd_mp3stream, we should add an
himd_mp3writestream or possibly add functions to write mp3 streams using
the same himd_mp3stream object structure, if sensible.
For a first attempt at getting the stuff work, you could even move the
whole himd_writemp3 function into libhimd (and declaring the utility
functiosn of it static) and leave cleanup for a later time.

You probably are right that this function really is one layer too high
for libhimd. The abstraction layer libhimd currently provides is on the
layer of streams and frames, so opening the file, extracting ID3 tags
and splitting it into frames should not really go into libhimd, but
generating the TOC entry for the track index (including adding the
strings to the string table) would belong there. I do not want to put
the burden of redesigning the interface onto you, though, so doing the
GUI part first, and just moving the stuff from himdcli is the right
start.

> If the latter, someone else would need to integrate it into these  
> strange Qt Project files. I have no idea at all how these work...
No problems, we can help you with that.

Regards,
  Michael Karcher