[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