darktable icon indicating copy to clipboard operation
darktable copied to clipboard

three regressions tonight

Open TurboGit opened this issue 3 years ago • 7 comments

We have three regressions detected tonight, this is due to some merge done yesterday.

Errors 3

  • 0016-lowpass-bilateral
  • 0019-color-mapping
  • 0043-dithering-fs

TurboGit avatar Aug 12 '22 07:08 TurboGit

The culprit is:

d540794cc07a3531825b6b2b33c1c93e3a96e5a0 is the first bad commit
commit d540794cc07a3531825b6b2b33c1c93e3a96e5a0
Author: ralfbrown <[email protected]>
Date:   Sat Jul 30 18:24:42 2022 -0400

    rewrite dt_Lab_to_XYZ to permit vector instructions

 src/common/colorspaces_inline_conversions.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

TurboGit avatar Aug 12 '22 10:08 TurboGit

For test 0043 it is actually one of:

There are only 'skip'ped commits left to test.
The first bad commit could be any of:
e9079df756fb0f52cad213a0ecac1639cd1d406d
b30c22a0bccb0eef8242c69886494677b9e77255
We cannot bisect more!

So

commit b30c22a0bccb0eef8242c69886494677b9e77255 (refs/bisect/bad)
Author: Ralf Brown <[email protected]>
Date:   Sat Jul 30 18:30:43 2022 -0400

    remove colorout.c SSE codepath

commit e9079df756fb0f52cad213a0ecac1639cd1d406d (refs/bisect/skip-e9079df756fb0f52cad213a0ecac1639cd1d406d)
Author: Ralf Brown <[email protected]>
Date:   Sat Jul 30 18:27:26 2022 -0400

    cleanup colorout.c
    
    Together with the rewrite of dt_Lab_to_XYZ, the compiler-generated
    code is now essentially the same speed as the SSE codepath.
    
    Thr     AutoV   SSE
      1     0.1028  0.1014
      2     0.0522  0.0510
      4     0.0268  0.0260
      8     0.0140  0.0130
     12     0.0120  0.0100
     16     0.0100  0.0090
     24     0.0070  0.0070
     32     0.0073  0.0070

TurboGit avatar Aug 12 '22 11:08 TurboGit

Test 0016 & 0019 have only a very limited number of pixel changed (216 and 1952), but test 0043 has 935056 which is bit more worrisome.

0016: diff16

0019: diff19

0043: diff43

@ralfbrown : Can you have a look at the three mentioned commits above? TIA.

TurboGit avatar Aug 12 '22 11:08 TurboGit

There is also the gcc7 problem here...

jenshannoschwalm avatar Aug 12 '22 12:08 jenshannoschwalm

Since the problematic commits are the entirety of #12258, it might be best to temporarily revert. I won't be able to investigate until the weekend.

ralfbrown avatar Aug 12 '22 13:08 ralfbrown

@ralfbrown : Here is what I have done:

I have reverted:

Revert "cleanup colorout.c"
This reverts commit e9079df756fb0f52cad213a0ecac1639cd1d406d.

and

Revert "remove colorout.c SSE codepath"    
This reverts commit b30c22a0bccb0eef8242c69886494677b9e77255.

To fix test 0043.

And I have fixed the dt_Lab_to_XYZ() implementation which fixes 0016 & 0019 (do not hesitate to double check my changes).

TurboGit avatar Aug 12 '22 15:08 TurboGit

Checking into this, I've discovered that current master (after the reversion) gives maxDE of 0.00 for tests 0016 and 0019 only when building RelWithDebInfo. Build type Release still gives maxDE of 8.9 and 15.1, respectively. My guess is that my recent changes merely enabled the compiler to perform some optimization at -O2 which was previously performed only at -O3.

The output difference on 0043 is due to some pixels being pushed across the quantization threshold when rounding errors change due to different code generation. The Floyd-Steinberg error diffusion then propagates that difference right and down.

More to come.

ralfbrown avatar Aug 14 '22 19:08 ralfbrown

Is this still valid?

jenshannoschwalm avatar Mar 23 '23 16:03 jenshannoschwalm

No fixed and we were all green on the 136 regression tests last night.

TurboGit avatar Mar 23 '23 17:03 TurboGit