[FFmpeg-devel] [PATCH] CD+G Demuxer & Decoder

[Michael Tison]

Hi,

Revised patch attached.

> That's obviously a paletted format, you should decode it as such and not convert it
> in the decoder.
Yeah, what I wrote before felt like a hack when writing it.
Fixed.

> And tabs are not allowed.
Gone.

>> +    result = url_fsize(pb);
>> +    if(result < 0)
>> +        return AVERROR_INVALIDDATA;
>
> I see no reason for this, CDG files should play fine when streamed, e.g. via stdin.
I think I got this one done (I just removed the whole if block).

>> +        ret = av_new_packet(pkt, CDG_PACKET_SIZE);
>> +        if(ret < 0)
>> +            return ret;
>> +
>> +        pkt->pos = url_ftell(pb);
>> +
>> +        ret = get_buffer(pb, pkt->data, CDG_PACKET_SIZE);
>> +        if(ret <= 0 || pb->eof_reached) {
>> +            av_free_packet(pkt);
>> +            return (pb->eof_reached == 1) ? -1 : ret;
>> +        } else {
>> +            av_shrink_packet(pkt, ret);
>> +            if(ret < 0)
>> +                return ret;
>
> av_get_packet handles all of this and in a more consistent way.
Replaced.

> This is missing documentation and changelog updates and you should bump
> library minor versions.  I keep repeating this for two out of three
> patches.  How can we make this more clear?
Added demuxer and decoder to general doc list, added line to
changelog, and bumped both minor versions.

>> +// Size of color code table
>> +#define COLOR_TABLE_SIZE            16
>
> pointless comment
Gone.

>> +#define CDG_INST_LOAD_COL_TBL_HIGH  31
>> +#define CDG_INST_TILE_BLOCK_XOR     38
>
> Could be right-aligned.
Aligned.

>> +static void cdg_scroll(CDGraphicsContext *cc, CdgPacket *cp, int roll_over);
>> +static void cdg_load_color_table(CDGraphicsContext *cc, CdgPacket *cp, int low);
>
> Avoid forward declarations.
Avoided.

>> +    cc->frame.buffer_hints = FF_BUFFER_HINTS_VALID |
>> +                             FF_BUFFER_HINTS_PRESERVE |
>> +                            FF_BUFFER_HINTS_REUSABLE;
>
> Indentation is off.
Done.

>> +static void cdg_write_frame(CDGraphicsContext *cc, uint8_t *dst, int h) {
>> +    int i, j, off;
>> +
>> +    for(j = 0; j < CDG_FULL_HEIGHT; j++) {
>> +       for(i = 0, off = 0; i < CDG_FULL_WIDTH; i++, off+=3) {
>> +           dst[j*cc->frame.linesize[0] + off] = cc->colortbl[cc->surface[i][j]] >> 16;
>> +           dst[j*cc->frame.linesize[0] + off+1] = cc->colortbl[cc->surface[i][j]] >> 8;
>> +           dst[j*cc->frame.linesize[0] + off+2] = cc->colortbl[cc->surface[i][j]];
>> +       }
>> +    }
>> +}
>> +
>> +static int cdg_decode_frame(AVCodecContext *avctx,
>> +                           void *data, int *data_size,
>> +                           AVPacket *avpkt)
>> +{
>
>
> You use completely inconsistent indentation and formatting.  Use K&R
> style with 4 space indentation.  This applies to all your code.
After reviewing it I only screwed up a couple of times on K&R, if I
missed anymore let me know.

>> +    cp.command = bytestream_get_byte(&buf);
>> +    cp.instruction = bytestream_get_byte(&buf);
>
> Here and in similar situations: Align the =.
Aligned.

>>  OBJS-$(CONFIG_CAVSVIDEO_DEMUXER)         += raw.o
>> +OBJS-$(CONFIG_CDG_DEMUXER)             += cdg.o
>>  OBJS-$(CONFIG_CRC_MUXER)                 += crcenc.o
>
> Lose the tabs.
Lost.

Thanks for the feedback!

Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cdgraphics.patch
Type: text/x-diff
Size: 17334 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091124/f752f259/attachment.patch>