[FFmpeg-devel] [PATCH] MTH demuxer (Gamecube format)

[Michael Niedermayer]

On Tue, Feb 03, 2009 at 02:58:28PM -0500, Michael Montanye wrote:
> Vitor Sessak wrote:
>
> [...]
>>> +static int mth_probe(AVProbeData *p)
>>> +{
>>> +    /* check file header */
>>> +    if (AV_RL32(p->buf) == MKTAG('M', 'T', 'H', 'P'))
>>> +        return AVPROBE_SCORE_MAX;
>>> +    else
>>> +        return 0;
>>> +}
>>>     
>>
>> This is a little false-positive prone (just four bytes). Something like 
>> the following is better
>>
>> if (AV_RL32(p->buf) != MKTAG('M', 'T', 'H', 'P'))
>>      return 0;
>>
>> if ({fps, width and weight are sane})
>>     return AVPROBE_SCORE_MAX;
>> else
>>     return AVPROBE_SCORE_MAX/3;
>>   
> Several other game-related formats already use this same four-byte check, 
> including the THP demuxer this was based on, so I'm not sure this is that 
> big a concern. (bethsoftvid, segafilm, smacker, and wc3movie also have the 
> same or similar checks, and there may be others I missed on the first pass)

checking more is always better if it can be easily done,not sure if
checking w/h for "sanity" is really such a sane idea though.


>
> [...]
>> Is there any reason to stop before EOF? If not, them you also don't need 
>> all the frame counting logic...
>>   
> Okay, that was mostly carryover from the THP demuxer, so it's all removed.
>> -Vitor
>>   
> The other fixes have been incorporated, and Diego's fixes are also added, 
> new patch attached.
>
> -Michael Montanye

[...]
[...]

> +    framerate            = av_d2q(fps, INT_MAX);
[...]
> +    st->codec->sample_rate  = av_q2d(framerate);

this code looks rather suboptimal


> +
> +    return 0;
> +}
> +
> +static int mth_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    MthDemuxContext *mth = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    int size;
> +    int ret;
> +

> +    url_fseek(pb, mth->next_frame, SEEK_SET);

unneeded


> +
> +    size = mth->next_framesz;
> +
> +    /* Locate the next frame and read out its size.  */
> +    mth->next_frame   += mth->next_framesz;
> +    mth->next_framesz  = get_be32(pb);
> +

> +    ret = av_get_packet(pb, pkt, size);
> +    if (ret != size) {
> +        av_free_packet(pkt);
> +        return AVERROR(EIO);
> +    }

id say av_free_packet() should only be called if ret>=0


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090203/a7419bbe/attachment.pgp>