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

[Paul B Mahol]

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.
>
>> +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.

Video codec make it kind of pointless.

It have key frame only when scene changes.