Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Artifacts when importing / opening old edit in current master #15489

Open
Macchiato17 opened this issue Oct 25, 2023 · 35 comments
Open

Artifacts when importing / opening old edit in current master #15489

Macchiato17 opened this issue Oct 25, 2023 · 35 comments
Labels
bug: pending someone needs to start working on that no-issue-activity reproduce: peculiar the bug seems to affect only a specific target and cannot be reproduced elsewhere scope: image processing correcting pixels understood: unclear devs lack most or all important info and can do nothing, the report will be closed after 2 weeks

Comments

@Macchiato17
Copy link
Contributor

Macchiato17 commented Oct 25, 2023

Describe the bug

see also:
https://discuss.pixls.us/t/artifacts-when-importing-opening-old-edit-in-current-master/40097

After opening / importing a darktable 3.2.1 edit, the picture is totally screwed up already in lighttable (see Screenshot 1 below). Same for darkroom.

Things I noted:

  • filmic settings are version 4 (2020), contrast in highlights/shadows are both set to “hard”. Changing the contrast settings doesn't matter
  • white luminance setting in filmic is e.g. 0,1742% instead of 100% (Screenshot 2)
  • when I go one step before filmic down in the history, the picture returns to normal.
  • my workflow default is scene referred (filmic)
  • turning openCL off or on has no impact

Finally: leaving everything as is after importing the picture and then changing the contrast (being 1.0 after importing) to the same value of 1.0, then everything “realigns” and the picture looks fine.
May this be some kind of mismatch between persisted values in the XMP and the value after reading the XMP (see attached file) or writing to the database?

Steps to reproduce

see description
if this can just be debugged with the RAW file available, please drop a note and I will add it to this ticket.
=> added together with the related XMP

Expected behavior

No response

Logfile | Screenshot | Screencast

Screenshot 1

Screenshot 2

RAW file

XMP file

Commit

No response

Where did you install darktable from?

self compiled

darktable version

darktable-4.5.0+958~ga37b7fb789-win64

What OS are you using?

Windows

What is the version of your OS?

Windows 11

Describe your system?

Laptop 8GB RAM
NVIDIA GeForce GTX 1050 Max-Q (4GB)
Intel(R) Core(TM) i5-8265U CPU @ 1.60GHz

Are you using OpenCL GPU in darktable?

Yes

If yes, what is the GPU card and driver?

NVIDIA GeForce GTX 1050 Max-Q (4GB), driver 31.0.15.3699 (04.08.2023)

Please provide additional context if applicable. You can attach files too, but might need to rename to .txt or .zip

No response

@todd-prior
Copy link

todd-prior commented Oct 25, 2023

May this be some kind of mismatch between persisted values in the XMP and the value after reading the XMP (see attached file) or writing to the database?

No idea but applying your side car to a variety of my images leads to no weird issues ...setting seem intact... maybe copy those images to a new location with the sidecars... then remove them from the db and reimport...any issue... it might have just been a database hiccup at some point along the way....

@Macchiato17
Copy link
Contributor Author

That's quite strange.
I already removed that film roll and re-imported it (even quitting dt inbetween). I typically just hold pictures in the dt database for as long as I work on them. I also tried your suggestion regarding copying the RAW + XMP to a different location, same result. Even if I put the XMP as a sidecar to any other picture, I see kind of similar issues.
May I ask you to try it once again with the RAW that I just attached to this ticket? Just to sort out, if the issue may be related to the RAW (Nikon D7500 NEF file).
Thanks so much for your support.

@todd-prior
Copy link

todd-prior commented Oct 26, 2023

Seems to open fine... What if you set DT to look for new xmp...maybe that will refresh the database or you could open DT with the --configdir option and give it a path for a set of fresh config files...if it opens fine your database might be the issue

image

@ptilopteri
Copy link

I also tried your image and it looks beautiful. believe you should not have added
denoise as I see no difference with it removed.

Tumbleweed openSUSE 20231023
darktable 4.5.0+993~g7b001ad6b

nice image

@jenshannoschwalm
Copy link
Collaborator

While importing from V4 the spline correction is not good. That's "deep inside filmicrgb dev history", i am currently not sure how we can handle that correctly.

@jenshannoschwalm jenshannoschwalm added reproduce: confirmed a way to make the bug re-appear 99% of times has been found scope: image processing correcting pixels labels Oct 29, 2023
@Macchiato17
Copy link
Contributor Author

Seems to open fine... What if you set DT to look for new xmp...maybe that will refresh the database or you could open DT with the --configdir option and give it a path for a set of fresh config files...if it opens fine your database might be the issue

Sorry for not replying earlier to your suggestions. But now I have some more time to come back to this.

Thanks for pointing me there @todd-prior . I just tried with --configdir and a non-existing path. Well, everything was initialized, but unfortunately the import shows exactly the same issue. Meeeee...

So I just kept on playing and had a closer look at your screenshot (thanks for sharing,). I noticed that after re-applying just the contrast value of "1", the filmic "look only" spline looked quite different on my system from what your screenshot shows. This was also reflected in the overall look when comparing your screenshot to my dt darkroom view. So I adjusted my filmic settings to match my spline with yours. And - hm - there are indeed some interesting value changes:

filmic_adjustments

Please ignore the highlight reconstruction, which I simply turned off without having any influence. But the rest is perhaps interesting

  • target white luminance was 18,45 before (sounds to me like this being the middle grey value instead of the target 100%)
  • latitude and shadow/highlight balance seem, as if they just were mixed-up
  • contrast was a nan before changing to "1"

That all still sounds to me like a persistence / reading issue. At the same time it seems as if under Linux there is no issue at all with this picture (am I right that you are also running dt on Linux as @ptilopteri does?) So I really have no clue what might be a Windows specific problem here. But another user in the pixls-thread mentioned, that he also experienced the same issue with one of the "Windows Insider" test-builds one / two weeks ago. His picture had also filmic v4 applied.

Copy link

This issue has been marked as stale due to inactivity for the last 60 days. It will be automatically closed in 300 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

@Macchiato17
Copy link
Contributor Author

I tried again with current master [0c2ad29] to please the github-actions bot 😁 and as I expected, the issue is still there.

@jenshannoschwalm, I was not sure, if you already had the opportunity to read my last comment on this in which I have tried to summarize my findings after successfully restoring the filmic RGB settings manually after the faulty import.
As a bottom line, I had the impression, that there was "just" some kind of "mixing-up effects" during import of the affected pictures / reading the affected XMP on Windows. Or is your comment "deep inside filmicrgb dev history" to be understood as to also point to the same effect?

Best regards and wishes for the advancing New Year 😃

@jenshannoschwalm
Copy link
Collaborator

Or is your comment "deep inside filmicrgb dev history" to be understood as to also point to the same effect?

I don't know the filmicrgb code well enough to understand how parameters would have to be adapted in some module version change.

@Macchiato17
Copy link
Contributor Author

I was still trying to gather some more supporting information on this effect to help narrowing things down a little bit more...

So I tried latest Ansel nightly [4d185b4] today and was indeed able to import the picture on my system without any of the above mentioned artifacts resulting after import 😃
After that I closed Ansel and imported the thus "migrated" XMP in darktable [bce83d5] and voilà no artifacts anymore.

Hopefully, these findings may help you...

@ralfbrown
Copy link
Collaborator

That would point to Filmic's legacy_params function as the culprit.

@jenshannoschwalm
Copy link
Collaborator

That would point to Filmic's legacy_params function as the culprit.

Think so too but couldn't spot the problem...

@TurboGit
Copy link
Member

TurboGit commented Jan 8, 2024

Tried importing this image from the first time and got no artifact. The Filmic UI looks fine:

image

And the image clean:

image

@Macchiato17
Copy link
Contributor Author

Thanks a lot for testing also on your side @TurboGit. Did you use Linux or Windows for the test? I'm wondering, since it seems, as if Linux works fine, whereas on Windows me and another user (from a pixls.us discussion) can see this effect.

@TurboGit
Copy link
Member

TurboGit commented Jan 8, 2024

@Macchiato17 : I'm using Linux. Hard to believe that this could be a Windows only issue... but everything is possible.

@jenshannoschwalm
Copy link
Collaborator

I added the "reproduced" label some time ago and as @TurboGit couldn't reproduce i just checked again on this mornings master and can still confirm the issue by

  1. downloading original raw and xmp
  2. OpenCL correctly working but that does not matter at all
  3. Checked for a) no available filmic presets and b) no style applied
  4. Checked with all "apply workflow" settings

Visual output is
Bildschirmfoto vom 2024-01-10 08-10-36

and the logs from -d params -d pipe also suggest a problem there as shown

     7.7145 commit params                 [full]           temperature            piece hash=58cd2398489aeef9, 
     7.7145 commit params                 [full]           highlights             piece hash=4b3de2e5a3cd1640, 
     7.7145 [iop_validate_params] `filmicrgb' failed for type "float", field: latitude
     7.7145 [iop_validate_params] `filmicrgb' failed for type "dt_iop_filmicrgb_params_t"
     7.7145 commit params                 [full]           filmicrgb              piece hash=9ad7f367250b12c8, 
     7.7145 commit params                 [full]           exposure               piece hash=e5452aad4c898038, 
     7.7145 commit params                 [full]           flip                   piece hash=0, 
     7.7145 [iop_validate_params] `lens' failed for type "float", field: scale_md_v1
     7.7145 [iop_validate_params] `lens' failed for type "dt_iop_lens_params_t"
     7.7152 commit params                 [full]           lens                   piece hash=356ed5356b38a292, 
     7.7159 commit params                 [full]           denoiseprofile         piece hash=a0ee6b805127c3e3, 

@TurboGit
Copy link
Member

@jenshannoschwalm : Seeing the error could this be a coma vs dot in float?

@TurboGit
Copy link
Member

I have tested with English locale instead of French and I have:

    42.1980 commit params                 [preview]        temperature            piece hash=58cd2398489aeef9, 
    42.1980 commit params                 [preview]        highlights             piece hash=4b3de2e5a3cd1640, 
    42.1980 commit params                 [preview]        filmicrgb              piece hash=a792a8d82e1a35b3, 
    42.1980 commit params                 [preview]        exposure               piece hash=e5452aad4c898038, 
    42.1980 commit params                 [preview]        flip                   piece hash=0, 
    42.1980 [iop_validate_params] `lens' failed for type "float", field: scale_md_v1
    42.1980 [iop_validate_params] `lens' failed for type "dt_iop_lens_params_t"
    42.1988 commit params                 [preview]        lens                   piece hash=356ed5356b38a292, 
    42.1999 commit params                 [preview]        denoiseprofile         piece hash=a0ee6b805127c3e3, 
    42.1999 commit params                 [preview]        filmicrgb              piece hash=0, 
    42.2000 commit params                 [preview]        exposure               piece hash=e5452aad4c898038, 
    42.2000 commit params                 [preview]        filmicrgb              piece hash=a792a8d82e1a35b3, 
    42.2000 synch all modules done        [preview]                               defaults 0.0222s, history 0.0024s

So not problem with FilmicRGB, something really fishy here.

@jenshannoschwalm
Copy link
Collaborator

So not problem with FilmicRGB, something really fishy here.

Yes, i checked this too. After compressing history it goes away, we basically check parameters for every history step i think.

@jenshannoschwalm
Copy link
Collaborator

@jenshannoschwalm : Seeing the error could this be a coma vs dot in float?

Well i can confirm the issue can not be reproduced if importing the original xmp with "german" being the dt language.

VERY stangely - after switching back to english i can not confirm :-(

@zisoft
Copy link
Collaborator

zisoft commented Jan 10, 2024

Well i can confirm the issue can not be reproduced if importing the original xmp with "german" being the dt language.

maybe something related to #16044 where the aperture filter didn't work because of locale issues (comma vs dot in floats)?

@TurboGit
Copy link
Member

For lens see #16079

@TurboGit
Copy link
Member

@zisoft : I don't think it is related to #16044 as parameters are just seen as binary blob overlaid on top of the C struct.

@TurboGit
Copy link
Member

@Macchiato17 : Can you still reproduce with 4.6.0 (or using branch darktable-4.6.x) or master ?

@Macchiato17
Copy link
Contributor Author

First of all, thank you all for your great commitment. I really appreciate it.

So I built dt on master [420bc45] and was unfortunately still able to see this issue.

Following your discussion, I did a darktable.exe -d params -d pipe and attached the relevant logfile output here. The log starts when the photo is imported in the lighttable and ends when the image is displayed in the darkroom.

There are several occurrences of an error related to filmicrgb concerning field latitude:

316,6005 [iop_validate_params] `filmicrgb' failed for type "float", field: latitude
316,6006 [iop_validate_params] `filmicrgb' failed for type "dt_iop_filmicrgb_params_t"

I'm on a German Windows 11, dt GUI is set to English. But even when changing dt GUI to German, this issue still resist.
If there is anything else I could try out for you, please don't hesitate...

@TurboGit
Copy link
Member

Since you are building from source, can you add this patch and report what is displayed for the wrong value?

diff --git a/src/develop/imageop.c b/src/develop/imageop.c
index dad3f86913..310ba4b572 100644
--- a/src/develop/imageop.c
+++ b/src/develop/imageop.c
@@ -1972,6 +1972,16 @@ gboolean _iop_validate_params(dt_introspection_field_t *field,
       || dt_isinf(*(float*)p)
       || (*(float*)p == field->Float.Default)
       || (*(float*)p >= field->Float.Min && *(float*)p <= field->Float.Max);
+     if(!all_ok)
+     {
+       printf("@@@@@ FLOAT : %d %d %d %d %d (%f)\n",
+              dt_isnan(*(float*)p),
+              dt_isinf(*(float*)p),
+              *(float*)p == field->Float.Default,
+              *(float*)p >= field->Float.Min,
+              *(float*)p <= field->Float.Max,
+              *(float*)p);
+     }
     break;
   case DT_INTROSPECTION_TYPE_INT:
     all_ok = (*(int*)p >= field->Int.Min && *(int*)p <= field->Int.Max);

TIA.

@Macchiato17
Copy link
Contributor Author

So we get this output now:

@@@@@ FLOAT : 0 0 0 0 1 (0,000000)
110,0760 [iop_validate_params] `filmicrgb' failed for type "float", field: latitude
110,0760 [iop_validate_params] `filmicrgb' failed for type "dt_iop_filmicrgb_params_t"

Hope that helps 🙂 have a nice day👋

@TurboGit
Copy link
Member

Somehow it helps, the value 0.0000 is not a correct one as latitude (and this since first implementation of FilmicRGB) should be between 0.01 and 99.0. So somehow the params is wrong, yet the float value is 'sane' (not inf or nan) and so looks like a real value not some corrupted one. But I have no idea on how 0.0 could have been set here, and so maybe some corruption after all... As you see, it helps just a little bit :)

Side question, if you set latitude to 0.01 manually does it fixes the FilmicRGB curve and image look?

What I really do not understand is why I don't see this on my side!

@TurboGit
Copy link
Member

I tried on a second computer with CLang instead of GCC and using a Release build instead of a Debug one... And still cannot reproduce :(

@TurboGit TurboGit added understood: unclear devs lack most or all important info and can do nothing, the report will be closed after 2 weeks reproduce: peculiar the bug seems to affect only a specific target and cannot be reproduced elsewhere bug: pending someone needs to start working on that and removed reproduce: confirmed a way to make the bug re-appear 99% of times has been found labels Jan 11, 2024
@TurboGit
Copy link
Member

Another try, can you test with this change:

diff --git a/src/iop/filmicrgb.c b/src/iop/filmicrgb.c
index 6043d2cdec..4828952935 100644
--- a/src/iop/filmicrgb.c
+++ b/src/iop/filmicrgb.c
@@ -421,7 +421,7 @@ static inline void convert_to_spline_v3(dt_iop_filmicrgb_params_t* n)
   contrast *= 8.0f / (n->white_point_source - n->black_point_source);
   contrast *= hardness * powf(grey_display, hardness-1.0f);
   // latitude is the % of the segment [b+safety*(w-b),w-safety*(w-b)] which is covered, where b is black_display and w white_display
-  const float latitude = CLAMP((shoulder_display - toe_display) / ((white_display - black_display) - 2.0f * scaled_safety_margin), 0.0f, 0.99f);
+  const float latitude = CLAMP((shoulder_display - toe_display) / ((white_display - black_display) - 2.0f * scaled_safety_margin), 0.0001f, 0.99f);
   // find balance
   float toe_display_ref = latitude * (black_display + scaled_safety_margin) + (1.0f - latitude) * grey_display;
   float shoulder_display_ref = latitude * (white_display - scaled_safety_margin) + (1.0f - latitude) * grey_display;

It is just changing CLAMP lower value to 0.0001f instead of 0.0f.

@jenshannoschwalm
Copy link
Collaborator

I checked filmicrgb code 3.0: we didn't have any default,min,max settings via introspection and there was no clamping for safe data, so some direct input or magic bauhaus stuff might have inserted a zero?

@Macchiato17
Copy link
Contributor Author

Macchiato17 commented Jan 11, 2024

@TurboGit after adding your patch for filmicrgb.c (in addition to the patch before that is still applied) we get this output, still based on [420bc45]:

80,0335 commit params                 [thumbnail]      denoiseprofile         piece hash=f7e474ab4dcea664, 
@@@@@ FLOAT : 0 0 0 1 0 (69,654640)
80,0335 [iop_validate_params] `filmicrgb' failed for type "float", field: contrast
80,0335 [iop_validate_params] `filmicrgb' failed for type "dt_iop_filmicrgb_params_t"

Don't know, if this will be of any help - I added a PDF file that shows the resulting settings in filmic RGB directly after the import in Ansel (abovementioned version) and in darktable (with both patches applied)

@ralfbrown
Copy link
Collaborator

The problems is definitely in convert_to_spline_v3 in filmicrgb.c, or the function dt_iop_filmic_rgb_compute_spline which it calls. Still tracking down the details, but latitude and contrast are fine on entry and are the bad values we're seeing after the function updates them in the params structure just before returning.

@ralfbrown
Copy link
Collaborator

Well, to show variable values, I compiled as Debug, and the problem disappeared....

I did notice an unrelated error in the v3->v6 conversion: n->noise_level and n->noise_distribution should be copied from o instead of set to default values.

Copy link

This issue has been marked as stale due to inactivity for the last 60 days. It will be automatically closed in 300 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: pending someone needs to start working on that no-issue-activity reproduce: peculiar the bug seems to affect only a specific target and cannot be reproduced elsewhere scope: image processing correcting pixels understood: unclear devs lack most or all important info and can do nothing, the report will be closed after 2 weeks
Projects
None yet
Development

No branches or pull requests

7 participants