| Message ID | 98f55a71-2854-904e-d681-e6eebcf2e12b@mail.de |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavf/mov: Always prefer tfdt over sidx | expand |
| Context | Check | Description |
|---|---|---|
| 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 |
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
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 },
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
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 --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),
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(-)