Darktable: Rewrite Crop & Rotate

[elstoc]

The crop & rotate module is a mess. It's very difficult to understand how the code works and for such a seemingly simple module it breaks constantly (one bug out, one bug in). It also contains some non-crop-and-rotate related stuff that would probably be best moved elsewhere.

I don't have strong opinions as to how this is achieved but the following is a suggestion to get the discussion started

• Create a new module that only does crop, rotate and flip, including the composition guides
• Either separate the keystone correction into a new module or incorporate it into the existing perspective correction module

[aurelienpierre]

Seconded.

[TurboGit]

As a long term project why not.

I certainly don't want to see the keystone be part of "perspective correction" as the later module works well and already complex enough. So I would certainly go with keystone in it's own module or completely remove it.

[johnny-bit]

Current perspective module is complex beast, but at least it's not crap and break ;)

I vote for simple Crop module doing absolute basic crop stuff and 2nd simple keystone module with basic keystone stuff.

[elstoc]

keystone in it's own module or completely remove it

I certainly don't want to remove it - I use it quite a bit and I like the simple interface. Separate module makes the most sense. We've also had comments from people who like it but didn't know it existed because it was hidden away in the crop & rotate module.

[matt-maguire]

When you do keystone corrections, you normally want to do a crop or auto-crop at the same time. So, I think any new keystone module would also need to do crop and rotate, and so in effect the new keystone module would end up being just a copy of the existing crop and rotate module. Maybe we should look at simplifying the code in the existing module instead, if possible.

[elstoc]

Well you could say the same for perspective correction too - that has an auto-crop mode. So you have auto-crop in the keystone module and creative crop in crop & rotate?

[parafin]

There was an argument that rotation should be done in the same step as lens correction, since both of them require pixel interpolation.

[matt-maguire]

I suppose technically a keystone correction should also use pixel interpolation -- so maybe it makes sense to imove the the keystone/tilt/shift and rotation to the lens correction. (and it already has auto-crop)

[AlicVB]

I remember to have propose such thing for the same reason long time ago. iirc, here were the resulting points :

• if all pixels interpolations are done in the same module, it'll be faster
• how to recover old settings if split in multiple modules (quite crucial are we are speaking of widely used settings)
• lensfun api already has everything in place to do the keystone (better iirc as it took in account the distance) and rotation
• some devs were used to that all-in-one module and put a veto...

So for me :

• rewrite everything from scratch
• keystone should have it's own module. That will allow to enhance it (autocropping, distance, ...)
• don't create another unmaintainable monster by adding other things to lensfun (not to speak that lensfun has no opencl)

Anyway, crop&rotate is actually so fragile that doing "real" change inside is almost impossible without breaking another part. I'm in fact quite surprised that it hasn't explode already !

[aurelienpierre]

I certainly don't want to see the keystone be part of "perspective correction" as the later module works well and already complex enough. So I would certainly go with keystone in it's own module or completely remove it.

Perspective correction module is merely a keystone, just look at the process() code. The complicated part is the GUI, which aims at detecting vertical and horizontal edges in the image and infer the best matrix of transformation to make them straight. But if you allow manual keystone input, bypassing the autodetection part, you don't even need to rewrite the pixel code, it's only GUI.

[junkyardsparkle]

I also remember some talk long ago about doing more of the interpolating operations in one place. In any case it would be nice to rethink this module in parallel with lens correction, at least.

To be honest, I think it would make more sense in some ways to remove the (not very usable) vignetting correction to a module just for vignetting-related stuff (correction or otherwise - the lens correction module can actually be used to apply rather than correct stuff). Then the interpolation stuff (lens correction, rotation, perspective warping, etc) in its own module, early in the pipe, with optional auto-cropping.Then a final simple cropper at the postion where crop/rotate is now,

One problem I can see is that changing the point in the pipe where rotation happens will probably break a lot of things... and probably lots of other problems that I'm missing altogether... but that's my thought for whatever it's worth.

[elstoc]

I suspect (though I have no hard evidence) that trying to do lots of stuff in one module is part of the problem with crop & rotate. It's a nice idea to improve performance by doing all of the 'interpolation' stuff in one module but slower is better than unmaintainable so let's make sure we don't just create another spaghetti-code module just for the sake of speed.

BTW I'd assumed we'd just keep the current crop&rotate for old edits and replace rather than rewrite.

[junkyardsparkle]

I suspect (though I have no hard evidence) that trying to do lots of stuff in one module is part of the problem with crop & rotate.

You may be right, but I suspect it depends on how you define "lots of stuff"... if it's all being done with a lensfun call, for example, is that still "lots of stuff"? In terms of developing a decent GUI, I guess it might be, don't know if it inherently is or not.

let's make sure we don't just create another spaghetti-code module just for the sake of speed.

Right (although I guess there might some quality advantage to avoiding multiple interpolation as well), but also not focus too much on the one module in isolation, if what's really needed is some re-thinking about how tasks are distributed within modules. (Part of my thoughts about breaking out vignetting are in hopes that the resulting module might go beyond the lensfun model and provide general flat-field corrections, but that's another topic).

[AlicVB]

I think there's 2 fundamental difficulties with crop&rotate actually :

• there's different pixels processing applied in the same module (only crop+rotation are applied together, keystone is applied separately iirc). And as all this produce distortion, the distort_transform functions are really difficult to deal with all the rounding, etc... issues
• there's 3 (more ?) different interactive gui in center view + the guides. They can appear together and some need the image to be partially processed by the module to be shown. Example : you apply rotation (and automatic crop associated) but not the manual cropping... and you may have keystone in the middle...

Now about the module update, the best would be to recover automatically all previous settings in the new interface, especially for cropping and rotation which are widely used. That means either :

• update current crop&rotate module with only cropping and rotation (old code need to be kept, but can be isolated) and create new module for keystone
• find a way to recover old cropping and rotation values and import them in new module

[junkyardsparkle]

Maybe also worth mentioning that there's some advantage to doing rotation early on along with other distorts in terms of retaining as much usable image area as possible. If you crop all blank pixels early on for distortions, then do it again later for rotation, you end up losing more edge pixels than you would actually need to for the combined operations.

[parafin]

Doing interpolation only in one place in the pipe will also improve quality, it's not (only) about performance really. The fact that a lot of GUI stuff will be in one module then just requires code to be written well. You don't have to write spaghetti just because better code design is not enforced by external API, you know.