[FFmpeg-devel] [HACK] mov.c: Handle MPEG-in-MOV (#241 and friends)

[Petter Ericson]

On Wed, Oct 17, 2012 at 05:15:20PM +0200, Michael Niedermayer wrote:
> On Wed, Oct 17, 2012 at 05:04:07PM +0200, Petter Ericson wrote:
> > On Wed, Oct 17, 2012 at 02:35:07AM +0200, Michael Niedermayer wrote:
> > > On Tue, Oct 16, 2012 at 09:33:24PM +0200, Petter Ericson wrote:
> > > > On Tue, Oct 16, 2012 at 06:43:50PM +0200, Hendrik Leppkes wrote:
> > > > > On Tue, Oct 16, 2012 at 5:18 PM, Petter Ericson
> > > > > <petter.ericson at codemill.se> wrote:
> > > > > > Greetings,
> > > > > >
> > > > > > Attached is a patch that handles the files provided in #241 and duplicates.
> > > > > > Needless to say, it is extremely hacky and would benefit from improvements,
> > > > > > but as it stands, it checks for the two places I have found the m1s tag, and
> > > > > > forces continued probing until mpegps_probe gets enough data to find its
> > > > > > own header. I considered using the mpegps demuxer as a "subprocess" simliar
> > > > > > to how DVDemuxer is used, but this proved to be too much trouble for me,
> > > > > > given that it really isn't made to work in that way, and the above
> > > > > > approach got the desired result.
> > > > > >
> > > > > > Improvements (rudimentary MOV parsing in the probe?) very welcome.
> > > > > >
> > > > > 
> > > > > The fixed offset of the tag in the file seems rather arbitrary to me.
> > > > > Did you confirm on multiple samples that its in that exact spot every
> > > > > time?
> > > > 
> > > > I checked two - my own sample and the one in the original ticket. They both
> > > > had the first offset (0x21E). A different duplicate (#600 something) had the
> > > > smaller offset due to missing the initial ftyp section before the moov atom.
> > > > 
> > > > I agree that something better should be found - a fixed offset from the
> > > > moov atom could probably work better, but that again takes us into 
> > > > rudimentary parsing territory. One might refactor the switch/case to use
> > > > score = MAX(returnvalue, score) or something like that, which would at least
> > > > allow slightly more flexibility - I'll cook something up tomorrow and
> > > > resubmit unless someone gets there before me.
> > > 
> > > if you dont want to write a simple parser. One could simply scan the
> > > first X bytes for a valid looking stsd + m1v tag, that would avoid
> > > the fixed offset and checking 2 tags should make false positives less
> > > likely
> > > 
> > 
> > Attempt two: I've reworked the code to save the offset for the moov atom, and
> > use that as a baseline for finding the stsd, and doing some checks to see if
> > a) there's only one stream and b) it has format = "m1s ".
> > 
> > I had to do a little bit of magic in the switch/case to make it behave, so my
> > guess is that a sanity check there might be a good idea. Specifically, the
> > default case of skipping ahead the supposed atom size might benefit from 
> > something better (for checking subatoms or smth).
> > 
> > Also, no tabs.
> > 
> > Best
> > 
> > /P
> > 
> > -- 
> > Petter Ericson
> > Systems developer, Codemill AB
> > petter.ericson at codemill.se
> > 
> 
> >  mov.c |   51 ++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 44 insertions(+), 7 deletions(-)
> > 4d0c4705a7089fb1d1942401a6760f11bed63992  0001-mov.c.patch
> > From dfc0e6bbb80876080e7bb10648ab5a2efe3a6e6c Mon Sep 17 00:00:00 2001
> > From: Petter Ericson <petter.ericson at codemill.se>
> > Date: Wed, 17 Oct 2012 16:53:19 +0200
> > Subject: [PATCH] mov.c:     Check for stsd + m1s tag indicating MOV-wrapped
> >  MPEG-PS, and force 	continued probing if found.
> > 
> > ---
> >  libavformat/mov.c |   51 ++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 44 insertions(+), 7 deletions(-)
> > 
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index b817bc9..c354536 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -2861,43 +2861,80 @@ static int mov_probe(AVProbeData *p)
> >      unsigned int offset;
> >      uint32_t tag;
> >      int score = 0;
> > +    int stsd = MKTAG('s','t','s','d');
> > +    int moov_offset = -1;
> >  
> >      /* check file header */
> >      offset = 0;
> 
> > +#define MAX(a,b) a > b ? a : b
> 
> see FFMAX

Well would you look at that..

> 
> [...]
> > +    if(tag > AVPROBE_SCORE_MAX - 50 && moov_offset != -1){
> > +        /* We should make sure that this is not a MOV-packed MPEG-PS */
> > +        offset = moov_offset;
> > +        
> > +        while(offset < (p->buf_size - 20) && /* Sufficient space */
> > +              AV_RL32(p->buf+offset) != stsd)
> > +            offset++;
> 
> why do you start at the moov atom and not 0 ?
> it would be simpler ...

Mainly to skip if some other atom comes first.

> 
> 
> > +
> > +
> > +        /* We didn't find an stsd atom for some reason */
> > +        if(offset == (p->buf_size - 20))
> > +            return score;
> > +
> > +        /* Make sure there's only one stream */
> > +        if(AV_RB32(p->buf + offset + 8) != 1) 
> > +            return score;
> > +
> > +        /* Check for m1s tag */
> > +        if(AV_RL32(p->buf + offset + 16) != MKTAG('m','1','s',' '))
> > +            return score;
> > +
> > +        av_log(NULL, AV_LOG_WARNING, "Found m1s tag indicating this is a MOV-packed MPEG-PS.\n");
> > +
> > +        /* We found an stsd atom describing an MPEG-PS-in-MOV, return a
> > +         * zero score to force expanding the probe window until
> > +         * mpegps_probe finds what it needs */
> > +        return 0;
> 
> you can safetly return a small value like 5 here,this way if mpeg-ps
> fails it will still be detected as mov
> 

Fixed.

Modified patch attached.

/P

-- 
Petter Ericson
Systems developer, Codemill AB
petter.ericson at codemill.se

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mov.c.patch
Type: text/x-diff
Size: 3888 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121017/57d7304b/attachment.bin>