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

[Diego Biurrun]

On Sun, Nov 22, 2009 at 12:56:04PM -0800, Michael Tison wrote:
> Sorry, I had trailing whitespace and tabs in that last patch. Revised
> patch is attached.

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?

> --- libavcodec/cdgraphics.c	(revision 0)
> +++ libavcodec/cdgraphics.c	(revision 0)
> @@ -0,0 +1,356 @@
> +
> +// Default screen sizes
> +#define CDG_FULL_WIDTH              300
> +#define CDG_FULL_HEIGHT             216
> +#define CDG_DISPLAY_WIDTH           294
> +#define CDG_DISPLAY_HEIGHT          204
> +#define CDG_BORDER_WIDTH            6
> +#define CDG_BORDER_HEIGHT           12
> +
> +// Size of color code table
> +#define COLOR_TABLE_SIZE            16

pointless comment

> +// Instruction Codes
> +#define CDG_INST_MEMORY_PRESET      1
> +#define CDG_INST_BORDER_PRESET      2
> +#define CDG_INST_TILE_BLOCK         6
> +#define CDG_INST_SCROLL_PRESET      20
> +#define CDG_INST_SCROLL_COPY        24
> +#define CDG_INST_LOAD_COL_TBL_LO    30
> +#define CDG_INST_LOAD_COL_TBL_HIGH  31
> +#define CDG_INST_TILE_BLOCK_XOR     38

Could be right-aligned.

> +static void cdg_memory_preset(CDGraphicsContext *cc, CdgPacket *cp);
> +static void cdg_border_preset(CDGraphicsContext *cc, CdgPacket *cp);
> +static void cdg_tile_block(CDGraphicsContext *cc, CdgPacket *cp, int b);
> +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.

> +static av_cold int cdg_decode_init(AVCodecContext *avctx)
> +{
> +    CDGraphicsContext *cc = avctx->priv_data;
> +
> +    avcodec_get_frame_defaults(&cc->frame);
> +    cc->frame.reference = 1;
> +    cc->frame.buffer_hints = FF_BUFFER_HINTS_VALID |
> +                             FF_BUFFER_HINTS_PRESERVE |
> +                            FF_BUFFER_HINTS_REUSABLE;

Indentation is off.

> +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.

> +    cp.command = bytestream_get_byte(&buf);
> +    cp.instruction = bytestream_get_byte(&buf);

Here and in similar situations: Align the =.

> --- libavformat/cdg.c	(revision 0)
> +++ libavformat/cdg.c	(revision 0)
> @@ -0,0 +1,91 @@
> --- libavformat/Makefile	(revision 20578)
> +++ libavformat/Makefile	(working copy)
> @@ -44,6 +44,7 @@
>  OBJS-$(CONFIG_CAVSVIDEO_DEMUXER)         += raw.o
> +OBJS-$(CONFIG_CDG_DEMUXER)		  += cdg.o
>  OBJS-$(CONFIG_CRC_MUXER)                 += crcenc.o

Lose the tabs.

Diego