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

[Diego Biurrun]

On Tue, Nov 24, 2009 at 01:15:06AM -0800, Michael Tison wrote:
> 
> Revised patch attached.
> 
> >> +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.

What did you review on what basis?  You're certainly not the first
person to claim your code conforms to a certain style when it does not
(hint: K&R has spaces after keywords).  I'd like to understand how this
happens.  I keep repeating the same things over and over.

> --- Changelog	(revision 20597)
> +++ Changelog	(working copy)
> @@ -43,9 +43,9 @@
>  - RF64 support in WAV demuxer
>  - MPEG-4 Audio Lossless Coding (ALS) decoder
>  - -formats option split into -formats, -codecs, -bsfs, and -protocols
> +- CD+G decoder and demuxer
>  
>  
> -
>  version 0.5:

The empty line was intentional, keep it.

> --- libavcodec/cdgraphics.c	(revision 0)
> +++ libavcodec/cdgraphics.c	(revision 0)
> @@ -0,0 +1,336 @@
> +
> +/// Default screen sizes
> +
> +/// Masks
> +
> +/// Instruction Codes
> +
> +/// Data sizes

nit: I would lowercase all these comments.

> +    cc->frame.buffer_hints = FF_BUFFER_HINTS_VALID |
> +        FF_BUFFER_HINTS_PRESERVE | FF_BUFFER_HINTS_REUSABLE;

This was more readable before.

> +        color = (cp->data[2*i]<<6) + (cp->data[2*i+1]&0x3F);
> +        r = (color>>8)&0x000F;
> +        g = (color>>4)&0x000F;
> +        b = (color   )&0x000F;
> +        r *= 17;
> +        g *= 17;
> +        b *= 17;
> +        palette[i+array_offset] = r<<16 | g<<8 | b;

Here and in other places: Spaces around operators would make this much
more readable.

> +    c0 = cp->data[0] & 0x0F;
> +    c1 = cp->data[1] & 0x0F;
> +    ri = (cp->data[2] & 0x1F) * 12;
> +    ci = (cp->data[3] & 0x3F) * 6;

align

> +    if(ri > (CDG_FULL_HEIGHT - TILE_HEIGHT)) return;
> +    if(ci > (CDG_FULL_WIDTH - TILE_WIDTH)) return;
> +
> +    v_scroll_pix = 0;
> +    if(vscmd == 2)    v_scroll_pix = -12;
> +    if(vscmd == 1)    v_scroll_pix =  12;
> +
> +    h_scroll_pix = 0;
> +    if(hscmd == 2)    h_scroll_pix = -6;
> +    if(hscmd == 1)    h_scroll_pix =  6;

Put statements on the next line.

Diego