[FFmpeg-devel] [PATCH] Add DPX decoder

Wed May 6 15:09:27 CEST 2009

Best Regards 
Jimmy Christensen 
Developer 
Ghost A/S 

----- "Diego Biurrun" <diego at biurrun.de> wrote:

| On Wed, May 06, 2009 at 01:27:52PM +0100, Jimmy Christensen wrote:
| > 
| > 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.
| 
| Get rid of all the tabs, check for places to prettyprint your code
| through vertical alignment and follow K&R style.
| 

Thanks, didn't know that. Will convert it.

| > --- libavcodec/dpx.c	(revision 0)
| > +++ libavcodec/dpx.c	(revision 0)
| > @@ -0,0 +1,157 @@
| > +
| > +	const uint8_t *headerBuffer = avpkt->data;
| > +	const uint8_t *buf = avpkt->data;
| > +	int buf_size = avpkt->size;
| > +	DPXContext * const s = avctx->priv_data;
| > +	AVFrame *picture = data;
| > +	AVFrame * const p= (AVFrame*)&s->picture;
| > +	uint8_t *ptr;
| > +	struct RGBField rgbField;
| 
| vertical alignment and spaces around operators where appropriate
| please
| 
| > +	magic_num = AV_RB32(headerBuffer); headerBuffer += 4;
| 
| Multiple statements on one line are ugly, same below.
| 

Will correct.

| > +	if(magic_num == 0x53445058) {
| > +		endian = 0x0;
| > +	}
| 
| useless {}
| 

Shouldn't be. Will explain in the revised patch

| Also, please use K&R style for new files, i.e. space between
| if/for/while/switch and (.
| 
| > +		// Jump in extra 744 bytes to end in address 744 + 4 + 4 + 8 =
| 760 = 0x2f8
| 
| an extra?
| 
Bascially I just want to end up at 0x2f8 and take into account at what position I'm at.

| > +static av_cold int dpx_init(AVCodecContext *avctx){
| > +
| > +static av_cold int dpx_end(AVCodecContext *avctx){
| 
| K&R style please, i.e. { on the next line.
| 
| > --- 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,
| 

Yes, misunderstood the header of the file.

| Add to the end, this will break ABI otherwise.
| 
| > --- libavformat/riff.c	(revision 18736)
| > +++ libavformat/riff.c	(working copy)
| > @@ -198,6 +198,7 @@
| >      { CODEC_ID_JPEG2000,     MKTAG('M', 'J', '2', 'C') },
| >      { CODEC_ID_VMNC,         MKTAG('V', 'M', 'n', 'c') },
| >      { CODEC_ID_TARGA,        MKTAG('t', 'g', 'a', ' ') },
| > +    { CODEC_ID_DPX,          MKTAG('d', 'p', 'x', ' ') },
| >      { CODEC_ID_PNG,          MKTAG('M', 'P', 'N', 'G') },
| >      { CODEC_ID_PNG,          MKTAG('P', 'N', 'G', '1') },
| >      { CODEC_ID_CLJR,         MKTAG('c', 'l', 'j', 'r') },
| 
| Add to the end.
| 

Will do.