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

[Michael Behrisch]

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?
How do you distinguish "invalid mp3" from "no mp3 at all"?
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.

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.


> 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?
Why does the seeking for movie_start at the end of the opening 
procedure not check for seekable?


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

Michael