[FFmpeg-devel] [PATCH] Support for variable frame duration
Michael Niedermayer
michaelni
Tue Apr 22 21:02:48 CEST 2008
On Tue, Apr 22, 2008 at 11:10:49AM +0200, Henrik Gulbrandsen wrote:
> On Fri, Apr 18, 2008 at 07:06:00AM +0200, Henrik Gulbrandsen wrote:
> > Well, time for the bug I am really trying to fix. What I have is a small
> > slide show generated as a set of timed JPEG images in an MP4 container.
> > This file should be converted to various other video formats, but FFmpeg
> > currently doesn't cope well with randomly varying frame durations.
> >
> > The original set of test slides and Ogg Theora files generated before
> > and after my preliminary patch attempt are found at the following URLs:
> >
> > http://www.gulbra.net/ffmpeg-devel/movies/slides.mov
> > http://www.gulbra.net/ffmpeg-devel/movies/before_video_duration_patch.ogv
> > http://www.gulbra.net/ffmpeg-devel/movies/after_video_duration_patch.ogv
> >
> > Attached output of the mp4info and mp4videoinfo tools from MPEG4IP shows
> > what the slides.mov file looks like. It's exactly 24 seconds long, with
> > new slides appearing at 1, 2, 3, 4, 6, 8, 12, and 16 seconds. Each slide
> > shows the slide number and the time when it should ideally be presented.
> >
> > Now, in an attempt to make a minimal patch set, I am trying to fix the
> > problem by hooking into the existing video synchronization code. This is
> > based on syncing all the way up to the next PTS, so it requires correct
> > duration info from the demuxer. Replacing our pts with next_pts actually
> > does the right thing (in terms of frame count) without other changes, so
> > there may potentially have been an off-by-one problem in the past...
> >
> > These changes are more central, more extensive, and probably much more
> > controversial than my earlier patches, so feel free to flame me now! :-)
> > I haven't checked the regression tests either, but will be back with any
> > needed updates within a day or two. Right now, I have a train to catch.
>
> On Fri, 2008-04-18 at 14:10 +0200, Michael Niedermayer wrote:
> > The change to mov demuxer change might be ok (ill leave it to baptiste to
> > review)
> > Ill review the ffmpeg change after you confirm that the regression tests
> > do passs.
> > Also mov and ffmpeg changes are independant and should be 2 seperate
> > patches.
>
> 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
> 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)
second contains 2 bugs
A. pkt->duration can be 0 (=unknown)
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;
> @@ -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 >=
> Index: libavformat/mov.c
Probably ok ...
baptiste?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/20080422/397fdfa0/attachment.pgp>
More information about the ffmpeg-devel
mailing list