[FFmpeg-devel] BFI Demuxer

[Sisir Koppaka]

Hi, Thanks a lot!

> [...]
>
> > +/*
> > + * Based on http://wiki.multimedia.cx/index.php?title=BFI
> > + */
>
> comment is not doxygen compatible
>
Changed /* to /**

>
>
> > +
> > +#include "avformat.h"
> > +
> > +typedef struct BFIContext {
> > +    int nframes;
> > +    int palette_set;
> > +    int width;
> > +    int height;
> > +    int chunk_header;
> > +    int audio_offset;
> > +    int video_offset;
> > +    int audio_size;
> > +    int video_size;
> > +    int chunk_size;
> > +    int audio_index;
> > +    int video_index;
> > +    int avflag;
>
> > +    int buffer_size;
>
> unused
>
Yes, this is an unnecessary relic...removed. Also bfi->remaining_size falls
into the same category, so it's also removed.

>
>
> > +    int sample_rate;
> > +    int channels;
>
> duplicate
>

Duplicate in the sense of? I'm sorry, I couldn't get you here... :(

Also, talking of channels, the BFI spec does mention :

 bytes 832-835  (+340) audio channels (?)

but with a question mark, and I haven't got it working by taking the
channels value in those bytes so far. It has worked perfectly for several
sample files, however, when channels is manually set to 1.


>
> [...]
> > +static int bfi_read_header(AVFormatContext * s, AVFormatParameters *
> ap)
> > +{
> > +    BFIContext *bfi = s->priv_data;
> > +    ByteIOContext *pb = s->pb;
> > +    AVStream *vstream;
> > +    AVStream *astream;
> > +    int i;
> > +    /* Setting total number of frames, nframes will change while
> nframesOrig will not over the course of execution */
>
> > +    url_fseek(pb, 8, SEEK_SET);
>
> url_fskip()
>

Changing it...


>
>
> [...]
> > +    /*Improving colour depth */
> > +    for (i = 0; i < vstream->codec->extradata_size; i++)
> > +        ((uint8_t *) vstream->codec->extradata)[i] =
> > +            ((uint8_t *) vstream->codec->extradata)[i] << 2;
>
> senseless casts, and the comment makes no sense.
>

I've replaced the above comment with this one - is this ok?
 /* Converts the given 6-bit palette values(0-63) to 8-bit values(0-255) to
improve the contrast. */
About the casts, did you mean that they were not necessary or that the type
wasn't right?

>
>
> > +    av_set_pts_info(vstream, 32, 1, bfi->fps);
> > +    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;
> > +    /* Setting up the audio codec now... */
> > +    astream = av_new_stream(s, 0);      /* shouldn't 0 be 1 here */
> > +    if (!astream)
> > +        return AVERROR(ENOMEM);
>
> > +    bfi->audio_index = astream->index;
>
> unneeded
>
This was just to eliminate any unforeseen errors :) Will change the code to
assume that the first stream declared has index 0 and the second stream has
index 1.

>
>
> [...]
> > +        while (get_byte(pb) != 'I') {
> > +            continue;
> > +        }
>
> infinite loop
>
But that is the intent, right? Usually, the loop proceeds only two, three
times, I think, before finally stopping. I'll put a av_log there and if it's
not too many times, then is it ok to have it?

>
>
> > +        url_fseek(pb, -1, SEEK_CUR);
>
> breaks unseekable input
>
Will remove this...requires altering the MKTAG and some more below...

>
>
> > +        av_log(NULL, AV_LOG_INFO, "\nFound a chunk...");
> > +        if (get_le32(pb) == MKTAG('I', 'V', 'A', 'S')) {
> > +            av_log(NULL, AV_LOG_INFO,
> > +                   "\nChunk number %d confirmed with IVAS
> identifier...",
> > +                   bfi->nframes);
> > +            bfi->chunk_size = get_le32(pb);
> > +            av_log(NULL, AV_LOG_INFO, "\nNext chunk header offset is
> %d",
> > +                   bfi->chunk_size);
> > +            get_le32(pb);
> > +            bfi->audio_offset = get_le32(pb);
> > +            av_log(NULL, AV_LOG_INFO, "\nAudio offset is %d",
> > +                   bfi->audio_offset);
> > +            get_le32(pb);
> > +            bfi->video_offset = get_le32(pb);
> > +            av_log(NULL, AV_LOG_INFO, "\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;
> > +            //url_fseek(pb,bfi->audio_offset - 16, SEEK_CUR);
> > +            av_log(NULL, AV_LOG_INFO, "\nReading audio of this
> chunk...");
> > +//        bfi->remaining_size = bfi->video_offset - bfi->audio_offset;
> > +        } else
> > +            goto move;
> > +    }
> > +    switch (bfi->avflag) {
> > +    case 0:                    //Audio will be sent now.
> > +        pkt->stream_index = bfi->audio_index;
>
> > +        pkt->pts = 1;
>
> as has already been pointet out, this is wrong
>
Working on this...

>
>
> > +        cod->codec_id = CODEC_ID_PCM_U8;
> > +        cod->sample_rate = bfi->sample_rate;
> > +        cod->bits_per_sample = bfi->bits_per_sample;
> > +        cod->bit_rate = cod->sample_rate * cod->bits_per_sample;
>  // * cod->channels ;
> > +        cod->channels = 1;
>
> doing that for each packet makes no sense
>

Yes, I was worried when the audio simply stopped playing and inserted all
this to double-check...guess the read_header ought to suffice. Will change.

>
>
> [...]
>
> > +static int bfi_read_close(AVFormatContext * s)
> > +{
> > +    BFIContext *bfi = s->priv_data;
> > +    av_free(s->streams[bfi->video_index]->codec->extradata);
>
> this is wrong
>

Should we free extradata in the decoder close function in that case?

Once again, thanks for the review.
-----------------
Sisir Koppaka