darktable-org
Fix crop aligning for "Original image"
We must avoid aligning if cropping to original image aspect as we would re-calculate width&height with wrong data thus resulting in square-sized crops. This problem was only evident
- with elder edits (likely before c0a8430e647a40f2325c2c6d8ec37b0b875f81b1)
- and using "original image" ratio
We now check for valid aligning calculations so making sure.
Fixes #19919
@TurboGit @pass712 would you check?
Doesn't work for me. When I load the XMP (I have edited it to disable the liquify module) the thumbnail is square:
When I go to darkroom it is square:
When I expand the crop tool, the crop ratio is now ok:
And if I collapse the crop module all ok:
@jenshannoschwalm : My scenario (wrong crop in thumb after loading XMP) seems to indicate that some wrong parameters are recorded. So maybe we need to bump crop module version and fix this wrong params? So a change in legacy_params() instead (or also) of commit_params(). Now I have no idea how a wrong params could have been committed.
I have tested on 5.2.1 and I fear that the problem is quite more complex :(
When I load the XMP, the thumb is (not as with this PR):
When I go to darkroom I have (not as with this PR):
When I just expand and collapse crop module (nothing else) I get:
And as you can see the final result on darkroom is the same than on 5.3 (or with this PR) as reported above.
What to do with all that???? No idea at the moment...
Is that something that helps you @jenshannoschwalm ?
On thing sure, 5.2.1 is broken as much as 5.3.0 as just expanding/collapsing the crop module changes the crop area.
@TurboGit indeed a bad and somewhat complex mess.
"Fortunately" i can spot any described issue only with ratio as "original image" ending in a squared crop with current master. In the latest commit you will find a commented line reporting the aligners and this output if i load your sidecar.
257,3214 crop aligners d=1 n=0
257,3214 modified roi OUT [thumbnail] crop 2900 (0/0) 3713x5575 sc=1,000 --> (0/0) 2996x2996 sc=1,000;
257,3215 pipe starting CPU [thumbnail] (0/0) 1200x1200 sc=0,401; 'img_18.cr2' ID=358
showing that there were bad crop parameters written, that happened with late master when commiting the box with bad alignment.
I'm deeply sorry about this but i don't see a good way to fix this mess.
Some questions when using this PR:
- Do you see any issue with expanding/collapsing (or anything else) with any other crop ratio preset? I do not.
- Do you see any issue after changing the crop on a bad "square original image edit" expanding/collapsing? I do not.
If 1+2 are both safe i would be possible to do a migration algo testing for "original image" and reset cropping accordingly. Not an easy one.
For 1: I cannot answer I do crop with image aspect ratio 99% of the time and otherwise square. For 2: No I do not see issue after changing the crop aspect ratio and collapsing/expanding crop. But I see an issue if I only move the crop area.
@TurboGit i may have found a way to somewhat revert the original->quadratic problem by some hack, see the second commit and comments there.
Maybe have a talk about this mess tonight, would be fine at 9pm?
@jenshannoschwalm : I see, the hack is frightening so close of the release.
Ok to talk at 9pm. What is your timezone and DST (daylight saving time)? At the time I wrote this message is was 18:24 on my side.
I have tested this PR and the thumbs is like this:
It is not as in 5.2.1 and not as in current master. I'm not sure now which crop is correct.
When I enter darkroom I do have the same crop area (which is good).
When I expand/collapse the crop module I have the crop area changed, and back to lighttable the thumb corresponds to what I had in darkroom (which is good):
Reading the code the change in modify_roi_out() is frightening and I'm not sure this is good for the release. We really need to discuss that.
No it's not for release, it:'s the closest hack i could think of.
Its now 8 past 9 now, signal or elements?
@jenshannoschwalm : Not sure you have my id on both. Maybe jitsi meet ?
Just click: https://meet.jit.si/dt
@jenshannoschwalm : The link to the archive: https://drive.google.com/file/d/1p1Bv5c-rMTVokVrIVPJIhN6PyxTJDEB7/view?usp=sharing
It also contains screen shot of the rendering of the thumbs in 5.2.1, in master and after edit (only expand/collapse crop).
My observations, I'm pretty sure the cx & cy are ok as the cropped area when loaded into darkroom (master) was correct. So the solution discussed should possibly save us here :)
Let me know if you need something else to help you.
Mmh, first try with fixes in legacy_paramas force-pushed. The old portrait example unfortunately has cx/cy at zero :-(
The old portrait example unfortunately has cx/cy at zero :-(
How is that possible? I mean when I open the picture on 5.2.1 my crop area is properly kept and the top/left corner is on the right position and not on 0x0. Given that, my expectation what that only the width/height were somehow broken otherwise it would not be possible for the crop to work at all. So I'm lost now.
This is what I have on 5.2.1 when hovering on crop in history:
No zero there.
@jenshannoschwalm : Maybe you're looking at the master .xmp? The ones in the xmp-5.2.1 directory don't have zero and those are the ones to migrate properly.
@jenshannoschwalm : What about this which seems to be working on the 2 samples I sent:
else if(old_version == 2)
{
const dt_iop_crop_params_t *o = (dt_iop_crop_params_t *)old_params;
dt_iop_crop_params_t *n = malloc(sizeof(dt_iop_crop_params_t));
memcpy(n, o, sizeof(dt_iop_crop_params_t));
// Let's check for bad square crops because of bad edits with original image ratio
if(abs(n->ratio_d) == 1 && n->ratio_n == 0)
{
// It seems that cw is always good on the legacy parameters
// Since we have a original aspect ratio, we want the crop
// width and height to be identical in percent. That is, we
// keep the aspect ratio.
n->ch = n->cw;
}
*new_params = n;
*new_params_size = sizeof(dt_iop_crop_params_t);
*new_version = 3;
return 0;
}
That for sure breaks fresh edits from master. It seems to be fine for 5.2.1 edits...
But maybe we cannot do better for master edit. Users using master are prepared for that.
@jenshannoschwalm : Or do you have an idea to fix master edit?
Some comments
- @TurboGit suggested above simple code; the relative width/height of the selected crop area for me is not defined via the cw/ch parameters but by the difference (cw-cx) / (ch-cy) - no? (Or am i just confused right now? See modify_roi_out(). Would we want to rename ch/cw as we did for rawprepare a while ago?)
- After committing the last version (some additional checks) and testing with various master crop-edits checks it became clear to me, we have also non-square but still wrongly sized areas with current master so we have to test for crop-area-ratio matches sensor-ratio.
- I still couldn't find any 5.2.1 edit that has the cropping area not matching the sensor ratio which seems to be good.
- Also i couldn't yet find any edits with current master that don't match.
- Current correcting version 2 tests for "crop-area-ratio matches sensor-ratio" if if bad does the "correcting code".
Pascal, the corrections for "original-5.2-but-master-edits" seem to be mostly good, i didn't fully test portraits and landscapes with the "original image" crop ratio toggled - so a portrait becomes a landscape.
- @TurboGit suggested above simple code; the relative width/height of the selected crop area for me is not defined via the cw/ch parameters but by the difference (cw-cx) / (ch-cy) - no? (Or am i just confused right now? See modify_roi_out(). Would we want to rename ch/cw as we did for rawprepare a while ago?)
I think you're confused. The cw and ch are width and height in percent of the original image size. In modify_roi_out() you want the margin top/left/bottom/right (odx is the margin on the right, no?) I think. Or am I the one to be confused :) ?
Hi @TurboGit and @jenshannoschwalm ,
there are issues too with an image that was cropped freehand under 5.2.1. It also converts to a 1:1 crop under 5.3.0 by only opening and closing the crop module. Hope you also solve this with your solution.
If you need the DNG and XMP please let me know.
- Opened image in darkroom
- Opened the crop module (the shown crop rectangle is correct)
- closed the crop module -> square crop
- Will export with 2999 : 3000
@jenshannoschwalm : So at this stage what do you think? Is my simple patch ok for 5.4 and we tell that any edit with master will be possibly broken? Or do you see a possible way to avoid this?
there are issues too with an image that was cropped freehand under 5.2.1.
So we are doomed :(
EDIT: And for freehand crop we don't have a way to recover since we know nothing about aspect ratio.