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

[Anssi Hannula]

Michael Niedermayer wrote:
> Hi

Hi!

> On Thu, Apr 05, 2007 at 07:19:19PM +0300, Anssi Hannula wrote:
>> Anssi Hannula wrote:
>>> As my Google Summer of Code qualification task, attached is a patch
>>> which adds C93 [1] demuxer and video decoder to FFmpeg. The samples from
>>> samples.mplayerhq.hu can be properly played out with it.
>>>
>>> I'm not too familiar with FFmpeg codebase yet, though, so there could be
>>> some silly mistakes in the code ;). Please review it.
>>>
>>> [1] http://wiki.multimedia.cx/index.php?title=C93
>> I had some time today, so I implemented the suggestions... Attached is a
>> new version of the patch.
>>
>> In addition to the suggested ones, I also made these changes:
>> - moved addition of audio streams to c93_read_packet() as some .c93
>> files do not have audio
>> - set time_base of AVStream instead of AVCodecContext's, and set
>> duration and start_time in AVStream, so that correct file duration is
>> reported
>> - always allocate the AVPacket with space for palette to avoid seeking
>> backwards
>> - fix palette endianness
>> - some cleaning of the non-predictive modes
> [...]
>> +static inline int c93_copy_block(AVCodecContext *avctx, uint8_t *to,
>> +        uint8_t *from, int offset, int height, int to_x, int to_y, int stride)
>> +{
>> +    int i;
>> +    int width = height;
>> +    int from_x = offset % WIDTH;
>> +    int from_y = offset / WIDTH;
>> +    int overflow = from_x + width - WIDTH;
>> +
>> +    if (!from) {
>> +        /* silently ignoring predictive blocks in first frame */
>> +        return 0;
>> +    }
>> +
>> +    if (overflow >= width || from_y + height > HEIGHT) {
> 
> how can overflow >= width happen?

... it can't, so dropping the test.

> 
>> +        av_log(avctx, AV_LOG_ERROR, "invalid offset %d during C93 decoding\n",
>> +               offset);
>> +        return -1;
>> +    }
>> +
>> +    if (overflow > 0) {
>> +        width -= overflow;
>> +        for (i = 0; i < height; i++) {
>> +            memcpy(&to[(to_y+i)*stride+to_x+width], &from[(from_y+i)*stride],
>> +                   overflow);
>> +        }
> 
> are you sure its not reading from the next line in case of overflow?

If by that you mean whether I'm sure that in case of overflow the
beginning of the same line should be read instead of the next one, then
yes, I checked a couple of places where the overflow happened and it
looked better when doing it like this.

> 
>> +    }
>> +
>> +    for (i = 0; i < height; i++) {
>> +        memcpy(&to[(to_y+i)*stride+to_x], &from[(from_y+i)*stride+from_x],
>> +               width);
>> +    }
> 
> the effects of to_y and to_x can not only be moved out of the loops but even
> out of this function

Right.

> [...]
>> +    if (*buf++ & C93_HAS_PALETTE) {
>> +        uint32_t *palette = (uint32_t *) newpic->data[1];
>> +        c93->palette_changed = 1;
>> +        for (i = 0; i < 256; i++) {
>> +            palette[i] = bytestream_get_be24(&buf);
>> +        }
>> +    } else {
>> +        buf += 256 * 3;
>> +        if (c93->palette_changed) {
>> +            memcpy(newpic->data[1], oldpic->data[1], 256 * 4);
>> +            c93->palette_changed = 0;
>> +        }
> 
> what is the palette_changed code good for?

So that the palette is only copied when it has really changed.
Is the overhead of copying so negligible that there is no need for that?

> 
> [...]
>> +                        for (i = 0; i < 4; i++)
>> +                            cols[i] = *buf++;
> 
> bytestream_get_buffer()

Ok, changed.

> 
> [...]
>> +    ret = av_new_packet(pkt, datasize + 768 + 1);
>> +    if (ret < 0)
>> +        return ret;
>> +    pkt->data[0] = 0;
>> +
>> +    ret = get_buffer(pb, pkt->data + 768 + 1, datasize);
>> +    if (ret < datasize) {
>> +        av_free_packet(pkt);
>> +        return AVERROR_IO;
>> +    }
>> +
>> +    datasize = get_le16(pb); /* palette size */
>> +    if (datasize) {
>> +        if (datasize != 768) {
>> +            av_log(s, AV_LOG_ERROR, "invalid palette size %u\n", datasize);
>> +            av_free_packet(pkt);
>> +            return AVERROR_INVALIDDATA;
>> +        }
>> +        pkt->data[0] |= C93_HAS_PALETTE;
>> +        ret = get_buffer(pb, pkt->data + 1, datasize);
>> +        if (ret < datasize) {
>> +            av_free_packet(pkt);
>> +            return AVERROR_IO;
>> +        }
>> +    }
> 
> if there is no palette pkt->size should be decreased by 768

Even when the 768 unused bytes are in the beginning of the pkt instead
of the end?

Of course, putting the palette in the end of pkt instead is quite simple
if that is needed.

-- 
Anssi Hannula