[FFmpeg-devel] [PATCH] CD+G Demuxer & Decoder
Michael Tison
blackspike
Tue Nov 24 10:15:06 CET 2009
Hi,
Revised patch attached.
> That's obviously a paletted format, you should decode it as such and not convert it
> in the decoder.
Yeah, what I wrote before felt like a hack when writing it.
Fixed.
> And tabs are not allowed.
Gone.
>> + result = url_fsize(pb);
>> + if(result < 0)
>> + return AVERROR_INVALIDDATA;
>
> I see no reason for this, CDG files should play fine when streamed, e.g. via stdin.
I think I got this one done (I just removed the whole if block).
>> + ret = av_new_packet(pkt, CDG_PACKET_SIZE);
>> + if(ret < 0)
>> + return ret;
>> +
>> + pkt->pos = url_ftell(pb);
>> +
>> + ret = get_buffer(pb, pkt->data, CDG_PACKET_SIZE);
>> + if(ret <= 0 || pb->eof_reached) {
>> + av_free_packet(pkt);
>> + return (pb->eof_reached == 1) ? -1 : ret;
>> + } else {
>> + av_shrink_packet(pkt, ret);
>> + if(ret < 0)
>> + return ret;
>
> av_get_packet handles all of this and in a more consistent way.
Replaced.
> This is missing documentation and changelog updates and you should bump
> library minor versions. I keep repeating this for two out of three
> patches. How can we make this more clear?
Added demuxer and decoder to general doc list, added line to
changelog, and bumped both minor versions.
>> +// Size of color code table
>> +#define COLOR_TABLE_SIZE 16
>
> pointless comment
Gone.
>> +#define CDG_INST_LOAD_COL_TBL_HIGH 31
>> +#define CDG_INST_TILE_BLOCK_XOR 38
>
> Could be right-aligned.
Aligned.
>> +static void cdg_scroll(CDGraphicsContext *cc, CdgPacket *cp, int roll_over);
>> +static void cdg_load_color_table(CDGraphicsContext *cc, CdgPacket *cp, int low);
>
> Avoid forward declarations.
Avoided.
>> + cc->frame.buffer_hints = FF_BUFFER_HINTS_VALID |
>> + FF_BUFFER_HINTS_PRESERVE |
>> + FF_BUFFER_HINTS_REUSABLE;
>
> Indentation is off.
Done.
>> +static void cdg_write_frame(CDGraphicsContext *cc, uint8_t *dst, int h) {
>> + int i, j, off;
>> +
>> + for(j = 0; j < CDG_FULL_HEIGHT; j++) {
>> + for(i = 0, off = 0; i < CDG_FULL_WIDTH; i++, off+=3) {
>> + dst[j*cc->frame.linesize[0] + off] = cc->colortbl[cc->surface[i][j]] >> 16;
>> + dst[j*cc->frame.linesize[0] + off+1] = cc->colortbl[cc->surface[i][j]] >> 8;
>> + dst[j*cc->frame.linesize[0] + off+2] = cc->colortbl[cc->surface[i][j]];
>> + }
>> + }
>> +}
>> +
>> +static int cdg_decode_frame(AVCodecContext *avctx,
>> + void *data, int *data_size,
>> + AVPacket *avpkt)
>> +{
>
>
> You use completely inconsistent indentation and formatting. Use K&R
> style with 4 space indentation. This applies to all your code.
After reviewing it I only screwed up a couple of times on K&R, if I
missed anymore let me know.
>> + cp.command = bytestream_get_byte(&buf);
>> + cp.instruction = bytestream_get_byte(&buf);
>
> Here and in similar situations: Align the =.
Aligned.
>> OBJS-$(CONFIG_CAVSVIDEO_DEMUXER) += raw.o
>> +OBJS-$(CONFIG_CDG_DEMUXER) += cdg.o
>> OBJS-$(CONFIG_CRC_MUXER) += crcenc.o
>
> Lose the tabs.
Lost.
Thanks for the feedback!
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cdgraphics.patch
Type: text/x-diff
Size: 17334 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091124/f752f259/attachment.patch>
More information about the ffmpeg-devel
mailing list