On 2021-12-28 05:18 am, Michael Niedermayer wrote:
On Tue, Dec 28, 2021 at 01:33:54AM +0530, Gyan Doshi wrote:
On 2021-12-28 12:38 am, Michael Niedermayer wrote:
On Mon, Dec 27, 2021 at 11:27:10AM +0530, Gyan Doshi wrote:
As per ISO 14496-12, sample duration of 0 is invalid except for
the last entry.
In addition, also catch 0 value for sample count.
---
libavformat/mov.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 2aed6e80ef..fb7406cdd6 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2968,6 +2968,18 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb,
MOVAtom atom)
av_log(c->fc, AV_LOG_TRACE, "sample_count=%d, sample_duration=%d\n",
sample_count, sample_duration);
+ if (!sample_count) {
+ av_log(c->fc, AV_LOG_WARNING, "invalid sample count of 0 in stts for st %d
at entry %u; changing to 1.\n",
+ c->fc->nb_streams-1, i);
+ sc->stts_data[i].count = sample_count = 1;
+ }
+
+ if (!sample_duration && i != entries-1) {
+ av_log(c->fc, AV_LOG_WARNING, "invalid sample delta of 0 in stts for st %d
at entry %u; changing to 1.\n",
+ c->fc->nb_streams-1, i);
+ sc->stts_data[i].duration = sample_duration = 1;
+ }
+
duration+=(int64_t)sample_duration*(uint64_t)sample_count;
total_sample_count+=sample_count;
This does not produce the same output
tickets/2096/m.f4v
videos/stretch.mov (2344 matches for "invalid" after this patch)
tickets/976/CodecCopyFailing.mp4
But there are many more, some maybe even generated by FFmpeg
Where do I find these files?
https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket976/CodecCopyFailing.mp4
https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket2096/m.f4v
i failed to find the 3rd online
Taking a step back, the problem started with
203b0e3561dea1ec459be226d805abe73e7535e5
which broke a real world file which was outside the specification
Just to clarify, it did not break that file. That file uses stts in an
unusual way.
Before 2015, lavf exported packets with the same desync as the other
demuxers do so till today.
Andreas' patch added a hack to make it play in sync. My patch 203b0e356
broke that hack.
The patch for max_stts_delta is a way to restore it back.
you then suggested a fix which crashed with some fuzzed files which
where outside the specification
and now this fix on top which changes real world files which
are outside the specification
I think, maybe you should consider the "outside the specification"
more. The code above directly and intentionally changes values.
So as a reviewer i have to ask the obvious, is that change a
bugfix or a bug ?
Not surprising that the output of out-of-spec files is different - that's
expected, intended and trivial.
It would be a bug if in-spec files were treated differently. FATE passes.
If there's a specific / "correct" playback for these files like sync issues,
I'll see if I can restore it.
First we need to find cases that broke. I certainly will not find every
If a patch is writen with the goal "dont break any file" it would be easy
But you said that changes are "expected, intended" so then my question
as reviewer would be what about these expected changes ?
Did it change any real files output? did it fix a bug?
What is the idea behind the change ?
please correct me if iam wrong but
Here it seems you dont care what happens with changed files unless
someone else finds such a file and reports it. (if its not in spec)
And the idea seems that 0 is inconvenient so you change it to 1
Its not that 0 could fundamentally not be intended to mean 0
We are before a release and id like to fix the regression
ATM objectively the only option i have is reverting
203b0e3561dea1ec459be226d805abe73e7535e5
can you provide another option ? something that fixes the regression
without breaking something else ?
1) The first priority ought to be to not mishandle compliant files
2) Subject to 1, we should accommodate for out-of-spec files as much as
we can.
So if you revert 203b0e3561d and do nothing else, lavf can then fail to
parse some valid files.
The 'regression' then is the loss to accommodate some out-of-spec files.
I've now seen the two files you linked to. It turns out that they appear
to use 0 delta values for
a similar purpose as mp4-negative-stts-problem.mp4. And I can
accommodate that use in my patches.
Updated patches have been sent.
I'm able to remux both CodecCopyFailing.mp4 and m.f4v. Their playback
looks acceptably similar and in sync.
Any individual timestamp changes don't accumulate and cause drift.
Regards,
Gyan
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
