Open agriggio opened 7 years ago
I think it's a great idea. Please let me know if I can help.
@agriggio :+1: You can count on me for this task. It will save us a lot of time once it's completed.
@agriggio On principle I totally agree and if I can be of any help.
It seems to me that it will be necessary to make an inventory of the system. Some procedures, for example "denoise" have almost all the code in dcrop.cc, others like locallab, have an important place in improccoordinator - with not simple links with Dcrop.cc, etc.
It would be a good idea to propose a "project" type approach, with goals, stakes, deadlines, etc. jacques
@agriggio
afaik:
rtengine/improccorrdinator.cc
full image (but scaled down, not full size) for the navigator and the pan backgroundrtengine/dcrop.cc
for the main preview and the detail windowsrtengine/simpleprocess.cc
for the final outputrtengine/rtthumbnail.cc
for thumbnailsThanks to all for your comments. Some replies:
@Beep6581: since this is not going to happen overnight, and require some changes to the internals, one thing that needs to be done is decide when to work on this, and what to do with the open branches, as I suppose that merging after the refactoring could be complicated...
@Desmis: I agree in principle about having a project plan. But I do RT for fun, and doing detailed plans is no fun :-) On the other hand, we cannot just go without any idea of the direction, it will never work. So, I agree with you, the first thing to do would be to collect some requirements and hear from people why some things are where they are now.
Regarding the goals: we just want one central place in which the pipeline is defined, that is flexible enough to be used wherever it is needed now (i.e. in the 4 places listed by @heckflosse).
I think the interface should be an extension of the current StagedImageProcessor
, with a couple of extras:
a thumbnail
flag that tells whether we are generating a thumbnail, so
that we know we can bypass some of the steps
a way of distinguishing between a "working" crop and skip factor and an
"output" crop and skip factor. This is needed e.g. for generating an
accurate preview for crops when there are some tools enabled that need the
full image to calculate their parameters (e.g. see #3895 and #3793). This
is also needed for correctly applying LCP distortion correction (currently
there's a hack in dcrop.cc
that internally computes a slightly larger
crop that what is shown, to make sure that we have all the input pixels
needed to perform distortion correction). Having this would also allow to
implement the "Give me a HQ preview no matter what" button that was
discussed recently.
a fast export
flag for the fast export pipeline
Once we have the design in place, I think we can aim for the following milestones:
replace the implementation of simpleprocess.cc
with the new
pipeline. This is both the easiest to do (no dependencies on the rest of
the code) and the easiest way of catching problems -- because if the output
changes when compared to the current dev
, we know there is a problem
replace the implementation of rtthumbnail.cc
with the new pipeline
use the new pipeline for improccoordinator.cc
finally, use the new pipeline for dcrop.cc
Of course, this is all still very vague, and subject to change. It is here essentially to serve as a kick-off for the discussion. I'm looking forward to your comments, suggestions, an corrections :-)
Currently RT has several (at least 3, possibly 4?) different processing pipelines, used in different places:
rtengine/improccorrdinator.cc
for the main preview panelrtengine/dcrop.cc
for the detail windowsrtengine/simpleprocess.cc
for the final outputThis makes the code maintenance harder than necessary. We should try to refactor the code so that the various processing routines are defined only in one place. Ideally, I think we should have only one processing pipeline, flexible enough to accomodate all the current use cases. I believe it should be possible, given enough motivation.
Since there are a number of issues popping up related to this (e.g. #3895 and #3793, and also #3826), I am willing to volunteer and give this a high priority (but as usual, remember I'm doing this in my free time, so take this with a grain of salt :-). If there's anyone else that is interested in helping (@heckflosse maybe?), let me know.
Also, if there is someone who thinks this is not a good idea, please do share your concerns, thanks!