[FFmpeg-devel] [PATCH] Support for variable frame duration
Henrik Gulbrandsen
henrik
Sat Apr 26 09:17:01 CEST 2008
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? :-(
> > 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.
> 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.
> 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.
> > @@ -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... :-)
> > Index: libavformat/mov.c
>
> Probably ok ...
> baptiste?
I'll cover this patch in a separate email.
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.
/Henrik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: video_duration_ffmpeg_2a.patch
Type: text/x-patch
Size: 2066 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080426/9b8adb99/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: video_duration_ffmpeg_2b.patch
Type: text/x-patch
Size: 826 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080426/9b8adb99/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: video_duration_ffmpeg_3a.patch
Type: text/x-patch
Size: 2947 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080426/9b8adb99/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: video_duration_ffmpeg_3b.patch
Type: text/x-patch
Size: 826 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080426/9b8adb99/attachment-0003.bin>
More information about the ffmpeg-devel
mailing list