[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