[FFmpeg-devel] [PATCH] PhotoCD image decoder
pross at xvid.org
pross
Mon Jan 11 15:29:46 CET 2010
On Mon, Jan 11, 2010 at 02:09:39PM +0100, Kenneth Vermeirsch wrote:
> Hi all,
>
> this is a port of the libpcd PhotoCD image decoder
comments below
> Index: Changelog
> ===================================================================
> --- Changelog (revision 21136)
> +++ Changelog (working copy)
> @@ -49,6 +49,7 @@
> - Auravision Aura 1 and 2 decoders
> - Deluxe Paint Animation playback system
> - SIPR decoding for modes 8k5, 6k5 and 5k0
> +- Kodak PhotoCD image decoder
consistency... everywhere else you've called it '... (a.k.a ImagePac)'.
also i would remove the a.k.a, and just call it 'Kodak PhotoCD/ImagePac image'
> +
> +typedef struct
> +{
compact to one line: typedef struct {
> + AVFrame picture;
> +
> + int thumbnails; //* number of thumbnails; 0 for normal image */
> + int resolution;
> + int orientation;
> +
> + const uint8_t *stream;
> + int streampos;
> +
> + uint8_t *seq [3]; //* huffman tables */
> + uint8_t *bits[3];
> +} PhotoCDContext;
> +
> +static const int img_start [] = { 0 /*dummy */ , 8192, 47104, 196608 };
> +static const short def_width [] = { 0 /*dummy */ , 192, 384, 768, 1536, 3072, 6144 };
> +static const short def_height[] = { 0 /*dummy */ , 128, 256, 512, 1024, 2048, 4096 };
def_height[] seems uneeded. you can caclulate the height using 2^(index+6)
there are common divisors for the other arrays
> +
> +#define HTABLE_SIZE 0x10000
> +
> +#define SETSHIFT { shiftreg=(((stream[0]<<16) | (stream[1]<<8 ) | \
> + (stream[2])) >> (8-bit)) & 0xffff; }
> +#define LEFTSHIFT { shiftreg=((shiftreg<<1) & 0xffff) | \
> + ((stream[2]>>(7-bit++))&1); stream += bit>>3; bit &= 7; }
comments describing the purpose of these macros do would be useful
> +static av_always_inline void interp_lowres(PhotoCDContext *ctx, AVFrame *picture,
> + int width, int height)
> +{
> + register uint8_t *dest;
> + uint8_t *ptr, *ptr1, *ptr2;
> + register int x;
> + int y;
> + const uint8_t *const start = ctx->stream + ctx->streampos + img_start[3];
> + const register uint8_t *src = start;
> +
> + ptr = picture->data[0];
> + ptr1 = picture->data[1];
> + ptr2 = picture->data[2];
> +
> + for (y = 0; y < height; y += 2) {
> + for (dest = ptr, x = 0; x < width - 1; x++) {
> + *(dest++) = src[x];
> + *(dest++) = (src[x] + src[x + 1] + 1) >> 1;
> + }
> + *(dest++) = src[x];
> + *(dest++) = src[x];
could be packed on one line e.g.
dst[0] = dst[1] = src[x];
elsewhere you have used an index variable to derefence dest (e.g. dest[x] = blah),
rather then inline pointer arith. this makes it more readable imho
> +static int read_hufftable(AVCodecContext *avctx, uint8_t ** aseq, uint8_t ** abits)
> +{
> + PhotoCDContext *const ctx = avctx->priv_data;
> + const uint8_t *src = ctx->stream + ctx->streampos;
> + const int tablen = *src;
> + int len = tablen, i, j;
> +
> + if (!*aseq)
> + *aseq = av_malloc(HTABLE_SIZE * sizeof (char));
> + if (!*abits)
> + *abits = av_malloc(HTABLE_SIZE * sizeof (char));
what happens if av_malloc fails?
> +
> + memset(*aseq, 0, HTABLE_SIZE * sizeof (char));
> + memset(*abits, 0, HTABLE_SIZE * sizeof (char));
there is a variant of av_malloc that zeroises the buffer for you
> +
> + for (i = 1; len >= 0; i += 4, len--) {
> + const int bits = src[i] + 1;
> + const int seq = (int) src[i + 1] << 8 | (int) src[i + 2];
> + const int seq2 = seq + (0x10000 >> bits);
> + for (j = seq; j < seq2; j++) {
> + if ((*abits)[j]) {
> + av_log(avctx, AV_LOG_ERROR, "invalid huffmann code table #%x\n", j);
> + return -1;
return AVERROR_INVALIDDATA
> + const uint8_t type2idx[] = { 0, 0xff, 1, 2 };
preferred style is to capitalise hexadecimal values
> + while (y < height) {
> + register uint8_t *data;
> + register int x;
> + uint8_t *seq;
> + uint8_t *bits;
> + int x2, idx;
> +
> + bit = 0;
> + for (;;) {
> + stream = memchr(stream, 0xff, 10240);
what is this magic 10240 value?
> + if (stream[1] == 0xff)
> + break;
> + stream++;
> + }
indent
> +static int photocd_decode_frame(AVCodecContext *avctx, void *data,
> + int *data_size, AVPacket *avpkt)
> +{
> + const uint8_t *const buf = avpkt->data;
> + PhotoCDContext *const ctx = avctx->priv_data;
> + AVFrame *picture = data;
> + AVFrame *const p = (AVFrame *) &ctx->picture;
> + int ptr_incr, size_read, n;
> + uint8_t *ptr, *ptr1, *ptr2;
> +
> + if (avpkt->size < 7) {
> + av_log(avctx, AV_LOG_WARNING, "invalid file\n");
> + return -1;
the av_log text should be consistent with that below. e.g.
''insufficient file size for this to be a PhotoCD image''
and return AVERROR_INVALIDDATA;
> + }
> +
> + if (!strncmp("PCD_OPA", buf, 7)) {
> + ctx->thumbnails = (int) buf[10] << 8 | (int) buf[11];
> + av_log(avctx, AV_LOG_WARNING, "this is a thumbnails file, "
> + "reading first thumbnail only\n");
> + } else if (avpkt->size < 786432) {
how did you derive this magic number ^^ ?
> + av_log(avctx, AV_LOG_ERROR, "probably not a PhotoCD image: too small\n");
> + return -1;
AVERROR_INVALIDDATA
> +static av_cold int photocd_decode_init(AVCodecContext *avctx)
> +{
> + PhotoCDContext *const ctx = avctx->priv_data;
> +
> + memset(ctx, 0, sizeof (PhotoCDContext));
libavcodec already zeroises the struct for you
and why the space immiediately after sizeof??
> +static av_cold int photocd_decode_close(AVCodecContext *avctx)
> +{
> + PhotoCDContext *const ctx = avctx->priv_data;
> +
> + if (ctx->picture.data[0])
> + avctx->release_buffer(avctx, &ctx->picture);
> +
> + av_free(ctx->seq [0]);
> + av_free(ctx->bits[0]);
> + av_free(ctx->seq [1]);
> + av_free(ctx->bits[1]);
> + av_free(ctx->seq [2]);
> + av_free(ctx->bits[2]);
for(i=0; i<3; i++) { av_free(foo[i]); } would simplify this
-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
-------------- 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/20100112/c38b34a4/attachment.pgp>
More information about the ffmpeg-devel
mailing list