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

[Michael Tison]

Revised patch attached.

On Fri, Dec 11, 2009 at 4:51 PM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de> wrote:
> On Thu, Dec 10, 2009 at 10:59:23PM -0800, Michael Tison wrote:
>> +static int read_packet(AVFormatContext *s, AVPacket *pkt)
>> +{
>> + ? ?int ret;
>> +
>> + ? ?ret = av_get_packet(s->pb, pkt, CDG_PACKET_SIZE);
>> + ? ?if (ret >= 0 && ret < CDG_PACKET_SIZE) {
>> + ? ? ? ?av_free_packet(pkt);
>> + ? ? ? ?ret = AVERROR(EIO);
>> + ? ?}
>> + ? ?pkt->stream_index = 0;
>> + ? ?return ret;
>
> I'd like to restate that I consider this kind of code highly silly, it
> 1) misuses AVERROR(EIO) for something that probably is not an I/O error
> 2) it means a single lost byte which might not make a visible difference
> ? otherwise now drops the whole (last) packet
> 3) it makes it unnecessarily difficult to test the decoder code for robustness
> ? and sufficient buffer end checks (no, those in libavformat are not of any
> ? use, libavcodec is not supposed to be only used with libavformat)
> 4) and all of that at the cost of additional code and complexity
>
> Or to put it succinctly, I think it is adding code to make it behave worse.

Agreed.

On Fri, Dec 11, 2009 at 2:33 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Thu, Dec 10, 2009 at 10:59:23PM -0800, Michael Tison wrote:
> [...]
>> +static int cdg_tile_block(CDGraphicsContext *cc, uint8_t *data, int b)
>> +{
>> +    int ci, ri;
>> +    int color;
>> +    int x, y;
>> +    int ai;
>> +    int stride   = cc->frame.linesize[0];
>> +    uint8_t *buf = cc->frame.data[0];
>> +
>> +    ri = (data[2] & 0x1F) * CDG_TILE_HEIGHT + cc->vscroll;
>> +    ci = (data[3] & 0x3F) * CDG_TILE_WIDTH  + cc->hscroll;
>> +
>> +    if ((unsigned) ri > (CDG_FULL_HEIGHT - CDG_TILE_HEIGHT)) {
>
> you could make ci/ri unsigned which would avoid this cast

Declared unsigned.

>> +        av_log(NULL, AV_LOG_ERROR, "tile is out of height range\n");
>
> NULL is a bad context

Fixed.

> [...]
>> +static void cdg_copy_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 stride)
>> +{
>> +    int y;
>> +
>> +    in  = in  + in_tl_x  + in_tl_y  * stride;
>
> in +=

Ouch. Fixed.

> [...]
>> +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 stride, int roll)
>> +{
>
>> +    if (roll)
>> +        cdg_copy_rect_buf(out_tl_x, out_tl_y, out, in_tl_x, in_tl_y,
>> +                          in, w, h, stride);
>> +    else
>
> missing {}
> btw, you should try tools/patcheck

Added.

> [...]
>> +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;
>> +    int ret, xor, cpy;
>> +    uint8_t command, inst;
>> +    uint8_t cdg_data[CDG_DATA_SIZE];
>> +    AVFrame new_frame;
>> +    CDGraphicsContext *cc = avctx->priv_data;
>> +
>> +    if (avctx->reget_buffer(avctx, &cc->frame)) {
>> +        av_log(avctx, AV_LOG_ERROR, "reget_buffer() failed\n");
>> +        return -1;
>> +    }
>> +
>> +    command = bytestream_get_byte(&buf);
>> +    inst    = bytestream_get_byte(&buf);
>> +    buf += 2;  /// skipping 2 unneeded bytes
>> +    bytestream_get_buffer(&buf, cdg_data, CDG_DATA_SIZE);
>> +
>> +    if ((command & CDG_MASK) == CDG_COMMAND) {
>> +        switch (inst & CDG_MASK) {
>> +        case CDG_INST_MEMORY_PRESET:
>> +            if (!(cdg_data[1] & 0x0F))
>> +                memset(cc->frame.data[0], cdg_data[0] & 0x0F,
>> +                       cc->frame.linesize[0] * CDG_FULL_HEIGHT);
>> +            break;
>> +        case CDG_INST_LOAD_PAL_LO:
>> +            cdg_load_palette(cc, cdg_data, 1);
>> +            break;
>> +        case CDG_INST_LOAD_PAL_HIGH:
>> +            cdg_load_palette(cc, cdg_data, 0);
>> +            break;
>> +        case CDG_INST_BORDER_PRESET:
>> +            cdg_border_preset(cc, cdg_data);
>> +            break;
>> +        case CDG_INST_TILE_BLOCK_XOR:
>> +        case CDG_INST_TILE_BLOCK:
>
>> +            xor = (inst & CDG_MASK) == CDG_INST_TILE_BLOCK_XOR ? 1 : 0;
>
> ?1:0 is superflous

Compacted.

> [..]
>> +static int read_header(AVFormatContext *s, AVFormatParameters *ap)
>> +{
>> +    AVStream *vst;
>> +
>> +    vst = av_new_stream(s, 0);
>> +    if (!vst)
>> +        return AVERROR(ENOMEM);
>> +
>> +    vst->codec->codec_type = CODEC_TYPE_VIDEO;
>> +    vst->codec->codec_id   = CODEC_ID_CDGRAPHICS;
>> +
>> +    /// 75 sectors/sec * 4 packets/sector = 300 packets/sec
>> +    av_set_pts_info(vst, 32, 1, 300);
>> +
>
>> +    vst->duration = (url_fsize(s->pb) * vst->time_base.den) / (CDG_PACKET_SIZE * 300);
>
> url_fsize() can fail, this is missing error checking

Checked.

Thanks again!

Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cdgraphicsv8.patch
Type: text/x-diff
Size: 18541 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091214/b1384b13/attachment.patch>