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

[Reimar Döffinger]

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.

> +AVInputFormat ff_smush_demuxer = {
> +    .name           = "smush",
> +    .long_name      = NULL_IF_CONFIG_SMALL("LucasArts Smush"),
> +    .priv_data_size = sizeof(SMUSHContext),
> +    .read_probe     = smush_read_probe,
> +    .read_header    = smush_read_header,
> +    .read_packet    = smush_read_packet,

Please implement seeking via GENERIC_INDEX if somehow possible.