[FFmpeg-devel] Fwd: [PATCH] Psygnosis YOP demuxer

[Reimar Döffinger]

On Sat, Dec 26, 2009 at 11:22:15PM +0530, Mohamed Naufal wrote:
> +    AVPacket sound_packet;

I'd suggest to replace "sound" with "audio" everywhere to be more
consistent with the rest of FFmpeg.
Also you always immediately return the audio packet, so I don't see
why you'd need this at all.

> +    video_dec->pix_fmt      = PIX_FMT_RGB24;

I don't think you should set this.

> +    if ((ret = get_buffer(pb, video_dec->extradata, 8)) != 8)
> +        return -1;

return ret < 0 ? ret : AVERROR_EOF;
or so.
Also personally I find assignments inside ifs exceedingly bad for
readability without any advantage, but that might just be me.

> +    yop->stream ^= 1;

I think it might be better to do this without extra state, e.g. just
check if video_packet.buffer is != NULL?

> +    if (!yop->stream) {
> +        ret = av_new_packet(&yop->video_packet,
> +                            yop->frame_size - yop->sound_chunk_length);
> +        if (ret < 0)
> +            return ret;
> +
> +        yop->video_packet.pos = url_ftell(pb);
> +        ret = get_buffer(pb, yop->video_packet.data, palette_size);
> +        if (ret < 0) {
> +            return ret;
> +        } else if (ret < palette_size) {
> +            yop_free_packets(yop);
> +            return AVERROR_EOF;
> +        }

Please use "goto err_out" instead of duplicating the freeing code.

> +        // 1840 samples per frame, 1 nibble per sample; hence 1840/2 = 920
> +        ret = av_get_packet(pb, &yop->sound_packet, 920);
> +        if (ret < 920) {
> +            yop_free_packets(yop);
> +            return AVERROR_EOF;
> +        }

Please return partial data where ever it makes even remotely sense.
The decoder should decide what to do with incomplete/broken data,
not the demuxer (since the demuxer just can't do anything sensible
about it).

> +        url_fskip(pb, yop->sound_chunk_length - 920);
> +
> +        ret = get_buffer(pb, yop->video_packet.data + palette_size,
> +                         actual_video_data_size);
> +
> +        if (ret < 0) {
> +            return ret;
> +        } else if (ret < actual_video_data_size) {
> +            yop_free_packets(yop);
> +            return AVERROR_EOF;
> +        }
> +
> +        // arbitrarily return the sound data first
> +        *pkt = yop->sound_packet;
> +        return yop->sound_chunk_length;
> +    } else {
> +        *pkt = yop->video_packet;
> +        pkt->stream_index = 1;
> +        return yop->frame_size - yop->sound_chunk_length;
> +    }

I think this would be more readable as
if (yop->stream) {
       // return buffered packet
       *pkt = yop->video_packet;
       pkt->stream_index = 1;
       return yop->frame_size - yop->sound_chunk_length;
}
remaining code without "else", saving one indentation level.