[FFmpeg-devel] [PATCH] Add DPX decoder

[Jimmy Christensen]

Best Regards 
Jimmy Christensen 
Developer 
Ghost A/S 

----- "Michael Niedermayer" <michaelni at gmx.at> wrote:

| On Wed, May 06, 2009 at 01:27:52PM +0100, Jimmy Christensen wrote:
| > Hi,
| > 
| > Added DPX decoder codec. Only support 10-bit and does no log to lin
| conversion, which means it only supports linear DPX files. Have tested
| with DPX files from various software and it seems to work ok with
| them.
| > 
| > Based largely on the TGA decoder.
| [...]
| > +struct RGBField {
| > +	unsigned A:2;
| > +	unsigned R:10;
| > +	unsigned G:10;
| > +	unsigned B:10;
| > +};
| 
| tabs are forbidden in svn
| 

Thanks, didn't know that.

| 
| > +
| > +typedef struct DPXContext {
| > +    AVFrame picture;
| 
| > +    int width;
| > +    int height;
| 
| probably redundant with AVCodecContext.width/...
| 

Ah, thanks will try and remove those.
| 
| [...]
| > +	if(endian == 0x0) {
| > +		offset = AV_RB32(headerBuffer); headerBuffer += 4;
| > +		bytestream_get_buffer(&headerBuffer, version, 8); headerBuffer +=
| 8;
| > +		// Jump in extra 744 bytes to end in address 744 + 4 + 4 + 8 =
| 760 = 0x2f8
| > +		headerBuffer += 744;
| > +		orientation = AV_RB32(headerBuffer); headerBuffer += 4;
| > +		w = AV_RB32(headerBuffer); headerBuffer += 4;
| > +		h = AV_RB32(headerBuffer); headerBuffer += 4;
| > +	} else {
| > +		offset = AV_RL32(headerBuffer); headerBuffer += 4;
| > +		bytestream_get_buffer(&headerBuffer, version, 8); headerBuffer +=
| 8;
| > +		// Jump in extra 744 bytes to end in address 744 + 4 + 4 + 8 =
| 760 = 0x2f8
| > +		headerBuffer += 744;
| > +		orientation = AV_RL32(headerBuffer); headerBuffer += 4;
| > +		w = AV_RL32(headerBuffer); headerBuffer += 4;
| > +		h = AV_RL32(headerBuffer); headerBuffer += 4;
| > +	}
| 
| code duplication
| 

No, one is for big endian numbers, the other is for little endian. There are 2 types of DPX files.

| 
| > +
| > +	s->width = w;
| > +	s->height = h;
| > +	s->bpp = 32;
| > +	avctx->pix_fmt = PIX_FMT_RGB32;
| > +
| > +	if(s->picture.data[0])
| > +		avctx->release_buffer(avctx, &s->picture);
| > +	if(avcodec_check_dimensions(avctx, w, h))
| > +		return -1;
| > +	if(w != avctx->width || h != avctx->height)
| > +		avcodec_set_dimensions(avctx, w, h);
| > +	if(avctx->get_buffer(avctx, p) < 0) {
| > +		av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
| > +		return -1;
| > +	}
| > +
| > +	buf += offset;
| > +
| > +	ptr = p->data[0];
| > +	stride = p->linesize[0];
| > +
| > +	for(x = 0; x < s->height; x++) {
| > +		uint8_t *dst = ptr;
| > +		for(y = 0; y < s->width; y++) {
| > +			rgbBuffer = AV_RB32(buf);
| > +			memcpy(&rgbField, &rgbBuffer, 4);
| > +			dst[0] = ((rgbField.R * 256) / 1024);
| > +			dst[1] = ((rgbField.G * 256) / 1024);
| > +			dst[2] = ((rgbField.B * 256) / 1024);
| 
| discarding 2 bits in the decoder is unacceptable
| also this way of reading is not portable
| 

How so? The last 2 bits are not used, since it's too small for alpha channel.

| 
| [...]
| > Index: libavcodec/avcodec.h
| > ===================================================================
| > --- libavcodec/avcodec.h	(revision 18736)
| > +++ libavcodec/avcodec.h	(working copy)
| > @@ -159,6 +159,7 @@
| >      CODEC_ID_VP6,
| >      CODEC_ID_VP6F,
| >      CODEC_ID_TARGA,
| > +    CODEC_ID_DPX,
| >      CODEC_ID_DSICINVIDEO,
| >      CODEC_ID_TIERTEXSEQVIDEO,
| >      CODEC_ID_TIFF,
| 
| breaks ABI
| 

I was unsure about this, but in the header of the file, it says to place new decoders next to those alike. Will move it last then.
| 
| [...]
| -- 
| Michael     GnuPG fingerprint:
| 9FF2128B147EF6730BADF133611EC787040B0FAB
| 
| It is not what we do, but why we do it that matters.
| 
| _______________________________________________
| ffmpeg-devel mailing list
| ffmpeg-devel at mplayerhq.hu
| https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel