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

[V2] Optional full image processing in darkroom #15910

Merged
merged 5 commits into from
Dec 23, 2023

Conversation

TurboGit
Copy link
Member

New version on top of #15893 which fixes the snapshots & duplicates when changing the full image processing mode.

@TurboGit TurboGit added this to the 4.8 milestone Dec 19, 2023
@TurboGit TurboGit added feature: enhancement current features to improve priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: image processing correcting pixels documentation-pending a documentation work is required release notes: pending labels Dec 19, 2023
@jenshannoschwalm
Copy link
Collaborator

Perfect.

@elstoc
Copy link
Contributor

elstoc commented Dec 19, 2023

I'm not sure about calling it "export view scaling" - it feels like a description of "why you might use it" rather than "what it is"? I can see the intention but I guess I might go for something that emphasises that we're processing the whole image (which incidentally will look just like a full export) and makes clear any performance implications. I haven't thought of anything decent yet but just wanted to throw it out there for consideration.

@jenshannoschwalm
Copy link
Collaborator

I would be happy with any better naming being not technical. I guess only a few users understand the implications of roi concept.

"High quality processing" as we have in export?

@TurboGit
Copy link
Member Author

@jenshannoschwalm

"High quality processing" as we have in export?

Works for me.

As we are at it, would it makes sense to use this new dev support for the export high quality processing? Or does it something different?

@jenshannoschwalm
Copy link
Collaborator

As we are at it, would it makes sense to use this new dev support for the export high quality processing?

The button is only visible in darkroom and as this implies a significant-to-huge amount of processing we should leave it that way. Or are you thinking about the export pipe?

Or does it something different?

No. Same processing way. But for export we have a different roi_out and might upscale.

Three more points. We haven't yet made up our mind about

  1. "How" do we scale for output? Should we scale while still being in linear space? I experimented with scaling done as pre-down-scale before sigmoid/filmic and also before colorout (my initial draft pr on this topic) with certainly different results. Not sure what is the best way color-maths wise.
  2. We thought about some additional post-scale sharpening.
  3. Not fully sure about fast pipe mode.

Those might require further changes so i would suggest to merge this rather soon as is represents the current pipeline processing and we might get more precise feedback on "what happens to my colors". For me the new thing is not only about inspecting export output but also about what might be bad processing design in the current pixelpipe.

@jenshannoschwalm
Copy link
Collaborator

@TurboGit tested this right now in-depth. Unfortunately it doesn't fix: we want to compare a snapshot taken without full processing vs "current state" meaning you just switched it on. Use -d pipe to watch for either downscaling in demosaic or finalscale.

Wouldn't this be right? (We see the displacement though ...)

diff --git a/src/iop/finalscale.c b/src/iop/finalscale.c
index a1a99b2746..abcfd070d1 100644
--- a/src/iop/finalscale.c
+++ b/src/iop/finalscale.c
@@ -58,12 +58,12 @@ dt_iop_colorspace_type_t default_colorspace(dt_iop_module_t *self,
   return IOP_CS_RGB;
 }
 
-static inline gboolean _gui_fullpipe(dt_dev_pixelpipe_iop_t *piece)
+static inline gboolean _gui_fullpipe(dt_dev_pixelpipe_iop_t *piece, dt_iop_module_t *self)
 {
   return piece->pipe->type & (DT_DEV_PIXELPIPE_FULL
                               | DT_DEV_PIXELPIPE_PREVIEW2
                               | DT_DEV_PIXELPIPE_IMAGE)
-    && darktable.develop->late_scaling.enabled;
+    && self->dev->late_scaling.enabled;
 }
 
 void modify_roi_in(dt_iop_module_t *self,
@@ -100,7 +100,7 @@ void modify_roi_in(dt_iop_module_t *self,
                                piece->buf_in.height));
   roi_in->scale = 1.0f;
 
-  if(_gui_fullpipe(piece))
+  if(_gui_fullpipe(piece, self))
   {
     roi_in->x = 0;
     roi_in->y = 0;
@@ -188,7 +188,7 @@ void commit_params(dt_iop_module_t *self,
                    dt_dev_pixelpipe_iop_t *piece)
 {
   piece->enabled = piece->pipe->type == DT_DEV_PIXELPIPE_EXPORT
-                  || _gui_fullpipe(piece);
+                  || _gui_fullpipe(piece, self);
 }
 
 void init_pipe(dt_iop_module_t *self,

@TurboGit
Copy link
Member Author

TurboGit commented Dec 20, 2023

we want to compare a snapshot taken without full processing vs "current state" meaning you just switched it on

I'm not sure. I want to compare snapshot vs current dev in the exact same mode so with or without full image processing for both. It makes no sense to me to compare otherwise, that's like ISO 12646 mode to me, we have it for the snapshot and the dev at the same time. Don't you agree? Did I missed some use case?

EDIT: Another way to say it, this mode is not a develop step so not part of the snapshot history.

@jenshannoschwalm
Copy link
Collaborator

Maybe we misunderstand here.

As @s7habo pointed out, it might be nice to do a snapshot without full data, switch full mode on and see the difference while moving the snap-line around. The patch would make this working.

Personally I don't care that much, my patch might even make the snap tool more confusing.

So probably let's go with what we have....

@TurboGit
Copy link
Member Author

Personally I don't care that much, my patch might even make the snap tool more confusing.

Yes, that's my fear too as now for every history step we could have two versions.

To me the way to compare with and without the full image processing is by enabling / disabling the mode by clicking on the new button.

So yes, let's settle with what we have.

As a side note I plan starting merging the 4.8 work on master the coming week-end or next week.

@TurboGit
Copy link
Member Author

"High quality processing" as we have in export?

Yes, I'll change the label.

jenshannoschwalm and others added 5 commits December 23, 2023 10:33
Adds another darktable-lower button, if active the canvas and second window pixelpipes will be processing
the full image data.

The finalscale module has been modified, it now does roi aware crop&scale instead of pure scaling
and is also enabled if the mentioned toogle-button is ON.

Toggle-button state is not saved between sessions.

sahvcdfiv
If we use darkroom full-image expanded roi using opencl we wantto make sure
the cldata are written to the cache so we can move canvas window around without
any significant reprocessing.
Add status to view context to ensure that snapshots and clone are
properly invalidated when changing the late scaling status.
@jenshannoschwalm
Copy link
Collaborator

Go ahead :-)

@TurboGit
Copy link
Member Author

Merging now :)

@TurboGit TurboGit merged commit 13cbc80 into darktable-org:master Dec 23, 2023
4 checks passed
@TurboGit TurboGit deleted the finalscale_darkroom branch December 23, 2023 10:19
@jenshannoschwalm
Copy link
Collaborator

Release note:

Implemented a toggle switch for the darkroom mode forcing the pixelpipe processing to use the whole image data instead of just the area displayed. This allows the user to inspect processed data without errors introduced via internal scaling and equals what we get by exporting in "high quality resampling" mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-pending a documentation work is required feature: enhancement current features to improve priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: image processing correcting pixels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants