Prepared buildtools for Safari

packagerChrome.py

Index: packagerChrome.py
===================================================================
--- a/packagerChrome.py
+++ b/packagerChrome.py
@@ -18,6 +18,9 @@
 import sys, os, re, json, struct
 from StringIO import StringIO
 
+import PIL.Image
+import PIL.ImageMath

[Wladimir Palant, 2013/09/10 10:15:27]

Please don't require these modules at the top level - building the Chrome
extension should be possible without it.

+
 import packager
 from packager import readMetadata, getMetadataPath, getDefaultFileName, getBuildVersion, getTemplate, Files
 
@@ -279,10 +282,11 @@
           files[operaFile] = files[chromeFile]
         del files[chromeFile]
 
-    # Hack: Replace "Chrome" by "Opera" in the locales
+  if params['type'] in ('opera', 'safari'):
+    # Hack: Replace "Chrome" by "Opera" or "Safari" in the locales
     for path, data in files.iteritems():
       if path.startswith("_locales/") and path.endswith("/messages.json"):
-        files[path] = re.sub(r"\bChrome\b", "Opera", data)
+        files[path] = re.sub(r"\bChrome\b", params['type'].capitalize(), data)
 
 def signBinary(zipdata, keyFile):
   import M2Crypto
@@ -308,6 +312,126 @@
     file.write(signature)
   file.write(zipdata)
 
+class ImageConverter(object):
+  def convert(self, params, files):

[Wladimir Palant, 2013/09/10 10:15:27]

Does this need to be an object? You are never really using the self variable,
this looks like abuse of object oriented programming. How about having a
function convertImage with multiple nested functions and a filters dictionary at
the top?

   filters = {"blend": filter_blend, ...}

Note that this entire functionality is only marginally related to the packager.
I am inclined to suggest moving all the generic functionality (meaning image
construction/saving and filters) into a separate module while only keeping the
real packager-related code here.

Then again, isn't blend the only filter we actually need? I would vote against
having code around just because "we might need it". This introduces unnecessary
complexity and leads to code that is broken because it isn't used anywhere.

[Sebastian Noack, 2013/09/10 12:40:43]

I'm using self to access the filters. Yes, I could use a dict instead, but it
would require to define the filter names redundant, once in the function
definitions and once in the definition of the filters dictionary.
Agreed.
All filters are used in the new metadata.chrome and metadata.safari files.

[Felix Dahlke, 2013/09/11 14:44:35]

I'd vote for having this as a bunch of free functions in a separate module. You
should be able to use locals() to call the correct filter function, no need for
a dict.

+    for filename, chain in params['metadata'].items('convert_img'):
+      steps = re.split(r'\s*->\s*', chain)
+      image = PIL.Image.open(steps.pop(0))

[Wladimir Palant, 2013/09/10 10:15:27]

This path should be relative to the metadata file it is specified in:

baseDir = os.path.dirname(metadata.option_source('convert_img', filename))
parts = steps.pop(0).split('/')
image = PIL.Image.open(os.path.join(baseDir, *parts))

Note that you also need to pass in baseDir to the filter functions.

+
+      for step in steps:
+        filter, args = re.match(r'([^(]+)(?:\((.*)\))?', step).groups()
+        args = tuple(re.split(r'\s*,\s*', args)) if args else ()
+        image = getattr(self, 'filter_' + filter)(image, *args)
+
+      f = StringIO()
+      f.name = filename
+      image.save(f)
+      files[filename] = f.getvalue()
+
+  def filter_blend(self, image, *args):
+    if len(args) == 2:    # args = (filename, opacity)
+      overlay = PIL.Image.open(args[0])

[Wladimir Palant, 2013/09/10 10:15:27]

This file name should be resolved relative to the base dir.

+
+      if image.mode != overlay.mode or image.mode == 'P':

[Wladimir Palant, 2013/09/10 10:15:27]

Please comment here that 'P' means "palette" (which is the only mode where two
images can have the same mode yet not support the same colors). That's not
really obvious.

+        overlay = overlay.convert('RGBA')
+        image   =   image.convert('RGBA')

[Wladimir Palant, 2013/09/10 10:15:27]

Style nit: we don't usually align equal signs and such (no real readability
improvement but makes changing code more complicated).

+    elif len(args) == 4:  # args = (red, green, blue, opacity)
+      overlay = PIL.Image.new('RGB', image.size, tuple(map(int, args[:3])))
+
+      if image.mode == 'P':
+        image = image.convert('RGBA')
+
+      if image.mode in ('RGBA', 'LA'):
+        overlay = PIL.Image.merge('RGBA', overlay.split() + image.split()[-1:])
+
+      if image.mode != overlay.mode:
+        image = image.convert(overlay.mode)
+    else:
+      raise TypeError

[Wladimir Palant, 2013/09/10 10:15:27]

How about:

    raise TypeError("Wrong number of blend filter parameters")

That will make debugging typos a bit easier.

+
+    return PIL.Image.blend(image, overlay, float(args[-1]))

[Wladimir Palant, 2013/09/10 10:15:27]

I suggest processing the opacity parameter when you handle the other parameters
above. E.g.:

    overlay = PIL.Image.open(args[0])
    opacity = float(args[1])

This will make this more obvious and won't cause errors if somebody adds a third
way to call this filter and forgets to put opacity last.

+
+  def filter_grayscale(self, image):
+    if image.mode == 'P':
+      image = image.convert('RGBA')
+
+    bands = list(image.split())
+    alpha = bands.pop(-1) if image.mode in ('RGBA', 'LA') else None
+
+    if len(bands) == 1:
+      return image
+
+    new_image = PIL.ImageMath.eval(
+      "convert((%s) / %d, 'L')" % (
+        ' + '.join('image%d' % i for i in xrange(len(bands))),
+        len(bands)
+      ),
+      **dict(('image%d' % i, band) for i, band in enumerate(bands))
+    )

[Wladimir Palant, 2013/09/10 10:15:27]

Won't image.convert("LA", dither=None) do the right thing? Note that your
approach is wrong - a proper grayscale conversion doesn't treat all colors
equally, the correct factors are actually 0.299, 0.587, 0.144.

+
+    if alpha:
+      new_image = PIL.Image.merge('LA', [new_image, alpha])
+
+    return new_image
+
+  def filter_colorToAlpha(self, image, *color):

[Wladimir Palant, 2013/09/10 10:15:27]

I cannot really imagine what we would use this for.

[Sebastian Noack, 2013/09/10 12:40:43]

Safari ignores all image data except the alpha channel for toolbar item icons.
So without converting white to alpha, our logo would be just a blank hexagon in
Safari.

+    bands = [band.convert('F') for band in image.convert('RGBA').split()]
+    color = map(float, color)

[Wladimir Palant, 2013/09/10 14:02:12]

Shouldn't we verify that three colors are given and throw a meaningful exception
if not?

[Sebastian Noack, 2013/09/10 16:50:05]

I'm not a big fan of adding extra code to throw an exceptions in cases where an
exception would be thrown anyway. I rather prefer to keep the code simple and
free of unneeded error handling, so the traceback is more meaningful. ;)

[Felix Dahlke, 2013/09/11 14:44:35]

I agree somewhat. However, in this case, I think it'd be most clear to just have
r, g and b in the signature instead of color*. It's the same for the caller and
will lead to friendlier errors.

+
+    # Find the maximum difference rate between source and color. I had to use two
+    # difference functions because ImageMath.eval only evaluates the expression
+    # once.
+    alpha = PIL.ImageMath.eval('''\
+      float(
+        max(
+          max(
+            max(
+              difference1(red_band, cred_band),
+              difference1(green_band, cgreen_band)
+            ),
+            difference1(blue_band, cblue_band)
+          ),
+          max(
+            max(
+              difference2(red_band, cred_band),
+              difference2(green_band, cgreen_band)
+            ),
+            difference2(blue_band, cblue_band)
+          )
+        )
+      )''',
+      difference1=lambda source, col: (source - col) / (255.0 - col),
+      difference2=lambda source, col: (col - source) / col,

[Wladimir Palant, 2013/09/10 14:02:12]

I have a pretty hard time judging whether this algorithm is correct. However, it
seems to make more sense to have a Python function difference() that will simply
calculate the difference to color and replace the weird max() sequence above by
max(difference(red_band, green_band, blue_band)).

Still, aren't we applying this to grayscale images only? If so then this is
seriously overcomplicating things - we can have a luminosityToAlpha filter which
will convert the image to grayscale first and use any band of the resulting
image as the alpha band (all color bands are the same for grayscale images by
definition).

[Sebastian Noack, 2013/09/10 16:50:05]

Yes you could do that in plain python instead of with an image expression. But
it would be way slower, which might be ok in our case since our icons are rather
small images. But I don't see how this would result in a much simpler
implementation.

At the moment images passed to that filter aren't converted to grayscale. But
yes in our specific case we could convert them to grayscale and hardcode the
color white, to make the algorithm simpler. But I would prefer to keep the more
flexible implementation.

[Wladimir Palant, 2013/09/11 06:41:36]

Given the complexity of this implementation I'm going to insist on a simpler
solution, this will become a maintenance burden otherwise.

[Felix Dahlke, 2013/09/11 14:44:35]

I prefer the approach suggested by Wladimir for this, simpler and easier to
follow. We can still look into using PIL.ImageMath and a faster algorithm if
this takes too long in practice. I somewhat doubt it though.

[Sebastian Noack, 2013/09/11 15:48:06]

I still don't see how not using PIL.ImageMath would actually make anything
simpler. PIL.ImageMath isn't only faster, it also avoids the otherwise required
boilerplate code to modify the image data pixel by pixel. Yes it's built-in
syntax and functions are limited, but you can easily make python functions
available within ImageMath when required, as already done here.

However I will look if I find a simpler approach for our use case.

[Wladimir Palant, 2013/09/11 16:14:56]

As I said, converting to greyscale and taking any color band as the alpha band
should be sufficient. PIL.ImageMath is certainly powerful but it seems to be
unnecessary here.

+
+      red_band  = bands[0],
+      green_band= bands[1],
+      blue_band = bands[2],
+
+      cred_band  = color[0],
+      cgreen_band= color[1],
+      cblue_band = color[2],
+    )

[Wladimir Palant, 2013/09/10 14:02:12]

Please indicate the source of your code whenever you reuse code. I incidentally
found out that the source of this code is
http://stackoverflow.com/a/1617909/785541. This particular piece of code appears
to be licensed under the LGPL and was posted on Stack Overflow in violation of
the license terms, something that we need to know and consider.

+
+    # Calculate the new image colors after the removal of the selected color
+    new_bands = [
+      PIL.ImageMath.eval(
+        "convert((image - color) / alpha + color, 'L')",
+        image=band,
+        color=col,
+        alpha=alpha
+      )
+      for band, col in zip(bands, color)
+    ]
+
+    # Add the new alpha band
+    new_bands.append(PIL.ImageMath.eval(
+      "convert(alpha_band * alpha, 'L')",
+      alpha = alpha,
+      alpha_band = bands[3]
+    ))
+
+    return PIL.Image.merge('RGBA', new_bands)
+
 def createBuild(baseDir, type='chrome', outFile=None, buildNum=None, releaseBuild=False, keyFile=None, experimentalAPI=False, devenv=False):
   metadata = readMetadata(baseDir, type)
   version = getBuildVersion(baseDir, metadata, releaseBuild, buildNum)
@@ -335,6 +459,9 @@
   if metadata.has_section('convert_js'):
     convertJS(params, files)
 
+  if metadata.has_section('convert_img'):
+    ImageConverter().convert(params, files)
+
   if metadata.has_section('import_locales'):
     importGeckoLocales(params, files)