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

blend mask feathering by a guided filter #1809

Merged
merged 1 commit into from Nov 9, 2018

Conversation

Projects
None yet
5 participants
@rabauke
Contributor

rabauke commented Nov 7, 2018

Feathering adjusts the blend mask by taking into account the image content
such that edges of the mask match those in the image.

Here a short demo. First make a rough selection via drawn or parametric masks:

Then select "feathering" as "mask enhancement" and adjust radius, brightness and contrast such that edges of the mask match those in the image.

@rabauke rabauke force-pushed the rabauke:guided_filter branch 2 times, most recently from f032189 to 486011a Nov 7, 2018

@TurboGit TurboGit self-requested a review Nov 8, 2018

@TurboGit TurboGit self-assigned this Nov 8, 2018

@TurboGit TurboGit added this to the 2.6 milestone Nov 8, 2018

@TurboGit

This comment has been minimized.

Member

TurboGit commented Nov 8, 2018

Can you give a bit more information about the way to use it? It will help testing... Thanks.

@aurelienpierre

This comment has been minimized.

Contributor

aurelienpierre commented Nov 8, 2018

@TurboGit It comes as an addition to gaussian blur within the blending parameters, and works when you have semi-transparent masks (parametric and/or drawn). Using ligthness, contrast, and blur radius, it allows you to refine masks edges. It's quite awesome actually.

@TurboGit

some comments for a first review.

also, I tried it and the feathered mode seems broken to me. With a radius of 0 and 0.1 nothing happens and with every value > 0.1 I have a full yellow mask on the picture. Any idea? Maybe I'm just using it wrongly?

__kernel void
blendop_mask_enhance_contrast(__read_only image2d_t mask_in, __write_only image2d_t mask_out,
const int width, const int height,
const float e, const float brightness) {

This comment has been minimized.

@TurboGit

TurboGit Nov 8, 2018

Member

{ should be at the start of a new line

G_CALLBACK(_blendop_blendif_brightness_callback), bd);
bd->brightness_slider = dt_bauhaus_slider_new_with_range(module, -1.0, 1.0, 0.01, 0.0, 2);

This comment has been minimized.

@TurboGit

TurboGit Nov 8, 2018

Member

we have two section identical, the code above is the same as this one.

bd->masks_blur_modes_combo = dt_bauhaus_combobox_new(module);
dt_bauhaus_widget_set_label(bd->masks_blur_modes_combo, _("mask enhancement"), _("mask enhancement"));
dt_bauhaus_combobox_add(bd->masks_blur_modes_combo, _("Gaussian blur"));

This comment has been minimized.

@TurboGit

TurboGit Nov 8, 2018

Member

No capital letters in UI label (G -> g).

@TurboGit

This comment has been minimized.

Member

TurboGit commented Nov 8, 2018

Also as we are very close from the feature freeze, can you describe more the actual algorithm behind this? To be clear my fear is that is not full ready and if we want to improve later we'll break history or we would have to support multiple algorithm on the blend layer. Not something we have at the moment. I would have like to see that earlier to have more field testing. Now if the algorithm are safe or state of the art I think it can go. All in all I'm seeking more background information about this. Thanks.

@rabauke

This comment has been minimized.

Contributor

rabauke commented Nov 8, 2018

The algorithm is described in detail in the references given in the sources, see also http://kaiminghe.com/eccv10/index.html
The suggested mask filtering is based on a so-called guided filter, which is also employed in the haze removal module. More testing and feedback would be highly appreciated.

@Kabouik

This comment has been minimized.

Kabouik commented Nov 8, 2018

I have been waiting for this feature since I saw this message from @rabauke on the list a long time ago. Eagerly looking forward to trying this when it is merged, and quite happy to see that the project has not been abandoned. Thanks for your work, hope merging won't require too much additional sweat.

@TurboGit

This comment has been minimized.

Member

TurboGit commented Nov 8, 2018

@rabauke : and what about the feathered mode which seems not working with parametric masks? Can you describe the way to use it?

@TurboGit

This comment has been minimized.

Member

TurboGit commented Nov 8, 2018

@Kabouik : if you can test it it would be great.

@TurboGit

This comment has been minimized.

Member

TurboGit commented Nov 8, 2018

@rabauke : I can't make this working... sorry I really need more information about the way it is supposed to be used. Do you have an example?

@aurelienpierre

This comment has been minimized.

Contributor

aurelienpierre commented Nov 8, 2018

@TurboGit

the feathered mode only works with semi-transparent regions (opacity < 100 %).

  • the mask blur controls the spread of the edge detection
  • the mask brightness controls how much you retain or exclude the outside of the mask
  • the mask contrast controls the sharpness/feathering of the edges of the mask.

@rabauke I'm not sure the sliders labels are super clear there. I would to go for:

  • mask edge detection radius
  • mask edge offset
  • mask edge contrast

I tested it today, works perfectly. Thanks !

@rabauke rabauke force-pushed the rabauke:guided_filter branch from 486011a to e953a64 Nov 8, 2018

@rabauke

This comment has been minimized.

Contributor

rabauke commented Nov 8, 2018

@rabauke : and what about the feathered mode which seems not working with parametric masks? Can you describe the way to use it?

There was an issue in the OpenCL code path. Now it works also with parametric masks (and OpenCL).

@aurelienpierre aurelienpierre referenced this pull request Nov 9, 2018

Merged

New module : Filmic #1811

@TurboGit

This comment has been minimized.

Member

TurboGit commented Nov 9, 2018

Ok, thanks for the example and the code fix! I'll have a look again.

@rabauke

This comment has been minimized.

Contributor

rabauke commented Nov 9, 2018

@rabauke I'm not sure the sliders labels are super clear there. I would to go for:

* mask edge detection radius

* mask edge offset

* mask edge contrast

I agree, the meaning of the sliders may not be obvious to the user. Basically a tone curve is applied to the mask, which is now explained in the revised tool tips. Note that this mask tone curve is completely unrelated to the edge-sensitive feathering algorithm. Thus I would not mention the word edge here.

@TurboGit One might consider to add further parameters, e.g. white and black points, to adjust the tone curve, giving even more flexibility but also cluttering the UI.

@TurboGit

This comment has been minimized.

Member

TurboGit commented Nov 9, 2018

@rabauke : please can you remove rawspeed update from the following commit: e953a6

@TurboGit

This comment has been minimized.

Member

TurboGit commented Nov 9, 2018

@rabauke : not sure you've seen but I have request some changes.

@rabauke

This comment has been minimized.

Contributor

rabauke commented Nov 9, 2018

@TurboGit Issues have already been solved, the rawspeed issue will be fixed soon.

@TurboGit

This comment has been minimized.

Member

TurboGit commented Nov 9, 2018

Another question, with the default parameters the blend is 100% identical to previous version. Right?

@rabauke rabauke force-pushed the rabauke:guided_filter branch from 911c6c0 to 7ac3325 Nov 9, 2018

blend mask feathering by a guided filter
Feathering adjusts the blend mask by taking into account the image content
such that edges of the mask match those in the image.

@rabauke rabauke force-pushed the rabauke:guided_filter branch from 7ac3325 to 20b0b24 Nov 9, 2018

@rabauke

This comment has been minimized.

Contributor

rabauke commented Nov 9, 2018

@TurboGit : There have not been any recent parameter changes. Do you have any specific in mind. Furthermore, I removed the rawspeed sources from the pull request. It was not intended to include any rawspeed stuff.

@TurboGit

This comment has been minimized.

Member

TurboGit commented Nov 9, 2018

@rabauke : my question was more to ensure that the current implementation does not break history and that the default behavior without the two new sliders is 100% equal to the current implementation. That's what I think from reading the code, but just wanted to be sure.

@rabauke

This comment has been minimized.

Contributor

rabauke commented Nov 9, 2018

@rabauke : my question was more to ensure that the current implementation does not break history and that the default behavior without the two new sliders is 100% equal to the current implementation.

It was always my aim not to break history when implementing this new feature. It's crucial indeed.

@TurboGit

This comment has been minimized.

Member

TurboGit commented Nov 9, 2018

Ok, thanks! Let's go with it and have more field testing. Thanks!

@TurboGit TurboGit merged commit c94173f into darktable-org:master Nov 9, 2018

1 check passed

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

This comment has been minimized.

Contributor

moy commented Dec 1, 2018

This seems to work only for modules between the input and output color profile modules, see https://redmine.darktable.org/issues/12429

Cc: @rabauke, in case you're not subscribed to the bugtracker.

@rabauke

This comment has been minimized.

Contributor

rabauke commented Dec 1, 2018

@moy Thanks for the bug report. Can reproduce the behavior. Will fix it as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment