[FFmpeg-devel] [PATCH] BFI demuxer

[Michael Niedermayer]

On Sat, Apr 12, 2008 at 12:30:38AM +0530, 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.

After a quick look i see at least 9 unneeded varibles in BFIContext


[...]
> +    /* Setting total number of frames. */
> +    url_fseek(pb, 8, SEEK_CUR);

url_fskip()


> +    bfi->chunk_header = get_le32(pb);
> +    bfi->nframes = get_le32(pb);
> +    bfi->audio_frame = 0;
> +    bfi->video_frame = 0;
> +    get_le32(pb);
> +    get_le32(pb);
> +    get_le32(pb);
> +    bfi->fps = get_le32(pb);
> +    get_le32(pb);
> +    url_fseek(pb, 44, SEEK_SET);
> +    bfi->width = get_le32(pb);
> +    bfi->height = get_le32(pb);

> +    url_fseek(pb, 828, SEEK_SET);
> +    bfi->sample_rate = get_le32(pb);
> +    get_le32(pb);

> +    bfi->channels = 1;

unused


> +
> +    /* Setting up the video codec... */
> +    vstream = av_new_stream(s, 0);
> +    if (!vstream)
> +        return AVERROR(ENOMEM);
> +
> +    /*Loading palette to extradata */
> +    url_fseek(pb, 60, SEEK_SET);

backward seeking


> +    vstream->codec->extradata = av_malloc(768);
> +    vstream->codec->extradata_size = 768;
> +    get_buffer(pb, vstream->codec->extradata,
> +               vstream->codec->extradata_size);
> +

> +    /* Converts the given 6-bit palette values(0-63) to 8-bit values(0-255) */
> +    for (i = 0; i < vstream->codec->extradata_size; i++)
> +        (vstream->codec->extradata)[i] =
> +            ((vstream->codec->extradata)[i] << 2) | ((vstream->codec->
> +                                                      extradata)[i] >> 4);

global header convertion does not belong in the demuxer.


[...]
> +    /* Setting up the audio codec now... */
> +    astream = av_new_stream(s, 0);
> +    if (!astream)
> +        return AVERROR(ENOMEM);
> +    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);

this can be done simpler


> +    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 ;
> +    url_fseek(pb, bfi->chunk_header - 2, SEEK_SET);
> +    av_set_pts_info(astream, 64, 1, astream->codec->sample_rate);
> +
> +    /* Almost done...initialize a flag and a couple of local variables and we're done with reading the header. */
> +    av_log(NULL, AV_LOG_DEBUG,
> +           "\nFinished reading the headers....starting to read the chunks...");

> +    bfi->audio_offset = 0;
> +    bfi->video_offset = 0;
> +    bfi->avflag = 0;

The context is automatically initialized to 0


[...]
> +        /* Trying to confirm the chunk by matching the header... */
> +      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;

infinite loop


[...]
> +        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;

wrong


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

these contradict each other


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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/50991357/attachment.pgp>