From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id E099F40D7D for ; Thu, 30 Dec 2021 17:09:32 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1B07068ACC3; Thu, 30 Dec 2021 19:09:30 +0200 (EET) Received: from mout-p-103.mailbox.org (mout-p-103.mailbox.org [80.241.56.161]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 8303868A8C7 for ; Thu, 30 Dec 2021 19:09:23 +0200 (EET) Received: from smtp2.mailbox.org (smtp2.mailbox.org [IPv6:2001:67c:2050:105:465:1:2:0]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-103.mailbox.org (Postfix) with ESMTPS id 4JPvrf3PYFzQkln for ; Thu, 30 Dec 2021 18:09:22 +0100 (CET) X-Virus-Scanned: amavisd-new at heinlein-support.de Message-ID: <22978838-0457-c659-f825-4af3082552ca@gyani.pro> Date: Thu, 30 Dec 2021 22:39:05 +0530 MIME-Version: 1.0 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <20211227055711.63060-1-ffmpeg@gyani.pro> <20211227190841.GW2829255@pb2> <1df7ec4a-45cd-a6b7-2096-afaa19d09d08@gyani.pro> <20211227234803.GX2829255@pb2> <66581536-ca84-4c2f-7dd9-f39ea2389244@gyani.pro> <20211229122829.GZ2829255@pb2> <566297af-bbf2-d2c1-7279-84596b2a69d1@gyani.pro> <20211229180850.GB2829255@pb2> <62684d93-1f0f-bf70-e7b8-37f887e9fc5c@gyani.pro> <20211230141634.GC2829255@pb2> From: Gyan Doshi In-Reply-To: <20211230141634.GC2829255@pb2> Subject: Re: [FFmpeg-devel] [PATCH] avformat/mov: correct 0 valued entries in stts X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On 2021-12-30 07:46 pm, Michael Niedermayer wrote: > On Thu, Dec 30, 2021 at 10:07:21AM +0530, Gyan Doshi wrote: >> >> On 2021-12-29 11:38 pm, Michael Niedermayer wrote: >>> On Wed, Dec 29, 2021 at 09:39:34PM +0530, Gyan Doshi wrote: >>>> On 2021-12-29 05:58 pm, Michael Niedermayer wrote: >>>>> On Tue, Dec 28, 2021 at 10:26:42PM +0530, Gyan Doshi wrote: >>>>>> 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. >>>>> Sounds good but that alone doesnt work well >>>>> The codebase is iteratively written, the demuxers move in steps towards >>>>> fewer bugs, more wide file support, better maintainability. >>>>> if you start with a demuxer which supports 500 files and then >>>>> it supports 550 then 700 then 800 and so on eventually it reaches 999 >>>>> and now someone finds a spec compliance bug and fixes it so 1 more file >>>>> is supported that developer has to try to not break past efforts of >>>>> supporting other files. Because otherwise alot of past work is thrown >>>>> out and thats just bad. Bad for users, bad for the people who did that work >>>> It depends if all files beyond the first 500 are out-of-spec files. If they >>>> are it suggests a spec widely ignored. >>>> >>>> In this specific case however, I haven't seen any complaints about broken >>>> MP4 demuxing outside of your samples. >>>> I am on Stack Overflow and other popular support forums where users tend to >>>> first go to ask or complain. >>>> FATE which is meant to catch undesirable behaviour passes. Both those things >>>> tell me that funky files whose demuxing >>>> has changed constitute a very tiny set of files at most. And my latest set >>> have you considered using https://samples.ffmpeg.org/ as a set of files to >>> test ? That should be a broader set than fate >> If some behaviour change is to be checked for, it should be in FATE. That's >> the point of a regression test suite. > i do agree, and i very much welcome everyone adding tests to fate to > improve its coverage. > > >> I see dozens, if not hundreds, of files in /mov and /ffmpeg-bugs > There should be around a thousand files that are parseable by the mov demuxer > in there, the whole archive is something between 100-200gb (not just such files) > > >> Is there anything specifc you have in mind? > yes > if a patch adds support for unsigned STTS, NO file thats not identified > as unsigned STTS should change. This is what my original patch for max_stts_delta did. It used a default value of INT_MAX so all deltas identified as negative pre-option would still be treated as negative post-option. But you asked for a different default value. > If a patch changes files, then the explanation cannot be > "its convenient for the implementation" without good evidence that this > convenience doesnt worsen the functionality. > That requires probably extensive testing, finding files that are handled > differently should be easy to do automatically. I'll survey all mov demuxer files in the sample suite over the weekend. But note the 0-value adjustment patch is a separate and detachable issue from max_stts_delta. You can test the latest version of max_stts_delta patch with the current default value or the original default of INT_MAX and it should not break demux or sync relative to 203b0e3561~ 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-request@ffmpeg.org with subject "unsubscribe".