[FFmpeg-devel] [PATCH] BFI demuxer

[Sisir Koppaka]

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...
-----------------
Sisir Koppaka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bfi_demuxer_02.patch
Type: text/x-diff
Size: 9188 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080412/f4fabcdf/attachment.patch>