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

Michael Niedermayer michaelni
Fri Dec 4 04:29:03 CET 2009


On Wed, Dec 02, 2009 at 12:44:21AM -0800, Michael Tison wrote:
[...]
> > [...]
> >> +static int read_packet(AVFormatContext *s, AVPacket *pkt)
> >> +{
> >> + ? ?int ret;
> >> + ? ?ByteIOContext *pb = s->pb;
> >> +
> >
> >> + ? ?ret = av_get_packet(pb, pkt, 24);
> >> + ? ?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, 24);
>     if (ret != 24)
> 	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.

it still looks like a memleak, av_get_packet() succeeds and allocates
memory and you turn it into a EIO


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

your code does actually look wrong, the warnings are correct i suspect


[...]

> +static void cdg_get_preset_values(CdgPacket *cp, int *c, int *r)
> +{
> +    *c = cp->data[0] & 0x0F;
> +    *r = cp->data[1] & 0x0F;
> +}
> +

> +static void cdg_memory_preset(CDGraphicsContext *cc, CdgPacket *cp)
> +{

> +    int color;
> +    int repeat;
> +
> +    cdg_get_preset_values(cp, &color, &repeat);
> +    if (!repeat)
> +       memset(cc->frame.data[0], color,
> +              cc->frame.linesize[0] * 216);

please try to write code a little more compactly.
like:

if(!(data[1]&0xf))
    memset(cc->frame.data[0], data[0]&0xf,
           cc->frame.linesize[0] * 216);

and cdg_memory_preset() is useless too, this single if and memset can
just be used directly at the single spot where the function call is

the 216 of course can be a named constant, i just replaced them for
the review as that way i could more clearly see what actual values
are used ...



> +}
> +
> +static void cdg_border_preset(CDGraphicsContext *cc, CdgPacket *cp)
> +{
> +    int color;
> +    int repeat;
> +    int y;

> +    int lsize    = cc->frame.linesize[0];

id call it stride if linesize where too long, but thats just me, feel
free to ignore this ...


> +    uint8_t *buf = cc->frame.data[0];
> +
> +    cdg_get_preset_values(cp, &color, &repeat);
> +
> +    if (!repeat) {
> +       /// fill the top and bottom borders
> +       memset(buf, color, 12 * lsize);
> +       memset(buf + (216 - 12) * lsize,
> +              color, 12 * lsize);
> +

> +       /// fill the side borders
> +       for (y = 12;
> +            y < 216 - 12; y++) {

thats a case where i think its clearer with the 80chars limit ignored and
no linebreak but i guess some people might disagree ...


> +           memset(buf + y * lsize, color, 6);
> +           memset(buf + 300 - 6 + y * lsize,
> +                  color, 6);
> +       }
> +    }
> +}

> +
> +static void cdg_load_palette(CDGraphicsContext *cc, CdgPacket *cp,
> +                            int low)
> +{
> +    uint8_t r, g, b;
> +    uint16_t color;
> +    int i;
> +    int array_offset  = low ? 0 : 8;

> +    uint32_t *palette = (uint32_t *) cc->frame.data[1];
[...]
> +    cc->frame.data[1] = (uint8_t *) palette;

unneeded


> +    cc->frame.palette_has_changed = 1;
> +}
> +
> +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) * 12;
> +    ci = (cp->data[3] & 0x3F) * 6;
> +
> +    if (ri > (216 - 12 - cc->vscroll)
> +        || (ri + cc->vscroll) < 0)
> +       return;

> +    if (ci > (300 - 6 - cc->hscroll)
> +        || (ci + cc->hscroll) < 0)

ci += cc->hscroll;

if(ci < 0 || ci > 300 - 6)

you could also write:
ci > (unsigned)(300 - 6)

and some warning message might be usefull for such outside tiles


> +       return;
> +
> +    for (y = 0; y < 12; y++) {
> +       byte = cp->data[4 + y] & 0x3F;
> +       for (x = 0; x < 6; x++) {
> +           pix = (byte >> (5 - x)) & 0x01;
> +
> +           ai = ci + x + cc->hscroll + (lsize * (ri + y + cc->vscroll));
> +
> +           if (!pix)

if(byte & 0x20)
and 
byte<<=1;

or you could try an array and
color= array[pix];
this might be faster or not

[...]
> +static void cdg_fill_wrapper(int out_tl_x, int out_tl_y,
> +                             uint8_t *out,
> +                             int in_tl_x, int in_tl_y,
> +                             uint8_t *in, int color,
> +                             int w, int h, int lsize, int roll)
> +{
> +    if (roll)
> +       cdg_copy_rect_buf(out_tl_x, out_tl_y, out, in_tl_x, in_tl_y,
> +                         in, w, h, lsize);
> +    else
> +       cdg_fill_rect_preset(out_tl_x, out_tl_y, out, color, w, h, lsize);
> +}
> +

> +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 lsize    = cc->frame.linesize[0];
> +    uint8_t *in  = cc->frame.data[0];
> +    uint8_t *out = new_frame->data[0];
> +

> +    cdg_get_scroll_data(cp, &color, &hscmd, &h_off, &vscmd, &v_off);

please dont fragment code so much over functions
one can split too little but i think you split too much


> +
> +    /// 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], 16 * 4);
> +
> +    for (y = FFMAX(0, vinc); y < FFMIN(216 + vinc, 216); y++)

> +       for (x = FFMAX(0, hinc); x < FFMIN(lsize + hinc, lsize); x++)
> +           out[x + lsize * y] = in[x - hinc + (y - vinc) * lsize];

memcpy


> +
> +    if (vinc > 0)
> +       cdg_fill_wrapper(0, 0, out,
> +                        0, 216 - vinc, in, color,
> +                        lsize, vinc, lsize, roll_over);
> +    else if (vinc < 0)
> +       cdg_fill_wrapper(0, 216 + vinc, out,
> +                        0, 0, in, color,
> +                        lsize, -1 * vinc, lsize, roll_over);
> +
> +    if (hinc > 0)
> +       cdg_fill_wrapper(0, 0, out,
> +                        300 - hinc, 0, in, color,
> +                        hinc, 216, lsize, roll_over);
> +    else if (hinc < 0)
> +       cdg_fill_wrapper(300 + hinc, 0, out,
> +                        0, 0, in, color,
> +                        -1 * hinc, 216, lsize, roll_over);
> +
> +}
> +
> +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);

its pointless to read these into a struct, they are used just once or twice
straight below


> +    buf += 2;  /// skipping 2 unneeded bytes
> +    bytestream_get_buffer(&buf, (uint8_t*) &cp.data, 16);

similarly only cp.data is passed to the functions, the struct again is
useless


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

> ... defining _GNU_SOURCE...
For the love of all that is holy, and some that is not, don't do that.
-- Luca & Mans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091204/755991ed/attachment.pgp>



More information about the ffmpeg-devel mailing list