[FFmpeg-devel] [PATCH] avformat/mxfdec: only check index_edit_rate when calculating the index tables

[Tomas Härdin]

lör 2024-05-04 klockan 03:49 +0200 skrev Marton Balint:
> 
> 
> On Fri, 3 May 2024, Tomas Härdin wrote:
> 
> > tor 2024-05-02 klockan 23:01 +0200 skrev Marton Balint:
> > > 
> > > 
> > > On Mon, 29 Apr 2024, Tomas Härdin wrote:
> > > 
> > > > mån 2024-04-15 klockan 21:34 +0200 skrev Marton Balint:
> > > > > Commit ed49391961999f028e0bc55767d0eef6eeb15e49 started
> > > > > rejecting
> > > > > negative
> > > > > index segment edit rates to avoid negative av_rescale
> > > > > parameters.
> > > > > There are two
> > > > > problems with this:
> > > > > 
> > > > > 1) there is already a validation for zero (uninitialized)
> > > > > rates
> > > > > later
> > > > > on
> > > > > 2) it rejects files with 0/0 index edit rates which do exist
> > > > > and
> > > > > we
> > > > > should
> > > > > continue to support those
> > > > 
> > > > There are no such files in FATE last time I checked. At the
> > > > very
> > > > least
> > > > we need to know which vendor is producing such broken files.
> > > > 
> > > > Without tests covering the workflows we want to support, we
> > > > have no
> > > > confidence in refactoriing. This leads to cargo culting. And to
> > > > be
> > > > sure, every workflow we support means a non-trivial amount of
> > > > work,
> > > > especially when it comes to MXF.
> > > 
> > > I can add a comment to the code or the commit message, we did
> > > this
> > > plenty 
> > > of times in the past. The broken software is Marquise
> > > Technologies MT
> > > Mediabase 4.7.2 by the way. I can also rework the patch to keep
> > > rejecting 
> > > negative values and only allow zero, as that is the only thing I
> > > *know* to 
> > > exist.
> > 
> > We could also tell Marquise Technologies to fix their software. MXF
> > is
> > a living ecosystem
> > 
> > > If we add a new FATE file for every fixed file or workflow, the
> > > amount of 
> > > FATE samples (and the time fate will run) will increase
> > > significantly, I 
> > > am not sure that is intended.
> > 
> > "Support any old workflow but don't add tests for them" is a
> > ridiculous
> > position. It also prevents refactoring. There are real costs to
> > supporting workflows that no one uses. Hence why I mention SLAs.
> > The
> > only people interested in MXF are professionals.
> 
> And what if a home user gets a file from a professional user?
> Crippling 
> professional use cases from ffmpeg, or make professional features a 
> payable option - via SLA or otherwise - is something I cannot
> support.

This is a blackleg mindset.

> > > In this case, I could only craft an MXF
> > > file, because I don't have access to the software.
> > 
> > Yes we can craft arbitrarily broken files. That doesn't help
> > anyone.
> > It's just cargo culting.
> 
> Cargo culting would be fixing files which do not exists in the wild.
> Files with 0/0 index edit rate do exist. So this patch clearly helps 
> anybody who is trying to play those.

My focus is on workflows and the MXF ecosystem in general, not
individual files. Files can be fixed. Other implementations can be
patched to no longer output broken files.

A liberal attitude towards broken implementations begets yet more
broken implementations and yet more busywork for us.

> I can create a fate test for supporting 0/0 index edit rates if that
> is 
> indeed preferred.

No, use an actual real sample demonstrating the need for the behavior.
Sadly the sample in the relevant ticket for this 404'd last time I
looked.

When we know why and who needs a specific fix for a broken muxer, we
have an avenue for making sure support for that broken muxer is still
needed and, when it is no longer necessary, we can do one of the most
productive things you can do in programming: removing dead code.

/Tomas