[FFmpeg-devel] [PATCH] BFI demuxer

[Sisir Koppaka]

Thanks!

On Sat, Apr 12, 2008 at 3:06 AM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> On Sat, Apr 12, 2008 at 02:43:41AM +0530, Sisir Koppaka wrote:
> > Thanks! Updated patch attached.
> >
> [...]
> > +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
>
Removed all of them. Now the only variables present are:
    int nframes;   //Reqd. to keep track of frames...
    int audio_frame;  // pts variable
    int video_frame;  // pts variable
    int video_size;     //created and required in different instances of a
function hence...
    int avflag;    // toggle variable

>
>
> [...]
> > +    /*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
>
Fixed.

>
>
> > +        /* 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?
>
> Yes, one of my files was damaged because it seemed to skip some frames but
I checked up the samples on the site now, and it looks like I must have
damaged the local files by mistake while hexediting. I've changed the above
code.

>
>
> > +      /* 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;
>
This part of the pkt-> setting should come after av_get_packet, so couldn't
be moved...

>
> > +        bfi->avflag       = 1;

Ideally, the flag should be set right before returning from the function to
make sure all conditions are met. Also, moving it to the if else above, and
hence pre-empting the flag state,seems to be causing problems when the size
of the audio/video parts of a chunk is 0.(from looking at the av_logs)

>
> > +        av_log(NULL, AV_LOG_DEBUG,
> > +               "\nSending %d bytes to the audio decoder...",
> > +               bfi->audio_size);
>
This could be moved, but logically, this should appear only when all
conditions are met and so we would be pre-empting in that case.

If ret<0 then an alternate exit is possible and if we moved the avflag and
av_log to the if else, then we wouldn't know which return was executed.

>
> > +        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);
>
Same reasons as above so far.

>
> > +        /* One less frame to read. A cursory decrement. */
> > +        bfi->nframes--;

Again, if we move this earlier, then we would be pre-empting and losing out
some info in the av_logs since an alternate return exists before this and
nframes-- helps us to know which return was executed.

>
> > +        return ret;
> > +
> > +    }
>
> part of the switch can be merged with the if() above
>

>
> > +    return 0;
>
> unreachable code
>
GCC gives a warning in it's absence:

bfi.c:193: warning: control reaches end of non-void function

-----------------
Sisir Koppaka