Skip to content
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

Test_file_webp fails on Windows #363

Closed
cgohlke opened this issue Oct 4, 2013 · 5 comments · Fixed by #365
Closed

Test_file_webp fails on Windows #363

cgohlke opened this issue Oct 4, 2013 · 5 comments · Fixed by #365

Comments

@cgohlke
Copy link
Contributor

cgohlke commented Oct 4, 2013

Current master fails in test_file_webp. This test passed with version 2.2.1. Probably due to PR #359. Tested on win-amd64-py3.3:

running test_file_webp ...
Tests\test_file_webp.py:66: assert_image_similar(image, target, 20.0) failed:
- average pixel value difference 42.4100 > epsilon 20.0000
Tests\test_file_webp.py:93: assert_image_similar(image, pil_image, 1.0) failed:
- average pixel value difference 13.0000 > epsilon 1.0000
@wiredfool
Copy link
Member

That pull request shouldn't have changed anything on the image side of the webp stuff, the only changes were in the metadata portion. On the other hand, I'm not sure what else it could be.

@cgohlke
Copy link
Contributor Author

cgohlke commented Oct 4, 2013

One issue: PY_SSIZE_T_CLEAN is defined but icc_size and exif_size are int. size is still Py_ssize_t.

@cgohlke
Copy link
Contributor Author

cgohlke commented Oct 4, 2013

This passes:

index e55b0e5..ea4c9cb 100644
--- a/_webp.c
+++ b/_webp.c
@@ -1,4 +1,3 @@
-#define PY_SSIZE_T_CLEAN
 #include <Python.h>
 #include "py3.h"
 #include <webp/encode.h>
@@ -19,7 +18,7 @@ PyObject* WebPEncode_wrapper(PyObject* self, PyObject* args)
     uint8_t *exif_bytes;
     uint8_t *output;
     char *mode;
-    Py_ssize_t size;
+    int size;
     int icc_size;  /* see comment below */
     int exif_size;
     size_t ret_size;

@wiredfool
Copy link
Member

Ok, I did that wrong. I didn't even see the py_ssize_t_clean macro. My initial version of the patch had a bunch of casts to int rather than changing the definition, perhaps that method is better, since as things go forward, py_ssizet is the preferred method.

I can see where the size for the image data would need to be done as well. I'll revisit this in the morning.

@wiredfool
Copy link
Member

I've gone back over it. https://github.com/wiredfool/Pillow/tree/pypy

Docmentation around PY_SSIZE_T_CLEAN supports not relying on PyArg_ParseTuple to return ints going forward, though it's managed to be that way through several py3 releases now after they were threatening to remove it.

As for the test failure, there's something here that I'm not getting with Pypy. The size parameter is never actually passed into the webp encoder, since the encoder expects h_w_(3,4) bytes of raw image data. So unless something else is going wonky, I don't see how it should changes. On the other hand, I was getting some really strange results of the icc_size and exif_size when they where Py_ssize_t and either interpreted as ints without casting or compared to 0. Printing as %ld was giving 0, printing%d was giving some large int, and (uncast) *_size > 0 was returning true.

Can you test head on my pypy branch and let me know if this all works?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants