| Message ID | 20201129040838.86636-1-mindmark@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/1] avformat/mov: fix timecode with counter mode flag set | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
On Sat, Nov 28, 2020 at 8:08 PM <mindmark@gmail.com> wrote: > From: Mark Reid <mindmark@gmail.com> > > The current behaviour ends up squaring the avg_frame_rate if the conter > mode flag is set. > This messes up the timecode calculation, and looks to me as a regression > that > seems to have been introduced 428b4aac. > > The new behaviour is use the "Number of frames" field for avg_frame_rate > from the timecode atom as describe here: > > https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html#//apple_ref/doc/uid/TP40000939-CH205-69831 > > Number of frames > An 8-bit integer that contains the number of frames per second for the > timecode format. > If the time is a counter, this is the number of frames for each > counter tick. > > Here is a sample mov file with the counter flag set > https://www.dropbox.com/s/5l4fucb9lhq523s/timecode_counter_mode.mov > > before the patch ffmpeg will report the timecode as: > 00:37:11:97 and warns that the timecode framerate is 576000000/1002001 > > after patch: > 14:50:55:02 > > --- > libavformat/mov.c | 17 +++++------------ > 1 file changed, 5 insertions(+), 12 deletions(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index 2b90e31170..76c1ceb82a 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -2336,24 +2336,17 @@ static int mov_parse_stsd_data(MOVContext *c, > AVIOContext *pb, > tmcd_ctx->tmcd_flags = val; > st->avg_frame_rate.num = AV_RB32(st->codecpar->extradata + > 8); /* timescale */ > st->avg_frame_rate.den = AV_RB32(st->codecpar->extradata + > 12); /* frameDuration */ > -#if FF_API_LAVF_AVCTX > -FF_DISABLE_DEPRECATION_WARNINGS > - st->codec->time_base = av_inv_q(st->avg_frame_rate); > -FF_ENABLE_DEPRECATION_WARNINGS > -#endif > + > /* adjust for per frame dur in counter mode */ > if (tmcd_ctx->tmcd_flags & 0x0008) { > - int timescale = AV_RB32(st->codecpar->extradata + 8); > - int framedur = AV_RB32(st->codecpar->extradata + 12); > - st->avg_frame_rate.num *= timescale; > - st->avg_frame_rate.den *= framedur; > + st->avg_frame_rate.num = st->codecpar->extradata[16] /* > fps, frames per counter tick in counter mode */; > + st->avg_frame_rate.den = 1; > + } > #if FF_API_LAVF_AVCTX > FF_DISABLE_DEPRECATION_WARNINGS > - st->codec->time_base.den *= timescale; > - st->codec->time_base.num *= framedur; > + st->codec->time_base = av_inv_q(st->avg_frame_rate); > FF_ENABLE_DEPRECATION_WARNINGS > #endif > - } > if (size > 30) { > uint32_t len = AV_RB32(st->codecpar->extradata + 18); /* > name atom length */ > uint32_t format = AV_RB32(st->codecpar->extradata + 22); > -- > 2.29.2 > > ping
Quoting mindmark@gmail.com (2020-11-29 05:08:38) > From: Mark Reid <mindmark@gmail.com> > > The current behaviour ends up squaring the avg_frame_rate if the conter mode flag is set. > This messes up the timecode calculation, and looks to me as a regression that > seems to have been introduced 428b4aac. > > The new behaviour is use the "Number of frames" field for avg_frame_rate from the timecode atom as describe here: > https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html#//apple_ref/doc/uid/TP40000939-CH205-69831 > > Number of frames > An 8-bit integer that contains the number of frames per second for the timecode format. > If the time is a counter, this is the number of frames for each counter tick. I'm no expert on mov, but this looks suspicious. avg_frame_rate is supposed to be per seconds, not per counter ticks, whatever those are. Looking at your sample, frame duration is 1001/24k, while the 'number of frames' field is just 24, which seems like it's losing precision. Then wouldn't it be better to just stop squaring the number without any other changes? Or am I misunderstanding this?
On Sun, Dec 13, 2020 at 2:05 AM Anton Khirnov <anton@khirnov.net> wrote: > Quoting mindmark@gmail.com (2020-11-29 05:08:38) > > From: Mark Reid <mindmark@gmail.com> > > > > The current behaviour ends up squaring the avg_frame_rate if the conter > mode flag is set. > > This messes up the timecode calculation, and looks to me as a regression > that > > seems to have been introduced 428b4aac. > > > > The new behaviour is use the "Number of frames" field for avg_frame_rate > from the timecode atom as describe here: > > > https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html#//apple_ref/doc/uid/TP40000939-CH205-69831 > > > > Number of frames > > An 8-bit integer that contains the number of frames per second for > the timecode format. > > If the time is a counter, this is the number of frames for each > counter tick. > > I'm no expert on mov, but this looks suspicious. avg_frame_rate is > supposed to be per seconds, not per counter ticks, whatever those are. > Looking at your sample, frame duration is 1001/24k, while the 'number of > frames' field is just 24, which seems like it's losing precision. > > Then wouldn't it be better to just stop squaring the number without any > other changes? Or am I misunderstanding this? > I'm not entirely clear on what "counter ticks" are either. I think what your saying sounds better, I'll submit a new patch. thanks for reviewing. > > -- > Anton Khirnov > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavformat/mov.c b/libavformat/mov.c index 2b90e31170..76c1ceb82a 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -2336,24 +2336,17 @@ static int mov_parse_stsd_data(MOVContext *c, AVIOContext *pb, tmcd_ctx->tmcd_flags = val; st->avg_frame_rate.num = AV_RB32(st->codecpar->extradata + 8); /* timescale */ st->avg_frame_rate.den = AV_RB32(st->codecpar->extradata + 12); /* frameDuration */ -#if FF_API_LAVF_AVCTX -FF_DISABLE_DEPRECATION_WARNINGS - st->codec->time_base = av_inv_q(st->avg_frame_rate); -FF_ENABLE_DEPRECATION_WARNINGS -#endif + /* adjust for per frame dur in counter mode */ if (tmcd_ctx->tmcd_flags & 0x0008) { - int timescale = AV_RB32(st->codecpar->extradata + 8); - int framedur = AV_RB32(st->codecpar->extradata + 12); - st->avg_frame_rate.num *= timescale; - st->avg_frame_rate.den *= framedur; + st->avg_frame_rate.num = st->codecpar->extradata[16] /* fps, frames per counter tick in counter mode */; + st->avg_frame_rate.den = 1; + } #if FF_API_LAVF_AVCTX FF_DISABLE_DEPRECATION_WARNINGS - st->codec->time_base.den *= timescale; - st->codec->time_base.num *= framedur; + st->codec->time_base = av_inv_q(st->avg_frame_rate); FF_ENABLE_DEPRECATION_WARNINGS #endif - } if (size > 30) { uint32_t len = AV_RB32(st->codecpar->extradata + 18); /* name atom length */ uint32_t format = AV_RB32(st->codecpar->extradata + 22);
From: Mark Reid <mindmark@gmail.com> The current behaviour ends up squaring the avg_frame_rate if the conter mode flag is set. This messes up the timecode calculation, and looks to me as a regression that seems to have been introduced 428b4aac. The new behaviour is use the "Number of frames" field for avg_frame_rate from the timecode atom as describe here: https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html#//apple_ref/doc/uid/TP40000939-CH205-69831 Number of frames An 8-bit integer that contains the number of frames per second for the timecode format. If the time is a counter, this is the number of frames for each counter tick. Here is a sample mov file with the counter flag set https://www.dropbox.com/s/5l4fucb9lhq523s/timecode_counter_mode.mov before the patch ffmpeg will report the timecode as: 00:37:11:97 and warns that the timecode framerate is 576000000/1002001 after patch: 14:50:55:02 --- libavformat/mov.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)