darktable-org / darktable

darktable is an open source photography workflow application and raw developer
https://www.darktable.org
GNU General Public License v3.0
9.61k stars 1.13k forks source link

three regressions tonight #12304

Closed TurboGit closed 1 year ago

TurboGit commented 2 years ago

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

Errors 3

TurboGit commented 2 years ago

The culprit is:

d540794cc07a3531825b6b2b33c1c93e3a96e5a0 is the first bad commit
commit d540794cc07a3531825b6b2b33c1c93e3a96e5a0
Author: ralfbrown <ralfbrown@users.noreply.github.com>
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 commented 2 years ago

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 <ralfbrown@users.noreply.github.com>
Date:   Sat Jul 30 18:30:43 2022 -0400

    remove colorout.c SSE codepath

commit e9079df756fb0f52cad213a0ecac1639cd1d406d (refs/bisect/skip-e9079df756fb0f52cad213a0ecac1639cd1d406d)
Author: Ralf Brown <ralfbrown@users.noreply.github.com>
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 commented 2 years ago

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.

jenshannoschwalm commented 2 years ago

There is also the gcc7 problem here...

ralfbrown commented 2 years ago

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.

TurboGit commented 2 years ago

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

ralfbrown commented 2 years ago

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.

jenshannoschwalm commented 1 year ago

Is this still valid?

TurboGit commented 1 year ago

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