[FFmpeg-devel] [PATCH] CDXL demuxer and decoder
Derek Buitenhuis
derek.buitenhuis at gmail.com
Fri Dec 30 01:09:14 CET 2011
> ---
> Changelog | 1 +
> doc/general.texi | 4 +
> libavcodec/Makefile | 1 +
> libavcodec/allcodecs.c | 1 +
> libavcodec/avcodec.h | 1 +
> libavcodec/cdxl.c | 270 ++++++++++++++++++++++++++++++++++++++++++++++
> libavcodec/version.h | 2 +-
> libavformat/Makefile | 1 +
> libavformat/allformats.c | 1 +
> libavformat/cdxl.c | 131 ++++++++++++++++++++++
> libavformat/version.h | 2 +-
> 11 files changed, 413 insertions(+), 2 deletions(-)
> create mode 100644 libavcodec/cdxl.c
> create mode 100644 libavformat/cdxl.c
[...]
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -259,6 +259,7 @@ enum CodecID {
> CODEC_ID_ESCAPE130 = MKBETAG('E','1','3','0'),
>
> CODEC_ID_G2M = MKBETAG( 0 ,'G','2','M'),
> + CODEC_ID_CDXL,
This should actually go under CODEC_ID_V410.
[...]
> + for (i = 0; i < c->palette_size / 2; i++) {
> + unsigned xxx = bytestream_get_be16(&c->palette);
> + unsigned r = ((xxx >> 8) & 0xF) * 17;
> + unsigned g = ((xxx >> 4) & 0xF) * 17;
> + unsigned b = (xxx & 0xF) * 17;
> + new_palette[i] = (0xFF << 24) | (b + g * 256u + r * 256u * 256u);
> + }
Nit: 256u * 256u could be merged. I doubt this makes any difference
to the compiler though.
> + init_get_bits(&gb, c->video, c->video_size * 8);
> + for (plane = 0; plane < c->bpp; plane++)
> + for (y = 0; y < avctx->height; y++)
> + for (x = 0; x < padded_width; x++)
> + c->frame.data[0][c->frame.linesize[0] * y + x] |= get_bits1(&gb) << (plane);
This chunk of code seems to be used in many areas. Perhaps
put it in its own inline function? Also, it could be wrapped
to fit nicely in 80 cols.
> + if (buf_size < 32)
> + return AVERROR_INVALIDDATA;
Nit: Could do with a line break here.
> + w = AV_RB16(&buf[14]);
> + h = AV_RB16(&buf[16]);
> + c->bpp = AV_RB16(&buf[18]);
> + c->palette_size = AV_RB16(&buf[20]);
> + c->palette = buf + 32;
Any reason why you switch notations here?
> + c->video = c->palette + c->palette_size;
> + c->video_size = buf_size - c->palette_size - 32;
Perhaps some sanity checks? Dunno how useful it is though.
> + if (encoding == 0) {
> + if (c->video_size < ((avctx->width + 15) & ~15) * avctx->height * c->bpp / 8)
> + return AVERROR(EINVAL);
> + avctx->pix_fmt = PIX_FMT_PAL8;
> + } else if (encoding == 1) {
> + if (c->video_size < avctx->width * avctx->height * c->bpp / 8)
> + return AVERROR(EINVAL);
> + avctx->pix_fmt = PIX_FMT_RGB32;
Shouldn't these also be AVERROR_INVALIDDATA? I could be wrong.
> + } else {
> + av_log_ask_for_sample(avctx, "unsupported video format: %d\n", encoding);
> + return AVERROR_PATCHWELCOME;
> + }
:D
> + if (av_image_check_size(w, h, 0, avctx))
> + return -1;
Should return a descriptive error code.
> + if (w != avctx->width || h != avctx->height)
> + avcodec_set_dimensions(avctx, w, h);
Can libavcodec actually handle changes in width or height? :/
> + if (avctx->get_buffer(avctx, p) < 0) {
> + av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> + return -1;
> + }
Should be an error code. Something like AVERROR(ENOMEM)?
> + p->pict_type = AV_PICTURE_TYPE_I;
> +
> + if (buf[0] != 1) {
> + av_log(avctx, AV_LOG_ERROR, "non-standard cdxl file\n");
> + return -1;
> + }
Should be AVERROR_INVALIDDATA, I think.
> + if (encoding == 0) {
> + cdxl_decode_rgb(c, avctx);
> + } else if (encoding == 1) {
> + c->new_video = av_mallocz(avctx->height * avctx->width);
> + if (c->new_video == NULL)
> + return AVERROR(ENOMEM);
> + if (c->bpp > 6)
> + cdxl_decode_ham8(c, avctx);
> + else
> + cdxl_decode_ham6(c, avctx);
> + av_freep(&c->new_video);
> + }
You should either add a final else statement here, or change the else
if to an else.
> + *data_size = sizeof(AVFrame);
> + *(AVFrame*)data = c->frame;
> + return buf_size;
> +}
A line break would be nice here. :)
[...]
> +#define CDXL_HEADER_SIZE 32
Perhaps the decoder should have this too?
> +typedef struct CDXLDemuxContext {
> + AVClass *class;
> + int raw_sound_size;
> + int sample_rate;
> + char *framerate;
> +} CDXLDemuxContext;
Can you align the the var declarations as you did in the decoder?
> + ast = avformat_new_stream(s, NULL);
> + if (!ast)
> + return AVERROR(ENOMEM);
> + ast->codec->bits_per_coded_sample = 8;
> + ast->codec->sample_rate = c->sample_rate;
Could use some line breaks. Also, the rest of the vars could be
aligned with the = in the first line... but that may make it ugly,
so take it with a grain of salt.
> + vst = avformat_new_stream(s, NULL);
> + if (!vst)
> + return AVERROR(ENOMEM);
> + vst->codec->codec_type = AVMEDIA_TYPE_VIDEO;
Ditto.
> + ast->codec->codec_tag = 0;
> + vst->codec->codec_id = CODEC_ID_CDXL;
> + vst->codec->width = AV_RB16(&header[14]);
> + vst->codec->height = AV_RB16(&header[16]);
Same question as above. Just a matter of consistency in
notation.
> + avio_seek(pb, 0, SEEK_SET);
> + return 0;
> +}
Line break? :) You needn't take all my line break comments too
seriously. It's all personal taste.
> + if (avio_read(pb, header, CDXL_HEADER_SIZE) !=
> + CDXL_HEADER_SIZE)
Should be indented like:
if (avio_read(pb, header, CDXL_HEADER_SIZE) !=
CDXL_HEADER_SIZE)
> +#define OFFSET(x) offsetof(CDXLDemuxContext, x)
Is something like this really needed? It seems to merely add an extra
layer of abstraction. I apologize for bikeshedding.
> +static const AVOption cdxl_options[] = {
> + { "sample_rate", "", OFFSET(sample_rate), AV_OPT_TYPE_INT, {.dbl = 11025}, 1, INT_MAX, AV_OPT_FLAG_DECODING_PARAM },
> + { "framerate", "", OFFSET(framerate), AV_OPT_TYPE_STRING, {.str = "50"}, 0, 0, AV_OPT_FLAG_DECODING_PARAM },
> + { NULL },
> +};
Is it possible to align these?
Sorry if I've missed some technical things, or been too cosmetics-
focused. Overall it looks good! :)
- Derek
More information about the ffmpeg-devel
mailing list