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

[Michael Niedermayer]

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

[...]
> +    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 ...


> +
> +
> +        /* 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

[...]


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121017/7fd08a75/attachment.asc>