[Ffmpeg-devel] [PATCH] C93 demuxer and decoder (GSoC qualification task)

[Anssi Hannula]

M?ns Rullg?rd wrote:
> Anssi Hannula <anssi.hannula at gmail.com> writes:
>> +    if (avctx->reget_buffer(avctx, newpic)) {
>> +        av_log(avctx, AV_LOG_ERROR, "reget_buffer() failed\n");
>> +        return -1;
>> +    }
>> +    if (avctx->reget_buffer(avctx, oldpic)) {
>> +        av_log(avctx, AV_LOG_ERROR, "reget_buffer() failed\n");
>> +        return -1;
>> +    }
>> +
>> +    stride = newpic->linesize[0];
>> +    out = newpic->data[0];
>> +
>> +    newpic->reference = 1;
>> +    if (buf[0] & 0x02) { /* first frame */
> 
> #define C93_FIRST_FRAME 2 or something and drop the comment.

OK.


>> +        }
>> +    }
>> +
>> +    bt1 = bt2 = 0;
>> +    for (x = y = 0; x < 320 || y < 184; x += 8) {
>> +        int offset, y_off, x_off, j, k, col0, col1;
>> +        int colbit = 0;
>> +        int cols[4];
>> +
>> +        if (x >= 320) {
>> +            y += 8;
>> +            x = 0;
>> +        }
>> +        if (!bt1) {
>> +            bt1 = buf[0] & 0x0F;
>> +            bt2 = (buf[0] >> 4) & 0x0F;
>> +            buf++;
>> +        }
>> +
>> +        switch (bt1) {
>> +        case C93_8X8_FROM_PREV:
>> +            offset = AV_RL16(buf);
>> +            buf += 2;
>> +            for (j = 0; j < 8; j++)
>> +                for (i = 0; i < 8; i++) {
>> +                    int loc = c93_conv_offset(offset, i, j, 8, stride);
> 
> I'm sure this could be calculated more efficiently than by calling
> that function for each iteration.

Will change.

>> +                    if (loc >= 192 * stride + 320) {
>> +                        av_log(avctx, AV_LOG_ERROR, "invalid offset %d for"
>> +                                " mode 2 at %dx%d\n", offset, x, y);
>> +                        return -1;
>> +                    }
>> +                    out[(y+j)*stride+x+i] = oldpic->data[0][loc];
>> +                }
> 
> Isn't this just another way of saying memcpy()?

Not if data pointed to by 'offset' is close to the right-side border of
frame, so precautions (c93_conv_offset()) have to be taken to ensure we
do not copy data from outside the frame area of oldpic. Should be made
more efficient, though.

> If it can't be
> removed, the outer for loop should also have braces.  There's enough
> stuff inside it that braces would make it easier to read.

Ok.

>> +            break;
>> +
>> +        case C93_4X4_FROM_PREV:
>> +            for (k = 0; k < 4; k++) {
>> +                y_off = k > 1 ? 4 : 0;
>> +                x_off = (k % 2) ? 4 : 0;
> 
> k & 1 or make k unsigned (or both).

OK.

>> +                offset = AV_RL16(buf);
>> +                if (offset >= 320 * 192) {
>> +                    return -1;
> 
> No reason?

Oops, I forgot to av_log the error (invalid offset).

Actually, the offset value should be checked more carefully here, so
that the check inside the loop below could be removed:

>> +                }
>> +                buf += 2;
>> +                for (j = 0; j < 4; j++)
>> +                    for (i = 0; i < 4; i++) {
>> +                        int loc = c93_conv_offset(offset, i, j, 4, stride);
>> +                        if (loc >= 192 * stride + 320) {
>> +                            av_log(avctx, AV_LOG_ERROR, "invalid offset %d for"
>> +                                    " mode 6 at %dx%d\n", offset, x, y);
>> +                            return -1;
>> +                        }
>> +                        out[(y+j+y_off)*stride+x+i+x_off] =
>> +                                                       oldpic->data[0][loc];
>> +                    }
>> +            }
>> +            break;
> 
> [...]
> 
> The other prediction modes are very similar.  Try to merge them into
> an inline function.  Constant arguments to an inline function allow
> the same optimizations as constants directly in the code.

I'll look into converting the similar ones to inline function(s).

>> +        case C93_NOOP:
>> +            break;
>> +
>> +        case C93_8X8_INTRA:
>> +            for (j = 0; j < 8; j++)
>> +                for (i = 0; i < 8; i++)
>> +                    out[(y+j)*stride+x+i] = (buf++)[0];
> 
> Replace the inner loop with a memcpy().

Right.


[...]
>> +            }
>> +        }
>> +    }
>> +    if (c93->current_frame >= br->frames) {
>> +        if (c93->current_block >= 511 || !(br+1)->length)
> 
> I prefer br[1].length.

Ok.

[...]
>> +        ret = get_buffer(pb, pkt->data + 1, palettesize);
>> +        if (ret < palettesize) {
>> +            av_free_packet(pkt);
>> +            return AVERROR_IO;
>> +        }
>> +    }
>> +    url_fskip(pb, -(palettesize + videosize + 2));
> 
> Somehow I don't like the look of that.

Indeed, see my reply to Reimar about a suggestion on how to remove it.




-- 
Anssi Hannula