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

Michael Tison blackspike
Tue Dec 1 09:13:24 CET 2009


Hi,

Attached is another revised patch.

On Tue, Nov 24, 2009 at 8:52 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Tue, Nov 24, 2009 at 01:15:06AM -0800, Michael Tison wrote:
>> +static void cdg_border_preset(CDGraphicsContext *cc, CdgPacket *cp)
>> +{
>> + ? ?int color;
>> + ? ?int repeat;
>> + ? ?int i, j;
>> + ? ?int lsize ? ?= cc->frame.linesize[0];
>> + ? ?uint8_t *buf = cc->frame.data[0];
>> +
>> + ? ?color ?= cp->data[0] & 0x0F;
>> + ? ?repeat = cp->data[1] & 0x0F;
>> +
>> + ? ?if(!repeat) {
>
>> + ? ? ? ?for(i = 0; i < CDG_FULL_WIDTH; i++) {
>> + ? ? ? ? ? ?for(j = 0; j < CDG_BORDER_HEIGHT; j++)
>> + ? ? ? ? ? ? ? ?buf[i + lsize * j] = color;
>> + ? ? ? ? ? ?for(j = CDG_FULL_HEIGHT - CDG_BORDER_HEIGHT; j < CDG_FULL_HEIGHT; j++)
>> + ? ? ? ? ? ? ? ?buf[i + lsize * j] = color;
>> + ? ? ? ?}
>
> painting along vertical columns is inefficient

Redone.

>> +
>> + ? ? ? ?for(j = 12; j < CDG_FULL_HEIGHT - CDG_BORDER_HEIGHT; j++) {
>
> you mix literals with named constants

Unmixed.

> [...]
>> + ? ? ? ? ? ?if(b) {
>> + ? ? ? ? ? ? ? ?if(!pix)
>> + ? ? ? ? ? ? ? ? ? ?xor_color = c0;
>> + ? ? ? ? ? ? ? ?else
>> + ? ? ? ? ? ? ? ? ? ?xor_color = c1;
>> +
>> + ? ? ? ? ? ? ? ?cur_color = buf[ci + x + (lsize * (ri + y))];
>> + ? ? ? ? ? ? ? ?new_color = cur_color ^ xor_color;
>> + ? ? ? ? ? ?} else {
>> + ? ? ? ? ? ? ? ?if(!pix)
>> + ? ? ? ? ? ? ? ? ? ?new_color = c0;
>> + ? ? ? ? ? ? ? ?else
>> + ? ? ? ? ? ? ? ? ? ?new_color = c1;
>> + ? ? ? ? ? ?}
>> + ? ? ? ? ? ?buf[ci + x + (lsize * (ri + y))] = new_color;
>
>
> if(pix) color = c1;
> else ? ?color = c0;
> if(b)
> ? ?color ^= buf[ci + x + (lsize * (ri + y))];
> buf[ci + x + (lsize * (ri + y))] = color;

Replaced.

>> +static void cdg_scroll(CDGraphicsContext *cc, CdgPacket *cp, int roll_over)
>> +{
>
>> + ? ?uint8_t temp_surface[CDG_FULL_WIDTH][CDG_FULL_HEIGHT];
>
> some people dislike large arrays on the stack

Heaped.

>> + ? ?int color, h_scroll, v_scroll;
>> + ? ?int hscmd, h_off, vscmd, v_off;
>> + ? ?int v_scroll_pix, h_scroll_pix;
>> + ? ?int vinc, hinc, x, y;
>> + ? ?int lsize ? ?= cc->frame.linesize[0];
>> + ? ?uint8_t *buf = cc->frame.data[0];
>> +
>> + ? ?color = cp->data[0] & 0x0F;
>> + ? ?h_scroll = cp->data[1] & 0x3F;
>> + ? ?v_scroll = cp->data[2] & 0x3F;
>> +
>> + ? ?hscmd = (h_scroll & 0x30) >> 4;
>> + ? ?h_off = (h_scroll & 0x07);
>> + ? ?vscmd = (v_scroll & 0x30) >> 4;
>> + ? ?v_off = (v_scroll & 0x0F);
>> +
>
>> + ? ?h_off = h_off < 5 ? h_off : 5;
>> + ? ?v_off = v_off < 12 ? v_off : 12;
>
> FFMIN

Replaced.

>> +
>> + ? ?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;
>> +
>> + ? ?if(!h_scroll_pix && !v_scroll_pix)
>> + ? ? ? ?return;
>> +
>> + ? ?vinc = v_scroll_pix + CDG_FULL_HEIGHT;
>> + ? ?hinc = h_scroll_pix + CDG_FULL_WIDTH;
>> +
>> + ? ?for(y = 0; y < CDG_FULL_HEIGHT; y++) {
>> + ? ? ? ?for(x = 0; x < CDG_FULL_WIDTH; x++) {
>
>> + ? ? ? ? ? ?temp_surface[(x + hinc) % CDG_FULL_WIDTH][(y + vinc) % CDG_FULL_HEIGHT] =
>> + ? ? ? ? ? ? ? ?buf[x + y * lsize];
>
> modulo is a slow operation

Gone.

>> + ? ? ? ?}
>> + ? ?}
>> +
>> + ? ?if(!roll_over) {
>> + ? ? ? ?if(v_scroll_pix > 0)
>> + ? ? ? ? ? ?for(y = 0; y < v_scroll_pix; y++)
>> + ? ? ? ? ? ? ? ?for(x = 0; x < CDG_FULL_WIDTH; x++)
>> + ? ? ? ? ? ? ? ? ? ?temp_surface[x][y] = color;
>> + ? ? ? ?else if(v_scroll_pix < 0)
>> + ? ? ? ? ? ?for(y = CDG_FULL_HEIGHT + v_scroll_pix; y < CDG_FULL_HEIGHT; y++)
>> + ? ? ? ? ? ? ? ?for(x = 0; x < CDG_FULL_WIDTH; x++)
>> + ? ? ? ? ? ? ? ? ? ?temp_surface[x][y] = color;
>> +
>> + ? ? ? ?if(h_scroll_pix > 0)
>> + ? ? ? ? ? ?for(x = 0; x < h_scroll_pix; x++)
>> + ? ? ? ? ? ? ? ?for(y = 0; y < CDG_FULL_HEIGHT; y++)
>> + ? ? ? ? ? ? ? ? ? ?temp_surface[x][y] = color;
>> + ? ? ? ?else if(h_scroll_pix < 0)
>> + ? ? ? ? ? ?for(x = CDG_FULL_WIDTH + h_scroll_pix; x < CDG_FULL_WIDTH; x++)
>> + ? ? ? ? ? ? ? ?for(y = 0; y < CDG_FULL_HEIGHT; y++)
>> + ? ? ? ? ? ? ? ? ? ?temp_surface[x][y] = color;
>> + ? ?}
>> +
>
>> + ? ?for(y = 0; y < CDG_FULL_HEIGHT; y++)
>> + ? ? ? ?for(x = 0; x < CDG_FULL_WIDTH; x++)
>> + ? ? ? ? ? ?buf[x + y * lsize] = temp_surface[x][y];
>
> avoidable copy

Avoided.

> [...]
>> +static int read_header(AVFormatContext *s, AVFormatParameters *ap)
>> +{
>> + ? ?int64_t result;
>> + ? ?AVStream *vst;
>> + ? ?ByteIOContext *pb = s->pb;
>> +
>> + ? ?vst = av_new_stream(s, 0);
>> + ? ?if(!vst)
>> + ? ? ? ?return AVERROR(ENOMEM);
>> +
>
>> + ? ?vst->codec->width ? ? ?= CDG_FULL_WIDTH;
>> + ? ?vst->codec->height ? ? = CDG_FULL_HEIGHT;
>> + ? ?vst->codec->pix_fmt ? ?= PIX_FMT_PAL8;
>
> these belong in the decoder

Moved.

>> +static int read_packet(AVFormatContext *s, AVPacket *pkt)
>> +{
>> + ? ?int ret;
>> + ? ?ByteIOContext *pb = s->pb;
>> +
>> + ? ?ret = av_get_packet(pb, pkt, CDG_PACKET_SIZE);
>> + ? ?if(pb->eof_reached == 1)
>> + ? ? ? ?return -1;
>> +
>> + ? ?pkt->stream_index = 0;
>> + ? ?return 0;
>
> dont ignore ret

Acknowledged.

On Tue, Nov 24, 2009 at 1:31 AM, Diego Biurrun <diego at biurrun.de> wrote:
> On Tue, Nov 24, 2009 at 01:15:06AM -0800, Michael Tison wrote:
>> >> +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.

Sorry! I casually glanced at wikipedia and thought it looked close enough.
Then I ran 'indent -kr' against the files and found it wasn't even close.
Fixed.

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

Kept.

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

Lowercased.

>> +    cc->frame.buffer_hints = FF_BUFFER_HINTS_VALID |
>> +        FF_BUFFER_HINTS_PRESERVE | FF_BUFFER_HINTS_REUSABLE;
>
> This was more readable before.

Undone.

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

Spaced.

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

Aligned.

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

Done.

Thanks for the feedback again!

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



More information about the ffmpeg-devel mailing list