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

[Vitor Sessak]

Michael Tison wrote:
> Hi,
> 
> Attached is another revised patch.

A few comments:

> +static av_cold int cdg_decode_init(AVCodecContext *avctx)
> +{
> +    CDGraphicsContext *cc = avctx->priv_data;
> +
> +    cdg_init_frame(&cc->frame);
> +
> +    cc->hscroll = 0;
> +    cc->vscroll = 0;

Unneeded, cc is already memset to 0 before calling decode_init().

> +
> +static void cdg_load_color_table(CDGraphicsContext *cc, CdgPacket *cp,
> +                                 int low)
> +{

I think a s/color.table/palette/ would be closer to usual multimedia 
terminology.

> +static void cdg_fill_rect_buf(int out_tl_x, int out_tl_y,
> +                              uint8_t *out,
> +                              int in_tl_x, int in_tl_y,
> +                              uint8_t *in, int w, int h, int lsize)
> +{
> +    int x, y;
> +    in = in + in_tl_x + in_tl_y * lsize;
> +    out = out + out_tl_x + out_tl_y * lsize;
> +    for (y = 0; y < h; y++)
> +       for (x = 0; x < w; x++)
> +           out[x + y * lsize] = in[x + y * lsize];
> +}
> +

The inner loop is a memcpy().

> +static void cdg_fill_rect_preset(int tl_x, int tl_y,
> +                                 uint8_t *out, int color,
> +                                 int w, int h, int lsize)
> +{
> +    int x, y;
> +
> +    for (y = tl_y; y < tl_y + h; y++)
> +       for (x = tl_x; x < tl_x + w; x++)
> +           out[x + y * lsize] = color;
> +}

The inner loop is a memset().

> +static void cdg_scroll(CDGraphicsContext *cc, CdgPacket *cp,
> +                      AVFrame *new_frame, int roll_over)
> +{
> +    int color;
> +    int hscmd, h_off, vscmd, v_off, dh_off, dv_off;
> +    int vinc = 0, hinc = 0, x, y;
> +    int ai;
> +    int lsize    = cc->frame.linesize[0];
> +    uint8_t *in  = cc->frame.data[0];
> +    uint8_t *out = new_frame->data[0];
> +
> +    assert(cc->frame.linesize[0] == new_frame->linesize[0]);
> +
> +    cdg_get_scroll_data(cp, &color, &hscmd, &h_off, &vscmd, &v_off);
> +
> +    /// find the difference and save the offset for cdg_tile_block usage
> +    dh_off = h_off - cc->hscroll;
> +    dv_off = v_off - cc->vscroll;
> +    cc->hscroll = h_off;
> +    cc->vscroll = v_off;
> +
> +    if (vscmd == UP)
> +       vinc = -12;
> +    if (vscmd == DOWN)
> +       vinc = 12;
> +    if (hscmd == LEFT)
> +       hinc = -6;
> +    if (hscmd == RIGHT)
> +       hinc = 6;
> +    vinc += dv_off;
> +    hinc += dh_off;
> +
> +    if (!hinc && !vinc)
> +       return;
> +
> +    memcpy(new_frame->data[1], cc->frame.data[1],
> +          CDG_COLOR_TABLE_SIZE * 4);
> +
> +    for (y = 0; y < CDG_FULL_HEIGHT; y++) {
> +       for (x = 0; x < lsize; x++) {
> +           ai = x + hinc + lsize * (y + vinc);
> +           if (ai >= 0 && ai < CDG_FULL_HEIGHT * lsize)
> +               out[ai] = in[x + y * lsize];
> +       }
> +    }

for (x = FFMAX(0, hinc); x < FFMIN(lsize, hinc); x++)
and similar for y and them there is no need for the slow branch in the 
inner loop.

> +static int cdg_decode_frame(AVCodecContext *avctx,
> +                            void *data, int *data_size, AVPacket *avpkt)
> +{
> +    const uint8_t *buf = avpkt->data;
> +    int buf_size = avpkt->size;
> +    AVFrame new_frame;
> +    CDGraphicsContext *cc = avctx->priv_data;
> +    CdgPacket cp;
> +
> +    if (avctx->reget_buffer(avctx, &cc->frame)) {
> +       av_log(avctx, AV_LOG_ERROR, "reget_buffer() failed\n");
> +       return -1;
> +    }
> +
> +    cp.command     = bytestream_get_byte(&buf);
> +    cp.instruction = bytestream_get_byte(&buf);
> +    bytestream_get_buffer(&buf, (uint8_t *) & cp.parityQ, 2);
> +    bytestream_get_buffer(&buf, (uint8_t *) & cp.data,   16);
> +    bytestream_get_buffer(&buf, (uint8_t *) & cp.parityP, 2);

Are those casts needed?

-Vitor