[FFmpeg-devel] [PATCH] BFI demuxer

[Vitor Sessak]

Hi

Sisir Koppaka wrote:
> All suggestions in the previous BFI thread were implemented...attached is
> the new patch for the demuxer.
> 
> Once this is committed, I'll continue working on the decoder; now, shuffling
> between both of them and running to and fro from the exam hall, I'm a bit
> out of air....the GSoC deadline is approaching(yikes!), so hopefully a
> commit now will atleast make me eligible for further consideration(and also
> give me something to cheer about...).
> 
> Note: I've removed most of the unnecessary variables...but there are, I
> think, three variables replicated in the local structure as well...only for
> reasons of code readability and the fact that it takes lesser pointer
> accesses to reach them compared to AVCodecContext. I hope this won't be a
> problem.

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...


> +    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...

> +    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;

Also there are some overly long lines that should be broken (<80 chars 
preferred)

(...)

> +      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 ...)
}

> +        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

> +    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)

-Vitor