[FFmpeg-devel] [PATCH] Pictor/PC Paint PIC decoder

[Reimar Döffinger]

> +OBJS-$(CONFIG_PIC_DECODER)             += picdec.o cga_data.o
> +    REGISTER_DECODER (PIC, pic);
> +    CODEC_ID_PICTOR,

I don't really care, but wouldn't it be nicer to make the codec name match as well?

> +    int mask = (1 << bits_per_plane) - 1;
> +    int shift = *plane * bits_per_plane;
> +
> +    while (run > 0) {
> +        int j;
> +        for (j = 8-bits_per_plane; j >= 0; j -= bits_per_plane) {
> +            d = s->frame.data[0] + *y * s->frame.linesize[0];
> +            d[*x] |= ((value >> j) & mask) << shift;

I think you missed my point.
My suggestion was to do something like (could be simplified,
I don't remember the type of value right now etc.):

int shift = *plane * bits_per_plane;
int plane_mask = ((1 << bits_per_plane) - 1) << shift;
int values = value << shift;
while (run > 0) {
    int j;
    for (j = 8-bits_per_plane; j >= 0; j -= bits_per_plane) {
        d = s->frame.data[0] + *y * s->frame.linesize[0];
        d[*x] |= (value >> j) & mask;

> +    bits_per_plane    = *buf & 0xF;
> +    if (bits_per_plane > 8) {
> +        av_log_ask_for_sample(s, "unsupported bit depth\n");
> +        return AVERROR_INVALIDDATA;

== 0 is hardly any better either.

> +    s->nb_planes      = (*buf++ >> 4) + 1;
> +    bpp               = s->nb_planes ? bits_per_plane*s->nb_planes : bits_per_plane;

bpp > 32 won't work either.

> +        while (buf_end - buf >= 6) {
> +            const uint8_t *buf_pend = buf + FFMIN(AV_RL16(buf), buf_end - buf);
> +            int marker = buf[4];
> +            buf += 5;

Documenting the meaning of buf[2] and buf[3] wouldn't be bad though.