Issue 1883 - Convert mode P to LA to work around a PIL bug when extracting the alpha channel

Issue 1883 - Convert mode P to LA to work around a PIL bug when extracting the alpha channel

[kzar, 2015-01-27 14:25:41]

http://codereview.adblockplus.org/5195120659922944/diff/5741031244955648/imag...
File imageConversion.py (right):

http://codereview.adblockplus.org/5195120659922944/diff/5741031244955648/imag...
imageConversion.py:18: def get_alpha(image):
I think there should be a comment here to explain why we need to convert the
image to greyscale + alpha before reading the alpha. (Just to work around a PIL
bug that stops us reading alpha for images with mode P?

[Sebastian Noack, 2015-01-27 14:29:25]

http://codereview.adblockplus.org/5195120659922944/diff/5741031244955648/imag...
File imageConversion.py (right):

http://codereview.adblockplus.org/5195120659922944/diff/5741031244955648/imag...
imageConversion.py:18: def get_alpha(image):
On 2015/01/27 14:25:42, kzar wrote:
> I think there should be a comment here to explain why we need to convert the
> image to greyscale + alpha before reading the alpha. (Just to work around a
PIL
> bug that stops us reading alpha for images with mode P?

Frankly, converting an image without alpha channel to a mode that has an alpha
channel, in order to extract the alpha channel, would be the obvious way doing
it in the first place. The old code were an optimized (but buggy) implementation
which might have needed a comment.

[kzar, 2015-01-27 14:32:53]

On 2015/01/27 14:29:25, Sebastian Noack wrote:
>
http://codereview.adblockplus.org/5195120659922944/diff/5741031244955648/imag...
> File imageConversion.py (right):
> 
>
http://codereview.adblockplus.org/5195120659922944/diff/5741031244955648/imag...
> imageConversion.py:18: def get_alpha(image):
> On 2015/01/27 14:25:42, kzar wrote:
> > I think there should be a comment here to explain why we need to convert the
> > image to greyscale + alpha before reading the alpha. (Just to work around a
> PIL
> > bug that stops us reading alpha for images with mode P?
> 
> Frankly, converting an image without alpha channel to a mode that has an alpha
> channel, in order to extract the alpha channel, would be the obvious way doing
> it in the first place. The old code were an optimized (but buggy)
implementation
> which might have needed a comment.

Right, but it's not immediately obvious that's what you're doing unless you
leave a comment, or you're already familiar with PIL. (I don't want to waste
loads of time arguing about comments again :( )

[Sebastian Noack, 2015-01-27 15:10:05]

http://codereview.adblockplus.org/5195120659922944/diff/5741031244955648/imag...
File imageConversion.py (right):

http://codereview.adblockplus.org/5195120659922944/diff/5741031244955648/imag...
imageConversion.py:18: def get_alpha(image):
On 2015/01/27 14:32:53, kzar wrote:
> On 2015/01/27 14:29:25, Sebastian Noack wrote:
> > Frankly, converting an image without alpha channel to a mode that has an
alpha
> > channel, in order to extract the alpha channel, would be the obvious way
doing
> > it in the first place. The old code were an optimized (but buggy)
> implementation
> > which might have needed a comment.
> 
> Right, but it's not immediately obvious that's what you're doing unless you
> leave a comment, or you're already familiar with PIL. (I don't want to waste
> loads of time arguing about comments again :( )

It's unneeded to document APIs in the calling code. We have discussed and agreed
on that a while ago. PIL is a well-documented library and we don't need to
duplicate their documentation in our code.

However, maybe it makes sense to mention the initial - even if less obvious -
approach, just to make sure that it's not used again in the future.

Also I realized that the original code returned None if no transparency is
involved. So I also recovered that behavior again.

[kzar, 2015-01-27 15:13:26]

LGTM