GNOME Bugzilla – Bug 464466
Downscaling quality
Last modified: 2008-08-25 08:05:54 UTC
Please describe the problem: Copy/pasted problem description of gimp developer list Downscaling currently : it examines 2x2 pixels for each output pixel. This means if you're downscaling to less than 50%, some source pixels are ignored. If Cubic was properly implemented for downscaling, it would examine 4x4 pixels for each output pixel, and some pixels would be ignored when scale < 25%. Presently, the solution to this is to scale down incrementally (reduce scale by 50% until you approach the desired scale, and then scale down to that exact size.) Maybe GIMP could implement the above workaround before 2.4. It would be inefficient (scaling down the image N times instead of once) but it would mean that the result was correctly dependant on ALL the source pixels. Steps to reproduce: 1. 2. 3. Actual results: Expected results: Does this happen every time? Other information:
Created attachment 93238 [details] [review] Attempt to implements a reduce by 50% until final scale Reducing the image 50% each step. Between each step a the image is blurred before starting the next reduce cycle. The final step performs a bilinear interpolation. For ex : 1000x1000 => 250x250 1. 1000 x 1000 => blur (3x3 gauss) => 500 x 500 2. 500 x 500 => blur (3x3 gauss) => 250 x 250 3. 250 x 250 => bilinear interpolation => 200x200
As far as I can see this code only looks at the horizontal dimension to decide if the user is scaling down or up. What about the case where the image is scaled differently in horizontal and vertical direction?
I have renamed some files in app/paint-funcs and added your new code in a new file reduce-region.c. I have also done some coding style cleanups. The new code is not yet used though. Please base further changes on the code in SVN. I am not very happy with the new code as it allocates large temporary buffers and iterates over the data row-by-row instead of working on it on a tile-by-tile basis. But it's a start.
As you said it is a start, I have tried to minimize the allocation, to 3 times a source row and 1/4th of the original image size (1/4 + 1/16). Freeing it each time the largest allocation. The extra memory is only allocated if the scale down factor is < 0.5. Otherwise the scaling is row by row using the PixelRegion. Any suggestions on how to do this tile by tile?
Most functions in GIMP that manipulate pixels use pixel_regions_register() and pixel_regions_process() to iterate over the pixel regions tile-by-tile. For scaling this is somewhat more difficult as the source and destination regions are not of the same size. If we remove the gaussian blur as has been suggested on the mailing-list, then the following should work for scaling down by a factor of 2: Iterate over the source region using pixel_regions_process(). Each iteration will give you another tile on the source region. In each iteration find the corresponding tile on the destination region using tile_manager_get_tile(). Then write directly to that tile. This will not need any extra memory and the data access pattern is most efficient.
Or even better yet, iterate over the destination region and use tile_pyramid_write_quarter() from app/base/tile-pyramid.c. We could move this function elsewhere so that this code can be shared.
This report is actually duplicate of bug #166130 but since it contains some new ideas we should better continue discussion here.
Created attachment 93821 [details] Drop in replacement for scale-region.c This is a attempt to implement stair step scaling for gimp. The image is reduced/expanded in x/y directions by a factor 2. After the image is reduced/expanded a final scaling is performed. The code can be tested by replacing the files scale-region.[c|h]. The files and some of my test images can also be found on : http://users.telenet.be/geert.jordaens/
Created attachment 93822 [details] Drop in replacement for scale-region.h
We should evaluate whether it makes sense to still include this for 2.4. We probably should, but I didn't check the code carefully yet.
geert, this scale-region.[ch] has a bug when input height < 4 (see bug #467554), even when no interpolation is used.
If these results look promising enough i'll continue working on it. The current code attached merely demonstrates the principle. I don't think i can rewrite all before 2.4. I've also tracked down a bug in the linear interpolation. Things to do : - fix linear interpolation (not difficult) - fix input height < 4 (width?) - Reduce memory consumption. (looking at it now, getting familiar with the TileManager api)
Created attachment 94327 [details] Drop in replacement for scale-region.c Size *2 /2 scaling : Tilemanager based Currently only for RGB bit depth 3. I will add bit depths 1,2,4 as soon as possible.
Created attachment 94353 [details] Drop in replacement for scale-region.c Scaling should be ok now for all bit depths, still needs to be test completely. TODO : - test bith depths other then 3 - make the progress bar working.
(for bit depth 1 == greyscale and 2== greyA): Lanczos works okay. None, Linear, Cubic all do what looks like a very slow nearest neighbour scaling. (for bit depth 3 == RGB, 4 == RGBA) Linear is broken, as you said, and looks; cubic does a very slow nearest neighbour scaling; and Lanczos works.
I cannot fix this before this weekend, but the error in the bilinear interpolation for RGBA it is a copy and paste error. Correction below. static void inline interpolate_bilinear (TileManager *srcTM, ... case 4: s00 = pixel1[3]; s10 = pixel2[3]; s01 = pixel3[3]; s11 = pixel4[3]; alphasum = (gint)((1 - yfrac) * ((1 - xfrac) * s00 + xfrac * s10) + yfrac * ((1 - xfrac) * s01 + xfrac * s11)); ... For the speed issue, since all scaling is performed in several steps, more time is needed. For Cubic and linear, I use the same logic as before. Maybee the scale steps are making them all look the same?
Comment #15 NONE = Nearest neighbour LINEAR = As long as one reduce the size by 2 the fraction for the 2x2 window will be 0.5 This meens that the 4 pixels will be averaged. (Act as a smoothing filter) CUBIC = Same as for linear, the fraction will be 0.5 as long as we reduce the size by 2. (Only for 1D) x-1, x, x+1 and x+2 are taken into account. LANCZOS = Benefits from size 2 down scaling because it is a low-pass filter. My conclusion would be that when executed reducing/expanding by 2: Linear is still better then NONE since it averages the pixel values and not copies the nearest pixel. Linear performs almost as good as cubic when executed in steps (reducing/expanding by 2). Lanczos benefits from the step by 2 scaling since applying the filter repeatadly widens the lanczos window. The biggest difference between linear/cubic will be made during the final scaling step.
This is the original test image I drew, at full size. It is RGB. http://img.photobucket.com/albums/v449/neota/tech/test.png If I do the scaling manually (scale to 50%, twice, Cubic.), using the version of scale-region.c that's currently in SVN, I get this result: http://img.photobucket.com/albums/v449/neota/tech/test-cubic-rgb-good.png When I scale to 25% using your scale-region.c, Cubic, I get this result: http://img.photobucket.com/albums/v449/neota/tech/test-cubic-rgb-wrong.png When I scale to 25% using your scale-region.c, Linear, with the change you mentioned applied, I get this result: http://img.photobucket.com/albums/v449/neota/tech/test-linear-rgb-wrong.png When I scale to 25% using your scale-region.c, Lanczos, I get this result: http://img.photobucket.com/albums/v449/neota/tech/test-lanczos-rgb-wrong.png IMO use of all scaling types except 'None' in the new scale-region.c are broken. I can see the difference between 'Lanczos' and 'Linear'; but all results seem much much rougher than they should be. Using a scaling factor that isn't 100% / (1,2,4,8,..) causes slightly different results -- some appropriate antialiasing occurs. So that makes me think that, until the final 'scale down' step occurs, somehow 'None' interpolation is used -- and the chosen interpolation type is only used in the final reduction. for example, scale to 24%, cubic == No-interpolation scale to 50% of original size -> No-interpolation scale to 25% of original size -> Cubic scale to 24% of original size. I don't see where in your code the mistake is that causes this, but if you experiment with different scaling factors, it seems pretty clear that that is what is happening. What I mention above also means that you can scale to a factor >50% and it will happen correctly, with the interpolation method you selected. So the reduction loop is definitely to blame -- what is different inside the loop compared to the final reduction? The only thing I see is the 'fallback' to NONE interpolation in scale(), which I guess is being triggered spuriously.
I'll try to check everything again, i must admit that I did not do any test other then lanczos after adding the fallback. The only difference between the final step and the steps before is that the difference in X/Y (xfrac, yfrac) is different from 0.5. for (y=y0 ; y <= y1 ; y++) { yfrac = ( y / scaley ); sy0 = (gint) floor (yfrac); sy1 = (gint) ceil (yfrac); sy1 = ( sy1 >= src_height) ? src_height-1 : sy1; yfrac = yfrac - sy0; for (x=x0 ; x <= x1 ; x++) { xfrac = (x / scalex); sx0 = (gint) floor (xfrac); sx1 = (gint) ceil (xfrac); sx1 = ( sx1 >= src_width) ? src_width-1 : sx1; xfrac = xfrac - sx0;
Created attachment 94791 [details] Drop in replacement for scale-region.c Rewrote scaling down taking into account comments from bug #464466#c19. New version reducing now works like expected. Current implementation : Scaling up : NONE => nearest point (copy pixel) LINEAR => bilinear interpolation CUBIC => bicubic interpolation LANCZOS => lanczos3 interpolation Scaling down : NONE => nearest point LINEAR => reduce by 2 average (similar to current gimp) + final cubic step CUBIC => reduce by 2 average (with gausian low-pass pre filter) LANCZOS => reduce by 2 average (with lanczos2 low-pass pre filter) TODO : full test on all bit depths
This latest scale-region.c prevents me from opening images -- the gimp crashes most times I open an image, just as it's trying to display it. I filed bug #472770 wrongly, since I didn't figure out the culprit until now. However, while #472770 includes only GRAY/GRAYA test cases, the crash also occurs with RGB drawables. I haven't tried RGBA. ... LATER: I have tried RGBA now. It tends to display properly, then once I move the mouse, GIMP crashes.
Created attachment 94806 [details] Drop in replacement for scale-region.c Don't see the crashes of last comment. - Fixed some out of range errors - cleanup of the code - Progress bar is working now.
I'm still not sure about he alpha channel handling in some cases. The principle I followed is the one from the tile-pyramid code. gint a = src0[1] + src1[1] + src2[1] + src3[1]; if (a) { dst[0] = ((src0[0] * src0[1] + src1[0] * src1[1] + src2[0] * src2[1] + src3[0] * src3[1]) / a); dst[1] = a / 4; } else { dst[0] = dst[1] = 0; } Is this correct because if the sum of alpha for the 4 source pixels is less then 4. The destination pixel will get a value != 0 and alpha will become 0.
re: crashing: It might be related to up-to-date-ness of your SVN checkout VS mine (mine is up to date as of September 2 (today)). That means you might need to update it to match latest SVN eventually. Attachment #94791 [details] was the last patch that didn't cause this crash. The following ChangeLog entries might be relevant: 2007-09-02 Martin Nordholts <martinn@svn.gnome.org> * app/display/gimpdisplayshell-draw.c (gimp_display_shell_draw_area): Remove the STRESS_TEST stuff, gimp_image_invalidate_without_render does not exist anymore anyway. 2007-08-31 Michael Natterer <mitch@gimp.org> * app/base/tile-manager.c (tile_manager_get): if tile_manager->cached_tile is requested for writing, clear the cache before giving out the tile. Fixes bug #472170. 2007-08-31 Øyvind Kolås <pippin@gimp.org> * app/display/gimpdisplayshell-render.c: (render_image_tile_fault): use nearest neighbour resampling when rendering for a 1byte per pixel drawable (ony happens when interacting with the SIOX tool, and then the buffer is a indexed; not a greyscale buffer), fixes bug #472027. 2007-08-31 Raphaël Quinet <raphael@gimp.org> * app/display/gimpdisplayshell-render.c (render_image_tile_fault): do not use anti-aliasing when the zoom is exactly 200%. Fixes bug #472150. Also miscellaneous cosmetic changes. 2007-08-23 Øyvind Kolås <pippin@gimp.org> * app/display/gimpdisplayshell-render.c: (compute_sample), (render_image_tile_fault), (render_image_tile_fault_one_row): refactored to use arrays of tiles/source pixels instead of individual variables, also modified the order of the 0..8 numbers to sorted in increasing row-major order. 2007-08-22 Øyvind Kolås <pippin@gimp.org> * app/display/gimpdisplayshell-render.c: replaced bilinear interpolation from next larger pyramid level with a boxfilter of the next larger pyramid level. This is probably the last update to the quality of the display in GIMP 2.4. The alpha weighting looks good to me.
Sorry, incompatibility between the two could be possible; but SVN GIMP just seems FUBARed. the crash occurs, arbitrarily, whether I use SVN scale-region.c or yours.
I'm testing using gimp 2.4rc1 now, which doesn't exhibit the crash. Global issues: * On a small RGB image (20x20), scaling only accounts for the first output line. It clips that line one pixel short of the edge of the image, and following lines are set to #000000 (in case of Linear or None; Cubic and Lanczos still only account for the first line, but they write 'sort-of-wrong' data for the following lines too.). I can attach a test case if needed. GRAY, GRAYA, and RGBA work correctly regarding this. *To my eye, all treatment of alpha appears to be correct. Good job. Testing for cases GRAY+GRAYA: * Linear looks good. * Lanczos is extreeeeeeeemely slow and has a MASSIVE memory leak. I cannot even test it. after spending 3 minutes scaling, gimp had used 2.8gb of memory, much of it virtual (I have only 1gb ram) * Cubic is .. blurry. Testing for case RGBA: * Linear works fine. * Cubic turns the image gray (and blurry). Are you only calculating one of the color channels and writing it to all 3 color channels (copy+paste mistake)? Testing for case RGB * Linear works fine. * Cubic is ..blurry. I appreciate that people expect Cubic to produce more quality than Linear, IMO the blurring reduces the quality though. Overall you've made some good progress here. I'm not sure how Cubic could be improved on; It certainly needs to be reviewed by more people before committing it. After the mentioned bugfixes and some review, this should be complete :)
Thanks for the input David, I realy appreciate the testing and feedback. I myself only have photo - like testing images, so I already used your previous image for doing some first test, they are realy usefull. On the topic : Cubic is ..blurry. I appreciate that people expect Cubic to produce more quality than Linear, IMO the blurring reduces the quality though. I did some reading and a low-pass filter (bluring) is alsmost always suggested as a anti-aliasing measure. I know this may render line-art drawings a bit blurry, therefor we should have a clear view on how we expect to do this. As it is currently, all options are there it is just a matter of choosing the right scaling methot for the job. Basicly the linear method is doing what gimp does with at the moment when it scales down (only in steps of 2). Currently when scaling down gimp (wo drop in) calculates how many pixels it needs to determine the averadge output value. So if scaling down line art gets to blurry with cubic, one should use linear to get the same type of scaling down as currently in gimp.
* Lanczos is extreeeeeeeemely slow and has a MASSIVE memory leak. I cannot even test it. after spending 3 minutes scaling, gimp had used 2.8gb of memory, much of it virtual (I have only 1gb ram) Actualy a very dumb bug. The lanczos kernel is allocated/calculated foreach destination pixel and only once freed. * Cubic turns the image gray (and blurry). Are you only calculating one of the color channels and writing it to all 3 color channels (copy+paste mistake)? Indeed a C&P mistake I will fix this as soon as possible, I could attach a new version however currently I'm not able to at least try and compile it. --- scale-region.c 2007-09-06 09:26:10.922404400 +0200 +++ scale-region.c 2007-09-06 09:24:52.494825400 +0200 @@ -345,8 +345,10 @@ interpolation = GIMP_INTERPOLATION_LINEAR; } } - - + + if (interpolation == GIMP_INTERPOLATION_LANCZOS ) + kernel_lookup = create_lanczos3_lookup(); + /* if scale is 2^n */ if (src_width==dst_width && src_height==dst_height) { @@ -442,7 +444,6 @@ kernel_lookup); break; case GIMP_INTERPOLATION_LANCZOS: - kernel_lookup = create_lanczos3_lookup(); if ( scalex == 0.5 || scaley == 0.5) decimate_lanczos2 (srcTM, sx0, sy0, sx1, sy1, @@ -515,7 +516,7 @@ { for (b = 0 ; b<3 ; b++) { - sum = p1[0] * p1[3] + p2[0] * p2[3] + p3[0] * p3[3] + p4[0] * p4[3]; + sum = p1[b] * p1[3] + p2[b] * p2[3] + p3[b] * p3[3] + p4[b] * p4[3]; sum /= alphasum; p[b] = (guchar) CLAMP (sum, 0, 255); } @@ -1165,4 +1166,5 @@ } break; }
"I did some reading and a low-pass filter (bluring) is alsmost always suggested as a anti-aliasing measure. I know this may render line-art drawings a bit blurry, therefor we should have a clear view on how we expect to do this." Actually, I made that image up as a predictable test case. I used others, including photos.. like the third major image on this page: http://en.wikipedia.org/wiki/Persimmon And I found the small elements, such as the persimmons, were unreasonably blurred, where much more of their shape would be preserved by linear decimation. I think the main issue here is the iterative application of the low-pass filter. By applying it iteratively, you are making it more and more blurry as the number of iterations increases (ie. for each 50% downscaling) IMO if you need to do this filtering iteratively, then you should reduce the kernel size. That is, something like 001 128 001 128 256 128 001 128 001 rather than the current 001 008 016 008 001 008 064 128 064 008 064 128 256 128 064 008 064 128 064 008 001 008 016 008 001 I have simulated this by using the 'gaussian blur' (radius = 1) filter before manually downscaling. I've actually done a few experiments that suggest that if the filter is only applied once, the appropriate diameter for the blur is: 8x (to 12%) shrinking -> 7 diameter (3.5 radius) 4x (to 25%) shrinking -> 3 diameter (1.5 radius) 2x (to 50%) shrinking -> 0 (none) Based on empirical quality of results, and the theory that the blur should effect as much as possible of a full pixel in the result, but never more (this locality prevents it from destroying significant detail) - eg. 5x shrink == 4/5 of each pixel is contributed to by the gaussian blur effect, 4x shrink == 3/4, .. IMO applying the filter once (on the largest-scale data), then linearly decimating it, is a more correct method -- it seems to me when you throw away data, you should only do so once, when possible. This variable-radius approach is probably too complex to implement in GIMP yet; although after GEGL becomes commonly used in it, it should be extremely easy to implement. (for the record, my best results were obtained by using 'selective gaussian blur' with the appropriate factor from above. This would probably also be a very nice way of letting the user adjust between smoothness and sharpness; I know some people do expect a degree of blurring from downscaling. A strategy to remember for the post-GEGL future.) On that paper that you reference by Ken Turkowski-- he treats images like sound waves, which they certainly are not -- frequency conveys mainly texture and only a little of shape (and nothing at all of value, saturation, chromaticity.). Raph Levien has previously observed this seems to be a common mistake when thinking about image filtering.
I see no problem in reducing the kernel, it would make it only faster. This is also the reason why i already reduced the width of the lanczos filter when downscaling. The variable filter radius would make the tile based handling more difficult if you scale down more then 2^6 times, then one sould read 4 tiles instead of one (if tile size = 64). I first played with that idea.
Created attachment 95074 [details] Drop in replacement for scale-region.c - Fixed the memory leak for lanczos - Fixed the RGBA error - Reduced the blur for cubic interpolation
This is getting to be quite good. * The amount of blur with Cubic interpolation is now quite reasonable * Lanczos looks like Cubic except with the excessively-blurred areas sharpened a bit, so now there is a progressive quality scale none < linear < cubic < lanczos * I've tested with GRAY/GRAYA/RGB again. RGBA was not tested for the below reason: * I'm now noticing that when I switch to the eraser tool, gimp crashes, with the following backtrace: 0x082d7ebd in read_pixel_data_1 (tm=0x0, x=0, y=0, buffer=0xbf827a50 "Ü:k·0") at tile-manager.c:756 756 if (x >= 0 && y >= 0 && x < tm->width && y < tm->height) (gdb) backtrace
+ Trace 161017
I suspect this might be related to bug #472770 (where the fix for bug #472170 has caused tiles to be invalidated as the cache is freshened and thus NULL tiles to be returned from tile_manager_get()).
Created attachment 95102 [details] demonstrates erroneous treatment of pixels with alpha = 0 Looks like I was wrong. Scale this to 25% using any of (linear, cubic, lanczos), and observe the dark halo around the ends of the black areas. The alpha = 0 areas I made sure to preclear to white, so the color in those areas should be treated as invisible white.
Again I'm not able to test, one cause of the gimp crashes could be in scale_region. The source regions tiles get unreffed incorrectly if no reduce/scalup by 2 step is performed. I'll fix the alpha thing later. scale_region (PixelRegion *srcPR, PixelRegion *dstPR, GimpInterpolationType interpolation, GimpProgressFunc progress_callback, gpointer progress_data) { >>> begin delete if ( tmpTM == NULL ) { tmpTM = srcTM; } scale (dstTM,tmpTM, interpolation, progress_callback, ... tile_manager_unref (tmpTM); >>> end delete >>> begin insert if ( tmpTM == NULL ) { scale (dstTM,srcTM, interpolation, progress_callback, progress_data, &progress, max_progress); } else { scale (dstTM,tmpTM, interpolation, progress_callback, progress_data, &progress, max_progress); tile_manager_unref (tmpTM); } >>> end insert.
Created attachment 95166 [details] Drop in replacement for scale-region.c - Handles alpha correctly during low pass filter - fix unref of tiles
Created attachment 95199 [details] lanczos testing image, after scale Great, now I found a bug in the Lanczos downscaling. This is a part of a larger image, after downscaling to 25%. Observe the artefacts -- crazily bright pixels around some of the borders between darker colors. This bug does not appear reliably, so I suspect it's an initialization or memory corruption bug. I have only encountered it for RGB images, not GRAY; I suspect that's only a matter of luck though, the bug probably exists for all image types. I can attach a rough 'before' image too, if needed.
Created attachment 95205 [details] 95166: Drop in replacement for scale-region.c Meanwhile I've done some minor changes, and I can't reproduce the problem. To make sure I ran a scaling of a RGB with lanczos trough valgrind, ut it dit not report any errors. I will not be able to work on this during next week.
Now I can't reproduce the bug either! So this (the newest version) is working quite well for me currently.
Try the following: 1. Load an RGBA image (this step may not be needed) 2. Select eraser tool 3. Tweak brush scale a lot 4. Switch to another brush 5. CrAsH! Backtrace: 0x082d908d in read_pixel_data_1 (tm=0x0, x=0, y=0, buffer=0xbfe62850 "") at tile-manager.c:764 764 if (x >= 0 && y >= 0 && x < tm->width && y < tm->height) (gdb) backtrace
+ Trace 161950
AFAIK the problem occurs around #2 or #3, then scale() receives a NULL TileManager, causing the crash.
Created attachment 95372 [details] Drop in replacement for scale-region.c Untested but I think I found the cause of the crash. I'm currently not able to do further tests. The crash happens if the size down/up by 2 is not equal to both directions. while ( levelx < 0 ) { .. if (srcTM != srcPR->tiles) tile_manager_unref (srcTM); >>> begin insert srcTM = tmpTM; >>> end insert levelx++; } while ( levely < 0 ) { ... if (srcTM != srcPR->tiles) tile_manager_unref (srcTM); >>> begin insert srcTM = tmpTM; >>> end insert levely++; } ... while ( levelx > 0 ) { ... if (srcTM != srcPR->tiles) tile_manager_unref (srcTM); >>> begin insert srcTM = tmpTM; >>> end insert levelx--; } while ( levely > 0 ) { ... if (srcTM != srcPR->tiles) tile_manager_unref (srcTM); >>> begin insert srcTM = tmpTM; >>> end insert levely--; } Geert
Unfortunately, that doesn't effect the crash in question; it still happens.
In gimp/app/paint-funcs/scale-region.c scale_region() implements a box filter vertically when the new height is smaller than the old height, and calls shrink_line() when the horizontal width is decreasing. shrink_line() ignores which interpolation method is actually passed in and does a horizontal box filter. The end result is that for downscaling it doesn't matter whether linear or cubic interpolation is chosen, _but_ for lanczos the result is to always use interpolation. Thus the claims about incorrectness of downsampling and pixels being discared is in fact only correct for lanczos. Using a box filter means that the resulting pixel is the integral of the contributing (possibly fractional) source pixels.
Created attachment 95679 [details] [review] Drop in replacement for scale-region.c The problem was : Brushes don't have a tile manager attached. Since the whole implementation was tile based, this could never have worked. Now the scaling is performed on a buffer if no tiles are available. Geert
Geert, I really appreciate your work, but I am personally not very keen on changing this for 2.4. With a change as big as this it is as good as guaranteed that further bugs will be introduced and further fixes will be needed, delaying the 2.4 release even more. IMO we should move this off of the 2.4 milestones list. There will always be improvements to do for 2.4, but we must at some point move on to 2.6 and get 2.4 out.
I agree with Martin that this should not block the release. We could however still keep it on the 2.4 milestone. This only means that we would accept a fix for this in the stable 2.4 release cycle. So this could be addressed for 2.4.1, if it is ready then and considered safe.
This is indeed a big change, and I do realized that this probably is not the right time. The longer i worked on it the more it became obvious that implementing this for the 2.4 release would have to be considered as a risk. I will not be disappointed if it is not in the 2.4 release. Geert
For 2.6 we will hopefully use GEGL operations for the transformations. But let's bump this off to 2.6. If a real bug shows up in the downscaling code, we can still open a bug report for it and put it on the 2.4 milestone.
In order to stimulate some progress in this area I propose the following two algorithms for acceptable downscaling using GIMP (be careful! patents are pending). Both algorithms work very well for the most common, larger JPG images taken with any digital camera. They work better than any current, strictly GIMP methods. First algorithm (GIMP-IE7-GIMP): 1. Open image with Gimp and do the usual retouching and adjustments without any scaling; save it. 2. Open the retouched image with IE7 3. Resize the IE7 window in order reach desired image dimensions 4. Take a screenshot of it using GIMP 5. Crop out the image part (Firefox or any other things I tried doesn't work as well as IE7; although, in GIMP a xx% view of an image often looks clearly better than the same image scaled xx%) Second algorithm (GIMP-Flickr-GIMP) 1. The same as in GIMP-IE7-GIMP 2. Upload the retouched image(s) to Flickr 3. View the image on Flickr and from available dimensions chose one closest to what's needed. Save the image directly from the page. 4. Proceed using GIMP It may seem like a joke (well it is presented as a joke) - it isn't. I used (and continue to use) both methods and shared them with some of my colleagues. Attachments could be easily provided. GIMP 2.4.2 (and all earlier, since 2.2) used.
You can obtain even better results by using the display down-scaling algorithm of GIMP itself. Just take a screen-shot of the GIMP image window.
If this is going to happen for 2.6 someone will have to take lead pretty soon. Otherwise we'll bump this to 2.8.
Since last change to the scale-region.c was 9 month's ago, the replacement should still work. Some test images can be found on : http://users.telenet.be/geert.jordaens Geert
Created attachment 112271 [details] [review] scale-region-replace-2008-06-06.patch Here's a patchified and cleanuped version of the drop in replacement. I have: * Added the static keyword in a lot of places * Cleaned the usage of whitespace * Aligned stuff * lancos -> lanczos There are certainly more cleanups to be done, but this is at least a pretty good start. Some things that I noticed while cleaning this up: * The link www.worldserver.com/turk/computergraphics/ResamplingFilters.pdf seems dead, and the code for it, gaussan_lanczos2() seems like a pain to maintain unless one have a reference document. Also is it OK to take the source from that document? * The usage of LANCZOS_WIDTH was removed. Wasn't there some problem with the lanczos kernel width being fixed in GEGL? In any case, please either remove or use symbolic constants. * The example page you link to seems to have quite a few dead links. It would also be nice to see the scaling result of grayscale images, transparent images, etc. We really want to get the code right the first time. * A lot of code in in scale_region_buffer() and scale_region_tile() is duplicated. Please try to put this in a helper function. It would be really great to get this into 2.6, so let's aim for that.
Created attachment 112338 [details] The link www.worldserver.com .. seems dead * The link www.worldserver.com/turk/computergraphics/ResamplingFilters.pdf. just checked it and it seems alive to me. * gaussan_lanczos2() seems like a pain to maintain unless one have a reference document. The gaussan_lanczos2() is no more then a hardcoded implemantation of the halfphase filter desribed on page 10 of the document. * A lot of code in in scale_region_buffer() and scale_region_tile() is duplicated. : Don't agree with that. * The example page you link to seems to have quite a few dead links. It would also be nice to see the scaling result of grayscale images, transparent images, etc. We really want to get the code right the first time. => When easter falls on christmas. Come on the code is there for test since 09/2007. * The usage of LANCZOS_WIDTH was removed. Wasn't there some problem with the lanczos kernel width being fixed in GEGL? In any case, please either remove or use symbolic constants. Shouldn't be aproblem since no wiidth larger then 3 would be needed if you first scale down / up by a factor of 2.
Modifying this patch to not downscaling only using the mean value of 4 surrounding pixels would probably mean ignoring bugs: Bug #166130 – Better image scaling algorithms, redux Bug #464222 – Lanczos3 Downsample Bug This patch adresses a lot of the issues raised in these bugreports. I think the suggestion that a different default for up and down scale of images as suggested on the mailing list is useful. [Gimp-developer] Improving image scale: reduction Sun Jun 10 09:31:32 PDT 2007 Why would one need different interpolation methods for downscaling? http://homepages.inf.ed.ac.uk/rbf/HIPR2/scale.htm a)* Replacement with upper left pixel. b)* Interpolation using the mean value. Linear interpolation when reducing a image by a factor of 2 is like method b, nearest would be method a. Cubic an Lanczos are just other methods of subsampling trying to pick the right pixel value for the purpose.
The only way to get this code more testing is to get it into SVN. Martin, unless you see obvious problems with this approach, please commit your cleaned up version.
I recompiled several times because I couldn't believe it was true, but the new scale-region.c (including both my cleaned up version and the original version) can't handle 1. Create a new image 2. Make the image larger with Image -> Scale Image...
Martin, please tell us exactly what fails here. What's the color of your new image, what's the size, does it have an alpha channel? How do you scale the image, what goes wrong when scaling the image?
1. Create new 610 x 377 RGB image that is filled with white and without an alpha channel (default with a clean ~/.gimp-2.5) 2. Image -> Scale Image... to 620 x 383 with Interpolation: Cubic Actual result: Image gets the desired dimension but is completely filled with black Expected result: That the image gets the desired dimension but that it is still filled with white As a variation, repeat the above with an alpha channel as well. Then the scale will result in a completely transparent image.
Did you try it without the cleaned-up version?
As I said; yes I also tried with your latest (not my cleaned up) version.
I can confirm Martins observation. After replacing scale-region.c, upscaling a solid white layer results in a solid black layer. Actually any input image seems to become completely black for scale ratios between 1.0 and 2.0.
Created attachment 114140 [details] [review] fixes scaling for factor < 1 forgot to put in the the case where levelx && levely are 0. if ( levelx == 0 && levely == 0 ) { scale (srcTM, dstTM, interpolation, progress_callback, progress_data, &progress, max_progress); }
Created attachment 114147 [details] [review] reformatted version of that patch
Now what we need is a comparison of the results before and after applying this code. It would be awesome if someone could make a comparison chart with some small example images of different style and different scale ratios. That would help us to decide if this is the right approach and that it does not introduce major regressions.
The comparison is online at http://svenfoo.org/scaletest/.
I've committed this patch with some further coding style cleanups to trunk: 2008-08-04 Sven Neumann <sven@gimp.org> * app/paint-funcs/scale-region.c: applied patch from Geert Jordaens as attached to bug #464466. Improves quality of scaling, in particular down-scaling.
*** Bug 464222 has been marked as a duplicate of this bug. ***
If you downscale an image with lancsoz, you end up with a selection appearing. To reproduce, make e.g. a 500x500 pixel image, e.g. solid white or solid black, or draw on it if you like, but with nothing selected. Now scale the image to 100x100 pixels. Then go to the select menu and see that Select none is available. Sven pointed out in IRC what is going on: <neo> now if the algorithm is buggy and doesn't transform an all black mask into an all black mask <neo> then this would look as if a selection would be "created" <neo> and indeed, scaling an all-black layer using cubic produces an all-black layer <neo> but scaling an all-black layer using lanczos results in some shades of gray
The problem is limited to the first row. There is garbage showing up there, no matter what image you scale using Lanczos. Please note that trunk now has an optimization where it skips scaling the selection mask if it is empty. So you might not be able to reproduce the problem that Liam outlined.
Another note on the patch - the progress bar goes smoothly half way, then stops for a long time, then goes smoothly the rest of the way, e.g. scaling a large image down 75% with lancsoz. This gives the impression that something went wrong half-way. The delay is several seconds long, maybe 8 or 10 seconds on a fairly powerful machine. Maybe it's acceptable, but, if it's easy to improve, it would be worth-while, I think.
(In reply to comment #70) > Another note on the patch - the progress bar goes smoothly half way, then stops > for a long time, then goes smoothly the rest of the way, e.g. scaling a large > image down 75% with lancsoz. You are scaling an image with a single layer, right? Then the selection mask is scaled first, followed by the layer. The progress-bar is divided into two parts for this. Looks like the new code does some pre-processing before it starts to update the progress. That's why it appears stuck in the middle (when it is preparing for scaling the layer). Would be nice if that could be improved.
The pause in the progress bar is due to the creation of a new tile manager. With the current code there will be a pause every time the size reduced or expanded by a factor of two.
Created attachment 116259 [details] [review] Fixed pixels at top row. The pixels originated from the fact that I had chosen to mirror the edge pixels. Since lanczos is wider then cubic more interference showed up. So the problem actually was also present for cubic. I changed the mirroring to copying the edge pixels, in this way the result should be more predictable.
I want to recall the last patch since it introduced a regression.
Created attachment 116261 [details] [review] Fixed pixels at top row. Fixed pixels at top row. The pixels originated from the fact that I had chosen to mirror the edge pixels. Since lanczos is wider then cubic more interference showed up. So the problem actually was also present for cubic. I changed the mirroring to copying the edge pixels, in this way the result should be more predictable.
Please use a space before an opening bracket. Even with a macro like CLAMP. And please don't use expressions in CLAMP and similar macros, at least not in inner loops. The reason is that the CLAMP macro evaluates its arguments multiple times. Geert, can you please update the patch accordingly?
Good to see the Lanczos aliasing fixed :-) A few comments about the page posted by Sven, comparing 3x50% and 12.5%: - the Lanczos version is sharper in the 3x50 case, is this expected? if so, is it oversharpened or would 3x50 yield better results than 12.5? - the bilinear and bicubic now look much worse, especially in the 3x50 version (whereas in the old version 3x50 and 12.5 used to deliver roughly the same decent image)
#77 when scaling to 12.5 internally the image is scaled 3x50 in fact the work around to scale 3x50 should not be used anymore. Is this after the patch from #75 or still with previous patch? Is it "looks worse" or a fact? #76 I'll modify the patch accordingly
Created attachment 116458 [details] [review] Bugfix + reformat Bugfix + Reformatted patch
#78: This is only based on the test images from #65, so it doesn't include your patch from #75. But I'm not talking about the edges of the image, so I guess it doesn't make any difference? And it looks worse in the sense that there is an awful lot of aliasing, you can see for yourself: http://svenfoo.org/scaletest/12-2.html http://svenfoo.org/scaletest/3-50-2.html Note that even in the single-resize case the image is more aliased than the original one, but of course the triple-resize makes the effect that much more obvious. I just noticed a third, minor problem: in the nearest neighbour case all the pixels match except for the middle vertical line. Not necessarily a big deal though :D
2008-08-13 Sven Neumann <sven@gimp.org> * app/paint-funcs/scale-region.c: applied patch from Geert Jordaens to fix scaling artifacts in top row (bug #464466).
In the meantime I have done some cleanups and optimizations. The bug in the progress code that caused the progress-bar to stuck at the end is also fixed. Let's close this bug as FIXED at this point.