| Message ID | 20240415193400.32630-1-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mxfdec: only check index_edit_rate when calculating the index tables | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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
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
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 [...]
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
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
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
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)
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(-)