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

[Michael Behrisch]

> > > > 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.

My problem is that I don't understand why the check for enough consecutive
valid frame headers wasn't placed inside the first loop. But never mind,
I'll try to incorporate my check inside the loop.

> > 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.

Maybe my subject was wrong then. To me the extra check is also detection
(see above).

>  
> > > 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).
> 

Is there some global var telling me the size. Because most of my files with
junk at start are in fact dumped streams, I would very much like the patch
to at least try as much as it can even on streams. Thus I could simply stop 
seeking when I'm about to leave the buffer.


> > 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.

My code skips only 100 bytes more than the old one. This is already a pb?

> 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.

OK, I change that

> 
> 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.

Exactly that is what I am trying to do: Find the first 6 consecutive
valid headers which give the same samplerate, channels. Is my coding 
that ugly?


Yours,
Michael