Project

General

Profile

Bug #11329

Perspective Correction & Crop and Rotate kill histogram and preview

Added by Joe Giampaoli over 3 years ago. Updated over 3 years ago.

Status:
Fixed
Priority:
Low
Assignee:
-
Category:
Darkroom
Target version:
Start date:
11/22/2016
Due date:
% Done:

100%

Estimated time:
Affected Version:
2.2.0rc0
System:
Debian
bitness:
64-bit
hardware architecture:
amd64/x86

Description

Was testing out "Perspective Correction", everything was good until I activated "Crop & Rotate", histogram, curvas data and preview on left above history stack disappeared. Seems to only happen when activating both those modules together.

Screenie attached...

Thanks!

Screenshot - 11222016 - 03_04_05 PM.png (797 KB) Screenshot - 11222016 - 03_04_05 PM.png Joe Giampaoli, 11/22/2016 11:09 PM
Terminal_Output (10.2 KB) Terminal_Output Joe Giampaoli, 11/23/2016 02:40 AM
ScreenCap.mp4 (20.4 MB) ScreenCap.mp4 Joe Giampaoli, 11/23/2016 02:43 AM

Associated revisions

Revision 805745f6 (diff)
Added by Ulrich Pegelow over 3 years ago

clipping: quick workaround for bug #11329

this is just a quick hack to overcome the bug before the 2.2.0 release.
all in all the clipping module is totally screwed and needs a major/full
rewrite.

Revision 9889e97f
Added by Ulrich Pegelow over 3 years ago

Merge pull request #1363 from upegelow/clipping_workaround

clipping: quick workaround for bug #11329

History

#1 Updated by Tobias Ellinghaus over 3 years ago

  • % Done changed from 0 to 20
  • Status changed from New to Incomplete

Please try starting darktable from a shell with "-d nan" added and see if it complains about strange intermediate values when you trigger that bug.

#2 Updated by Joe Giampaoli over 3 years ago

OK

Ran with that switch and did a screen-cap also with the output from terminal.

On the screencap you can see that the problem appears when actually changing rotation value in the "Crop & Rotate" module once "Perspective Correction" is enabled. Strangely, I discovered that with this specific image when activating the "Levels" module histogram and preview were rendered back to normal. Turning "Levels" off shows the problem again.

Terminal output is exactly what you see in the screen-cap.

I tested also on my laptop Ubuntu 16.04 with same results except that turning "Levels" module ON didn't fix it like on the Debian PC.

Thanks!

#3 Updated by Tobias Ellinghaus over 3 years ago

  • Target version changed from Future to 2.2.0
  • Status changed from Incomplete to Triaged

It seems that the preview pipe moves the image away when rotating with enabled perspective correction. Might be a wrong rotation center.

#4 Updated by Ulrich Pegelow over 3 years ago

Confirmed here too. Seems that this happens regardless if the perspective correction module applies any changes or not.

#5 Updated by Ulrich Pegelow over 3 years ago

I bisected the problem and it goes down to this commit (which was merged only in October '16 as part of PR#1161):

de35b592455ddd9a512eb9e6de4ecb4fa9d56843 is the first bad commit
commit de35b592455ddd9a512eb9e6de4ecb4fa9d56843
Author: AlicVB <alic.vb@gmail.com>
Date:   Fri Feb 26 18:08:11 2016 +0100

    clipping: workaround for roi int / float rounding

    as dt_iop_roi_t contain int values and not floats, we can have some rounding errors
    this is especially visible for masks, as they use preview pipe with low resolutions
    Fixes bug #10928

:040000 040000 729cd87e5281a4d5d0642a4cc3bd08854fcc912e bc222f0c35ad1647a7a7e4f1dc4927219af74073 M      src

Background: ashift uses the dt_dev_distort_backtransform_plus() mechanism within process() to determine if the user-visible image is flipped or not. We do this in order to give correct labels to the "lens shift" parameters ("vertical"/"horizontal").

With the changes in the a.m. commit distort_transform() and distort_backtransform() in clipping.c were changed in a way that they now deliver other results for the preview pipe. This obviously no longer works.

I don't yet understand the details. However, these two functions look anyhow very fishy as they call self->modify_roi_out() as a side effect. This side effect has been introduced with commit 3bf3b3a48420389c8180bacf647b5d0bff71723c long time ago. IMHO changing roi_out within the distort_{transform,backtransform} functions might not be a good idea. Is this really needed?

I'm putting a few other devs in copy as I'm not sure how to fix this.

#6 Updated by Ulrich Pegelow over 3 years ago

Small correction: functions distort_{transform,backtransform}() in clipping.c only work on their own copies of roi_in and roi_out. The problem comes from the fact that self->modify_roi_out() is called which as a side effect changes values in piece->data (k_h, k_v, cix, ciy to name just a few). Those values are used then later in clipping.c:process().

Now, the a.m. commit constructs a roi_in with artificial data (scaled by 100 for the preview pipe to improve calculating accuracy) and then calls modify_roi_out() with that virutal roi_in. Unfortunately, this step as a side effect modifies piece->data in a non compatible way. Meaning process() is affected by the wrong values in piece->data.

#7 Updated by Ulrich Pegelow over 3 years ago

OK, we have two short-term options to fix this issue before 2.2.0:

  1. we either revert commit de35b592455ddd9a512eb9e6de4ecb4fa9d56843 which will bring back bug #10928,
  2. or we implement a quick hack that restores reasonable defaults into piece->data after distort_{transform,backtransform}(). A proposal is offered in PR1363.

Opinions?

Mid-term we will need a major rewrite of the clipping module. IMHO it's f****d up beyond all repair.

#8 Updated by Roman Lebedev over 3 years ago

Should this be closed then?

#9 Updated by Ulrich Pegelow over 3 years ago

  • % Done changed from 20 to 100
  • Status changed from Triaged to Fixed

At least we have a temporary fix.

Also available in: Atom PDF

Go to top