GNOME Bugzilla – Bug 357085
Extending matrix size in Convolution Matrix plug-in to 7x7
Last modified: 2006-10-06 19:57:42 UTC
Expanding convolution matrix to 7x7 for it to be capable of Lanczos3 Now that the expand/resize menus have a working lanczos filter it would be good to extend this for use as a plugin. Lanczos on expand is great but does nothing to and already expanded image with pixelisation. The strong point of Lanczos techniques is removing pixelisation from raw digital images (preferable before all other processing). The current arrangement does not offer this.
This is by and large a request to myself . I have already a working proto for this , including considerable clean up of existing code to improve readability and maintainability. I opened this a somewhere to send patches.
Created attachment 73168 [details] [review] code clean-up patch * Replace numerical consts by named const, much variable renaming for maintainability * Generalisation of code w.r.t. matrix dimension with aim to support 7x7 * Some minor bug fixes. This should be identical in function to current code with the exception of a couple of small bug fixes. It's main purpose is to make the code more readable by more expressive variable names and more maintainable by identifying constants.. Proto-type Lanczos3 matrix can be built by removing #undef LANCZOS3
gg, it would help a lot of you could try to explain the purpose of this bug-report. Please do it without going into technical details too much. I am confused about what this is all about. Do you plan to add a plug-in or are you talking about improvements to the code in the core?
Sorry if the original post lacked clarity. Maybe I'm mixing two objectives but I did not think it merited two setparate bugs , you may want to advise o/w. 1/ General code tidying and gerneralisation. It took me about two days decrypt all this because it was full of anomimous numerical constants. This meant I had to "decompile" all the matrix code to see what every line did before I could touch it. Once it was more explicit it was about half an hour's work to generalise the matrix code to 7x7. I hope that the patch I submitted adresses that so that any future maintainance on this code become a lot less time consuming (and hence more likely to happen). That was a necessary precursor to what I actually got in here to do: 2/ Generalise the matrix code to handle 7x7 (indeed now it's done NxN would be trivial in the future) that would give it a window large enough to do Lanczos3 filtering. Bottom line is to have a plugin that can take image with pixelisation on say a 4px by 4px scale and filter it with the same effectiveness as the build-in expand function now does. I hope that makes the objectives clearer.
I have compiled and tested the patched code without the 7x7 extentions (ie as submitted). There should be no difference to the existing code function in this state apart from trivial bug fixes. I suggest, subject to review, that this could be committed to CVS as code clean up readability. One of the items on the TODO list from 1997 requests such a code cleanup.
No need to justify yourself. Code cleanup is very good and important. In fact, most of the maintainance work in the last couple of years was code cleanup and refactoring. Thanks for your cleanup patches :)
Sure, thanks for the cleanup. But I am still confused about the plug-in. Your patches all address the Lanczos code in the core. What plug-in are we talking about?
Index: gimp/plug-ins/common/convmatrix.c =================================================================== RCS file: /cvs/gnome/gimp/plug-ins/common/convmatrix.c,v
If I understand this correctly this patch is to extend the size of the convolution matrix to implement the lanczos3 filter. The lanczos3 filter then acts as a low-pass filter removing the pixelisation. Will expanding the matrix size not impact all matrix operations? Would'nt it be better to have a seperate plugin, maybe allowing lanczos3 .. lanczosn?
From the tests I have done , such a crude matrix seems of little user for sinc filtering I am taking a different approach as a separate plug-in. This patch , as it was submitted changes nothing and will not affect existing code or other plug-ins that may call it non-interactively. The main function as it stands is a major code clean up making a lot easier follow and maintain. The code itself has been fully generalised so that simply removing an #undef and recompiling will allow to offer 7x7. Altering the dimensions of default array will allow experimenting with NxN matrix. (I've used it to work on a 11x11) If I or anyone else finds a larger matrix useful it would seem fairly simple to devise a way to get around the need to hard code the default matrix at a fixed size. Once this patch is committed the code will correctly convolve any size of matrix by altering one constant in a #define. It was a major headache to correctly identify all the numerical constants and code this in a generalised form. If you would like to test it , it may advance the cvs commit. There is a glitch in the last rows of matrix, correct with this: if ((row % 10 == 0) && !preview) gimp_progress_update ((double) (row - src_y1) / src_h); } /* put the remaining rows in the buffer in place */ for (i = 1; i < DEST_ROWS; i++) gimp_pixel_rgn_set_row (&destPR, dest_row[i], src_x1, src_y2 + i - 1 - HALF_WINDOW, src_w); I have a couple of other tidy ups to post as an update but I need to trim out my hacking and make a clean patch. Hopefully tonight.
Created attachment 73600 [details] [review] update to previous clean up patch Clean up code , update. Fixes glitch noted above. Patch as submitted is retains the std 5x5 matrix code suitable for commital. To verify the generalisation work, rebuild with BIG_MATRIX and have fun filling in an 11 x 11 matrix.
Gg, you're doing so much work that we busy people are having trouble keeping up with you :-). My understanding is that the patch in comment #11 should clean up the code in the convmatrix plug-in without changing its functionality, unless BIG_MATRIX is defined. Is that right? Also, note that // comments are forbidden in GIMP code, because some compilers that we support can't handle them. There is no need to update the patch just for this, though.
Yes that's right. It should slot straight in. I got rather sidetacked by the UI issues because I was finding the interface frustrating while testing the lanczos stuff. Lots of wasted clicking and dragging. I'll just hack those issues locally. That should mean a bit less interaction and let you get on. I'll now concentrate my effort on my original aim when I came in here : to produce a sinc based depixelisation filter. This one should be good to commit now if you clean up the // comment. The fact that it works both as 5x5 and 11x11 must mean I correctly identified all the numeric constants the rest is a rename clean up to make the code more maintainable.
Created attachment 73820 [details] [review] inc. final changes reqd. by mitch
I am attaching a version of your patch with a number of coding style fixes, plus a fix of an important "unitialized variable" warning. However when I built and tested it, it appears to be broken in several ways -- it does not seem to handle alpha correctly, it comes up with all of the color channels turned off, and it doesn't seem to do the right thing for various controls.
Created attachment 73973 [details] [review] updated patch, with mainly coding style fixes
Are those comments suggesting further improvements or differences to the original plug-in? Also what's "various controls" you dont think are right? thx.
They are differences from the original plug-in. Anytime you do clean-ups to a piece of code, you need to test it afterward to make sure you haven't broken it, even if you can't see any way you could have changed the functionality. Even experienced and wizardly programmers like mitch commonly cause accidental breakage when they clean things up :-). The "various controls" are Normalize and Alpha-weighting.
I have just recompiled with the original cvs version of this file and it comes up with all channels off. I have not aimed to change this "feature" in my patch. As you say, we are all open to error, but I am pretty sure I did not touch this in any way. Equally normalise seems correct in the original and I see no difference in my patch , could you be specific about the anomally you are seeing.
Okay, I see that the problems were there even before you started working with the code. Since the patch is definitely an improvement, I have committed it to HEAD: 2006-10-06 Bill Skaggs <weskaggs@primate.ucdavis.edu> * plug-ins/common/convmatrix.c: commit patch from GG that cleans up code and adds (commented out) support for larger matrices, with some coding style fixes; see bug #357085. This plug-in still needs help, though. Resolving as FIXED. We should open new bug reports for problems that did not result directly from these changes.
Your humble appologies for your libelous attack on my competance are gratefully recieved. Now the ten year wait for a clean up of this plugin is over maybe it will be a simpler task for someone else to correct some of the sloppy errors you so justly hightlight above. ;) One small step for mankind , as someone once said.