GNOME Bugzilla – Bug 167956
scaling image causes an offset, particularly with Lanczos
Last modified: 2007-04-25 08:51:24 UTC
1. Create a 64*64 pixel image. 2. Render plasma on it. 3. Duplicate the layer. 4. Rotate duplicate 180 degrees. 5. Scale the image 3x using linear, cubic or lanczos. 6. Rotate the duplicate 180 degrees again. 7. Play with the view toggle and see how they are slightly different, cubic and linear seem to be offseted in one way, lanzcos in other. Nearest is the only that returns the same result.
That is mentioned in bug #162250 already and it is the reason why this report is still open. Does it make sense to have a separate bug report for it? We should probably close bug #162250 then.
IMO it does make sense to have a seperate bug report about the transformations and their possible errors. But I don't think that this bug report here is useful in it's current state... it involves two tranforms, not one as the title might suggest. We will have to come up with a better test suite, maybe a script.
There is only one issue, scaling, or maybe the way interpolation is performed. The rotation does not use interpolation, it is the one that moves pixel as a whole, so top left pixel becomes bottom right, and so on. The test is from http://www.interpolatethis.com/phpBB2/viewtopic.php?t=132 Comment #54 of bug #162250 indeed mentions the transform with lanczos offsets the image data, this one shows it can happen with others too, just different amounts.
We can at least confirm this bug then.
I have seven layers in my image, all left justified to the same x coordinate. I scale all of them to the same width and the result is that some of them are at the left x as before, but the rest are varied between 1 and 7 pixels to the right. I tried both local and image center for the third argument.
Josh, your problem is probably unrelated to this bug report, and there have been recent changes to cvs HEAD that may fix it. Concerning the scaling issues, my investigations show that: (1) nearest neighbor gets it right. (2) linear and cubic are constently off by one pixel, regardless of the amount of scaling, so they probably just have a rounding error somewhere. (3) Lanczos is off by an amount proportional to the amount of scaling, with the center displaced in oppoite directions for X and Y. Therefore, it contains an error in the calculations somewhere. I will continue invetigating.
Clarifying summary, and changing target and priority.
I have spent some hours looking at the Lanczos code, and have found that the offset is approximately, but not exactly, -0.5 pixels for x, and +0.5 pixels for y, in coordinates in the source image. However, I have not been able to figure out where in the code it is coming from -- the complexity defeats me. It seems that it will be necessary for Geert to look at this for it to get solved. Accordingly, I am adding him as a CC for this bug report. If he is unable to do anything with it, and no alternative miracle happens, then I'm afraid I agree with Sven that the Lanczos option will have to be removed.
Created attachment 71875 [details] [review] first patch for review This is what I have been able to do. The code is rewritten, it should be more readable. I still did not pinpoint the source of the offset however it is consitently off in booth directions and proportionaly to to the scaling If anyone could review this patch. My best guess at the moment is that the offset is comming from the covolution. Geert
Created attachment 71913 [details] [review] Fixes offset during lanczos scaling This patch should solve the problem. Geert
Well, the patch applies cleanly to HEAD, but the result is a complete disaster for me. When I do "scale image", I either get a core dump or a seriously incorrect result.
Following up on irc discussion of this: even with your test images, if I try a couple of times (i.e., scale, undo, scale), I start seeing artifacts on the upper and left edges of the image. Scaling by a factor of 10, that is. The offsetting does appear to have been solved, however.
I'm not able to reproduce this behaviour, if I understand it correctly You are suggesting that the scaling routine overwrites ste source data. I do not exactly know how the undo process works.
I did a "scale image" using Lanczos, scaling a 10x10 new white image to 100x100, while running with "valgrind --tool=memcheck", and got the following: ==6095== ==6095== Invalid read of size 8 ==6095== at 0x82493A9: scale_region (scale-funcs.c:995) ==6095== by 0x81CFCCE: gimp_drawable_scale (gimpdrawable.c:401) ==6095== by 0x8218483: gimp_selection_scale (gimpselection.c:208) ==6095== by 0x8202CDE: gimp_item_scale (gimpitem.c:782) ==6095== by 0x81F80D2: gimp_image_scale (gimpimage-scale.c:120) ==6095== by 0x8077EFF: image_scale_callback (image-commands.c:594) ==6095== by 0x8088C22: image_scale_callback (image-scale-dialog.c:159) ==6095== by 0x8095166: scale_dialog_response (scale-dialog.c:246) ==6095== by 0x1BECA474: g_cclosure_marshal_VOID(i_xx_t) (gmarshal.c:216) ==6095== by 0x1BEBECD1: g_closure_invoke (gclosure.c:490) ==6095== by 0x1BECD4EA: signal_emit_unlocked_R (gsignal.c:2438) ==6095== by 0x1BECE7FA: g_signal_emit_valist (gsignal.c:2197) ==6095== Address 0x1FCD79F0 is 2136 bytes inside a block of size 2148 free'd ==6095== ==6095== Invalid read of size 8 ==6095== at 0x82493B6: scale_region (scale-funcs.c:996) ==6095== by 0x81CFCCE: gimp_drawable_scale (gimpdrawable.c:401) ==6095== by 0x8218483: gimp_selection_scale (gimpselection.c:208) ==6095== by 0x8202CDE: gimp_item_scale (gimpitem.c:782) ==6095== by 0x81F80D2: gimp_image_scale (gimpimage-scale.c:120) ==6095== by 0x8077EFF: image_scale_callback (image-commands.c:594) ==6095== by 0x8088C22: image_scale_callback (image-scale-dialog.c:159) ==6095== by 0x8095166: scale_dialog_response (scale-dialog.c:246) ==6095== by 0x1BECA474: g_cclosure_marshal_VOID(i_xx_t) (gmarshal.c:216) ==6095== by 0x1BEBECD1: g_closure_invoke (gclosure.c:490) ==6095== by 0x1BECD4EA: signal_emit_unlocked_R (gsignal.c:2438) ==6095== by 0x1BECE7FA: g_signal_emit_valist (gsignal.c:2197) ==6095== Address 0x1FCD79F0 is 2136 bytes inside a block of size 2148 free'd So there seems to be some invalid memory-handling going on.
Created attachment 71974 [details] [review] Patch fixes offset and memory problem Never used valgrind before. The error is fixed.
Okay, good progress. The offset problem is either solved or at least greatly reduced, and there are no longer any artifacts. I think that some sort of normalization problem has crept in, though -- try creating an image with a single black pixel on a white background, and scaling the image using Lanczos, and you should see what I mean.
Created attachment 72060 [details] Norming fixed except scaling layers (need help) The scaling is fixed for images, for layers I still get the normalisation error. I need som help with this all suggestions are welcome. scale-funcs.h * changed LANCZOS_WIDTH added 1 to LANCZOS_WIDTH * 2 for symetric kernel. scale-funcs.c * Function scale_region - don't call scale_region_lanczos for scaling down * Function mirror_edge - removed * Functions lanczos_sum & lanczos_sum_mul - matched parameters with variables in scale_region_lanczos for better understanding. * Function scale_region_lanczos (rewritten) - added parameters for progressbar - if no change is scale source pixel region is copied as is to destination. - offset fix, added -0.5 to the inverse transformation to get the correct source coordinates. - changed pré-calculation of convolution kernel - comments added at crucial points
Okay, I've committed this patch, after cleaning up the coding style. There is still an off-by-one-pixel error, which looks the same as for linear and cubic interpolation, so the problem here is likely outside the Lanczos code. I am not myself seeing a normalization error for layers, but I may not be looking at the right thing. Note that the fixes still need to be done for the Lanczos code in gimpdrawable-transform.c. 2006-09-06 Bill Skaggs <weskaggs@primate.ucdavis.edu> * app/paint-funcs/scale-funcs.[ch]: apply patch from Geert Jordaens to improve Lanczos scaling, with coding style cleanups; partly fixes bug #167956.
ok, I'll try to fix this this weekend Geert
Created attachment 72490 [details] Patch to fix lanczos scaling in gimpdrawable-transform For review
I tried this patch in the hope that it would perhaps also fix bug #355178. I am getting weird artefacts for rotations. The rotated layer looks OK despite a color change, but the background isn't transparent but shows horizonzal stripes.
I look further into it, I did my tests with the perspective tool. However, i do not see the stripes when rotating, I did just notice some strange colors apear.
[OFF]Sven , could you retest this with the normalisation patch I posted earlier in #355178 I just tried a rotation and I dont see the distortions you describe. [/OFF] [ON] Also I tried replacing the matrix inversion with a simple division to get the coordinated , this seems not to need the -0.5 tweek: app/paint-funcs/scale-funcs.c /* dsrc_x = itrans[0] * ((gdouble) x) + itrans[1] * ((gdouble) y) + itrans[2] - 0.5; dsrc_y = itrans[3] * ((gdouble) x) + itrans[4] * ((gdouble) y) + itrans[5] - 0.5; */ /*** why use matrix , try division without offset */ dsrc_x = x/scale_x; dsrc_y = y/scale_y; maybe some guru will tell me 3 array derefs , two mults and three additions is quicker than a divide but I suspect this is just a relic of more general code in the tools section. going direct seems to avoid the offset.
also seeing some badness here. three rotatations of 30 supposedly about the centre takes the image almost out of frame. Also ugly artifacts creeping into background area. tested on current cvs
Created attachment 73330 [details] source image used for test
Created attachment 73331 [details] shot of resulting rotations
Created attachment 73333 [details] result of three 30 deg rotations.
forgot to mention this was with lanczos sampling , not seen with bicubic. First 30 deg looks to be correctly placed then it drifts off.
Patch from #20 is no longer valid. attachment (id=72490) Patch to fix lanczos scaling in gimpdrawable-transform Please use patch from #355178. Geert
so if I follow, I need to reverse #20, apply patch from other thread , then this one. I'll give it a go. could you consolidate all this into one patch against current cvs to save confusion, and obselete #20? Thx.
OK. clean cvs of this file plus #29: lanczos now looks clean but produces a small drift in x and y. six rotations of 30 left the image inverted as required but +ve offset of 4 or 5 pixels in both x and y. One thing I noticed on all rotations is that positive angle rotate anti-clockwise. Finally as a user request is it possible that the rotation dlg retains the angle I gave it last time. There's a fair chance I may want to repeat an operation . One thing I certainly wont be wanting to do is a zero degree rotation. Thz.
Created attachment 73378 [details] high contrast black and white elipse test image just evaluated the distortion of this image with three 30 deg rotations followed by thre -30. lanczos pretty much desimated it. bicubic did a great job and it ended up looking slightly better than the pixelated image in this attachment!
Created attachment 73379 [details] lanczos rotated image different types of image may fair better , but this shows that lanczos is not well suited to filtering rounding errors on images basically of the same scale as the original. It is essencially a bandpass filter and the errors introduced in a rotation are of the same dimension as the pixelisation of the image. This is probably not an appropriate use of Lanczos.
Created attachment 73381 [details] same test with bicubic filter Here the results are excellent. Apart from being clipped by the image area, after 6 rotations the image shows no amonolies and the successive interpolations have even made a clean job of removing a lot of the pixelisation. By comparison, effecting a x8 expansion with the two filters Lanczos produces cleaner results. On photos it is streets ahead. I think the use of common code in enums for these two functions with very different technical requirements needs to be reconsidered. It may be more appropriate to add a new enum for expansion and retain a lesser list for rotation and distortion transforms. (none/lin/bi) Each could maintain its own setting/default. It seems, in general use, the best defaults would be Lanczos for expand and bicubic for the rest. It would be very cumbersome for the user to be constantly swapping depending on which transform he was doing and it seems likely that he could be expanding and rotating in the same editting session. It would seem advantageous (and very simple) to separte the two.
http://www.path.unimelb.edu.au/~dersch/interpolator/interpolator.html provides some test images and methods, as well as results of other systems. Also interesting is http://imagebeat.com/mandala/thumbnail.html in which they use lanczos (and hybrid) for downsampling. Test images provided too. In general the docs I find point towards Lanczos being capable of working for any resampling (better or worse) like upsizing, downsizing or rotation. http://forum.doom9.org/showthread.php?p=188410#post188410 demos upsize and downsize with the multi ring image, for example. Of course, Lanczos name covers a group of systems (see Lanczos3 and so on), so not everyone refers to the exact same thing. So that leads me to a question related to using different defaults or even limiting the avaliable options: what happens if you do an op that fits different conditions, like going from 100*100 to 110*90 image? What others do?
thanks a lot for that info. Those ring images are pretty unforgiving and provide a semi-standard test to compare against other software. Dont know if anyone's tried yet but do 360 in 15 degree steps with current gimp filters and you get something more like a fractal. I'll drop the interface discussion till some of the wrinkles get ironed out in the transforms. very useful, thx.
Created attachment 73495 [details] [review] interpolation enums clean-up simple patch to tidy up interpolation names. More correctly named Bilinear and Bicubic. Renames Lanczos to it's generic filter type to be consistant with existing entries (we dont call bicubic "Catmull-Rom") now called: sinc (Lanczos3) finally adds #define to enable extended interpolation types patch as submitted does not alter existing enum length due to #undef line.
from #29 >> Patch from #20 is no longer valid. attachment (id=72490) [edit] Patch to fix lanczos scaling in gimpdrawable-transform Please use patch from #355178. >> Geert , could you clarify all this and obelete any patches that are to be ignored. You indicated elsewhere that you had no outstanding patches, yet I see no comment that this was committed. I am unclear about it's relation to the patch in #355178 sinc this was posted later. thx.
The latest version/patch that i contributed was to bugreport : bug #355178 – transformation tools with Lanczos interpolation brighten the result Fixes lanczos in gimpdrawable-transform.c patch 2006-09-22 19:04 UTC 12.19 KB committed As you can see it got commited, it is still the version that is present in current CVS : 1.117 Sun Sep 24 16:45:12 2006 UTC Log: Bill Skaggs <weskaggs@primate.ucdavis.edu> * app/core/gimpdrawable-transform.c: apply patch from Geert Jordaens to improve Lanczos performance; probably fixes bug #355178. The relation between the 2 patches is that since gg was working on scale-funcs.c I started working on gimpdrawable-transform.c to not interfere with work of gg. To solve the brighten problem I rewrote the transformation since it was also buggy (keeping in mind the improvements already done in scale-funcs). To summarize all patches I had are commited. I hope this clarifies the situation.
Comment on attachment 72490 [details] Patch to fix lanczos scaling in gimpdrawable-transform Obsolete
Comment on attachment 72060 [details] Norming fixed except scaling layers (need help) Obsolete
Created attachment 74601 [details] [review] finally fixes the offset bug. Amazing this trivial bug has been open so long, crux of it was , predicatably, a truncation error. This patch also brings the drawables code in line with some recent changes in scale-funcs.c , in particular the lanczos accuracy changes. There still seems to be an issue with the interpolation it is using but that probably should be a new bug. Please verify and commit this and we can at least close this one.
any chance of looking at the patch in #37 as well?
hmm, dispite it's number the patch in #42 is not the answer to life, the universe and everything. It resolves the problem for NONE and LANCZOS but there seems to be an additional error in the matrix transforms: bilinear and bicubic.
rotate tool for ex. with image of 256 . left hand pixel is 0 , rh pixel is 255 but rotate tool centers at 128 , hence the centre of rotations if off. now if s.o. can work out where that set .... geert: FIXME ;)
gg i think this should be corrected in gimpdrawable-transform I'm not able to work on it before next weekend. for (y = y1; y < y2; y++) { if (progress && !(y & 0xf)) gimp_progress_set_value (progress, (gdouble) (y - y1) / (gdouble) (y2 - y1)); /* set up inverse transform steps */ tu[0] = uinc * x1 + m.coeff[0][1] * y + m.coeff[0][2] + .5; tv[0] = vinc * x1 + m.coeff[1][1] * y + m.coeff[1][2] + .5; tw[0] = winc * x1 + m.coeff[2][1] * y + m.coeff[2][2];
hmm, I dont think that will resolve the rotate tool centering on the wrong pixel. As I commented above, I think there are several issues here. The mod you suggest would also add the fudge factor to GIMP_INTERPOLATION_NONE. If that is necessary it would seem to indicate there is a bug the matrix transform calcs, which I doubt. I would suggest gets checked out methodically otherwise we are likely to create secondary bugs that will come back on us later.
width even coordinates the the center coordinate is at (127.5, 127.5) So translating the center -.5 the rotation should be centered again. gdouble wincx = (width%2 == 0) ? m.coeff[2][0] - 0.5 : m.coeff[2][0]; gdouble wincy = (height%2 == 0) ? m.coeff[2][1] - 0.5 : m.coeff[2][1]; for (y = y1; y < y2; y++) { if (progress && !(y & 0xf)) gimp_progress_set_value (progress, (gdouble) (y - y1) / (gdouble) (y2 - y1)); /* set up inverse transform steps */ tu[0] = uinc * x1 + m.coeff[0][1] * y + m.coeff[0][2]; tv[0] = vinc * x1 + m.coeff[1][1] * y + m.coeff[1][2]; tw[0] = wincx * x1 + wincy * y + m.coeff[2][2];
If that's the case the rotate tool needs modifying to accept non integer input. There is currently no way to use the rotate dlg without inducing an offset. btw could you pls indicate the file you mods relate too, it would save a bit of time/
sorry, that is gimpdrawable-transform.c
thanks I found it in the end. this is getting a bit long and drifting off the subject in the title since we are drifting into a rotation issue. I have opened a new bug here: http://bugzilla.gnome.org/show_bug.cgi?id=363775
What remains to be done here? Can be report be closed?
The patch doesn't seem to apply cleanly anymore.
which patch?
Attachment #74601 [details]
Peter, please summarize the state of this bug report in general and the patch (see comment #55) in particular.
You will notice that the attachment you refer to applies to gimpdrawable-transform.c , this is rotate et al not scaling . This bug relates to scaling offset. This as you are aware is handled by this file. That confusion is the exact reason why I split these related but different issues into separte bugs with separte titles. I thought my comment in #51 made that clear enough. Sorry if I lost you. gg
PS. geert was basically handling this rotation code but has not been seen in either thread since it seems.
I guess we need to get you Bugzilla powers for the future so that you can handle such things yourself then. For now just assume that I have no clue whatsoever and try to give clear instructions on what to do about the attachements and this bug report. I would like to see something like "close this report as FIXED and mark the patch as obsolete" or "apply the two outstanding patches but keep this report open because xyz still needs to be handled.
mark the patch as obselete if it no longer applies. In any case , as I pointed out again in #57 it does not relate to the image scaling code. there is still a (-1,-1) drift per transform measured in destination coords.
What about the enums cleanup patch. I don't think it makes sense to add placeholder values to an enum but it might make sense to change the label displayed for "Lanczos" to "Sinc (Lanczos3)" as this patch suggests. But then the enum value should probably be renamed to GIMP_INTERPOLATION_SINC or GIMP_INTERPOLATION_LANCZOS3. We could consider doing that now before the API is frozen for 2.4.
Cool , I thought that idea had recieved a disapproving silence. Maybe it just got overlooked. GIMP_INTERPOLATION_LANCZOS3 would be the most accurate since it is a windowed sinc, not just a sinc. Since I've been taking a close look at the other "interpolation" method code I also realised that NONE is not correct. Clearly no interpolation would leave gaps in the data. What is being done in "nearest neighbour" interpolation. I'm very short of time today so I'll look closer a bit later. While we're on a clean up here, do you agree it would make sense to use a more correct terminology on the other labels: biliniear , bicubic. I would also like to see the comment for bicubic include Catmull-Rom. Technically aware users should not have to open up the source to see what it uses and profane users will none the wiser one way or the other. (Intermediate users may be inspired to find out more). The extra item was to provide an easy means of compiling in an extra option for development and in anticipation of doing further work on the bug calling for a better choice of methods. If the default is that the #define maintains things as they are I dont see it doing any harm, but I'm not going to waster effort campaigning for it either. I just meant one less patch to pull in an out when syncing to cvs.
I think that we should try to keep the displayed strings simple. Experience shows that even what we are using now is too complex for some users. They would like us to us "None", "Fast" and "Best" or something like that. If we add "Catmull-Rom" to the displayed value, we will confuse and slow down 99.9% of our users.
OK , I take your point on Catmull, this is probably better placed in the documentation, it does not need to be on screen but it should be somewhere more accessible than in the bowels of the source code. "None", "Fast" and "Best". None is nonsense , Simple maybe, it's nice and short. Best is subjective , it depends what you want to do. If you have a very large image or old hardware bilinear may be best. If you want smooth depixelisation Lanczos is your current best. If you want the sharpest image possible but are prepared to accept staircasing bicubic may be best. I'm not sure where fast would fit in. Though it's probably fair to add (slower) to lanczos in the tool tip, this could be very long on some h'ware and big images , seems best to warn. If the enumerator goes from Simple to Lanczos (slow) they should not need a degree in maths to join the dots.
This discussion doesn't belong here. It is not related to the bug report and would anyway better be had on the mailing-list. Please note that Fast and Best was meant as an example of what some users want. This wasn't a serious suggestion. And now move this to the list please. And if you want to do all of us a favor, please open a new bug report for the remaining issue with the scaling offset. This one now has way too many comments to work with it any longer.
Thanks for ignoring my requests. I have now opened a new bug report (bug #410066) for this as this one simply has too many comments to be useful. Hopefully we can keep the other report more on-topic.
Bug #433241 now resumes this original issue scaling offsets. bug #410066 dealing with rotation offsets in gimpdrawable-transform.c , which are currently larger.