[MPlayer-dev-eng] Re: [Patch] Trying harder to detect MP3

[Alban Bedel]

Hi Michael Behrisch,

on Sun, 25 Jan 2004 20:17:28 +0100 you wrote:

> On Sun, Jan 25, 2004 at 01:40:39PM +0100, Alban Bedel wrote:
> > Hi Michael Behrisch,
> > 
> > on Sat, 24 Jan 2004 20:46:11 +0100 you wrote:
> > 
> > > On Sun, Dec 28, 2003 at 02:29:35PM +0100, Alban Bedel wrote:
> > > > Hi Michael Behrisch,
> > > > 
> > > > on Sat, 27 Dec 2003 21:14:32 +0100 you wrote:
> > > > 
> > > > > Hello,
> > > > > 
> > > > > this one incorporates code from mp3check in order
> > > > > to recognize mp3's with (at most 100 bytes of) junk at start.
> > > > > It works only with files (seekable streams).
> > > > 
> > > > The main idea is ok to me, but it's not implemented at the right
> > > > place. The function is cut into 2 parts. The first try to findout
> > > > the file type. The second one setup the demuxer stream, etc.
> > > > In the case of mp3 it's not so clear bcs the second part also
> > > > perform some extra test to ensure that the file is really an mp3.
> > > > So imho your extra check should go into the first part where it
> > > > currently only set frmt to MP3.
> > > 
> > > i thought it is very similar to the check, which already is there,
> > > looking for 5 consecutive valid headers, that's why I put it there
> > 
> > No. You misunderstood again i fear. Past the switch(frmt) the demuxer
> > type is know. The tests that follow are there to ensure that the file
> > really is an mp3. You must put your test in the first loop, where it
> > try to detect the file type !!!!
> 
> Why is this other check past the switch then? To detect invalid mp3's?

Yes, it was added later to prevent misdetection.

> How do you distinguish "invalid mp3" from "no mp3 at all"?

They don't have enouth consecutive valid frame headers. The current
test requiere 5 or 6 valid consecutive frame.
Distinguish ? How care ? What you have to care about is if it's playable
or not.

> The examples I had all passed the first test (maybe because there is
> additional code ripping off something at start?) but failed in the
> second.

This mean that you have a valid mp3 frame followed by junk or that some of
the junk is recognized as mp3 frame.

> I thought adding my quite expensive test in the late phase would be more
> acceptable, because then it does not have to be done on any input file.

I still read "Subject: Re: Re: [Patch] Trying harder to detect MP3".
So you want to try harder ? Then do so, but at the right place.
The current code just take the first frame. Extend it to do your check
that will skip over large chunk of junk, etc.
But don't remove the extra check. Detect how you want, but starting from
the first valid frame (at demuxers->movi_start) there must be 4 or 5
valid frame following, without junk in betwen.
 
> > Also you should not do this test for non-seekable stream as it was in
> > your first patch.
> 
> The funny thing is, that I've tested it with streams and it still works
> (even with -nocache) Is there some additional internal caching?

There's the internal stream buffer, but it's not that big (1 vcd sector).

> Why does the seeking for movie_start at the end of the opening 
> procedure not check for seekable?

It doens't really matter if it fail bcs the current detection code doesn't
skip that many frames. If you go too far away in non-seekeable stream it can
be a pb.

 
> > 
> > > I did it a little rework and resent the patch in the appendix
> > > Maybe it's clearer then why it simply replaces the check which
> > > was there. Also demuxer->movi_start is ok now.
> > 
> > Don't remove that without extended testing !!!!!
> > 
> 
> I did not remove, I built upon.

No you changed it's purpose and there were alredy enouth pb bcs of
too lax mp3 detection.
Anyway your test is broken imho. This call to mp_get_mp3_header() at
the end of loop where you don't even check return value doesn't make
sense. This will just add potential bugs. At least test the return
value before assuming that the samplerate,channels it gave is valid.

Imho a better test would be to simply find the 2 first consecutive
frames with valid length (ie a valid header is always found right
after the frame, so that would check 3 header) and with the same
samplerate,channels.
This should be done in the first loop. It may be needed to increase the
loop length (so it search more data).

Then when this is working well and have been extendly tested (ie was at
least some weeks in cvs without any user complaints) then we can think
about carefully shorten (or remove) the "invalid mp3" test.

	Albeu
-- 

Everything is controlled by a small evil group
to which, unfortunately, no one we know belongs.