diff mbox series

[FFmpeg-devel,1/2] avformat/mov: fix timecode with high frame rate content

Message ID 20220410181200.3181-1-cus@passwd.hu
State New
Headers show
Series [FFmpeg-devel,1/2] avformat/mov: fix timecode with high frame rate content | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Marton Balint April 10, 2022, 6:11 p.m. UTC
60 fps content have "Number of Frames" set to 30 in the tmcd atom, but the
frame duration / timescale reflects the original video frame rate.

Therefore we multiply the frame count with the quotient of the rounded timecode
frame rate and the "Number of Frames" per second to get a frame count in the original
(higher) frame rate.

Note that the frames part in the timecode will be in high frame rate which will
make the timecode different to e.g. MediaInfo which seems to show the 30 fps
timecode even for 120 fps content.

Regression since 428b4aacb1a91a267650de644519882a5f700388.

Fixes ticket #9710.
Fixes ticket #9492.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/isom.h |  1 +
 libavformat/mov.c  | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Anton Khirnov April 11, 2022, 7:54 a.m. UTC | #1
Quoting Marton Balint (2022-04-10 20:11:59)
> 60 fps content have "Number of Frames" set to 30 in the tmcd atom, but the
> frame duration / timescale reflects the original video frame rate.
> 
> Therefore we multiply the frame count with the quotient of the rounded timecode
> frame rate and the "Number of Frames" per second to get a frame count in the original
> (higher) frame rate.
> 
> Note that the frames part in the timecode will be in high frame rate which will
> make the timecode different to e.g. MediaInfo which seems to show the 30 fps
> timecode even for 120 fps content.
> 
> Regression since 428b4aacb1a91a267650de644519882a5f700388.
> 
> Fixes ticket #9710.
> Fixes ticket #9492.

Sounds like there should be a test for this.
Marton Balint April 11, 2022, 8:57 p.m. UTC | #2
On Mon, 11 Apr 2022, Anton Khirnov wrote:

> Quoting Marton Balint (2022-04-10 20:11:59)
>> 60 fps content have "Number of Frames" set to 30 in the tmcd atom, but the
>> frame duration / timescale reflects the original video frame rate.
>>
>> Therefore we multiply the frame count with the quotient of the rounded timecode
>> frame rate and the "Number of Frames" per second to get a frame count in the original
>> (higher) frame rate.
>>
>> Note that the frames part in the timecode will be in high frame rate which will
>> make the timecode different to e.g. MediaInfo which seems to show the 30 fps
>> timecode even for 120 fps content.
>>
>> Regression since 428b4aacb1a91a267650de644519882a5f700388.
>>
>> Fixes ticket #9710.
>> Fixes ticket #9492.
>
> Sounds like there should be a test for this.

The smallest file I managed to find which is affected by this is
mov/canon_6d/mvi_9114.mov, but that is still 12 MB, therefore probably not
fit for addition to fate-samples.

With our muxer, the issue is not reproducible, so remuxing is not an
option.

Regards,
Marton
Marton Balint April 20, 2022, 5:59 p.m. UTC | #3
On Mon, 11 Apr 2022, Marton Balint wrote:

>
>
> On Mon, 11 Apr 2022, Anton Khirnov wrote:
>
>>  Quoting Marton Balint (2022-04-10 20:11:59)
>>>  60 fps content have "Number of Frames" set to 30 in the tmcd atom, but
>>>  the
>>>  frame duration / timescale reflects the original video frame rate.
>>>
>>>  Therefore we multiply the frame count with the quotient of the rounded
>>>  timecode
>>>  frame rate and the "Number of Frames" per second to get a frame count in
>>>  the original
>>>  (higher) frame rate.
>>>
>>>  Note that the frames part in the timecode will be in high frame rate
>>>  which will
>>>  make the timecode different to e.g. MediaInfo which seems to show the 30
>>>  fps
>>>  timecode even for 120 fps content.
>>>
>>>  Regression since 428b4aacb1a91a267650de644519882a5f700388.
>>>
>>>  Fixes ticket #9710.
>>>  Fixes ticket #9492.
>>
>>  Sounds like there should be a test for this.
>
> The smallest file I managed to find which is affected by this is
> mov/canon_6d/mvi_9114.mov, but that is still 12 MB, therefore probably not
> fit for addition to fate-samples.
>
> With our muxer, the issue is not reproducible, so remuxing is not an
> option.

Will apply soon.

Regards,
Marton
diff mbox series

Patch

diff --git a/libavformat/isom.h b/libavformat/isom.h
index 5caf42b15d..99408a42d1 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -214,6 +214,7 @@  typedef struct MOVStreamContext {
     int has_palette;
     int64_t data_size;
     uint32_t tmcd_flags;  ///< tmcd track flags
+    uint8_t tmcd_nb_frames;  ///< tmcd number of frames per tick / second
     int64_t track_end;    ///< used for dts generation in fragmented movie files
     int start_pad;        ///< amount of samples to skip due to enc-dec delay
     unsigned int rap_group_count;
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 6c847de164..8527591ff3 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2364,6 +2364,7 @@  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 */
+            tmcd_ctx->tmcd_nb_frames = st->codecpar->extradata[16]; /* number of frames */
             if (size > 30) {
                 uint32_t len = AV_RB32(st->codecpar->extradata + 18); /* name atom length */
                 uint32_t format = AV_RB32(st->codecpar->extradata + 22);
@@ -7893,11 +7894,16 @@  static int mov_read_timecode_track(AVFormatContext *s, AVStream *st)
     FFStream *const sti = ffstream(st);
     int flags = 0;
     int64_t cur_pos = avio_tell(sc->pb);
-    uint32_t value;
+    int64_t value;
+    AVRational tc_rate = st->avg_frame_rate;
+    int rounded_tc_rate;
 
     if (!sti->nb_index_entries)
         return -1;
 
+    if (!tc_rate.num || !tc_rate.den || !sc->tmcd_nb_frames)
+        return -1;
+
     avio_seek(sc->pb, sti->index_entries->pos, SEEK_SET);
     value = avio_rb32(s->pb);
 
@@ -7910,6 +7916,12 @@  static int mov_read_timecode_track(AVFormatContext *s, AVStream *st)
      * No sample with tmcd track can be found with a QT timecode at the moment,
      * despite what the tmcd track "suggests" (Counter flag set to 0 means QT
      * format). */
+
+    /* 60 fps content have tmcd_nb_frames set to 30 but tc_rate set to 60, so
+     * we multiply the frame number with the quotient. */
+    rounded_tc_rate = (tc_rate.num + tc_rate.den / 2) / tc_rate.den;
+    value = av_rescale(value, rounded_tc_rate, sc->tmcd_nb_frames);
+
     parse_timecode_in_framenum_format(s, st, value, flags);
 
     avio_seek(sc->pb, cur_pos, SEEK_SET);