GNOME Bugzilla – Bug 72862
Incorrect RGBA resampling in Despeckle plug-in
Last modified: 2007-12-11 20:23:19 UTC
If two pixels have different opacity values (alpha channel), then their colors are not averaged correctly by the Enhance->Despeckle plug-in. The preview window of this plug-in is also affected. It looks like the RGB channels are resampled without taking the opacity into account. As a result, the (invisible) color of a transparent pixel can bleed into an opaque pixel. The resulting image is incorrect. See bug #70335 for some test images and a longer description of the problem. This problem affects many other tools and plug-ins.
Changed target milestone of several bugs to 2.0 - these are not features, and don't look to me like blockers for a pre-release, but they have to be addressed before 2.0. 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.
I ve been looking at the plug-in and think it is simply not possible to despeckle a RGBA image correctly. The thing is that the despeckle algorithm takes the median and not the average value. The only possible way is to flatten the image and the despeckle. So the right thing to do would be change the parameters for the gimp_install_procedure
Created attachment 28138 [details] Attempt to resolve the incorrect RGBA resampling I've added a whole file since it changed drasticly.
Uhh, how am I supposed to review a whole file? And why doesn't the new file follow the GIMP coding style?
I didn't supply a patch file since I think it should be tested further. I was not able to download the files from the tracking bug. To do more thorrow testing. Please give me a hint concerning the GIMP coding style. If I have had the chance to test it more I'll make it follow the coding style. And I'll provide a patch to minimize your work.
Created attachment 28300 [details] [review] Patch for RGBA Rather a big change. Further all tabs ar also replaced. Did testing with images from the tracking bug and it seams OK to me.
Looks good except the C++ style comments. These aren't portable and need to be replaced when the patch is applied. I will do that later.
I have done some major cleanup but unfortunately I seem to have broken the preview mode. I am also having trouble to understand the code you added. Why is there a despeckle_new() function that is only used for the preview? Or have I misunderstood your code? I will attach my version. Perhaps you can review it.
Created attachment 28370 [details] despeckle.c after my cleanups Should even be a little bit faster but the preview doesn't look correct :(
Sven, i had made a new patch after your comments. I think you used the old one (complete file). Patch for RGBA patch 2004-06-03 12:20
I used the latest patch you attached (and yes, it still had C++ style comments). Anyway, I am not going to go through this cleanup once more. It took me almost two hours to get the code into a readable state.
Created attachment 28377 [details] [review] Correct patch for RGBA Sven, sorry for the inconvinience, it was the wrong patch. Now it should also be more clear, only one function for preview and drawing.
I will need a few days off from this code before I can attempt to merge your and my changes.
Created attachment 28548 [details] [review] geert's patch after some coding style cleanups I've applied your new patch to my local tree and did some coding style and whitespace cleanups. Was about to commit it but decided to do some testing and found that the patched version introduces some very well visible artefacts. Pure red pixels appeared all over the image. Looks like you patch breaks the plug-in :(
Didn't see that with the test images I used. Did you use another one? Used it on different of images of my own RGB and GRAY + the RGBA images from the tracking bug. If you have test case I'll look into it
I ve tried the new patch and indeed I did observe strange cyan pixels near the edge of my drawing. This happend only in the preview window. Further more the white level has to be on 256 which is kinda out of range isn't it?
Created attachment 28566 [details] [review] Strange pixels at the edge The only strange pixels I found where on the edge of the preview and drawing. So this patch ingores the edges.
woke up this moring drove to work and was thinking: The patch of yesterday evening does fix(skips) the edge conditions however where could I possebly make pixels visible. I've found one possible place. /* * Sort pixels and * Assign the median value... */ switch (valuecount) { case 0: /* No values within histogram */ - *(row_buffer+rowpos) = 255; + if (channel < 3 ) + *(row_buffer+rowpos) = 255; break; This should keep the original alpha value for the pixel. I'll test it when I'm back home.
Created attachment 28851 [details] [review] revised patch Comments / testcases/images are welcome
Created attachment 28852 [details] test image Here's a test image that shows ugly artefacts when the patched version of despeckle is run with default arguments. The artefacts already show up in the preview.
Created attachment 28854 [details] complete file cvs seems out at the moment Test image served his purpose well. After the merge of different changes the plugin was broken. Now I think the result is OK.
Created attachment 28856 [details] comparison of old and new code This is better but I am starting to wonder what exactly your patch changes and why. If you compare the results of Despeckle before and after your changes, there are quite noticable differences. The old code already introduced wrong colors at places with high contrast. The new code seems to be worse in this respect. So why are we changing the plug-in to behave worse?
The improvement is mostly noticable in RGBA images. However you ar probaly reffering to the pixels turning yellow at the shoulder? >The old code already introduced wrong >colors at places with high contrast. ahh. The high contrast areas are bound reveal other colors since you adjusting the histogram (in this case chopping off) shifts the median value. A possible solution could be adding for each chopped off value either a value of 0 or 255 to the list. Also with adaptive it is not sure that the radius is the same for each color this could yeald one pixel having calculated its median value on 9 pixels while another could have 49 if radius is 3.
I am more worried about the very noticeable introduction of magenta. IMHO this plug-in is mainly used to enhance RGB drawable w/o an alpha channel. So our main focus should be to obtain the best results for this use case. If it turns out that fixing the RGBA case causes degradation in the RGB use case, then IMHO the plug-in should be changed to work only on RGB images.
To summarize. When using the white and black values one influences the median filter. I don't think this is necessary in most circumstances. Black and white drawings do benefit from these parameters. -With highly saturated images the white level should not be chopped off becaus it will have a big influence on the image however one can safely chop off with the black level since this would not make a big difference. -Mostly white images : stay of the white level (set it to 256) If dark spots have to be removed one can play around with the black level. -Mostly black images : stay of the black level. (set it to -1) If white spots have to be removed one can play with the white level.
Created attachment 32304 [details] new despeckle algorithm's I've been playing around with some algorithm's for despeckling RGB(A) images. This is a test version that I already wanted to share.
Created attachment 32651 [details] [review] Despeckle patch Despeckle plugin: type median: - quickselect method to determinde median value - uses median of the intensity for rgb pictures. - like previous versions this does not work (correctly) for RGBA or GRAYA. type FMVMF : "Fast Modified Vector Median Filter" although the name suggests being fast this is a slow algorithm but it can be used for RGB(A) and Gray(A). (internaly limited to a maximum radius of 2 (5 by 5 kernal)) type DDF : "Distance Directional Filter" can be used for RGB(A) and GRAY(A). Not so fast also limited to radius 2. This filter combines the standard VMF (Vector median filter) BVDMF (Basic vector directional median filter) and the TFVMF (Threshold vector With threshold also median filter). - weight parameter weigths the "distance" and "direction" components of the filter. - threshold can be used to limmit the number of changed pixels
Geert, do you suggest we apply this patch for 2.2?
Maybe the vector part is to slow although ig gives good result. The median filter as before is improved but does not work for RGBA. So I think it could be implemented but I'd like to see som tests by others before.
Created attachment 33086 [details] [review] Patch with only median filter based on pixel intensity(only). Maybee a patch containing only the modified median filter (based on pixel intensity) should only be applied at this moment. Therefore I'm attaching this patch. patch 32651 contains the vector based filters that give a good result on RGBA images but are very slow this is also new functionality.
That code would need some cleanup before it can be accepted. There's at least one C++ style comment (//) and there are K&R style function prototypes (dialog_adaptive_callback). Also, would you mind to elaborate what this patch does? If I see it correctly, it adds a new algorithm to the plug-in. Since this new algorithm has the same problem as what this bug report is about, this would count as a new feature, right?
The plugin does not introduce new pixelvalues it selets the median pixel based on it's intenisty. You'll see the strange colours that poped-up with the previous versions are gone. Also the quickselect should be able to find the medium value faster. Despeckle plugin: type median: - quickselect method to determinde median value - uses median of the intensity for rgb pictures. - like previous versions this does not work for RGBA or GRAYA (to say : it only takes RGB in account) I'll try to change those type definitions and the comment asap.
Waiting for your patch...
Created attachment 33250 [details] [review] Modified according comments Modified.
Don't your idea hurt if you look at this code? Mine surely do. I really can't understand how you can submit a patch that completely ruins the code formatting in that file. The rest of the code was all cleaned up and properly indented and your patch just ignores that code and introduces a completely different and inconsistent coding style. Anyway, I am cleaning it up now...
Err, that should have read "Don't your eyes hurt ..." /me goes to get more coffee...
Created attachment 33253 [details] [review] patch after coding style cleanups
Applied this version to the HEAD branch. Moving the bug-report from the 2.2 milestone now. 2004-10-30 Sven Neumann <sven@gimp.org> * plug-ins/common/despeckle.c: applied a patch from Geert Jordaens that improves the Despeckle algorithm. See bug #72862.
I'm going to resolve this as FIXED because it ended with the application of a patch, and isn't worth the pain of reading through at this point. If there are still problems with the Despeckle plug-in, please open a new bug report to describe them.