[FFmpeg-devel] [PATCH] BFI demuxer

Michael Niedermayer michaelni
Fri Apr 11 23:36:47 CEST 2008


On Sat, Apr 12, 2008 at 02:43:41AM +0530, Sisir Koppaka wrote:
> Thanks! Updated patch attached.
> 
> On Sat, Apr 12, 2008 at 12:59 AM, Vitor Sessak <vitor1001 at gmail.com> wrote:
> 
> > Hi
> > A few suggestions...
> >
> > > +/**
> > > + * @file bfi.c
> > > + * @brief Brute Force & Ignorance (.bfi) file demuxer
> > > + * @author Sisir Koppaka ( sisir.koppaka at gmail dot com )
> > > + * @sa http://wiki.multimedia.cx/index.php?title=BFI
> > > + */
> > > +
> > > +#include "avformat.h"
> > > +
> > > +typedef struct BFIContext {
> > > +    int nframes;
> > > +    int width;
> > > +    int height;
> > > +    int chunk_header;
> > > +    int audio_offset;
> > > +    int video_offset;
> > > +    int audio_frame;
> > > +    int video_frame;
> > > +    int audio_size;
> > > +    int video_size;
> > > +    int chunk_size;
> > > +    int avflag;
> > > +    int sample_rate;
> > > +    int channels;
> > > +    int bits_per_sample;
> > > +    int fps;
> > > +} BFIContext;
> >
> > A lot of those variables are unneeded. For example, width and height are
> > used in only one function and can be local variables. I can't see how it
> > improves readability...
> 
> Done...only those variables necessary to be preserved over several instances
> of a function are now put in the structure, the rest are now local
> variables.
> 
> >
> >
> >
> > > +    vstream->codec->codec_type = CODEC_TYPE_VIDEO;
> > > +    vstream->codec->codec_id = CODEC_ID_BFI;
> > > +    vstream->codec->width = bfi->width;
> > > +    vstream->codec->height = bfi->height;
> > > +    vstream->codec->pix_fmt = PIX_FMT_PAL8;
> >
> > this...
> >
> Done.
> 
> >
> > > +    astream->codec->codec_type = CODEC_TYPE_AUDIO;
> > > +    astream->codec->codec_id = CODEC_ID_PCM_U8;
> > > +    astream->codec->sample_rate = bfi->sample_rate;
> > > +    av_log(NULL, AV_LOG_DEBUG, "\n sample_rate = %d",
> > > +           astream->codec->sample_rate);
> > > +    astream->codec->channels = bfi->channels;
> > > +    bfi->bits_per_sample =
> > > +        av_get_bits_per_sample(astream->codec->codec_id);
> > > +    astream->codec->bits_per_sample = bfi->bits_per_sample;
> > > +    astream->codec->bit_rate = astream->codec->sample_rate *
> > astream->codec->bits_per_sample;   // * astream->codec->channels ;
> >
> > ... and this would look much better aligned like
> >
> >     astream->codec->codec_type  = CODEC_TYPE_AUDIO;
> >     astream->codec->codec_id    = CODEC_ID_PCM_U8;
> >     astream->codec->sample_rate = bfi->sample_rate;
> >
> Fixed.
> 
> >
> > Also there are some overly long lines that should be broken (<80 chars
> > preferred)
> >
> Changed.
> 
> >
> > (...)
> >
> > > +      search:
> > > +        c = get_byte(pb);
> > > +        /* A chunk with a damaged header must also be found...hence,
> > the following. */
> > > +        if (c == 'I') {
> > > +            if (get_byte(pb) == 'V' && get_byte(pb) == 'A'
> > > +                && get_byte(pb) == 'S')
> > > +                goto read;
> > > +        } else if (c == 'V') {
> > > +            if (get_byte(pb) == 'A' && get_byte(pb) == 'S')
> > > +                goto read;
> > > +        } else if (c == 'A') {
> > > +            if (get_byte(pb) == 'S')
> > > +                goto read;
> > > +        } else
> > > +            goto search;
> > > +
> > > +
> > > +        /* Now that the chunk's location is confirmed, we proceed... */
> > > +      read:
> >
> > This is both an infinite loop an an ugly way to write an
> >
> > while(1){
> >     if (something)
> >          break;
> >     (... etc ...)
> > }
> >
> Converted to while + now checks url_feof(pb) within the loop to return...now
> it shouldn't be an infinite loop.
> 
> >
> > > +        get_le32(pb);
> > > +        bfi->video_offset = get_le32(pb);
> > > +        av_log(NULL, AV_LOG_DEBUG, "\nVideo offset is %d",
> > > +               bfi->video_offset);
> > > +        bfi->audio_size = bfi->video_offset - bfi->audio_offset;
> > > +        bfi->video_size = bfi->chunk_size - bfi->video_offset;
> > > +        bfi->chunk_header = bfi->chunk_size - bfi->video_offset;
> >
> > alignment
> 
> Done.
> 
> >
> >
> > > +    case 0:                    //Tossing an audio packet at the audio
> > decoder.
> > > +        ret = av_get_packet(pb, pkt, bfi->audio_size);
> > > +        if (ret < 0)
> > > +            return ret;
> > > +        pkt->stream_index = 1;
> > > +        pkt->pts = bfi->audio_frame;
> > > +        bfi->audio_frame += ret;
> > > +        pkt->duration = 1;
> > > +        av_log(NULL, AV_LOG_DEBUG,
> > > +               "\nSending %d bytes to the audio decoder...",
> > > +               bfi->audio_size);
> > > +        bfi->avflag = 1;
> > > +        return ret;
> > > +
> > > +    case 1:                    //Tossing a video packet at the video
> > decoder.
> > > +        ret = av_get_packet(pb, pkt, bfi->video_size);
> > > +        if (ret < 0)
> > > +            return ret;
> > > +        pkt->stream_index = 0;
> > > +        pkt->pts = bfi->video_frame;
> > > +        bfi->video_frame += ret / bfi->video_size;
> > > +        pkt->duration = 1;
> > > +        av_log(NULL, AV_LOG_DEBUG,
> > > +               "\nSending %d bytes to the video decoder...",
> > > +               bfi->video_size);
> > > +        bfi->avflag = 0;
> > > +        /* Now that both audio and video of this frame are packed, we
> > have one less frame to read. A cursory decrement. */
> > > +        bfi->nframes--;
> > > +        return ret;
> >
> > A lot of code can be factored out the switch (tip: bfi->avflag =
> > !bfi->avflag)
> >
> This seems to break the demuxer...the audio degenerates into half audio-half
> noise and the video no longer appears...I tried bfi->avflag =
> (bfi->avflag==0?1:0)  but to no avail...

[...]
> +typedef struct BFIContext {
> +    int nframes;
> +    int chunk_header;

> +    int audio_offset;
> +    int video_offset;

unneeded


> +    int audio_frame;
> +    int video_frame;

> +    int audio_size;

unneeded


> +    int video_size;

> +    int chunk_size;

unneeded


[...]
> +    /*If all previous chunks were completely read, we try to find a new one...*/
> +    if (!bfi->avflag) {
> +

> +        /* Trying to confirm the chunk by matching the header... */
> +      while(1) {
> +        c = get_byte(pb);
> +        if(url_feof(pb))
> +          return AVERROR(EIO);

inconsistant indention


> +        /* A chunk with a damaged header must also be found...hence... */
> +        if (c == 'I') {
> +            if (get_byte(pb) == 'V' && get_byte(pb) == 'A'
> +                && get_byte(pb) == 'S')
> +                break;
> +        } else if (c == 'V') {
> +            if (get_byte(pb) == 'A' && get_byte(pb) == 'S')
> +                break;
> +        } else if (c == 'A') {
> +            if (get_byte(pb) == 'S')
> +                break;
> +        }
> +      }

Could you explain what the above code is needed for? Are some of the files
damaged?



> +      /* Now that the chunk's location is confirmed, we proceed... */
> +      av_log(NULL, AV_LOG_DEBUG, "\nFound a chunk...");
> +      av_log(NULL, AV_LOG_DEBUG,
> +             "\nChunk number %d confirmed with IVAS identifier...",
> +             bfi->nframes);
> +      bfi->chunk_size = get_le32(pb);
> +      av_log(NULL, AV_LOG_DEBUG, "\nNext chunk header offset is %d",
> +             bfi->chunk_size);
> +      get_le32(pb);
> +      bfi->audio_offset = get_le32(pb);
> +      av_log(NULL, AV_LOG_DEBUG, "\nAudio offset is %d",
> +             bfi->audio_offset);
> +      get_le32(pb);
> +      bfi->video_offset = get_le32(pb);
> +      av_log(NULL, AV_LOG_DEBUG, "\nVideo offset is %d",
> +             bfi->video_offset);
> +      bfi->audio_size   = bfi->video_offset - bfi->audio_offset;
> +      bfi->video_size   = bfi->chunk_size - bfi->video_offset;
> +      bfi->chunk_header = bfi->chunk_size - bfi->video_offset;
> +      av_log(NULL, AV_LOG_DEBUG, "\nReading audio of this chunk...");
> +    }
> +
> +    switch (bfi->avflag) {
> +
> +    case 0:                    //Tossing an audio packet at the audio decoder.
> +        ret = av_get_packet(pb, pkt, bfi->audio_size);
> +        if (ret < 0)
> +            return ret;
> +        pkt->stream_index =  1;
> +        pkt->pts          =  bfi->audio_frame;
> +        bfi->audio_frame  += ret;
> +        pkt->duration     = 1;
> +        bfi->avflag       = 1;
> +        av_log(NULL, AV_LOG_DEBUG,
> +               "\nSending %d bytes to the audio decoder...",
> +               bfi->audio_size);
> +        return ret;
> +
> +    case 1:                    //Tossing a video packet at the video decoder.
> +        ret = av_get_packet(pb, pkt, bfi->video_size);
> +        if (ret < 0)
> +            return ret;
> +        pkt->stream_index =  0;
> +        pkt->pts          =  bfi->video_frame;
> +        bfi->video_frame  += ret / bfi->video_size;
> +        pkt->duration     = 1;
> +        bfi->avflag       = 0;
> +        av_log(NULL, AV_LOG_DEBUG,
> +               "\nSending %d bytes to the video decoder...",
> +               bfi->video_size);
> +        /* One less frame to read. A cursory decrement. */
> +        bfi->nframes--;
> +        return ret;
> +
> +    }

part of the switch can be merged with the if() above


> +    return 0;

unreachable code

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

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- 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/20080411/cad39e14/attachment.pgp>



More information about the ffmpeg-devel mailing list