Re: [FFmpeg-devel] [PATCH] tests/fate/mov: Disable fate-mov-invalid-elst-entry-count, the test does not work reliable currently

[Sasi Inguva]

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 
wrote:

> On Tue, Dec 05, 2017 at 08:17:14PM +, 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
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] tests/fate/mov: Disable fate-mov-invalid-elst-entry-count, the test does not work reliable currently

[Derek Buitenhuis]

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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] tests/fate/mov: Disable fate-mov-invalid-elst-entry-count, the test does not work reliable currently

[Michael Niedermayer]

On Tue, Dec 05, 2017 at 08:17:14PM +, 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.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] tests/fate/mov: Disable fate-mov-invalid-elst-entry-count, the test does not work reliable currently

[Derek Buitenhuis]

>> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] tests/fate/mov: Disable fate-mov-invalid-elst-entry-count, the test does not work reliable currently

[Michael Niedermayer]

On Tue, Dec 05, 2017 at 01:54:27PM +, 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 
> > ---
> >  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?

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

The worst form of inequality is to try to make unequal things equal.
-- Aristotle


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] tests/fate/mov: Disable fate-mov-invalid-elst-entry-count, the test does not work reliable currently

[Derek Buitenhuis]

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 
> ---
>  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] tests/fate/mov: Disable fate-mov-invalid-elst-entry-count, the test does not work reliable currently

[Tobias Rapp]

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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel