[FFmpeg-devel] GSoc: BFI Demuxer/Decoder

Michael Niedermayer michaelni
Wed Mar 26 15:28:44 CET 2008


On Wed, Mar 26, 2008 at 11:16:05AM +0530, Sisir Koppaka wrote:
> Hi,
> The BFI patch is not yet working completely, but I've reached a plateau and
> haven't been able to do much in the past couple of days...so if it's
> possible, please have a look at the code and give your suggestions where it
> might be going wrong.
> 1. ffmpeg -i displays the details of the file correctly right now

> 2. ffplay doesn't work. Doing a Valgrind memcheck reveals some serious
> memory problem in the decoder. (t reveals some problems in line 41 of the
> decoder)

fix the problems valgrind reports in your code!


> 3. The palette is not yet transported to the decoder from the demuxer - I've
> read of various ways to do that and all but it's got a bit confusing...so I
> think a few more specific advice based on the code below would really help
> me a lot.

The palette in BFI seems to be a single global palette, if this is true.
It should be transported through extradata from demuxer to decoder.

[...]

> Index: trunk/libavcodec/Makefile
> ===================================================================
> --- trunk/libavcodec/Makefile	(revision 1)
> +++ trunk/libavcodec/Makefile	(working copy)
> @@ -42,6 +42,7 @@
>  OBJS-$(CONFIG_ATRAC3_DECODER)          += atrac3.o mdct.o fft.o
>  OBJS-$(CONFIG_AVS_DECODER)             += avs.o
>  OBJS-$(CONFIG_BETHSOFTVID_DECODER)     += bethsoftvideo.o
> +OBJS-$(CONFIG_BFI_DECODER)	       += bfi.o

tabs are forbiden in ffmpeg svn


[...]
> +static int bfi_decode_frame(AVCodecContext *avcc,void *data, int *data_size, const uint8_t *buf, int buf_size)
> +{
> +	BFIContext *bfi = avcc->priv_data;
> +	uint8_t *dst;
> +	uint8_t *frame_end;
> +	unsigned int code, byte, length, offset, colour;
> +	int remaining = avcc->width,i;
> +	if(avcc->reget_buffer(avcc, &bfi->frame))	{
> +		av_log(avcc, AV_LOG_ERROR, "reget_buffer() failed\n");
> +		return -1;
> +	}

> +	avcodec_set_dimensions(avcc, avcc->width, avcc->height);

This makes no sense.


> +	dst = bfi->frame.data[0];
> +	frame_end = bfi->frame.data[0] + avcc->width * avcc-> height;
> +	while(dst != frame_end)	{	
> +		byte = *buf++;
> +		code = byte >> 6;
> +		length = byte & ~0xC0;
> +		switch(code)	{
> +			case 0:	//Normal Chain
> +				if(length==0)	{
> +				length = bytestream_get_le16(&buf); 
> +				}
> +				memcpy(dst,buf,length);
> +				break;
> +			case 1:	//Back Chain
> +				if(length == 0)	{
> +				length = bytestream_get_byte(&buf);
> +				offset = bytestream_get_le16(&buf);
> +				}
> +				else	{
> +				offset = bytestream_get_byte(&buf);
> +				}
> +				memcpy(dst,data-offset,length);				
> +				break;
> +			case 2:	//Skip Chain
> +				if(length == 0)	{
> +				length = bytestream_get_le16(&buf);
> +				}
> +				if(length == 0)	goto finish;
> +				dst += length;		 //Leaves length bytes of output unchanged from last frame.
> +				break;
> +			case 3: //Fill Chain
> +				if(length == 0)	{
> +				length = bytestream_get_le16(&buf);
> +				}
> +				colour = bytestream_get_le16(&buf);
> +				for(i=0;i<length;i++)	{
> +					*dst++ = colour >> 8;
> +					*dst++ = colour & ~0xFF00;
> +				}	//Filling length words of the output with colour.
> +				break;
> +			default:
> +				av_log(NULL,AV_LOG_INFO,"\nOops! Couldn't recognize the 'code'...");
> +		}
> +	}

this contains unreachable code and duplicated code which can be factored out.
It is also exploitable.


> +	finish:
> +		*data_size = sizeof(AVFrame);
> +		*(AVFrame*)data = bfi->frame;
> +	return buf_size;
> +}
> +

> +static int bfi_decode_end(AVCodecContext *avcc)
> +{
> +	BFIContext *bfi = avcc->priv_data;
> +	if(bfi->frame.data[0])
> +		avcc->release_buffer(avcc,&bfi->frame);
> +	return 0;
> +}

unused


> +
> +static int bfi_decode_close(AVCodecContext *avcc)
> +{
> +	return 0;
> +}

memleak


[...]


> Index: trunk/libavformat/bfi.c
> ===================================================================
> --- trunk/libavformat/bfi.c	(revision 0)
> +++ trunk/libavformat/bfi.c	(revision 4)
> @@ -0,0 +1,232 @@
> +#include "avformat.h"
> +
> +typedef struct BFIContext {
> +	int nframes;

> +	int nframesOrig;

unused


> +	int palette_set;
> +	int chunk_header;
> +	int chunk_header_orig;
> +	int audio_offset;
> +	int video_offset;
> +	int audio_size;
> +	int video_size;
> +	int audiovideo;

> +	int sample_rate;

unneeded


> +	int buffer_size;

unused


> +	int fps;
> +} BFIContext;
> +

> +static int bfi_probe(AVProbeData *p)
> +{
> +	/* Checking file header */
> +	if(p->buf[0]=='B' && p->buf[1]=='F' && p->buf[2]=='&' && p->buf[3]=='I')

see AV_RB32() and MK(BE)TAG


[...]
> +	url_fseek(pb,12,SEEK_SET);
> +	bfi->nframes = get_le32(pb);
> +	bfi->nframesOrig = bfi->nframes;
> +	/* Where do these chunks begin... */
> +	url_fseek(pb,8,SEEK_SET);
> +	bfi->chunk_header = get_le32(pb);
> +	bfi->chunk_header_orig = bfi->chunk_header;

read data in order dont seek around randomly


[...]
> +static int bfi_read_audio(AVFormatContext *s, AVPacket *pkt)
> +{
> +	BFIContext *bfi = s->priv_data;
> +	ByteIOContext *pb = s->pb;
> +	int audio_length;
> +	int ret_value;
> +	url_fseek(pb,bfi->chunk_header+bfi->audio_offset,SEEK_SET);

> +	if(bfi->nframes==bfi->nframesOrig)	{
> +	//	get_le16(pb);	/* Doubtful about this. Check if this is required */
> +		s->streams[1]->codec->sample_rate = bfi->sample_rate;
> +		s->streams[1]->codec->bit_rate = s->streams[1]->codec->channels * s->streams[1]->codec->sample_rate * s->streams[1]->codec->bits_per_sample;
> +	}

this is nonsense


> +	audio_length = bfi->video_offset - bfi->audio_offset; 
> +	ret_value = av_get_packet(pb, pkt, audio_length);
> +	pkt->stream_index = 1;
> +	bfi->audiovideo = 1;	/* Video to be read next. */
> +	return (ret_value != audio_length ? AVERROR(EIO) : ret_value);

memleak


> +}
> +
> +static int bfi_read_video(BFIContext *bfi, ByteIOContext *pb, AVPacket *pkt, AVFormatContext *s, int npixels)
> +{
> +	uint8_t *bfibuf_start = NULL;
> +	int position, bfibuf_nbytes = 0;
> +	unsigned int code, length, byte, offset, colour, count = 0;	//lack of clarity on the right shift. Hence unsigned to be safe and sound.
> +	//unpacked_size=get_le32(pb);
> +	position = url_ftell(pb);	//Verify if -1 is required here.
> +	if(url_feof(pb))	return AVERROR(EIO);
> +	bfibuf_start = av_malloc(npixels);
> +	if(!bfibuf_start)
> +		return AVERROR(ENOMEM);
> +	url_fseek(pb,bfi->chunk_header + bfi->video_offset, SEEK_SET);
> +	do	{
> +		byte = get_byte(pb);
> +		count++;
> +		bfibuf_start[bfibuf_nbytes++] = byte;
> +		code = byte >> 6;
> +		length = byte & ~0xC0;
> +		switch(code)	{
> +			case 0:	//Normal Chain
> +				if(length==0)	{
> +				length = get_le16(pb); 
> +				bfibuf_start[bfibuf_nbytes++] = length & ~0xFF00;
> +				bfibuf_start[bfibuf_nbytes++] = length >> 8;
> +				}
> +				get_buffer(pb, &bfibuf_start[bfibuf_nbytes], length);
> +				count += length;
> +				bfibuf_nbytes += length;
> +				break;
> +			case 1:	//Back Chain
> +				if(length == 0)	{
> +				length = get_byte(pb);
> +				bfibuf_start[bfibuf_nbytes++] = length;
> +				offset = get_le16(pb);
> +				bfibuf_start[bfibuf_nbytes++] = offset & ~0xFF00;
> +				bfibuf_start[bfibuf_nbytes++] = offset >> 8;
> +				}
> +				else	{
> +				offset = get_byte(pb);
> +				bfibuf_start[bfibuf_nbytes++] = offset;
> +				}
> +				count += length;
> +				break;
> +			case 2:	//Skip Chain
> +				if(length == 0)	{
> +				length = get_le16(pb);
> +				bfibuf_start[bfibuf_nbytes++] = length & ~0xFF00;
> +				bfibuf_start[bfibuf_nbytes++] = length >> 8;
> +				}
> +				if(length == 0)	goto finish;
> +				count += length;	 //Leaves length bytes of output unchanged from last frame.
> +				break;
> +			case 3: //Fill Chain
> +				if(length == 0)	{
> +				length = get_le16(pb);
> +				bfibuf_start[bfibuf_nbytes++] = length & ~0xFF00;
> +				bfibuf_start[bfibuf_nbytes++] = length >> 8;
> +				}
> +				colour = get_le16(pb);
> +				bfibuf_start[bfibuf_nbytes++] = colour & ~0xFF00;
> +				bfibuf_start[bfibuf_nbytes++] = colour >> 8;
> +				count += length;	//Filling length words of the output with colour.
> +				break;
> +			default:
> +				av_log(NULL,AV_LOG_INFO,"\nOops! Couldn't recognize the 'code'...");
> +		}
> +	}while(count<npixels);

code duplication and video decoding does not belong in the demuxer


> +	finish :
> +	if(av_new_packet(pkt, bfibuf_nbytes) < 0)
> +		goto fail;
> +	memcpy(pkt->data, bfibuf_start, bfibuf_nbytes);
> +	av_free(bfibuf_start);	
> +	pkt->pos = position;
> +	pkt->stream_index = 0;

> +	pkt->pts = 100/(bfi->fps);	

wrong


[...]
> +	if(get_byte(pb) == 'I')
> +		url_fseek(pb, -1, SEEK_CUR);
> +	url_fseek(pb,bfi->chunk_header+(bfi->chunk_header == bfi->chunk_header_orig?0:bfi->chunk_header_orig),SEEK_SET);

nonsense


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- 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/20080326/30d5489e/attachment.pgp>



More information about the ffmpeg-devel mailing list