[FFmpeg-devel] [PATCH] Support for variable frame duration

Michael Niedermayer michaelni
Sat Apr 26 12:21:26 CEST 2008


On Sat, Apr 26, 2008 at 09:17:01AM +0200, Henrik Gulbrandsen wrote:
> On Tue, 2008-04-22 at 21:02 +0200, Michael Niedermayer wrote:
> > On Tue, Apr 22, 2008 at 11:10:49AM +0200, Henrik Gulbrandsen wrote:
> 
> [...]
> 
> > > Two separate patches are attached. The regression tests pass now, after
> > > complaining about two small problems: First, packet duration should have
> > > been interpreted in stream time base units rather than codec time base.
> > > Second, the corrected(?) version of video sync wanted to compensate for
> > > the one-frame MPEG codec latency by duplicating the first frame when it
> > > arrived in time for the second presentation interval. I had to update
> > > sync_opts to keep it from doing that.
> > > 
> > > /Henrik
> > > 
> > 
> > > Index: ffmpeg.c
> > > ===================================================================
> > > --- ffmpeg.c	(revision 12920)
> > > +++ ffmpeg.c	(working copy)
> > > @@ -453,7 +453,7 @@ static double
> > >  get_sync_ipts(const AVOutputStream *ost)
> > >  {
> > >      const AVInputStream *ist = ost->sync_ist;
> > > -    return (double)(ist->pts - start_time)/AV_TIME_BASE;
> > > +    return (double)(ist->next_pts - start_time)/AV_TIME_BASE;
> > >  }
> > >  
> > >  static void write_frame(AVFormatContext *s, AVPacket *pkt, AVCodecContext *avctx, AVBitStreamFilterContext *bsfc){
> > > @@ -1169,12 +1169,24 @@ static int output_packet(AVInputStream *
> > >                          goto fail_decode;
> > >                      if (!got_picture) {
> > >                          /* no picture yet */
> > > +                        for(i = 0; i < nb_ostreams; i++) {
> > > +                            if (ost_table[i]->source_index == ist_index)
> > > +                                ost_table[i]->sync_opts++;
> > > +                        }
> > >                          goto discard_packet;
> > >                      }
> > 
> > This looks wrong
> 
> Well, it does reflect the latency of the input stream. Whether it should
> do that or not isn't entirely clear to me, but something must be done to
> prevent the next frame from being duplicated when it arrives with opts=0
> and must sync to (next) ipts=2. We could also add a latency counter that
> keeps track of this without influencing ipts or opts. Or are you trying
> to point out that some of my basic assumptions are wrong and that the
> code really doesn't make sense at all? :-(

So far it appears to make no sense at all. There might be a small bug in
the video frame duplication code under if(video_sync_method).


> 
> > >                      if (ist->st->codec->time_base.num != 0) {
> > > -                        ist->next_pts += ((int64_t)AV_TIME_BASE *
> > > -                                          ist->st->codec->time_base.num) /
> > > -                            ist->st->codec->time_base.den;
> > > +                        if (pkt) {
> > > +                            ist->next_pts += av_rescale(
> > > +                                (int64_t)pkt->duration * AV_TIME_BASE,
> > > +                                ist->st->time_base.num,
> > > +                                ist->st->time_base.den);
> > > +                        } else {
> > > +                            ist->next_pts += av_rescale(
> > > +                                (int64_t)AV_TIME_BASE,
> > > +                                ist->st->codec->time_base.num,
> > > +                                ist->st->codec->time_base.den);
> > > +                        }
> > >                      }
> > 
> > these are really 2 changes,
> > first changing to av_rescale()
> > second considering pkt->duration
> > 
> > first is ok (if it works, passes regressions and is in a seperate patch)
> 
> Sure. I guess it's a cosmetic fix, but I included it since I had to
> change the indentation anyway, and it increased the symmetry :-)
> On the other hand, there is probably no point in using av_rescale()
> here, since there is no risk of overflow. Therefore, I've reverted the
> change and made both blocks use the basic operators. The indentation
> change is attached as a separate cosmetic patch.

The current code cannot overflow, your new code though can.


> 
> > second contains 2 bugs
> > A. pkt->duration can be 0 (=unknown)
> 
> Looking at compute_frame_duration(..), I'd say that this can not happen
> for video packets, but since the check is so easy, I'll add it anyway.

It can happen.


> 
> > B. even though rare some demuxers-codecs have no means to split packets
> >    into frames thus a packet can contain several frames in which case
> >    pkt->duration is no longer correct here.
> > 
> > 
> > >                      len = 0;
> > >                      break;
> 
> Correct me if I'm wrong, but that "len = 0;" seems to indicate that the
> multi-frame objection is not relevant here. It looks like only audio
> packets can contain multiple frames, so video-only code should be safe.

No code in ffmpeg.c can prevent a demxuer from sending multiple video
frames together. Normal they should be split, but as with some
audio codecs, its possible that spliting requires decoding.
The current code might not support such multiple video frames in a
packet but that doesnt mean its ok to add more code which knowingly
will fail for that.


> 
> > > @@ -1257,7 +1269,7 @@ static int output_packet(AVInputStream *
> > >  #endif
> > >          /* if output time reached then transcode raw format,
> > >             encode packets and output them */
> > > -        if (start_time == 0 || ist->pts >= start_time)
> > > +        if (start_time == 0 || ist->next_pts >= start_time)
> > >              for(i=0;i<nb_ostreams;i++) {
> > >                  int frame_size;
> > >  
> > 
> > this looks wrong, it should at least be > not >=
> 
> You are absolutely correct! The worst part is that I have a vague memory
> that it used to be >, but I changed it in a late update with an unworded
> thought that would translate to something like "I'll keep this and then
> someone will complain about it; minimal is better than correct at this
> point..."; don't ask me to explain it... :-)
> 
[...]
> You have a choice of patches now: either 2a + 2b, which keep the current
> sync_opts update, or 3a + 3b that use a tentative latency counter. I'm
> happy to come back with 4a + 4b if you have better ideas. In any case,
> the regression tests still seem to work.

Could you split the patch a little more please, even though small, many
of the changes are unrelated and should be in seperate patches.

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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20080426/973e50b9/attachment.pgp>



More information about the ffmpeg-devel mailing list