Re: [FFmpeg-devel] [PATCH] avformat/mov: correct 0 valued entries in stts

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 2735 bytes --]

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


Taking a step back, the problem started with 
203b0e3561dea1ec459be226d805abe73e7535e5
which broke a real world file which was outside the specification

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 ?
The change refers to the specification but the specification
will not help me when it to comes to how to handle all the wierd
and wonderful files the exist out there ...

thx

PS: also if you want to write fate tests for some of the odd files we find
in the process here, this may be a good idea and might simplify future work

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".