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.76k stars 1.14k forks source link

Drawn mask misaligned #3904

Closed kofa73 closed 3 years ago

kofa73 commented 4 years ago

Describe the bug Drawn masks in (at least) channel mixer take effect not where they are drawn (shifted a few pixels).

To Reproduce

  1. Download the NEF and XMP from https://tech.kovacs-telekes.org/files/dt-drawn-mask/ and import into darktable. I was trying to correct a red eye using a masked instance of channel mixer.
  2. Open channel mixer
  3. In the drawn masks UI, display the drawn shapes. You'll see that the effect is not where the shape is. (Depicted in https://tech.kovacs-telekes.org/files/dt-drawn-mask/drawn_mask.png)
  4. Display the mask. You'll see it's not yellow where it should be according to the shape, but shifted a bit, consistent with the affected area as visible when the mask is not shown. (Depicted in https://tech.kovacs-telekes.org/files/dt-drawn-mask/drawn_mask-shown.png)

Expected behavior Mask covers the same area as the shape (there is no shift).

Screenshots Done.

Platform (please complete the following information):

elstoc commented 4 years ago

I've noticed a few odd issues similar to this in current master (3.1.0+326~g250fafcca). I had an issue (which I couldn't subsequently reproduce after exiting darktable) where a drawn mask on the exposure module wasn't respecting the changed orientation (was 180 or 90 degrees off w.r.t the centre of the image until I changed the orientation to match). At the moment I'm having issues with gradient masks not landing in the same place they were when I let go the mouse button. Interestingly if I switch off lens correction, that seems to resolve the issue.

Where does darktable get the information about where in the image masks are placed? Presumably for consistency (and so you could reuse masks in later modules) it would have to work from quite an early module in the pipeline (I'm just guessing)?

Could the recent reordering of the modules have caused this behaviour and the lens correction module be throwing the mask off?

github-actions[bot] commented 4 years ago

This issue did not get any activity in the past 30 days and will be closed in 7 days if no update occurs. Please check if the master branch has fixed it since then.

kofa73 commented 4 years ago

Reproducible with 8e5874931 Also with 1f264f909

elstoc commented 4 years ago

This is an issue with how masks are transformed through modules that distort the image, for example, lens correction or liquify. My (albeit incomplete) understanding is that the masks are essentially drawn based on co-ordinates on the original image and that mask is then transformed through the pixelpipe. This should work fine for rotations but for more complicated distortions the logic fails.

Transformations like liquify or lens correction can distort the mask significantly. As an example, I've used a nice polka-dot grid which has been distorted using the liquify module. I've then attempted to draw various masks in the exposure module over the most distorted part of the image:

A (straight line) gradient mask:

Screenshot_2020-04-04_11-11-12

A (rough) rectangle path:

Screenshot_2020-04-04_11-13-31

A circle:

Screenshot_2020-04-04_11-14-25

I could go on but you get the point. The issue is that the entire shape is being drawn on the untransformed grid and then distorted through the pipe. But really what you want to do is draw the individual reference points (the centre of the circle, the points on a path, the centre of a gradient) on the original grid and then use those reference points to generate the shape on the target module.

I've probably used some wrong terminology and not explained myself very well, because I don't entirely get how it works. I suspect it might require quite a big change to make it work in a more intuitive way.

A more common use case is that you use lens correction to straighten the lines of a square, then you're unable to actually draw a straight path over that square because the path is now distorted by lens correction.

AlicVB commented 4 years ago

Well, not sure that it's a solvable issue :( Correct me if I misunderstand the point here, but if you go this way

  1. the user expect that when he draw a circle in module X it appears as a real circle on screen, right ?
  2. Technically, that means that you want to use the final image (== pipe output ; == what is shown on screen) as reference for the mask coordinate.
  3. But what if pipe output change with another distort module after you have drawn your mask ? your mask will be completely off...

iirc, that's how it was working long time ago in dt, and the instructions for end users were : "crop/rotate your image first, and then draw your masks (was only for spot removals) and don't touch crop/rotate anymore..."

elstoc commented 4 years ago

I'd want to use the original image (start of the pipe) as a reference for the mask reference co-ordinates but draw the actual shape with reference to the input of the module in question (or the end of the pipe). That way any change to the intervening distorting modules would keep those reference points unchanged (I think). Though how you manage that when you're dealing with angles and rotation I don't know. It doesn't seem simple.

I tend to agree that this might not be resolvable but it would be nice to at least resolve with respect to the lens correction module because this is a very commonly-used module. Perhaps we could move the base co-ordinates to post-lens-correction and pop up a warning if the user enables lens correction later in their pipeline and has used any masks? I don't know, but I hate to give up on something like this because it is very difficult to mitigate this issue (particularly for an image containing straight lines) and it's very common when dealing with cameras having smaller sensors. I use an RX100 and the distortion is quite significant at wide angles.

parafin commented 4 years ago

Even if implemented, this idea will present its own problems - let's say you draw a circle mask for some iop closer to the end of the pipe, then enable liquify iop, which comes before, and do some transformation. Should this circle remain a circle or what? If it remains a circle, then it will cover a different area. If not - by what rules should it be transformed? Making output of lens correction as reference coordinates makes it impossible to use masks for iops coming before it, plus if those iops do transformations, then your masks will move.

elstoc commented 4 years ago

In the default pipe order none of the modules before lens correction do any transformations. I do agree that there aren't any good solutions here (and perhaps no solutions at all) - they all will have side-effects that need to be understood. The questions you ask definitely need answering but there are multiple possible answers and we shouldn't necessarily assume that the current answer to those questions is the best one (though it may be). More common use-cases should take priority.

I used liquify as an example because it was best for illustrating the origin of the problem, though it's perhaps not a common enough module for us to overly consider it when deciding what the best solution is. I'd imagine, for example, that nobody ever enables it by default.

I mentioned lens correction because it's a real and common issue that I've had on multiple occasions. With my RX100 images and those on my point-and-shoot cameras drawing consistent masks is a big problem.

AlicVB commented 4 years ago

there's just one other way : When you draw the mask, transform all it's points to original image reference and store that points instead. That would means lot of drawbacks, thought :

really, I'm not sure it's solvable :(

elstoc commented 4 years ago

@AlicVB that was the only alternative I thought of but I agree it's not practical.

elstoc commented 4 years ago

Could lens correction realistically go any earlier? e.g. before the denoise and highlight reconstruction modules - which are the only ones with masks?

AlicVB commented 4 years ago

@elstoc : I would say no, as lens correction use rgb values as input, and highlight reconstruction work directly on raw data (before demosaïc, iirc) but I don't really know the internals of those modules...

elstoc commented 4 years ago

That makes sense :(

aurelienpierre commented 4 years ago

Given that whatever happening before the input color profile is supposed to be technical and not creative, there is no reason to use drawn masks in that part of the pipe. We could just enforce creative stuff between colorin and colorout, hence consider coordinates valid at the output of colorin. Which might require to move liquify and retouch higher in the pipeline.

elstoc commented 4 years ago

I'd want to be able to move retouch/spot removal before crop & rotate in the pipeline as I often want to take my source areas from outside of a cropped area.

aurelienpierre commented 3 years ago

Tested and reproduced, the bug happens even with no distortion module on (no lens correction, no perspective, no cropping, nothing).

TurboGit commented 3 years ago

@kofa73 : do you confirm that this only happens for RAW files (tested with NEF and RAF) ? The same image exported full size as TIFF does not exhibit the issue on my side and I just want to get confirmation on this.

kofa73 commented 3 years ago

@TurboGit:

  1. I still have the problem with the raw.
  2. I tried exporting the raw without any touch-up as a TIF, loaded that into darktable, and applied some effect (exposure -3 EV and also color balance RGB -> 4 ways -> global luminance -100%) with circular and elliptic drawn masks. The affected area match the mask, just like for you. The bad news is, the masks look horrible, jagged. Exported files are also affected, this is not just the darkroom presentation. image I've uploaded the tif + sidecar + my darktablerc revision: e7ab836c8
TurboGit commented 3 years ago

Thanks for testing!

2\. The affected area match the mask, just like for you.

So this is really affecting only RAWs !!! Don't know why but that's a point to consider to pinpoint the issue.

The jagged part is probably another issue and let's concentrate on the mask shifting for now.

AlicVB commented 3 years ago

looks like a missing/buggy (back)transform routine in a module that only affect raw files... don't we have something to crop the raw data in entry ? or maybe demosaic ?

kofa73 commented 3 years ago

The jagged part is probably another issue and let's concentrate on the mask shifting for now.

Agreed. Opened #8685 to track that.

TurboGit commented 3 years ago

Found the issue, it is that rawprepare (which handles raw crop) has a transform/backtransform that is not called at all and the distort_mask is not implemented.

jenshannoschwalm commented 3 years ago

I also observed that while on the details mask work. In #8643 you find find the fix for rawprepare distort_mask as

void distort_mask(struct dt_iop_module_t *self, struct dt_dev_pixelpipe_iop_t *piece, const float *const in,
                  float *const out, const dt_iop_roi_t *const roi_in, const dt_iop_roi_t *const roi_out)
{
  dt_iop_copy_image_roi(out, in, 1, roi_in, roi_out, TRUE);
}
TurboGit commented 3 years ago

I also observed that while on the details mask work. In #8643 you find find the fix for rawprepare distort_mask as

void distort_mask(struct dt_iop_module_t *self, struct dt_dev_pixelpipe_iop_t *piece, const float *const in,
                  float *const out, const dt_iop_roi_t *const roi_in, const dt_iop_roi_t *const roi_out)
{
  dt_iop_copy_image_roi(out, in, 1, roi_in, roi_out, TRUE);
}

Well, distort_mask is only used for raster masks. So this is not the issue at hand. What I found is that if I set the crop for left/top different than 0 then everything is aligned.

To be able to set the crop manually one need to set the hidden pref plugins/darkroom/rawprepare/allow_editing_crop to true.

But I'm still not sure why this is so...

jenshannoschwalm commented 3 years ago

What I found is that if I set the crop for left/top different than 0 then everything is aligned.

But I'm still not sure why this is so...

I think you were referring these two together. Because we want the raw modules and demosaicers input to be aligned for better performance and tiling?

jenshannoschwalm commented 3 years ago

rawprepare (which handles raw crop) has a transform/backtransform that is not called at all

Wrong setting of transf_direction == DT_DEV_TRANSFORM_DIR_FORW_EXCL or it's back friend

TurboGit commented 3 years ago

In fact no, I was wrong. It is properly called just that when x / y is 0 (no offset) it is skipped. That part is ok.

What I don't understand is that setting x / y to whatever but zero fixes the issue.

@@ -627,8 +652,8 @@ void commit_params(dt_iop_module_t *self, dt_iop_params_t *params, dt_dev_pixelp
   const dt_iop_rawprepare_params_t *const p = (dt_iop_rawprepare_params_t *)params;
   dt_iop_rawprepare_data_t *d = (dt_iop_rawprepare_data_t *)piece->data;

-  d->x = p->x;
-  d->y = p->y;
+  d->x = 1; // p->x;
+  d->y = 1; // p->y;
   d->width = p->width;
   d->height = p->height;

But of course this makes no sense. BTW, you can set 20 or 50 instead of 1 and all is still ok.

I'm lost as to what this could activate at the moment. Any idea? Or maybe there is some other part of the code (using introspection) that look for those values?

jenshannoschwalm commented 3 years ago

In the distort routines we test for x/y offsets ==0 and leave with true flag immediately, i guess you meant that part to be ok.

In process and the roi changes we use round_smart, we don't do in the transforms. Is that really right? this could lead to offset mismatches.

TurboGit commented 3 years ago

@jenshannoschwalm : Indeed related to round_smart(), I have a propose PR #8689 which is maybe not correct. I'll experiment using round_smart() in distrort routines.

TurboGit commented 3 years ago

Well using round_smart() in distort does not work and anyway the distort in the test image was not calling distort routines as x & y were 0 anyway. So my current fix is the only I can think of at the moment.

TurboGit commented 3 years ago

@jenshannoschwalm : if you have any other idea do not hesitate. I'd like also your review on my PR. TIA.