GNOME Bugzilla – Bug 105568
Incorrect alpha treatment in plug-in selection masking
Last modified: 2008-08-13 19:04:10 UTC
Selection masking is transparent to plug-ins. They don't need to do anything to correctly merge the plug-in output with the image when a selection is present, just draw as if no selection was present, and the merge is done by Gimp itself. However, when output of a plug-in is merged to a fuzzy selection, the (invisible) color of a transparent pixel in the image can bleed into an opaque pixel in the plugin output (and probably the other way round too, but this is harder to check). The resulting image is incorrect. Demonstration: 1. Take test2.xcf.gz from bug #70335. 2. Use "Select elliptical regions" tool and select a medium-sized ellipse near the center of the image. 3. Run Select -> Feather and feather it by 50pixels. 4. Run any plug-in. Beacuse many of them treat alpha incorrectly themselves, and others make the problem hardly observable, run Filters -> Render -> Clouds -> Plasma (it doesn't do pixel averaging at all, so it does handle alpha correctly). 5. Observe the green/red color mysteriously appearing in the image. Current CVS behaves the same. Once confirmed, this bug should be added to bug #70335 as a dependency.
Changing milestone on a bunch of bugs to 2.0 - none of these could be considered a blocker for a pre-release, IMHO. Many of these have patches or someone working on them, but they're not urgent. Dave.
Bumping the milestone for this bug, and all that depend on it, to 2.2. In the absence of someone actively working on this, there is no point keeping it on the 2.0 milestone. If someone wants to work on this, fixes to this family would also be accepted on the stable branch. Dave.
So we got the preview (GimpDrawablePreview) right but the actual masking is still wrong? Perhaps we shouldn't slow down our previews and just let them show the wrong result that it yold when applying the effect. It would be nice to have a dedicated test case for this problem attached to this report.
This is another one of those fun merging-with-alpha bugs that we got rid of a ton of. At any rate, since we know how to do it right (in the preview), it should be trival to do it right in the real mixing code. It is cool that the previews show how the merged effect will look now. The old preview widgets did not.
I think we should definitely try to fix at least this problem for 2.2. Raising priority.
am I correct to say that the same modifications made in plugin gauss.c should also be made in gimp/app/paint-funcs/paint-funcs.c function : gaussian_blur_region. More specificly the pre multiplication of the alpha channel.
Created attachment 31650 [details] [review] handle alpha in selection
Created attachment 31651 [details] [review] handle alpha in selection
Created attachment 31652 [details] [review] handle alpha in selection
I fail to see how gaussian_blur_region() is related to this bug-report.
Geert, why do you split your patch across multiple files? That makes it very hard to review it as well as to apply it.
well to be honnest, i haven't figured out what's the best way to setup my development system. I realy should take some time to figure out how cvs works
In order to create a single diff of all your changes in the app directory go to the toplevel directory of your gimp CVS checkout and type cvs diff app > foo.diff You can also specify a list of files if you want to pick a particular change out of multiple changes in your tree. Please make sure you have the line 'diff -up' in your ~/.cvsrc. But please explain why your patch deals with gaussian_blur_region().
>>I fail to see how gaussian_blur_region() - 3. Run Select -> Feather and feather it by 50pixels. - Since gimp_channel_real_feather calls gaussian_blur_region()
But feathering the selection is just a way to show the problem. It is not part of the problem at all. Also a channel has no alpha channel so your change should be without any effect.
Looking again at this problem, I noticed that the plugin does average the pixel's. The code used for the averaging does not take in account the alpha channel IMHO. If sombody could confirm my assupmtion I would be happy to spend some time on it. static void average_pixel (guchar *dest, const guchar *src1, const guchar *src2, gint bpp) { for (; bpp; bpp--) { *dest++ = (*src1++ + *src2++) / 2; } }
Does it makes sense to keep this one on the 2.2 milestone? Is someone actively working on this one?
Just a note: I believe the code that would have to be changed is in the function combine_inten_a_and_inten_a_pixels(), in app/paint-funcs.c, and also corresponding code in app/composite. Those are some scary places to be making changes at this point in the 2.2 cycle.
I agree, the code should probably not be touched before we branched for the next development cycle. In the meantime, you could have a look at libgimpwidgets/gimppreviewarea.c. It seems that the preview of the selection masking is done incorrectly for the alpha channel. Please create a selection and preview the effect of the "Color to Alpha" plug-in.
Comment on attachment 31650 [details] [review] handle alpha in selection Setting needs-work on patch, as indicated in comments.
Comment on attachment 31651 [details] [review] handle alpha in selection Setting needs-work on patch, as indicated in comments.
Comment on attachment 31652 [details] [review] handle alpha in selection Setting needs-work on patch, as indicated in comments.
The "Color to Alpha" thing does not come from any incorrectness in gimppreviewarea.c -- it happens because colortoalpha draws its preview using gimp_preview_area_draw() rather than gimp_drawable_preview_draw_area(). Thus, it never looks at the selection mask. This may be a mistake, but I think probably not -- the thing about colortoalpha is that it takes an RGB* input but always produces an RGBA output, and the preview masking code is not capable of combining layers of different types. The only reasonable fix, as far as I can see, is to add the alpha channel to the layer before any previewing. I don't see anything obviously wrong with this, so long as care is taken to remove the added alpha channel if the plug-in is cancelled -- but it does seem a bit tricksy.
I don't believe any of the changes discussed here are appropriate for the stable branch, so I am bumping to 2.4.
Not a blocker for 2.4, so I am bumping the target.
This is still broken in 2.4. Another way to show that the code is still buggy is to load test2.xcf and, without doing any selection, apply the Plasma plug-in. Then use Fade to reduce the opacity with which the effect is applied. You will see the hidden Tux appearing. Probably the best we can do here is to start to replace all that crappy code in app/paint-funcs with calls into GEGL. We would only use 8bit per color channel for now and we should make sure that the 8bit code paths in GEGL are reasonably well optimized. Later, when all this legacy code is gone, we can start to use higher bit depths in GIMP.
(In reply to comment #26) Sounds theoretically possible to do for 2.8, setting milestone appropriately.
Created attachment 115749 [details] [review] Correct the blending formula in replace mode This patch change the previous formula used in the replace mode used for plug-in selection masking and fade. In the following: - (a1,c1) to describe the alpha channel, and a particular component of the color of the original content - (a2,c2) for the new content that should replace the old one - (ar,cr) for the result - m the product of the opacity and the mask - every thing between 0 and 1 (instead of 0 - 255) Previous formula: ar = m.a2 + (1-m).a1 cr = m.c2 + (1-m).c1 Patch formula: ar = m.a2 + (1-m).a1 cr = (m.a2.c2 + (1-m).a1.c1) / ar The patch is not well optimized (conditions in the loop should be extracted outside the loop, etc...) and may be made more readable, but It tries to use maximum already existing building block. The version is just made to give the idea the more directly possible while using those building block.
Makes sense for me. I don't know if this patch is the best option, but it appears to work - and we would have the correct behavior for 2.6 series before moving all this code to use Gegl.
Created attachment 115825 [details] [review] Correct the blending formula in replace mode Same patch but with a better code style (I hope)
Looks good to me. But it would probably be worth the effort to apply some optimization on this code.
Created attachment 116388 [details] [review] Correct the blending formula in replace mode (integer only optimized version) This version of the patch uses only integers instead of floating numbers. This leads to (a bit more than) twice faster execution of the blending on my computer compared to the previous version. Tested on a 3200x2400 white image and plasma plugin + fade: - float version: 2.3 sec - this version: 0.95 sec Computer Spec: - Thinkpad X31 - Pentium M 1.4Ghz - 768Mo - Ubuntu Hardy The computation has been deaply tested and produce almost surely the good result without any quality loss.
Thanks a lot! Almost perfect, except that your code did not catch the case where new_alpha becomes zero. I've now committed a version to trunk that does handle this case:
Created attachment 116464 [details] [review] Correct a forgotten member of the formula I've found that both patches forgot one member of a multiplication. A little patch is required on the current SVN to add this member. The error appears when the replacement content has an alpha value less then 255, since the forgotten member is presicely src2_alpha.
2008-08-13 Sven Neumann <sven@gimp.org> * app/paint-funcs/paint-funcs.c (replace_inten_pixels): applied patch from Luidnel Maignan (followup to bug #105568).