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

Reimar Döffinger Reimar.Doeffinger
Thu Dec 31 11:39:17 CET 2009


On Wed, Dec 30, 2009 at 08:47:00AM +0530, Mohamed Naufal wrote:
> +    if (AV_RB16(probe_packet->buf) == AV_RB16("YO")  &&
> +        probe_packet->buf[6]                         &&
> +        probe_packet->buf[7]                         &&
> +        !(AV_RL16(probe_packet->buf + 8) & 1)        &&
> +        !(AV_RL16(probe_packet->buf + 10) & 1))

It's rather pointless to read 16 bits when you only use the value
of a single bit.

> +    static int ret;

static is not acceptable, that would result in complete chaos when an
application decodes two yop files at the same time.

> +    if (yop->video_packet.data) {
> +        *pkt = yop->video_packet;
> +        av_destruct_packet_nofree(&yop->video_packet);
> +        pkt->stream_index = 1;

Why not set the stream index to 1 for yop->video_packet and be done with
it?

> +        return palette_size + ret;

Why not just pkt->size?

> +    ret = av_new_packet(&yop->video_packet,
> +                        yop->frame_size - yop->audio_block_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) {
> +        av_free_packet(&yop->video_packet);
> +        return AVERROR_EOF;
> +    }

You need to free the packet in both cases since you already allocated
it.
Of course this would all be much easier if we had the av_append_packet I
proposed but some people seem to prefer duplicating this kind of code
despite the fact that it's done wrong almost every time.

> +    ret = av_get_packet(pb, pkt, 920);

Note that this will set pkt.pos to the start of the audio data, but your
demuxer can not start demuxing from that position, thus your demuxer
can not support seeking via AVFMT_GENERIC_INDEX - which it IMO should
though.

> +    ret = get_buffer(pb, yop->video_packet.data + palette_size,
> +                     actual_video_data_size);
> +    if (ret < 0)
> +        return ret;

Missing av_shrink_packet for ret < actual_video_data_size.

> +    if(yop->video_packet.data)
> +        av_free_packet(&yop->video_packet);

I think the if should not be necessary.



More information about the ffmpeg-devel mailing list