OK, I made a new commit. So Carne can still see the previous behavior with the changing scale factors.
Regarding the tests: OK, good to know. I added now two more tests, which require rounding the output size and confirm Matlab compatibility. I removed the test with the Gimp output, since no compatibility is desired here. Also for Gimp approach 2 is fine, since it shows the user the modified scale factors before scaling is done.
|
@Christof:
- Regarding your argumentation about rounding issues: I would also prefer to use the path (1.), i.e. keeping the scale factor untouched and slightly increase the output image pixel number if necessary. If you have checked (without looking at the Matlab source code) that this is also the Matlab behavior, then this path is definitly even better, because it is more Matlab compatible (and still correct in some sense). Image analysis (like done with Octave's image package) might differ here from image edition (like done with Gimp).
- Regarding your second patch: I think it doesn't make a difference if you prepare a new patch based on your old patch, or based on the image package repo version. It is good that you are already presenting your patches in a separate (bitbucket) repository, which is the preferred way of Carne to review patches (if I understood him right).
- The philosophy about tests in the image package is to have the test results Matlab compatible (if somehow possible). If there is a (intentional or also unwanted) Matlab incompatibility, then it's good to leave a failing (!) test in place, to document this behavior. So please do not change existing test, just because they are failing, please only change tests if those tests weren't Matlab compatible and your improved function now is. You can always add new tests, if you want to.
Eventually we need to wait for Carne's comments and review, here. He is not only the package maintainer of the image package, but he is also the latest author of the current imresize.m function and its current indexing approach.
|
Regarding the changed tests, there are several reasons.
I added some tests to make the suite more complete:
- nearest neighbor with x-scale 1 and y-scale 2
- bilinear scaling with gimp output for reference
- checkerboard
I replaced the nearest neighbor check, that used a reference matrix to a more general test. The new pixel centers are all lying in between 4 points, similarly as shown in comment #4. So all neighbors have the same distance and thus there is no unique nearest neighbor. Hence I only checked that one of the 4 neighbors is used in the output.
The tests for bilinear and bicubic interpolation with matlab reference outputs were all failing. This is currently still the case for bicubic interpolation (I like to look at that later). However, to avoid rounding of the output size, I changed the input matrices from 7 x 10 to 8 x 10. This gives a 4 x 5 or a 12 x 15 matrix with scale factors 0.5 or 1.5, respectively. Because of the changed input I had to redo the reference outputs (but I could also not confirm all of the old Matlab results). Now the new bilinear interpolation results in the same output as the one from Matlab (without anti-aliasing).
Let me explain the issue with the output size rounding with a tiny example:
Let in = [1 2 3];
When scaling with 0.5 and the output size should be 1 x 2, there are two options:
- Replace the input by in = [1 2 3 3]; which will result in the output out = [1.5 3];
- Replace the scaling by 0.666..., which will result in the output out = [1.25 2.75];
(Note, this does not run in octave currently, since interp2 needs at least a 2 x 2 matrix to work.)
Matlab goes for the first approach, while I chose the second. Also Gimp seems to use the second. However, this might need some further discussion.
- Replacing the input seemed odd to me at first glance. However, it preserves the scale factor and gives accurate results for all pixels, that are not at the right or bottom border. In the example above you can see this from the first two values, which have been scaled down exactly to 1.5, which is what you expect with a factor of 0.5.
- Replacing the scale factor seemed better to me, since the output is accurate for the new scaling. However, this might result in a different scaling for x and y. This does clearly not preserve shapes. Also, the user might not notice that the scaling changed at all.
So, now I think the first approach is better, since it gives less surprises. What do you think? If you agreed, I would like to change it. Are there any restrictions here about how to do it? Should I make a new commit or rather try to change the current one?
Regarding your question for the bicubic interpolation: Yes, the changed interpolation points affect all methods. Hopefully in a good way.
Sorry for the length of this comment and thanks for the good discussion!
|
Here is the Matlab result on simple sample inputs:
In general I think the approach in comment #1 and comment #4 sounds reasonable.
I have not understood yet, why you needed to change existing tests in the code? Where they never Matlab compatible? Did the Matlab result change over the last years?
Do the results of bicubic interpolation also change with your patch?
To finally review (and hopefully commit in some way) your propsed code change, we need to wait for Carne, the current image package maintainer.
|
This problem is not mainly handling Matlab incompatibility. The approach was just not correct. Raster graphics have pixels and pixels represent the mean color (or value) of an area. They are not samples of infinitesimal small points. This is what I changed.
The image should cover the same area before and after scaling. A pixel has a width and height of 1 unit. This means the first pixel starts at (0.5, 0.5), has its center at (1, 1) and ends at (1.5, 1.5). The last pixel starts at (end-0.5, end-0.5), has its center at (end, end) and ends at (end+0.5, end+0.5). So the whole image starts at (0.5, 0.5) and ends at (end+0.5, end+0.5). The scaled image has to cover the same area, but with a different resolution. In the example of my first comment I scaled the image down by using a factor of 0.5. This means in the system of the original resolution the pixels have now a size of 2 units. So the new first pixel will start at (0.5, 0.5), have its center at (1.5, 1.5) and end at (2.5, 2.5) and so on.
From this you can see that the checkerboard in the example should give the mean value, since the new pixel centers (marked with an X) are exactly in the middle of 4 pixel groups:
Matlab shows this behavior in this example, but when a rounding occured in the new image size, it would not adjust the scale factor. I handled rounding errors in the commit. So I was not trying to be 100% compatible, but rather to do it correct.
To answer you question: With the old behavior all positions were a bit off, since the first pixel center has always been at 1 and the last at end. The pixel centers in between were linearly spread. So for the checkerboard it just selected the 4 corner pixels.
|
Is this problem (i.e. Matlab incompatible results) only about pixels on the edge of a resized image, or are the pixels "inside" also wrong sometimes?
|
added people to cc
|
So I made a patch for that:
https://bitbucket.org/ChristofKaufmann/octave-image/commits/branch/imresize-fix
(If you like to have a patch as attached file rather, please tell me.)
The problem in the original code were mainly the indices. These were quite wrong. I fixed the indices, which changes the behaviour of all methods; 'bilinear' works now, 'nearest' was working and is still working, 'bicubic' was not passing the test and is still not. So 'bicubic' might have to be fixed. Also, anti-aliasing is not implemented yet.
The patch also updates the matlab reference outputs and adds and modifies a few tests. It is split into two commits, since the first just makes a few coding style changes, but does not change any behaviour. The second commit has the real content.
|
octave-image version 6.2.1 (default branch from repository)
Hi,
I noticed very inaccurate results when using the 'bilinear' method with imresize. I can give a small example:
The expected behaviour would be (which matlab would give for a larger example):
|