Skip to content

Conversation

TurboGit
Copy link
Member

…int.

Fixes #17536

@TurboGit TurboGit added this to the 5.0 milestone Sep 25, 2024
@TurboGit TurboGit added bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes scope: image processing correcting pixels scope: DAM managing files, collections, archiving, metadata, etc. labels Sep 25, 2024
@TurboGit
Copy link
Member Author

@wpferguson : Can you double check this. TIA.

@wpferguson
Copy link
Member

wpferguson commented Sep 25, 2024

I saw this last night, but didn't realize that it was my change that caused it. When I went to find the problem, I couldn't reproduce. Testing now....

I still see it on tif, dng, and a cr3. I'll do some digging....

Another datapoint, opened a skull image in darkroom and it's fine, but the filmstrip thumbnails are all skulls

@wpferguson
Copy link
Member

I backed up before my change and loaded some tif's with -d imageio -d cache

Here's part of the log...

     7.1830 [tiff_open] 3089x2048 8bpp, 3 samples per pixel.
     7.1888 [tiff_open] 3089x2048 8bpp, 3 samples per pixel.
     7.1892 [tiff_open] 3089x2048 8bpp, 3 samples per pixel.
     7.1898 [tiff_open] 2545x3393 16bpp, 3 samples per pixel.
     7.1902 [tiff_open] 1729x1729 16bpp, 3 samples per pixel.
     7.1906 [tiff_open] 4816x3219 16bpp, 4 samples per pixel.
     7.2731 [dt_imageio_export_with_flags]  modules: colorin colorout gamma
     7.2731 [dt_imageio_export] thumbnail imgid 38474, 3089x2048 --> 1357x900 (scale=0.4395, maxscale=1.0000). upscale=no, hq=no
     7.2785 [dt_imageio_export_with_flags]  modules: colorin colorout gamma
     7.2786 [dt_imageio_export] thumbnail imgid 38475, 3089x2048 --> 1357x900 (scale=0.4395, maxscale=1.0000). upscale=no, hq=no
     7.3014 [dt_imageio_export_with_flags]  modules: colorin colorout gamma
     7.3014 [dt_imageio_export] thumbnail imgid 38476, 3089x2048 --> 1357x900 (scale=0.4395, maxscale=1.0000). upscale=no, hq=no
     7.3149 [dt_imageio_export_with_flags]  modules: colorin colorout gamma
     7.3149 [dt_imageio_export] thumbnail imgid 38477, 2545x3393 --> 675x900 (scale=0.2653, maxscale=1.0000). upscale=no, hq=no
     7.3184 [mipmap_cache] generate mip 3 for ID=38474 from scratch
     7.3191 [tiff_open] 4816x3219 8bpp, 4 samples per pixel.
     7.3322 [mipmap_cache] generate mip 3 for ID=38475 from scratch
     7.3340 [tiff_open] 2550x3300 8bpp, 3 samples per pixel.
     7.3431 [mipmap_cache] generate mip 3 for ID=38476 from scratch

trying to trace the execution to see how we get from tiff open to dt_imageio_export

@wpferguson
Copy link
Member

The problem seems to be in dt_imageio_large_thumbnail but I haven't figured out what it is or how to solve it.

Unless you have an idea, I would say revert my PR and I'll work on it until I solve the problems. At least that way it wont affect people trying to use master.

@TurboGit TurboGit force-pushed the po/fix-export-and-cache branch 2 times, most recently from f41859f to 3280a1a Compare September 26, 2024 07:09
@AndDiSa
Copy link

AndDiSa commented Sep 26, 2024

For me the fix works, at least for NEF and DNG

@TurboGit
Copy link
Member Author

@AndDiSa : Indeed, it works for RAW & JPEG but still buggy for TIFF.

@wpferguson : I have pushed the revert I had prepared.

@TurboGit TurboGit force-pushed the po/fix-export-and-cache branch from 108df3b to 89ca157 Compare September 26, 2024 11:25
@wpferguson
Copy link
Member

After a nights sleep and some time to think this is what I came up with.

The original problem was that dt_imageio_export_with_flags is a gboolean returning "backwards" indicators. dt_imageio_export_with_flags used to return an int, 0 for success and 1 for error which is standard. Nothing downstream uses dt_imageio_export_with_flags as a boolean and all the downstream code still treats it as returning an int.

So, I propose simply turning dt_imageio_export_with_flags back into an int and let it return 0 and 1. The sanity crowd is happy because the return value is standard, and almost no code needs to be changed (I'll look at the Lua stuff).

@wpferguson
Copy link
Member

As for dt_imageio_large_thumbnail I'm still going to play with this and see if I can at least understand the coupling between it and the mipmap_cache code. I'm starting to suspect that the real issue may be the mipmap_cache not asking (or not asking correctly) for the thumbnail.

@wpferguson
Copy link
Member

Thanks for cleaning up my mess.

@TurboGit
Copy link
Member Author

TurboGit commented Sep 26, 2024

@wpferguson : No problem, I spend quite some time trying to fix the issue but could not either... One data point:

In dt_imageio_large_thumbnail we have (in this PR):

gboolean dt_imageio_large_thumbnail(const char *filename,
                                    uint8_t **buffer,
                                    int32_t *width,
                                    int32_t *height,
                                    dt_colorspaces_color_profile_type_t *color_space)
{
  int res = TRUE;

So res to TRUE, and some early return:

  // get the biggest thumb from exif
  if(dt_exif_get_thumbnail(filename, &buf, &bufsize, &mime_type))
    goto error;

But the goto error here is misleading, a whole mess :( This is not an error as we exit with 1 (TRUE) !!!

I tried fixing that but the last state of this PR is ok for everything expect TIFF.

I would say that we want to continue to work on this... We need to fix for a better future :)

@wpferguson
Copy link
Member

Maybe I should change my username to Pandora :-D

I would say that we want to continue to work on this... We need to fix for a better future :)

I agree. My brain segfaulted a few times trying to make sense of the code. I'll keep playing with it. I enjoy puzzles

@TurboGit
Copy link
Member Author

Good, start over this PR which has already quite some fixes.

* src/imageio/imageio.c - changed dt_imageio_export_with_flags to return
                        TRUE on success and FALSE on error.  Since
                        dt_imageio_export calls dt_imageio_export_with_flags
                        to do the actual export, dt_imageio_export now
                        returns TRUE for success and FALSE for error.

src/imageio/storage/disk.c
src/imageio/storage/email.c
src/imageio/storage/gallery.c
src/imageio/storage/pwigo.c
src/lua/luastorage.c         - updated the places where dt_imageio_export was
                               called to use the boolean return value for
                               success tests.
@TurboGit TurboGit force-pushed the po/fix-export-and-cache branch from 89ca157 to 83be4b3 Compare September 26, 2024 16:54
@TurboGit TurboGit force-pushed the po/fix-export-and-cache branch from 83be4b3 to a25fccc Compare September 26, 2024 16:55
@TurboGit
Copy link
Member Author

@wpferguson : I have just pushed a new version with your changes and mine on top. Let's play :)

@TurboGit TurboGit added the wip pull request in making, tests and feedback needed label Sep 27, 2024
@TurboGit TurboGit marked this pull request as draft September 27, 2024 14:35
@TurboGit TurboGit modified the milestones: 5.0, 5.2 Nov 29, 2024
@TurboGit TurboGit modified the milestones: 5.2, 5.4 May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes scope: DAM managing files, collections, archiving, metadata, etc. scope: image processing correcting pixels wip pull request in making, tests and feedback needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Git master : darktable action gives death skull icon in lighttable : import of tiff gives death skull in lighttable : two issues

3 participants