diff mbox series

[FFmpeg-devel] lavf/mov: Always prefer tfdt over sidx

Message ID 98f55a71-2854-904e-d681-e6eebcf2e12b@mail.de
State New
Headers show
Series [FFmpeg-devel] lavf/mov: Always prefer tfdt over sidx
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed
andriy/make_ppc success Make finished
andriy/make_fate_ppc fail Make fate failed
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed

Commit Message

Thilo Borgmann Nov. 3, 2021, 9:46 a.m. UTC
Hi,

this effectively reverts 071930de724166bfb90fc6d368c748771188fd94 and fixes the underlying issue by always preferring TFDT.

Furthermore, the current solution fails if the -use_tfdt flag is given but no TFDT box is found in the stream.

-Thilo
From 1ae6d7f213d0ba2360b9b1cf7fde2e3d0adde70d Mon Sep 17 00:00:00 2001
From: Thilo Borgmann <thilo.borgmann@mail.de>
Date: Tue, 2 Nov 2021 22:49:37 +0100
Subject: [PATCH] lavf/mov: Always prefer tfdt over sidx

---
 doc/demuxers.texi  |  6 ------
 libavformat/isom.h |  1 -
 libavformat/mov.c  | 12 +++++-------
 3 files changed, 5 insertions(+), 14 deletions(-)

Comments

Gyan Doshi Nov. 3, 2021, 4:41 p.m. UTC | #1
On 2021-11-03 03:16 pm, Thilo Borgmann wrote:
> Hi,
>
> this effectively reverts 071930de724166bfb90fc6d368c748771188fd94 and fixes the underlying issue by always preferring TFDT.
>
> Furthermore, the current solution fails if the -use_tfdt flag is given but no TFDT box is found in the stream.

Thanks for the heads-up.

The original impetus for my patch was a client's sample file which had 
rubbish sidx values but sane tfdt ones, likely due to a buggy muxer.
The motivation for an option was to give the user control.

I think hardcoding a reversed preference can lead to the same impasse. 
How about change the default value of use_tfdt and check for tfdt 
presence when enabled?

Regards,
Gyan
Thilo Borgmann Nov. 4, 2021, 11:03 a.m. UTC | #2
Am 03.11.21 um 17:41 schrieb Gyan Doshi:
> 
> 
> On 2021-11-03 03:16 pm, Thilo Borgmann wrote:
>> Hi,
>>
>> this effectively reverts 071930de724166bfb90fc6d368c748771188fd94 and fixes the underlying issue by always preferring TFDT.
>>
>> Furthermore, the current solution fails if the -use_tfdt flag is given but no TFDT box is found in the stream.
> 
> Thanks for the heads-up.
> 
> The original impetus for my patch was a client's sample file which had rubbish sidx values but sane tfdt ones, likely due to a buggy muxer.
> The motivation for an option was to give the user control.
> 
> I think hardcoding a reversed preference can lead to the same impasse. How about change the default value of use_tfdt and check for tfdt presence when enabled?

Changed the default to use TFDT and add warnings about fallback to SIDX or TFDT where useful.
Looks better?

Thanks,
Thilo
From 435196bbb406ed7d4311f327d96664cb140af048 Mon Sep 17 00:00:00 2001
From: Thilo Borgmann <thilo.borgmann@mail.de>
Date: Thu, 4 Nov 2021 12:00:07 +0100
Subject: [PATCH] lavf/mov: Change default to prefer TFDT time and allow for
 fallback to SIDX or TFDT

---
 doc/demuxers.texi |  2 +-
 libavformat/mov.c | 42 ++++++++++++++++++++++++++++--------------
 2 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/doc/demuxers.texi b/doc/demuxers.texi
index 1c9575b2e8..cab8a7072c 100644
--- a/doc/demuxers.texi
+++ b/doc/demuxers.texi
@@ -691,7 +691,7 @@ Don't use mfra box to set timestamps
 
 @item use_tfdt
 For fragmented input, set fragment's starting timestamp to @code{baseMediaDecodeTime} from the @code{tfdt} box.
-Default is disabled, which will preferentially use the @code{earliest_presentation_time} from the @code{sidx} box.
+Default is enabled, which will prefer to use the @code{tfdt} box to set DTS. Disable to use the @code{earliest_presentation_time} from the @code{sidx} box.
 In either case, the timestamp from the @code{mfra} box will be used if it's available and @code{use_mfra_for} is
 set to pts or dts.
 
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 3fcb1dc908..8a910a3165 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4828,20 +4828,34 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom)
             dts = frag_stream_info->first_tfra_pts;
             av_log(c->fc, AV_LOG_DEBUG, "found mfra time %"PRId64
                     ", using it for dts\n", pts);
-        } else if (frag_stream_info->sidx_pts != AV_NOPTS_VALUE && !c->use_tfdt) {
-            // FIXME: sidx earliest_presentation_time is *PTS*, s.b.
-            // pts = frag_stream_info->sidx_pts;
-            dts = frag_stream_info->sidx_pts - sc->time_offset;
-            av_log(c->fc, AV_LOG_DEBUG, "found sidx time %"PRId64
-                    ", using it for pts\n", pts);
-        } else if (frag_stream_info->tfdt_dts != AV_NOPTS_VALUE) {
-            dts = frag_stream_info->tfdt_dts - sc->time_offset;
-            av_log(c->fc, AV_LOG_DEBUG, "found tfdt time %"PRId64
-                    ", using it for dts\n", dts);
         } else {
-            dts = sc->track_end - sc->time_offset;
-            av_log(c->fc, AV_LOG_DEBUG, "found track end time %"PRId64
-                    ", using it for dts\n", dts);
+            int has_tfdt = frag_stream_info->tfdt_dts != AV_NOPTS_VALUE;
+            int has_sidx = frag_stream_info->sidx_pts != AV_NOPTS_VALUE;
+            int fallback_tfdt = !c->use_tfdt && !has_sidx && has_tfdt;
+            int fallback_sidx =  c->use_tfdt && !has_tfdt && has_sidx;
+
+            if (fallback_sidx) {
+                av_log(c->fc, AV_LOG_DEBUG, "use_tfdt set but no tfdt found, using sidx instead\n");
+            }
+            if (fallback_tfdt) {
+                av_log(c->fc, AV_LOG_DEBUG, "use_tfdt not set but no sidx found, using tfdt instead\n");
+            }
+
+            if (has_tfdt && c->use_tfdt || fallback_tfdt) {
+                dts = frag_stream_info->tfdt_dts - sc->time_offset;
+                av_log(c->fc, AV_LOG_DEBUG, "found tfdt time %"PRId64
+                        ", using it for dts\n", dts);
+            } else if (has_sidx && !c->use_tfdt || fallback_sidx) {
+                // FIXME: sidx earliest_presentation_time is *PTS*, s.b.
+                // pts = frag_stream_info->sidx_pts;
+                dts = frag_stream_info->sidx_pts - sc->time_offset;
+                av_log(c->fc, AV_LOG_DEBUG, "found sidx time %"PRId64
+                        ", using it for pts\n", pts);
+            } else {
+                dts = sc->track_end - sc->time_offset;
+                av_log(c->fc, AV_LOG_DEBUG, "found track end time %"PRId64
+                        ", using it for dts\n", dts);
+            }
         }
     } else {
         dts = sc->track_end - sc->time_offset;
@@ -8533,7 +8547,7 @@ static const AVOption mov_options[] = {
         FLAGS, "use_mfra_for" },
     {"pts", "pts", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_MFRA_PTS}, 0, 0,
         FLAGS, "use_mfra_for" },
-    {"use_tfdt", "use tfdt for fragment timestamps", OFFSET(use_tfdt), AV_OPT_TYPE_BOOL, {.i64 = 0},
+    {"use_tfdt", "use tfdt for fragment timestamps", OFFSET(use_tfdt), AV_OPT_TYPE_BOOL, {.i64 = 1},
         0, 1, FLAGS},
     { "export_all", "Export unrecognized metadata entries", OFFSET(export_all),
         AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, .flags = FLAGS },
Gyan Doshi Nov. 5, 2021, 3:52 a.m. UTC | #3
On 2021-11-04 04:33 pm, Thilo Borgmann wrote:
> Am 03.11.21 um 17:41 schrieb Gyan Doshi:
>>
>> On 2021-11-03 03:16 pm, Thilo Borgmann wrote:
>>> Hi,
>>>
>>> this effectively reverts 071930de724166bfb90fc6d368c748771188fd94 and fixes the underlying issue by always preferring TFDT.
>>>
>>> Furthermore, the current solution fails if the -use_tfdt flag is given but no TFDT box is found in the stream.
>> Thanks for the heads-up.
>>
>> The original impetus for my patch was a client's sample file which had rubbish sidx values but sane tfdt ones, likely due to a buggy muxer.
>> The motivation for an option was to give the user control.
>>
>> I think hardcoding a reversed preference can lead to the same impasse. How about change the default value of use_tfdt and check for tfdt presence when enabled?
> Changed the default to use TFDT and add warnings about fallback to SIDX or TFDT where useful.
> Looks better?

LGTM.

Thanks,
Gyan
Thilo Borgmann Nov. 5, 2021, 3:23 p.m. UTC | #4
Am 05.11.21 um 04:52 schrieb Gyan Doshi:
> 
> 
> On 2021-11-04 04:33 pm, Thilo Borgmann wrote:
>> Am 03.11.21 um 17:41 schrieb Gyan Doshi:
>>>
>>> On 2021-11-03 03:16 pm, Thilo Borgmann wrote:
>>>> Hi,
>>>>
>>>> this effectively reverts 071930de724166bfb90fc6d368c748771188fd94 and fixes the underlying issue by always preferring TFDT.
>>>>
>>>> Furthermore, the current solution fails if the -use_tfdt flag is given but no TFDT box is found in the stream.
>>> Thanks for the heads-up.
>>>
>>> The original impetus for my patch was a client's sample file which had rubbish sidx values but sane tfdt ones, likely due to a buggy muxer.
>>> The motivation for an option was to give the user control.
>>>
>>> I think hardcoding a reversed preference can lead to the same impasse. How about change the default value of use_tfdt and check for tfdt presence when enabled?
>> Changed the default to use TFDT and add warnings about fallback to SIDX or TFDT where useful.
>> Looks better?
> 
> LGTM.

Pushed with updated reference for tests/ref/seek/extra-mp4.

Thanks!
-Thilo
diff mbox series

Patch

diff --git a/doc/demuxers.texi b/doc/demuxers.texi
index 1c9575b2e8..fa669f88fe 100644
--- a/doc/demuxers.texi
+++ b/doc/demuxers.texi
@@ -689,12 +689,6 @@  Set mfra timestamps as PTS
 Don't use mfra box to set timestamps
 @end table
 
-@item use_tfdt
-For fragmented input, set fragment's starting timestamp to @code{baseMediaDecodeTime} from the @code{tfdt} box.
-Default is disabled, which will preferentially use the @code{earliest_presentation_time} from the @code{sidx} box.
-In either case, the timestamp from the @code{mfra} box will be used if it's available and @code{use_mfra_for} is
-set to pts or dts.
-
 @item export_all
 Export unrecognized boxes within the @var{udta} box as metadata entries. The first four
 characters of the box type are set as the key. Default is false.
diff --git a/libavformat/isom.h b/libavformat/isom.h
index f3c18c95be..55e5fa7b23 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -279,7 +279,6 @@  typedef struct MOVContext {
     int moov_retry;
     int use_mfra_for;
     int has_looked_for_mfra;
-    int use_tfdt;
     MOVFragmentIndex frag_index;
     int atom_depth;
     unsigned int aax_mode;  ///< 'aax' file has been detected
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 3fcb1dc908..00205bb1be 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4828,16 +4828,16 @@  static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom)
             dts = frag_stream_info->first_tfra_pts;
             av_log(c->fc, AV_LOG_DEBUG, "found mfra time %"PRId64
                     ", using it for dts\n", pts);
-        } else if (frag_stream_info->sidx_pts != AV_NOPTS_VALUE && !c->use_tfdt) {
+        } else if (frag_stream_info->tfdt_dts != AV_NOPTS_VALUE) {
+            dts = frag_stream_info->tfdt_dts - sc->time_offset;
+            av_log(c->fc, AV_LOG_DEBUG, "found tfdt time %"PRId64
+                    ", using it for dts\n", dts);
+        } else if (frag_stream_info->sidx_pts != AV_NOPTS_VALUE) {
             // FIXME: sidx earliest_presentation_time is *PTS*, s.b.
             // pts = frag_stream_info->sidx_pts;
             dts = frag_stream_info->sidx_pts - sc->time_offset;
             av_log(c->fc, AV_LOG_DEBUG, "found sidx time %"PRId64
                     ", using it for pts\n", pts);
-        } else if (frag_stream_info->tfdt_dts != AV_NOPTS_VALUE) {
-            dts = frag_stream_info->tfdt_dts - sc->time_offset;
-            av_log(c->fc, AV_LOG_DEBUG, "found tfdt time %"PRId64
-                    ", using it for dts\n", dts);
         } else {
             dts = sc->track_end - sc->time_offset;
             av_log(c->fc, AV_LOG_DEBUG, "found track end time %"PRId64
@@ -8533,8 +8533,6 @@  static const AVOption mov_options[] = {
         FLAGS, "use_mfra_for" },
     {"pts", "pts", 0, AV_OPT_TYPE_CONST, {.i64 = FF_MOV_FLAG_MFRA_PTS}, 0, 0,
         FLAGS, "use_mfra_for" },
-    {"use_tfdt", "use tfdt for fragment timestamps", OFFSET(use_tfdt), AV_OPT_TYPE_BOOL, {.i64 = 0},
-        0, 1, FLAGS},
     { "export_all", "Export unrecognized metadata entries", OFFSET(export_all),
         AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, .flags = FLAGS },
     { "export_xmp", "Export full XMP metadata", OFFSET(export_xmp),