| Message ID | 20171205003847.29487-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
On 05.12.2017 01:38, Michael Niedermayer wrote: > Noone is known to work on fixing this, so it should be disabled > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > tests/fate/mov.mak | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak > index 680baea773..869784fd31 100644 > --- a/tests/fate/mov.mak > +++ b/tests/fate/mov.mak > @@ -6,7 +6,6 @@ FATE_MOV = fate-mov-3elist \ > fate-mov-1elist-ends-last-bframe \ > fate-mov-2elist-elist1-ends-bframe \ > fate-mov-3elist-encrypted \ > - fate-mov-invalid-elst-entry-count \ > fate-mov-gpmf-remux \ > fate-mov-440hz-10ms \ > fate-mov-ibi-elst-starts-b \ > Maybe add a FIXME comment to the test itself so that somebody reading the file doesn't think it is missing in this list by accident? Regards, Tobias
On 12/5/2017 12:38 AM, Michael Niedermayer wrote: > Noone is known to work on fixing this, so it should be disabled > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > tests/fate/mov.mak | 1 - > 1 file changed, 1 deletion(-) *NAK* Disabling failing tests entirely defeats the point of having test! The commit that broke it should be reverted until the author of that commit can explain why it changed, or fix it. - Derek
On Tue, Dec 05, 2017 at 01:54:27PM +0000, Derek Buitenhuis wrote: > On 12/5/2017 12:38 AM, Michael Niedermayer wrote: > > Noone is known to work on fixing this, so it should be disabled > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > tests/fate/mov.mak | 1 - > > 1 file changed, 1 deletion(-) > > *NAK* > > Disabling failing tests entirely defeats the point of having test! > > The commit that broke it should be reverted until the author > of that commit can explain why it changed, or fix it. The commit that added the test was the one that broke fate. It never worked. So this "sort of" reverts what caused the issue. Ill make this more clear in the commit message in case you otherwise agree to the change ? I can also exactly revert the commit that added the test if thats preferred? [...]
>> The commit that broke it should be reverted until the author >> of that commit can explain why it changed, or fix it. > > The commit that added the test was the one that broke fate. It never > worked. > So this "sort of" reverts what caused the issue. Wasn't the code it tests added directly before the commit that added this test? That's the code that is broken. The way I see it, the code is workaround for broken files, but it doesn't actually work. It should either be fixed, or the workaround removed if nobody (especially the author) is willing to fix it. > Ill make this more clear in the commit message in case you otherwise > agree to the change ? > > I can also exactly revert > the commit that added the test if thats preferred? See above. As I stated before, the entire point of tests is to show something is broken. They make FATE red so someone will fix it. Disabling the test demeans the entire point of having tests - if nobody is willing to fix the broken code it tests, that broken could should be removed, or not have been committed. - Derek
On Tue, Dec 05, 2017 at 08:17:14PM +0000, Derek Buitenhuis wrote: > >> The commit that broke it should be reverted until the author > >> of that commit can explain why it changed, or fix it. > > > > The commit that added the test was the one that broke fate. It never > > worked. > > So this "sort of" reverts what caused the issue. > > Wasn't the code it tests added directly before the commit that added this > test? That's the code that is broken. > > The way I see it, the code is workaround for broken files, but it doesn't > actually work. It should either be fixed, or the workaround removed if > nobody (especially the author) is willing to fix it. The test produces different output on qemu arm and x86-64. From this we know there is a bug, but not where the bug is. It can be in the test, the newly added code tested or code that was there before. My guess was, its the test, i cannot logically explain why. ive looked into this now and its missing -idct, adding that makes it produce the same result here ill push a fix for this thanks [...]
On 12/6/2017 12:12 AM, Michael Niedermayer wrote: > The test produces different output on qemu arm and x86-64. > From this we know there is a bug, but not where the bug is. > It can be in the test, the newly added code tested or code that was > there before. > > My guess was, its the test, i cannot logically explain why. > > ive looked into this now and its missing -idct, adding that makes it > produce the same result here > > ill push a fix for this Thank you for taking the time to look into it. - Derek
i was on vacation so I missed this. sorry for the trouble and thanks for fixing the test. This test was fixed once before http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=4ccc1ef2a3b1a22d849861423df830e110c9a4ab . I've no idea why only this test needs to have the "-flags +bitexact -idct simple" while the other tests are fine. I've used the same file as the other tests, but just changed the Entry count atom using an Atom editor. On Tue, Dec 5, 2017 at 4:12 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Dec 05, 2017 at 08:17:14PM +0000, Derek Buitenhuis wrote: > > >> The commit that broke it should be reverted until the author > > >> of that commit can explain why it changed, or fix it. > > > > > > The commit that added the test was the one that broke fate. It never > > > worked. > > > So this "sort of" reverts what caused the issue. > > > > Wasn't the code it tests added directly before the commit that added this > > test? That's the code that is broken. > > > > The way I see it, the code is workaround for broken files, but it > doesn't > > actually work. It should either be fixed, or the workaround removed if > > nobody (especially the author) is willing to fix it. > > The test produces different output on qemu arm and x86-64. > From this we know there is a bug, but not where the bug is. > It can be in the test, the newly added code tested or code that was > there before. > > My guess was, its the test, i cannot logically explain why. > > ive looked into this now and its missing -idct, adding that makes it > produce the same result here > > ill push a fix for this > > thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > There will always be a question for which you do not know the correct > answer. > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak index 680baea773..869784fd31 100644 --- a/tests/fate/mov.mak +++ b/tests/fate/mov.mak @@ -6,7 +6,6 @@ FATE_MOV = fate-mov-3elist \ fate-mov-1elist-ends-last-bframe \ fate-mov-2elist-elist1-ends-bframe \ fate-mov-3elist-encrypted \ - fate-mov-invalid-elst-entry-count \ fate-mov-gpmf-remux \ fate-mov-440hz-10ms \ fate-mov-ibi-elst-starts-b \
Noone is known to work on fixing this, so it should be disabled Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- tests/fate/mov.mak | 1 - 1 file changed, 1 deletion(-)