[FFmpeg-devel] [PATCH] BFI demuxer

[Michael Niedermayer]

On Sat, Apr 12, 2008 at 12:26:02PM +0530, Sisir Koppaka wrote:
[...]
> >
> >
> > > +      /* 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,

yes


> so couldn't
> be moved...

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

I suggest you remove all av_log(NULL, AV_LOG_DEBUG,
They do not really belong in finished code which is what would be commited to
ffmpeg.


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

And same awnser, remove it!


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

same


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

that will disapear when the switch and if are merged ...

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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/20080412/2bb781ff/attachment.pgp>