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

[Michael Tison]

Hi,

Attached is another attempt.

On Tue, Dec 1, 2009 at 9:42 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Tue, Dec 01, 2009 at 12:13:24AM -0800, Michael Tison wrote:
>> + ? ?int color;
>> + ? ?int repeat;
>> +
>> + ? ?color = cp->data[0] & 0x0F;
>> + ? ?repeat = cp->data[1] & 0x0F;
>> + ? ?if (!repeat)
>> + ? ? ? memset(cc->frame.data[0], color,
>> + ? ? ? ? ? ? ?cc->frame.linesize[0] * CDG_FULL_HEIGHT);
>> +}
>> +
>> +static void cdg_border_preset(CDGraphicsContext *cc, CdgPacket *cp)
>> +{
>> + ? ?int color;
>> + ? ?int repeat;
>> + ? ?int y;
>> + ? ?int lsize ? ?= cc->frame.linesize[0];
>> + ? ?uint8_t *buf = cc->frame.data[0];
>> +
>> + ? ?color ?= cp->data[0] & 0x0F;
>> + ? ?repeat = cp->data[1] & 0x0F;
>
> looks duplicated

Separated and put into function.

>> +static void cdg_tile_block(CDGraphicsContext *cc, CdgPacket *cp, int b)
>> +{
>> + ? ?int c0, c1;
>> + ? ?int ci, ri;
>> + ? ?int byte, pix, color;
>> + ? ?int x, y;
>> + ? ?int ai;
>> + ? ?int lsize ? ?= cc->frame.linesize[0];
>> + ? ?uint8_t *buf = cc->frame.data[0];
>> +
>> + ? ?c0 = ?cp->data[0] & 0x0F;
>> + ? ?c1 = ?cp->data[1] & 0x0F;
>> + ? ?ri = (cp->data[2] & 0x1F) * CDG_TILE_HEIGHT;
>> + ? ?ci = (cp->data[3] & 0x3F) * CDG_TILE_WIDTH;
>> +
>> + ? ?if (ri > (CDG_FULL_HEIGHT - CDG_TILE_HEIGHT))
>> + ? ? ? return;
>> + ? ?if (ci > (CDG_FULL_WIDTH - CDG_TILE_WIDTH))
>> + ? ? ? return;
>> +
>> + ? ?for (y = 0; y < CDG_TILE_HEIGHT; y++) {
>> + ? ? ? byte = cp->data[4 + y] & 0x3F;
>> + ? ? ? for (x = 0; x < CDG_TILE_WIDTH; x++) {
>> + ? ? ? ? ? pix = (byte >> (5 - x)) & 0x01;
>> + ? ? ? ? ? ai = ci + x + cc->hscroll + (lsize * (ri + y + cc->vscroll));
>
>> + ? ? ? ? ? assert(ai >> 0 && ai < CDG_FULL_HEIGHT * lsize);
>
> is it certain that no random input can make this fail?

Double negative? Typo with the bitwise shift too :(
I just got rid of the assert and ensured that the index wouldn't go
out of bounds before the for loops.

> [..]
>> +static void cdg_fill_rect_buf(int out_tl_x, int out_tl_y,
>
> copy not fill

Replaced.

>> +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);
>
> parity is unused, use it or dont read it.

Gone.

> [...]
>> + ? ? ? *data_size = sizeof(AVFrame);
>> + ? ? ? *(AVFrame *) data = cc->frame;
>> + ? ? ? return buf_size;
>> + ? ?}
>> +
>> + ? ?*data_size = 0;
>> + ? ?*(AVFrame *) data = cc->frame;
>> + ? ?return 0;
>
> this looks like it can be done simpler

Simplified.

> [...]
>> +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;
>
> possible memleak, and wrong return code

Corrected I hope:
static int read_packet(AVFormatContext *s, AVPacket *pkt)
{
    int ret;
    ByteIOContext *pb = s->pb;

    ret = av_get_packet(pb, pkt, CDG_PACKET_SIZE);
    if (ret != CDG_PACKET_SIZE)
	return AVERROR(EIO);

    pkt->stream_index = 0;
    return 0;
}
I just took this return value from iss.c because I wasn't entirely
sure what to return.

On Tue, Dec 1, 2009 at 5:58 AM, Vitor Sessak <vitor1001 at gmail.com> wrote:
> Michael Tison wrote:
>> +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().

Removed.

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

Substituted.

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

Done.

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

Done.

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

Changed.

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

I get a compiler warning if they aren't there.

On Tue, Dec 1, 2009 at 5:14 AM, Diego Biurrun <diego at biurrun.de> wrote:
> On Tue, Dec 01, 2009 at 12:13:24AM -0800, Michael Tison wrote:
>> --- libavcodec/cdgraphics.c     (revision 0)
>> +++ libavcodec/cdgraphics.c     (revision 0)
>> @@ -0,0 +1,416 @@
>> +    frame->buffer_hints = FF_BUFFER_HINTS_VALID |
>> +                          FF_BUFFER_HINTS_PRESERVE |
>> +                          FF_BUFFER_HINTS_REUSABLE;
>
> Align the |.

Aligned.

>> +    color = cp->data[0] & 0x0F;
>> +    repeat = cp->data[1] & 0x0F;
>
> Align the =.

Aligned.

>> +       r = (color >> 8) & 0x000F;
>> +       g = (color >> 4) & 0x000F;
>> +       b = (color) & 0x000F;
>
> Align the &.

Aligned.

>> +    in = in + in_tl_x + in_tl_y * lsize;
>> +    out = out + out_tl_x + out_tl_y * lsize;
>
> align

Aligned.

Again, thanks for the feedback!

Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cdgraphicsv3.patch
Type: text/x-diff
Size: 19126 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091202/6000827c/attachment.patch>