-
-
Notifications
You must be signed in to change notification settings - Fork 55.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PIL #17068
base: 4.x
Are you sure you want to change the base?
PIL #17068
Conversation
…nd sample image samples/cpp/mask.bmp
@AlexanderShashkin Thanks for the contribution. Could you describe the purpose of PIL-style resize? What is your use-case? Also please take a look CI bot failures. |
We definitely know that there is difference between OpenCV and PIL bilinear resize. Is that the same for NN interpolation? I think there are difference only with specific configurations like non integer scale factor (i.e. 10x11 to 13x14) or with downsampling. So please provide an example when OpenCV NN resize does not match PIL's one. Thanks! /cc @l-bat |
@AlexanderShashkin thanks for detailed description. |
Sorry for bot failures, this is because of test file that I included to show the difference. I've deleted it for now and than I will add test. |
@@ -261,6 +261,8 @@ enum InterpolationFlags{ | |||
/** flag, fills all of the destination image pixels. If some of them correspond to outliers in the | |||
source image, they are set to zero */ | |||
WARP_FILL_OUTLIERS = 8, | |||
/** PIL nearest neighbor interpolation */ | |||
INTER_NEAREST_PIL = 9, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have slightly more verbose description in the documentation, something like differs from INTER_NEAREST in the way it calculates pixel coordinates: center of the square instead of top left corner. For example, pixel (0, 0) will have coordinates (0.5, 0.5) in this mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this new enum value interfere with flags? INTER_NEAREST_PIL = 9 // 1001
will imply INTER_LINEAR = 1
with WARP_FILL_OUTLIERS = 8 // 1000
flag set. Maybe it is better to add new flag: WARP_SQUARE_PIXELS
or WARP_SHIFT_COORDINATES
with value 32
? Or maybe INTER_NEAREST_PIL
should have value 6
?
Sorry for bot failures, this is because of test file that I included to show the difference. I've deleted it for now and than I will add test.
Yes, these are really differencies in NN algorithm as well. I've added pictures in a descriprion of this PR. The original binary mask ias 256x256 and than it was downscaled and upscaled with thae factor of 16. Now my solution works only for grayscale and squared images, I'm going to support not only squared and add test. |
{ | ||
for( x = 0; x < dsize.width; x++ ) | ||
{ | ||
dst.at<char>(y, x) = src.at<char>(yi, x_ofs[x]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You support only CV_8UC1
. What about other types of Mat
? You should at least add Not implemented
assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found in resize.cpp similar situation:
if (interpolation == INTER_LINEAR_EXACT && (_src.depth() == CV_32F || _src.depth() == CV_64F))
interpolation = INTER_LINEAR; // If depth isn't supported fallback to generic resize
and add my code:
if (interpolation == INTER_NEAREST_PIL && _src.depth() != CV_8UC1)
interpolation = INTER_NEAREST; // INTER_NEAREST_PIL is supported only for grayscale images
If image is not a mask (binary) or grayscale generic INTER_NEAREST will be used. I suppose it could be better than Not implemented assertion. What do you think?
In case NotImplemented is a demand - I'll do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Case with INTER_LINEAR_EXACT
is slightly different, it is same algorithm as INTER_LINEAR
but with stable calculation method which produces same results on all platforms. While INTER_NEAREST_PIL
is separate algorithm and can produce completely different results so the assertion is more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've added
if (interpolation == INTER_NEAREST_PIL && _src.depth() != CV_8UC1)
CV_Error(Error::StsNotImplemented, "INTER_NEAREST_PIL supports only CV_8UC1");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does not look very fast.
const char* srcrow = src.ptr<char>(yi);
char* dstrow = dst.ptr<char>(y);
for(x = 0; x < dsize.width; x++)
dstrow[x] = srcrow[x_ofs[x]];
should be a bit better
@@ -3723,6 +3796,11 @@ void resize(int src_type, | |||
return; | |||
} | |||
|
|||
if (interpolation == INTER_NEAREST_PIL) { | |||
resizeNNPIL(src, dst, inv_scale_x, inv_scale_y); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functionality must be tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added tests in test_resize_pil.cpp
samples/cpp/resize_pil.cpp
Outdated
int main(int argc, char** argv) | ||
{ | ||
Mat image; | ||
image = imread("d:\\mask.bmp", IMREAD_GRAYSCALE); // Read the file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to use synthetic image, maybe gradient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've added tests with gradient.png file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mshabunin, I've added test with gradient.png but my tests haven't passed due to absence of this file in test folder. Here is my code:
const string root = cvtest::TS::ptr()->get_data_path();
const string filename = root + "gradient.png";
Mat img;
ASSERT_NO_THROW(img = imread(filename, IMREAD_GRAYSCALE));
ASSERT_FALSE(img.empty());
and the error is:
Value of: img.empty()
Actual: true
Expected: false
Should I add tests with another image or it is possible to add gradient.png in test data folder as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that you can generate source image like here on the fly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've done it in test_resize_pil.cpp
samples/cpp/resize_pil.cpp
Outdated
} | ||
resize(target_image, image, Size(256, 256), 0, 0, INTER_NEAREST_PIL); | ||
|
||
imshow("Target image", image); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it makes sense to show results of both NEAREST_PIL
and NEAREST
to show difference, otherwise the sample does not have any value. Or, even better, all modes and flags should be visualized with lena.jpg
and/or some synthetic images - that would be a great example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's even better to have Python sample instead of C++ which can run both PIL and OpenCV and compare the difference between, INTER_NEAREST, potential INTER_NEAREST_PIL and PIL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sure. I've created resize_pil.py in which I've demonstrated:
- Difference between cv.INTER_NEAREST and PIL NEAREST
- Equivalence betwen cv.INTER_NEAREST_PIL and PIL NEAREST
I've used lena.jpg
Change value of flag INTER_NEAREST_PIL
…gradient.png image Add resize_pil.py for INTER_NEAREST_PIL demo, based on lena image
Delete mask.bmp Correct resize_pil to include Image from PIL
@asmorkalov , could you please remove tags needs docs and need tests as I've added them both |
@AlexanderShashkin good job! done. |
@mshabunin @alalek Are you ready to merge the PR? |
I agree with @catree , we need to think about better naming for this mode. Maybe it should be another flag? |
Yes, probably another flag would be better. To be able to do something like:
Not sure, but I think pixel coordinates convention should matter also for Comparison between OpenCV, Pillow, scikit-images and Matlab NN interpolation mode.
Results:
Looks like output of Pillow, scikit-images and Matlab match. |
PIL has the same problems with arithmetic which you try to fix and is abandoned for a long time. The geometry for affine transformations was fixed in Pillow. Please consider change the name of the PR and correct all comments to prevent misleading. |
|
@homm yes, we're definetely talking about Pillow. I've seen your comment to the issue |
@mshabunin @dkurt Friendly reminder. I re-triggered CI build to update status. |
@asmorkalov , we need design decision on adding a flag for shifted pixel coordinates mode and its naming (see #17068 (comment)). Implementations for other resize modes and same flag for other functions (e.g. warpAffine) can be added later (should it be named differently for each function? or should it be placed in separate enum?). |
@AlexanderShashkin, thank you for the patch! |
Again, PIL originally has the same issue. It was fixed only in Pillow |
@vpisarev , thanx! I can add a support for color images when naming is approved |
yes, sure, as I've mentioned before, we're definetely talking about Pillow and there is no that issue in my INTER_NEAREST_PIL implementation |
And you still call it PIL… |
Please disregard my comments in this thread above. From my point of view: About adding a flag for pixel center coordinates:
About using Speaking about documentation, it would be great to add the formula to iterate over the pixels. From the code I have no idea how it works. I believe the correct formula to take into account pixel center coordinates in the naive implementation is the following:
Plus the rounding / border methods. About this PR:
Finally my last words. Whatever solution, flag name is chosen, from this statement:
Please check that the user will not be able to shoot himself in the foot:
|
To exclude mess with common "interpolation" enum (shared between other warp functions) and avoid creation of God functions (like cvtColor) we should:
Unification of resize pixel centers in 0 / 0.5 should be done as a separate process (through OE). |
@AlexanderShashkin, @alalek, @homm, I played a bit with it and think and we need just one INTER_NEAREST that works well enough in all cases. Whether it will be a universal formula or some adaptive one, depending on the combination of the input and output size – we need to see. Here are some partial cases where the behaviour is either obvious or just standard convention that people rely on:
Also, preferably the code should not use floating-point arithmetics to give the same results on any hardware. I found that the following algorithm (in Python) satisfies all conditions (1-5):
where Maybe in general that Probably, this should be put it into the current implementation of INTER_NEAREST (after OpenCV 4.4 release). |
jenkins cn please retry a build |
I love the terminology discussion here … I think it's fair to continue to refer to Pillow internals as PIL in variable names, particularly since the Pillow distribution still contains the PIL package. For example I recall we deprecated a |
I would like to add a possibility to resize in a "PIL nearest neighbor style".
I've started with the issue
#9096
Than try it by myself and see differencies:
Left is a original binary mask, middle is a PIL resize and right is OpenCV resize.
Both libraries were launched with nearest neighbor algorithm.
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.