[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