Skip to content

Conversation

@kofa73
Copy link
Contributor

@kofa73 kofa73 commented Jul 20, 2025

Suggested by @TurboGit here: #19026 (comment)

There are many places where the min/max of a 3 or 4-element float or double array is calculated. The aim was to provide a shared implementation. Later, it turned out we already have some:

openmp_maths.h already has

DT_OMP_DECLARE_SIMD(aligned(vector:16))
static inline float v_maxf(const float vector[3])

and

DT_OMP_DECLARE_SIMD(aligned(vector:16))
static inline float v_minf(const float vector[3])

There is also dt_vector_channel_max in common/math.h from @ralfbrown edbaa84. As I understand it, that's vectorised code, it looks very surprising to me (counter-intuitive, with the extra allocations and extra work), but perf tuning is magic. :-)

We have a few places that may not be aligned, there are some that use 4 components, and there are a few in double precision.

@kofa73 kofa73 marked this pull request as ready for review July 27, 2025 11:19
@kofa73 kofa73 force-pushed the array-min-max-component branch from c7ec2e4 to 3e3213f Compare August 23, 2025 08:09
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor style comment, apart from that it is good to go to me. TIA.

@TurboGit TurboGit added this to the 5.4 milestone Sep 6, 2025
@TurboGit TurboGit added scope: codebase making darktable source code easier to manage scope: performance doing everything the same but faster labels Sep 6, 2025
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll handle the const.

@TurboGit TurboGit merged commit e0feb42 into darktable-org:master Sep 6, 2025
6 checks passed
@kofa73
Copy link
Contributor Author

kofa73 commented Sep 6, 2025

Thanks! I was away of the computer most of today, saw the last couple of messages only now.

@TurboGit
Copy link
Member

TurboGit commented Sep 6, 2025

No problem, the changes were trivial and you've done already all the hard work.

@zisoft
Copy link
Collaborator

zisoft commented Sep 7, 2025

With this PR merged, the compilation of noiceprofile.c is broken.

This is not detected by CI, but it breaks the nightly builds:

[738/965] Building C object share/darktable/tools/noise/CMakeFiles/darktable-noiseprofile.dir/noiseprofile.c.o
FAILED: [code=1] share/darktable/tools/noise/CMakeFiles/darktable-noiseprofile.dir/noiseprofile.c.o
/usr/bin/cc -DGDK_DISABLE_DEPRECATED -DGTK_DISABLE_DEPRECATED -DGTK_DISABLE_SINGLE_INCLUDES -DOS_OBJECT_USE_OBJC=0 -D_XOPEN_SOURCE=700 -I/Users/mario/src/darktable/build/share/darktable/tools/noise -I/Users/mario/src/darktable/tools/noise -fopenmp-version=51 -D_DARWIN_C_SOURCE -Wall -Wformat -Wformat-security -Wshadow -Wtype-limits -Wvla -Wthread-safety -Wno-unknown-pragmas -Wno-error=varargs -Wno-format-truncation -Wno-error=address-of-packed-member -Wno-typedef-redefinition -g -fPIE -MD -MT share/darktable/tools/noise/CMakeFiles/darktable-noiseprofile.dir/noiseprofile.c.o -MF share/darktable/tools/noise/CMakeFiles/darktable-noiseprofile.dir/noiseprofile.c.o.d -o share/darktable/tools/noise/CMakeFiles/darktable-noiseprofile.dir/noiseprofile.c.o -c /Users/mario/src/darktable/tools/noise/noiseprofile.c
In file included from /Users/mario/src/darktable/tools/noise/noiseprofile.c:6:
/Users/mario/src/darktable/tools/noise/../../src/common/dttypes.h:30:10: fatal error: 'gmodule.h' file not found
   30 | #include <gmodule.h>
      |          ^~~~~~~~~~~
1 error generated.

To reproduce you need to compile with -DBUILD_NOISE_TOOLS=ON

@kofa73
Copy link
Contributor Author

kofa73 commented Sep 7, 2025

Will look into it ASAP

@zisoft
Copy link
Collaborator

zisoft commented Sep 7, 2025

ok, this can be fixed by adding

    find_package(PkgConfig)
    pkg_check_modules(GMODULE REQUIRED gmodule-2.0)
    include_directories(${GMODULE_INCLUDE_DIRS})

in src/darktable/tools/noise/CMakeLists.txt

I will provide a PR, ok?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: codebase making darktable source code easier to manage scope: performance doing everything the same but faster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants