[FFmpeg-devel] [PATCH 3/3] LucasArts SMUSH demuxer

[Reimar Döffinger]

On Tue, Mar 27, 2012 at 07:55:51PM +0000, Paul B Mahol wrote:
> On 3/27/12, Reimar Doeffinger <Reimar.Doeffinger at gmx.de> wrote:
> > On Tue, Mar 27, 2012 at 05:28:45PM +0000, Paul B Mahol wrote:
> >> +    if (!smush->version) {
> >> +        int i;
> >> +
> >> +        vstream->codec->extradata = av_malloc(1024 + 2);
> >> +        if (!vstream->codec->extradata)
> >> +            return AVERROR(ENOMEM);
> >> +        vstream->codec->extradata_size = 1024 + 2;
> >> +        AV_WL16(vstream->codec->extradata, vinfo->subversion);
> >> +        for (i = 0; i < 256; i++)
> >> +            AV_WL32(vstream->codec->extradata + 2 + i * 4,
> >> vinfo->palette[i]);
> >> +    }
> >> +
> >> +    if (smush->version) {
> >
> > That should just be an else I think?
> >
> >> +        if ((ret = read_ainfo1(pb, &smush->ainfo)) < 0) {
> >> +            av_log(ctx, AV_LOG_ERROR, "Invalid audio information\n");
> >> +            return AVERROR_INVALIDDATA;
> >> +        } else if (ret) {
> >
> > Since you have a return you don't really need an else here, I find
> > it more readable to have error handling standing out clearly
> > separate.
> >
> >> +                if (av_get_packet(pb, pkt, size) != size)
> >> +                    return AVERROR(EIO);
> >
> > Why? Generally FFmpeg policy (admittedly mostly driven by me) is
> > to return also partial packets and let the decoder do the best
> > it can with them.
> > I.e. only fail with the result is < 0.
> > This is also relevant since I think you leak memory here when
> > returning when the result is not < 0.
> >
> >> +            if (av_get_packet(pb, pkt, size) != size)
> >> +                return AVERROR(EIO);
> >> +
> >> +            av_grow_packet(pkt, 2);
> >
> > Huh? You don't seem to initialize those extra 2 bytes.
> 
> av_grow_packet() initializes it to 0.

I checked once more, and no it does not. It only initializes the
(new) padding but none of the new data.