[FFmpeg-devel] [PATCH] PhotoCD image decoder

[Kenneth Vermeirsch]

pross at xvid.org wrote:
> 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
>
>   

Thanks for reviewing.

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

done locally.

>> +
>> +typedef struct
>> +{
>>     
>
> compact to one line: typedef struct {
>
>   

done locally.

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

replaced it by a macro.

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

added.

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

I added a check.

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

I need the buffer zeroed every time the function is called, that's why I 
used av_malloc+memset.

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

done.

>> +    const uint8_t type2idx[] = { 0, 0xff, 1, 2 };
>>     
>
> preferred style is to capitalise hexadecimal values
>
>   

didn't know, changed here and elsewhere.

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

fixed.

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

this is the minimum size a valid file (with the exception of thumbnail 
files) can be since it must contain at least resolution levels 1, 2 and 
3 and these are stored uncompressed.

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

removed

> and why the space immiediately after sizeof??
>
>   

same as for "if (", "for (", "while (" i think -- sizeof being an 
operator rather than a function

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

done.

I will send an updated patch one I get the colorspace thing working.

Best,
  Kenneth