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

[Michael Niedermayer]

On Tue, Oct 16, 2012 at 05:18:01PM +0200, Petter Ericson 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.
> 
> Best
> 
> /P
> 
> -- 
> Petter Ericson
> Systems developer, Codemill AB
> petter.ericson at codemill.se
> 

>  mov.c |   24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> d92524ed4ec555f4389f8f13f2f37b4f87c1bf80  0001-mov.c.patch
> From f7e8135450ca4e7b7a17d7c70d49956b23909da9 Mon Sep 17 00:00:00 2001
> From: Petter Ericson <petter.ericson at codemill.se>
> Date: Mon, 15 Oct 2012 16:43:37 +0200
> Subject: [PATCH] mov.c:     
> * Add hacky check for specific "m1s " tag indicating a mov-encapsulated MPEG-PS stream.
> 
> ---
>  libavformat/mov.c |   24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index bf695e7..36a0708 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -2860,6 +2860,30 @@ static int mov_probe(AVProbeData *p)
>  
>      /* check file header */
>      offset = 0;
> +    /* Check if this is an MPEG-PS stream encapsulated in MOV 
> +     * We could conceptually handle this by resetting stuff in
> +     * mov_read_header somehow, or through refactoring the whole of
> +     * mpeg.[c,h] to store its internal data in MpegDemuxContext, and only
> +     * take that as its parameter for several _internal functions. However,
> +     * this is a quick-and-dirty solution that works for the two provided
> +     * samples. 
> +     */
> +    if(p->buf_size >= 0x222){
> +        tag = AV_RL32(p->buf + 0x21E);
> +        if(tag == MKTAG('m','1','s',' ')){
> +            av_log(NULL,AV_LOG_WARNING,"Found m1s tag indicating mpeg-in-mov. "
> +                            "Ignoring further signs of this being a mov file\n"); 
> +            return 0;
> +		}
> +        /* ftyp header is sometimes missing, placing the tag 0x20 bytes earlier */
> +        tag = AV_RL32(p->buf + 0x1FE);
> +        if(tag == MKTAG('m','1','s',' ')){
> +            av_log(NULL,AV_LOG_WARNING,"Found m1s tag indicating mpeg-in-mov. "
> +                            "Ignoring further signs of this being a mov file\n"); 
> +            return 0;
> +        }

hmm
i think the position should not be hardcoded this is very fragile
and the surrounding atom should be checked too, your patch just checks
31bits that means we would have 1 out of 2 billion valid mov files
that would be detected as mpeg ps while it actually where not
i think that failure rate while appearing small is too high

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- 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/20121016/5e29a062/attachment.asc>