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

[Michael Tison]

Hi,

Attached is a revised patch.

On Thu, Dec 3, 2009 at 7:29 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> 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

Closer to right?
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) {
	av_free_packet(pkt);
	return ret < 0 ? ret : AVERROR(EIO);
    }

    pkt->stream_index = 0;
    return 0;
}

I've noticed in a few formats they call av_free_packet() after a
failed av_get_packet() but most don't free the memory. Could these
formats have possible memory leaks?
Also those same codecs send an AVERROR(EIO) after a failed av_get_packet() too.

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

Fixed without any weird casts this time.

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

Compacted.

> 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

Function removed.

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

s/lsize/stride

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

Ignored limit.

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

Removed.

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

Added messages and error codes.

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

United.

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

memcpy'd

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

Removed struct.

Thanks again!

Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cdgraphicsv4.patch
Type: text/x-diff
Size: 18334 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091206/42f52898/attachment.patch>