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

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

[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

Let's solve these problems by removing the new check and extending the old
check for negative index edit rates. This should fix the original issue and
also restore support for 0/0 index edit rates.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mxfdec.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 233d614f78..71692c36bd 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -1264,9 +1264,6 @@ static int mxf_read_index_table_segment(void *arg, AVIOContext *pb, int tag, int
     case 0x3F0B:
         segment->index_edit_rate.num = avio_rb32(pb);
         segment->index_edit_rate.den = avio_rb32(pb);
-        if (segment->index_edit_rate.num <= 0 ||
-            segment->index_edit_rate.den <= 0)
-            return AVERROR_INVALIDDATA;
         av_log(NULL, AV_LOG_TRACE, "IndexEditRate %d/%d\n", segment->index_edit_rate.num,
                 segment->index_edit_rate.den);
         break;
@@ -2135,7 +2132,7 @@ static int mxf_compute_index_tables(MXFContext *mxf)
 
         /* fix zero IndexDurations */
         for (k = 0; k < t->nb_segments; k++) {
-            if (!t->segments[k]->index_edit_rate.num || !t->segments[k]->index_edit_rate.den) {
+            if (t->segments[k]->index_edit_rate.num <= 0 || t->segments[k]->index_edit_rate.den <= 0) {
                 av_log(mxf->fc, AV_LOG_WARNING, "IndexSID %i segment %i has invalid IndexEditRate\n",
                        t->index_sid, k);
                 if (mxf_track)
-- 
2.35.3

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

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

[Tomas Härdin]

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.

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

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

[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.

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. In this case, I could only craft an MXF 
file, because I don't have access to the software.

Regards,
Marton
_______________________________________________
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".

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

[Michael Niedermayer]

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

On Thu, May 02, 2024 at 11:01:02PM +0200, Marton Balint wrote:
[...]
> 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. In this case, I could only craft an MXF file,
> because I don't have access to the software.

IMO add as many tests as you like.
If we really hit a "too many tests" situation, then we can look at
them all and remove the most expensive/least usefull tests.

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus

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

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

[Tomas Härdin]

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.

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

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

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

[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.

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

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

Regards,
Marton
_______________________________________________
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".

Re: [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
_______________________________________________
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".