[FFmpeg-devel] [PATCH] Bitmap Brothers JV demuxer

[Kostya]

On Wed, Mar 09, 2011 at 09:45:41PM +1100, Peter Ross wrote:
> ---
> 
> On Tue, Mar 08, 2011 at 04:35:54PM +0100, Kostya wrote:
> > On Wed, Mar 09, 2011 at 02:10:50AM +1100, Peter Ross wrote:
> [..]
> > > +static int read_probe(AVProbeData *pd)
> > > +{
> > > +    if (pd->buf[0] == 'J' && pd->buf[1] == 'V' &&
> > > +        !memcmp(pd->buf + 4, MAGIC, FFMIN(strlen(MAGIC), pd->buf_size - 4)))
> > > +        return AVPROBE_SCORE_MAX;
> > > +    return 0;
> > > +}
> >
> > I think it's wrong to return score_max if you have too small probe buffer not
> > to fit all magic string.
> 
> Isn't buf_size guaranteed to be at least 32-bytes? So in the worst case, the
> memcmp will compare 28 bytes, which is still a strong enough indicator IMHO.

indeed
 
> > [...]
> >
> > BTW, do you initialise state explicitly somewhere?
> 
> JvContext.state defaults to zero when priv_data is allocated. In this updated
> patch, i've forced the JV_AUDIO state enum to be zero. That way theres no
> need to specifically initialise it in decode_init().

I guessed that but for AVCodecs I think it's better to put that explicitly -
makes people wonder less from what state it starts.

> > > +            avio_skip(pb, e->size - jvf->audio_size - jvf->video_size - (jvf->palette ? 768 : 0));
> >
> > Is that guaranteed to be always non-negative?
> 
> Noo! Nicely spotted. While all known JV samples have >=0 padding bytes, a fuzzed
> file could cause a endless loop here, so its best that to clamp the range.

indeed
 
> >
> > Nice state machine you have here though.
> >
> > [the rest rises no comments from me]
> 
> Thanks for the prompt review.

Well, someone should review those game codecs anyway.

[...]
> diff --git a/libavformat/jvdec.c b/libavformat/jvdec.c
> new file mode 100644
> index 0000000..d804ed9
> --- /dev/null
> +++ b/libavformat/jvdec.c
[...]
> +
> +static int read_packet(AVFormatContext *s,
> +                       AVPacket *pkt)

nit: this line has no need to be broken into two

[...]
> +        case JV_PADDING:
> +            avio_skip(pb, FFMAX(e->size - jvf->audio_size - jvf->video_size - (jvf->palette ? 768 : 0), 0));

...but this one probably should be