[FFmpeg-devel] [PATCH] add tag/comment support to the raw flac demuxer

[Reimar Döffinger]

On Mon, Dec 01, 2008 at 02:18:02PM -0800, Jim Radford wrote:
> +        unsigned char header[4];

uint8_t

> +        if ((ret = get_buffer(s->pb, header, sizeof(header))) <= 0)
> +            return AVERROR(EIO);
> +        if (ret != sizeof(header))
> +            return AVERROR(EINVAL);

I wouldn't consider differentiating between these worth the code. Also
ret < sizeof(header) can be due to an IO-error, too (depending on your
definition it might even always be one).

> +        init_get_bits(&gb, header, ret*8);
> +        flac->metadata_done = get_bits(&gb, 1);
> +        type = get_bits(&gb, 7);
> +        length = get_bits(&gb, 24);

Seems very much like overkill to use get_bits here. Actually get_be32
to me seems much more appropriate than get_buffer to get the header anyway.

> +        if (av_new_packet(pkt, sizeof(header) + length) < 0)
> +            return AVERROR(EIO);

Certainly not an EIO?

> +        ret = get_buffer(s->pb, pkt->data + sizeof(header), length);
> +        if (ret < 0)
> +            return AVERROR(EIO);
> +        if (ret != length)
> +            return AVERROR(EINVAL);

Also this is inconsistent to above where you had the function call
inside the if (which I consider much uglier).

> +        pkt->size = ret += sizeof(header);

"length" instead of "ret" seems more self-explanatory to me, also I see
no reason to increment "ret", it is never used below,,,
No, it did not comment on the actual "content" of the patch yet, I have
no clue about flac, I just disabled the MPlayer metadata reading code
for it because someone tried to reach the one-exploint-per-line-of-code
goal with it...

Greetings,
Reimar D?ffinger