Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retouch #1548

Merged
merged 5 commits into from Sep 3, 2018

Conversation

Projects
None yet
6 participants
@edgardoh
Copy link
Contributor

commented Oct 29, 2017

The retouch.c iop does a wavelet decompose and for each scale allows to clone, heal, blur or fill.

It's based on the spots iop, the wavelet decompose plug-in from GIMP and the healing from GIMP.

The healing works fine for small areas, for larger areas it can be slow and results not so good, but the idea of the module is to use it for retouch (and sensor dust) and healed areas should be small on those cases.
On noisy images results can also be not so good, but when healing on a scale I always be able to get good results.

@LebedevRI

This comment has been minimized.

Copy link
Member

commented Oct 30, 2017

This is the third pr, and both the previous two were closed by you.
All of them had a pretty large diffs, but zero communications.
Just trowing code at us is not how it works, depending on the endgoal of course.

So i guess i have to ask, what is your agenda?

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2017

This is my first pr and I don't know how things work, so my apologies if everything is not ok.

I had some build errors when pushing the pr, so I fixed & pushed again, I think that's the closed ones.

I also thought that with the description on the first one was enough, so I'll try again:

I have developed a new iop (that can also work as an enhancement), based on the spot removal, the wavelet decompose plug-in from GIMP and the healing from GIMP.

It performs a wavelet decompose and for each wavelet scale it allows to clone, heal, blur or fill.

The retouch.c is the new iop.
I have created separate files for the wavelet decompose (dwt.c) and the heal (heal.c) on src/common, and they have the opencl versions too.

There is some modification on the base code, some are required and others are nice-to-have:
-I created a new mask type, DT_MASKS_NON_CLONE (yep, not the best name). The new module uses clone masks (like the spot removal) and regular masks (for the blur and fill). The regular masks are added to the mask manager while the clone masks are not, so this new type is a mix of both, it acts like a regular mask but is not added to the mask manager.
-the brush can act like a clone mask, this is nice when healing
-the user can select a shape by clicking on it, this is the best way I can think of in order to allow to change the blur radius and the colour on a shape (and any other parameter that in time may be added)
-I added a preview when creating a circle (like the brush does)
-I added a short-cut to allow to skip mouse events on the shapes (or masks). Sometimes there is not too much room left to drag the image or zoom in/out, so it's nice to press a key and be able to drag the image or zoom even if the mouse is over a shape
-I allow to resize a path even if the mouse in on a segment or a node
-Some small changes on the default size of the shapes and the increase step, to make it more consistent or because I find it better when retouching. As an example, very often I needed a smaller circle than dt allows right now.

I have designed it for retouching and with my worflow in mind, but of course all of the above is up for discussion.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Oct 30, 2017

I have tried it but I could not make it to work. Maybe some bugs? But most of all it does not seems to be obvious to use... Many buttons... At some point during my testing I had lost all visible shape to control the forms and did not find a way to get them back. Anyway, first step would be to ensure that the iop does work almost in all cases and maybe add here some notes about way to use it.

@LebedevRI

This comment has been minimized.

Copy link
Member

commented Oct 30, 2017

This is my first pr and I don't know how things work, so my apologies if everything is not ok.

The first thing to do is to talk to us first, as it was suggested quite some time ago in the first pr
#1090 (comment)
Unless somehow you are not the same gh user as ^ ?

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2017

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2017

@theres

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2017

Hi @edgardoh,
I've tested your iop a bit. I missed these functions a lot!
A few comments from the user point of view to consider:

  • new created shape should remain selected - I need to click once again to eg. change blur radius
  • if shape is selected, user may expected that change sliders will affect given shape - this work for eg. blend factor, but not for current scale - I need to cut/paste shape - unclear and unproductive
  • I assume that the scale is common parameter for all shapes, so maybe move it to the top - now it looks like each shape can have it's own number of scales.
  • what's happen when current scale > scale?
  • seems that sometimes I need to click current scale once again to see shapes
  • a combobox with all created shapes to select would be a plus
  • I'd consider the possibility of changing the algorithm for the selected shape
@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2017

@TurboGit

This comment has been minimized.

Copy link
Member

commented Oct 31, 2017

Ok, I have been able to have the clone tool working. I have been able to have the heal tool working and it is indeed quite good as the result is clean and there is no halo or whatever. That's great!

For the decompose, well I just still don't understand how to use it. I have tried it given your instructions, but well...

First what is it supposed to do?

Also, the set of options cannot be applied to a selected shape, it makes the tools difficult to test.

@theres

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2017

@TurboGit you can use this eg. for skin retouch (similar method to https://www.youtube.com/watch?v=81yxqfHNElE)

@TurboGit

This comment has been minimized.

Copy link
Member

commented Oct 31, 2017

@themes: thanks, but now what are the steps to achieve the same with the retouch module. I mean to create all the masks with different scale? And work with them... I'll try again, but the usage is not obvious to me!

@TurboGit

This comment has been minimized.

Copy link
Member

commented Oct 31, 2017

Some progress, looks like I have some result with the clone/heal tool and scale. Is that possible to use scale with blur tool? Looks like it does nothing on my side...

@TurboGit

This comment has been minimized.

Copy link
Member

commented Oct 31, 2017

BTW, I do think that the simple heal tool should be also copied into the current spots removal IOP. I would even make it the default algorithm.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2017

@TurboGit , I made some UI changes and wrote a more detailed description. I'll post it when the build check finish (hopefully ok)

The blur algorithm has a default radius of zero, maybe that's why you don't see the effect? To change it, select the shape (by clicking on it), the blur radius will be displayed (if not already) and change it to taste.
Another way is to change the radius before adding the shape, it will take the new value.
If you are adding it on a low scale (like "current scale"=1) the result will be hard to see, you can use the display option "display current scale" to see the effect on the scale itself.

All combinations of shape type / algorithm can be added on any scale (current scale), even if it is > "number of scales"

This IOP can be an enhancement on the spot removal. With the default values it acts almost exactly like the spots, and I think that the only thing needed is to add the legacy_params(). But I feel that we are very far from this decision.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2017

I made some changes to the UI, hopefully it is more clear now.

Here is a more detailed description on how the module works:

"retouch tools" section

-The first toolbar allows to select the type of shape (circle, ellipse, path, brush). The arrow allow to show/hide them.

-The second toolbar allows to select the algorithm (clone, heal, gaussian blur, fill)
clone and heal does not have any properties
gaussian blur has a radius
fill has fill mode: erase or color
erase is used to erase detail on a detail scale
color fills the shape with the selected color, only useful in the original image ("current scale" = 0) or the residual image ("current scale" = "number of scales" + 1)
The properties are displayed on the "shapes" section and they show/hide depending on the algorithm selected.

"wavelet decompose" section

-"number of scales" controls the number of scales the image will be decomposed. 0 means no decomposition, > 0 means it will decomposed on "number of scales" + 1 (the residual image).
-The 3 buttons to the right allows to: display the reconstructed image ("show final image"), the current scale ("show current scale", only for preview, it will display the reconstructed image when the iop looses focus) or to keep the current scale ("keep current scale", it will display the current scale even if the iop looses focus)

-"current scale" controls in which scale the shapes are added and which scale is displayed, depending on the display option (the 3 buttons on the right of "number of scales").
0 means original image, 1 to "number of scales" are the detail scales, "number of scales" + 1 is the residual image.
If shapes are visible, it will display only the shapes for this scale.

-The next 2 buttons allows to cut and paste from one scale to another. Cut on the current scale, change to another scale and paste it, all the shapes are moved from one scale to the other. Right now there is no other way to change the scale where a shape belongs.

-"blend factor" adds the value selected to the detail scale so it can be preview. This works only when the display options "show current scale" or "keep the current scale" are selected.
It is for display only and it has no effect on the final image, unless the display option "keep the current scale" is selected, in which case "what you see is what you get"
0.128 usually works fine, but for bright or dark images it can be adjusted.

The maximum number of scales depend on the image size, but the module allows to select up to 15. If the user select a number of scale grater than the maximum, the program will process only the maximum number of scales.
The algorithm works like this: it process the shapes on the original image ("current scale" = 0), it decomposes the image by "number of scales" (or the maximum number of scales for this image) and process the shapes for each scale.
In the case where "number of scales" > maximum number of scales and there are shapes added to the residual image ("current scale" = "number of scales" + 1) it will process the new residual image (maximum number of scales + 1) using the shapes of the original residual image ("current scale" = "number of scales" + 1).
I've done this because dt works very often with a scaled image, so the input for the algorithm may be very small (on thumbnails or when exporting to 100x100 pix and not in HQ). In this case the maximum number of scales may be smaller than the one on the preview, and with this method I get a closer look to the final image.
It's not a very common scenario, but it can happen.

"shapes" section

-"shape selected" is a label that informs the selected shape. To select a shape click on it. A shape needs to be selected in order to change it's properties: the blur radius (in case of blur algorithm) or the color (in case of fill algorithm)

Next are the controls to change the shapes properties, depending on the algorithm selected.

If a shape is selected, an opacity slider is displayed, allowing to change the shape's opacity.

-"masks display" allows to switch off the shapes (so, not process it) and display the masks (this only for the current scale). In this last case I couldn't make it work when parametric masks are active, I'll keep working on it.

When adding a gaussian blur or a fill, the initial properties values are the ones displayed on the controls in the "shapes" section.
When selecting a shape, the value of the properties controls is updated.
The opacity slider is not used as initial value for the shapes, dt "remembers" the last value used and I think is best to keep that functionality.

The most common scenario is to do a clone/heal on the original image, in order to remove sensor dust or to do a basic retouch.
In this case, with the default values the user selects the algorithm (usually clone or heal), a shape type (usually a circle) and clone/heal by clicking on the image.

More advanced editing will require the wavelet decompose, this allows to edit the image on different detail scales (frequencies?)
In this case the user selects a number of scales, a current scale, and do the healing/cloning. It may need to edit on several scales, depending on the image, editing, etc.
To help decide on which scale to edit, it may select the "show current scale" option to see the effect of the editing on the current scale instead of the final effect.
If the displayed scale is too dark or too bright it can be adjusted with the "blend factor"

On very noisy images the heal sometimes give some strange results. In this case healing on the residual image with 3 scales works fine for me. Select "number of scales" = 3 and "current scale" = 4 and do the healing in there.

On large areas, the healing can be slow and results not so good. In this case applying one heal over the other or using two instances of the retouch module give me good results, but again, it can be slow.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Oct 31, 2017

Thanks, I'm starting to understand how it is working. Well I must say that the work is a bit tedious but the result is nice. I have been able to work on a portrait with good result indeed.

@houz

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

Just a quick comment from my side: I didn't look at the code yet, didn't try it and didn't even read all the UI discussion in here, but if the results are nice, the computation is quick and the UI can be made really simple I am all for getting this into the existing spot removal tool. I understand that it will take some time before we are getting there. So for the time being it might make sense to break some of the small improvements (allowing to pan&zoom while editing masks for example) out into a separate PR as it sounds useful independent from this work.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2017

As suggested by @houz I have created a new branch (retouch_gui) with the optional GUI changes, details are on the new PR.

I kept here:

-brush as a clone mask

-allow to select a shape inside an iop
this description is not accurate, the modifications allow the user to select a shape with left-click and informs the parent IOP about those changes

-new mask type DT_MASKS_NON_CLONE
I don't like this very much, but it has the advantage that there's no need to modify the spots IOP and worry about backward compatibility.
A better option (for me) is a DT_MASKS_DONT_ADD_TO_MASK_MANAGER (with a shorter name) new mask type. Then a shape can be created with DT_MASKS_CLONE|DT_MASKS_DONT_ADD_TO_MASK_MANAGER or just DT_MASKS_DONT_ADD_TO_MASK_MANAGER.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2017

I made a couple of changes:

  • a shape is now selected when is added, as suggested by @theres
    I also added the "ready for drag" feature from the circle to the ellipse. The path and brush are just highlighted.

  • fixed an invalid memory access when dt is shutting down and there's a shape selected, dev->gui_module had an invalid memory address.

I think that the modifications on step and size on the brush belongs here instead of the other branch, but I'll wait for your feedback on that.

@theres

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2017

@edgardoh
Two small bugs reports.
Scenario 1:

  1. select any shape creator (eg. circle)
  2. select clone or healing algorithm
  3. add shape -> two shape outlines visible, source & destination -> ok
  4. select tool once again
  5. select blur or fill tool
  6. add shape -> two outlines for new shape -> nok.
    if you start with blur/fill and then heal/clone it will result the opposite behavior.

Scenario 2:

  1. select any shape & algorithm, add shape
  2. cut shape
  3. change current scale
  4. paste shape -> it is moved to new scale, but outline is not visible, you need to do something eg, click on current scale slider or show shapes to see shapes again.

Two additional questions:

  1. did you consider/tested other filters - especially bilateral or guided filter? It potentially can give quite good results.
  2. did you consider to extend/equalize histogram of scale preview a bit? For less pronounced features the preview is just gray and it's quite hard see anything.
@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2017

@theres ,
both issues should be fixed now, thanks for reporting. Let me know if it doesn't work as expected.

About your questions:

  1. I don't really use the blur, I just added because I know it is common to use it to soften the skin and I thought that the gaussian is the right tool for that, but if other blur types are better or can be used in another way it shouldn't be a problem to add it.

  2. I agree that depending on the image, and specially on low scales, is difficult (or impossible) to see the details, but I couldn't find a one-slider algorithm that give good results and I didn't want to add extra controls just for the preview, there's already the blend factor, and it usually gives good results on scales >= 3, and those are the most used scales. But if you have a specific algorithm in mind it should be easy to try it in GIMP and see if we like the results.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2017

@theres ,
I've been playing a bit with the preview and maybe I was too picky (is that the word?) before, so I added a contrast slider. It doesn't do any magic, but it improves the preview. I get better results with curves or levels, but not by much, and it will add too much complexity.
So now there's two sliders that control the scale preview: contrast & brightness. It only work on CPU for now, so to test it you have to disable OpenCL. If this is good enough I'll add GPU.

Just a warning, I have modified the parameters but not the version, so you will loose any previous edits that you may have. I don't know the protocol on this cases, but I guess this won't be the last time that I need to modify the parameters. Should I update the version each time and when this goes to master revert it to version 1?

@houz

This comment has been minimized.

Copy link
Member

commented Dec 8, 2017

While developing I guess it's ok to break backwards compatibility, so just keep the version at 1. Once something is merged that may no longer happen of course.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2017

I have fixed some issues and as was there I also added the GPU side of the contrast and the bilateral filter. This last one break backwards compatibility again.

There's still a couple of issues that I couldn’t fix:

-previously I used the blend module for the display mask (the option at the bottom), but that didn't feel right, so I codded my own based on the gamma iop, so it has the same look. But this has its own issues, now the retouch iop return an image with the displayed mask, so any module that goes after this one will affect the mask. Since the retouch is very early on the pipe this leaves most of the modules, and that is not how the display mask is supposed to work.

-something similar with the preview current scale option, any changes by an iop after the retouch will be applied over the detail scale. I also get some strange results if the blend module is used on the retouch iop.

I don't think that there's something that I can do from inside the iop, and I don't want to keep modifying the base code, at least without a previous discussion, so this is just to let you know about this issues and see what you think about it.

@theres

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2017

@edgardoh thanks! I've tested out the changes, everything works great.

About the contrast slider - I've some idea with the histogram equalization, not sure if it will work. I'll try patch the code and see what's happen as soon as I find a couple of minutes.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

I have played with the module on two images for real. No crash, working great so far. That's a really nice module!

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

I just added a new feature that allows to process several scales at once. There's a new slider "merge from scale" after "current scale".
If > 0, it will merge all the detail scales from this one up to current scale, including the edits, so it will be like performing the same edit on all those scales.
It can be a bit confusing at first, but with some practice I get better results with less work.

This broke backwards compatibility again, sorry about that.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

Thanks for this new feature. Do not worry about backward compatibility, this is not merged yet. I'll test this new feature.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Dec 20, 2017

@edgardoh, I'm wondering now that there is the possibility to apply shapes on multiple scales at once if a visual feedback of the scales being worked on wouldn't be nice.

I'm thinking at a widget like in the zones iop, that is, a set of boxes all the same color, each box represent a scale, and the current scales being colored (yellow?). If multiples scales are selected, multiples boxes are being colored. This would help to see the selected scales.

And maybe even better, use two cursor under the set of boxes to select the current scales. And then just remove the two current sliders. What do you think?

@TurboGit

This comment has been minimized.

Copy link
Member

commented May 17, 2018

IRC doesn't really work for me, I know is not the same but here or mail is better for me.

I understand, IRC is not my preferred channel either as I prefer asynchronous work. At least if you can e-mail Tobias or ping him or IRC about this PR it would be nice to get this going.

@hanatos

This comment has been minimized.

Copy link
Member

commented May 17, 2018

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2018

@hanatos , I work on a copy of the input buffer, ivoid is never modified.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2018

Hi @houz , following is a list of known issues for this PR, I'm not sure what else is needed for this to be merged, so just let me know.

-I think that the most urgent is the above discussion: should this be a new iop or an upgrade for the spot removal?

-the auto levels button is saving the parameters on the event DT_SIGNAL_DEVELOP_UI_PIPE_FINISHED and I'm not sure that's correct. @TurboGit and I have been using it for a while now and it seems to work fine, but better be safe than sorry.

This two are low priority, is a display issue on not-very-common scenarios:

-the display masks option is not using the standard functionality, I wrote my own. It works fine, but since the mask is returned in ovoid it is affected by the next modules in the pipe. I would like to use the standard one, but the blend module gets in the way.

-something similar with the display scale, when the option is active and the blend is also active it affects the output, and that is not what I want, the display scale should display the wavelet scale without being affected by the blend module.

Not an issue but a functionality thing, the keep current scale option allows to set the current wavelet scale as the output. I never use this, I just added to try some effects with different blend modes but I didn't really like any of them. This probably has nothing to do in this module, as it is not used for any retouch operation and the wavelet scales are not 100% scale independent, so result will vary depending on the export size.
All this to say that I don't know if keep it or remove it. @TurboGit has been using this module for a while, so maybe he can help with this.

@TurboGit

This comment has been minimized.

Copy link
Member

commented May 24, 2018

the wavelet scales are not 100% scale independent, so result will vary depending on the export size.

Expect if the option "do high quality resampling during export" is selected, right?

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2018

edgardoh added some commits Oct 29, 2017

@edgardoh edgardoh force-pushed the edgardoh:retouch branch from e30fd0f to 02e68fb Jun 11, 2018

edgardoh added some commits Jun 13, 2018

@TurboGit

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

I'll do another path on this soon. If no more issues found on my side I'll merge. We are four months away from next release, this needs some more tests base.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

Just a comment that I did not forget about this. I had no time to do another full test until now. I hope to be able to test and merge before september.

In any case I think that the "heal" tool should be an option also available in the "sport removal" module. Using the full retouch module just for that is no good, the simple spot removal should support the healing mode to me.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2018

@TurboGit

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

It is quite a complex module just for removing a spot. When the only goal is to remove a spot from the sensor on the sky I feel that it would be better to be able to use the simple "spot removal" module. The Retouch module is quite advanced and will not be easy for some people. My feeling...

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2018

@TurboGit

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

Ok, I have reviewed again and did a quite intensive testing. All is working good to me, so it is time to let this go in and have a more widely field testing before the next release.

Thanks again for this very nice addition.

@TurboGit TurboGit merged commit 4e73bfe into darktable-org:master Sep 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

@TurboGit

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

Sorry I may be missing some context. What dependencies are you talking about? A PR for what?

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

@TurboGit

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

I see, you've manually coded the priority into retouch.c so fine on this side.

But we should update iop_dependencies.py by placing retouch module into the graph. Can you do that?

Then, we don't need to run iop_dependencies.py but in case we want to recompute the priority values based on a new graph this can be done anytime.

@TurboGit

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

BTW, you've put 176 as value, the same as spot :) I suppose it should come after spot? Then please update to 180 which is a free slot after spot. Thanks.

@edgardoh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

@edgardoh edgardoh deleted the edgardoh:retouch branch Oct 4, 2018

@TurboGit

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@edgardoh : can you comment on #1754

Thanks.

@LebedevRI LebedevRI added this to the 2.6 milestone Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.