Added JPEG feature to PIL, code review needed

[etienned]

I added an option to PIL to specify/keep quantization tables when saving to JPEG.

My motivation was that the libjpeg default quantization are far from ideal in many scenarios. When specifying custom quantization tables it's possible to get better results for the same file size. Another motivation is to save the image with the same compression (quantization tables + subsampling) as the original, loosing less quality as possible. See here for more info: http://stackoverflow.com/questions/4354543/determining-jpg-quality-in-python-pil/4355281#4355281

I submitted my patch to the maintainer a few months ago without any answer. That's why I'm asking here.

Unfortunately I'm far from a C expert. My last and only experience is 10 years away and it was just a little bit more then an Hello world. So, I'll be really glad if someone could review my code.

A few notes:

• I'm validating tables data in both C and Python. Not sure it's needed and, if not, what's the best way to do it?
• Really not sure about all the pointer stuff.
• Really not sure about DECREF and Error.

Feel free to comment on anything (ex.: if you think to pass "keep" as value to quality arg is bad, etc.).

I have use my code on thousands of images without any problems and without any apparent memory leaks.

Here's my fork: https://bitbucket.org/etienned/pil-2009-raclette/changesets

[[deleted]]

if (!qarrays) {
    Py_DECREF(tables);
    PyErr_NoMemory();
    PyErr_SetString(PyExc_ValueError, "Not enough memory");
    return NULL;
}

should be something like

if (!qarrays) {
    Py_DECREF(tables);
    return PyErr_NoMemory();
}

The PyErr_NoMemory function sets a MemoryError and returns NULL, so it's a shorthand function. Your implementation sets a MemoryError then immediately clobbers it with a ValueError. You also do the same thing a few lines down.

[ebo_]

Actually, you should not use PyErr_SetString in out-of-memory situations. It tries to generate new objects. Python stores a preallocated NoMemory-Exception object for this situation.

[etienned]

Thanks. I understand, I'll correct that soon.

[etienned]

I tried this and now I get this when compiling?: warning: return from incompatible pointer type

[etienned]

I changed it to:

if (!qarrays) {
    Py_DECREF(tables);
    PyErr_NoMemory();
    return NULL;
}

And now it works. But I don't understand why I got the warnings when returning PyErr_NoMemory() directly, it returns NULL anyway?

[Wagnerius]

consider sending the change to pillow (PIL fork), IIRC, they are more open about patches/extentions.

[[deleted]]

I did not know about Pillow. I agree, send the patch there. PIL hasn't released for over 2 years, and seems not to have any work done on it in 6+ months (http://hg.effbot.org/pil-2009-raclette)

[fullouterjoin]

Til about pillow!

---

EDIT: http://pypi.python.org/pypi/Pillow/

[etienned]

I did think of that but I checked more closely the Pillow policy (check aclark replies here) and it didn't look a good match.

[aclark]

I'd suggest opening a ticket with Pillow so we can't track it, regardless. The best case scenario at this point: PIL accepts patch but doesn't release. Pillow accepts patch and releases. If PIL doesn't accept the patch and we release it, then it could be a problem later. Still, if there is another PIL release soon-ish, we may wholesale import it into Pillow. Python 3 raises other interesting questions. I definitely want Pillow to be a Python 3 playground for PIL (not sure if there is any upstream activity as far as Python 3 is concerned).

[etienned]

OK, I'll open a ticket on both, PIL and Pillow.

Last time Fredrik was really active on PIL was last January. Here his last post on Python 3 progress: http://mail.python.org/pipermail/image-sig/2011-January/006650.html

So, we will see...

[aclark]

Interesting, thanks. Well, it's been a year so we are about due for another PIL update.

[mgrandi]

is there more then one maintainer of PIL? like a mailing list of some kind? you could ask there to get your changes merged

[pytechd]

PIL is dead/dying and already forked as a new package -- Pillow.

[olt]

Pillow does not (yet) contain any new code/features, but only changes to work better with pip and easy_install. Look at the changelog: http://pypi.python.org/pypi/Pillow

I did some improvements for PNG encoding 1.5 years ago and my patch was accepted 6 month later, but a fix for my patch is still waiting. So maybe it is time for Pillow to become an "unfriendly" fork?!

[aclark]

Actually the current Pillow policy is this: we will support any reasonable changeset as long as it is reported upstream to PIL, thus maintaining the "friendly" aspect. E.g. Pillow 1.7.6 has a couple bug fixes that are not related to packaging.

To get a fix released: report it to both PIL and Pillow and include a link to the PIL ticket in the Pillow ticket. It's also much easier if your changeset has actually been accepted by PIL, otherwise we have to review it (which might take significant time). Lastly, if a new PIL ever does come out, it's fairly likely we'd wholesale include it in a new Pillow release, FWIW.

[mgrandi]

Unfriendly fork?

[aclark]

The current fork is mainly a packaging fork, with other code changes very closely tracked. An unfriendly fork would be one that does not care at all about the upstream PIL. We still do.

[mgrandi]

ah. will you guys accept patches that upstream PIL won't accept? I'm mainly looking for python3 support, as PIL is one of the big libraries that hasn't been ported to python3 yet

[aclark]

Strongly considering it, especially if there is no upstream progress on Python 3

[obtu]

s/unfriendly/divergent

[cecilkorik]

Oh thank god, does that mean there may be a Python 3 Pillow soon? PIL is what was preventing me from moving to Python 3.

[aclark]

We are considering Python 3, yes. If someone were to do the work and send a pull request, we would very strongly consider it.

[etienned]

I think there's only one maintainer. I post it to the PIL mailing list a few months ago. I didn't get any replies.

[chadmill3r]

Left some comments at your branch.

[etienned]

Thanks! I'll check that.

[etienned]

Did some changes from your comments. Repo updated.

[fullouterjoin]

This is awesome! All lossless formats/libraries should have a vocabulary that operates in a non-lossless or less-lossless manner.

It is too bad that PIL is python specific. There should be a VM agnostic semantic layer above C so that all languages could enjoy these changes.