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 E522440D94 for ; Thu, 30 Dec 2021 18:13:25 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3E21568AE9A; Thu, 30 Dec 2021 20:13:23 +0200 (EET) Received: from mout-p-102.mailbox.org (mout-p-102.mailbox.org [80.241.56.152]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id F13A368A569 for ; Thu, 30 Dec 2021 20:13:16 +0200 (EET) Received: from smtp202.mailbox.org (smtp202.mailbox.org [80.241.60.245]) (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-102.mailbox.org (Postfix) with ESMTPS id 4JPxGN0ykMzQjYK for ; Thu, 30 Dec 2021 19:13:16 +0100 (CET) X-Virus-Scanned: amavisd-new at heinlein-support.de Message-ID: Date: Thu, 30 Dec 2021 23:42:59 +0530 MIME-Version: 1.0 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <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> <22978838-0457-c659-f825-4af3082552ca@gyani.pro> <20211230175500.GD2829255@pb2> From: Gyan Doshi In-Reply-To: <20211230175500.GD2829255@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 11:25 pm, Michael Niedermayer wrote: > On Thu, Dec 30, 2021 at 10:39:05PM +0530, Gyan Doshi wrote: >> >> 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~ > v4 seems to work, i have just now tested it > thanks So, I'll push max_stts_delta soon and test the 0 value patch on the weekend. 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".