Convert colormap hex values to RGB beforehand instead of on demand

[radarhere]

The colormap dictionary maps color names to hex values.

Pillow/src/PIL/ImageColor.py

Lines 161 to 166 in bfbfff0

	colormap = {
	# X11 colour table from https://drafts.csswg.org/css-color-4/, with
	# gray/grey spelling issues fixed. This is a superset of HTML 4.0
	# colour names used in CSS 1.
	"aliceblue": "#f0f8ff",
	"antiquewhite": "#faebd7",

When it is used, the hex values is converted to an RGB tuple. That value is then saved back to colormap, as a way of caching the conversion to RGB.

Pillow/src/PIL/ImageColor.py

Lines 40 to 45 in bfbfff0

	rgb = colormap.get(color, None)
	if rgb:
	if isinstance(rgb, tuple):
	return rgb
	colormap[color] = rgb = getrgb(rgb)
	return rgb

It would be more efficient to always have the RGB values stored in the code. This also makes colormap consistent, rather than changing during runtime.

[hugovk]

Hmm, the original behaviour is a little strange.

If you access the values from colormap directly, you get hex values:

>>> from PIL import ImageColor
>>>
>>> ImageColor.colormap["whitesmoke"]
'#f5f5f5'
>>> ImageColor.colormap["cadetblue"]
'#5f9ea0'

Fine.

But, as noted, as soon as you call getrgb() it replaces the value in colormap:

>>> ImageColor.getrgb("whitesmoke")
(245, 245, 245)
>>> ImageColor.colormap["whitesmoke"]
(245, 245, 245)
>>> ImageColor.colormap["cadetblue"]
'#5f9ea0'

So could this PR be considered a breaking change for anyone accessing ImageColor.colormap directly and expecting hex values?

https://grep.app/search?q=ImageColor.colormap

[radarhere]

The options I can think of are
a) do nothing
b) this PR, changing colormap to have RGB tuple values
c) remove the caching, and make getrgb() calculate the value each time
d) add a new variable for caching, so that colormap stays consistently hex values

If b) is out, then c) would seem the best?

[hugovk]

If we started from scratch, I'd avoid changing colormap in getrgb(), but this is literally 20-year-old code (PIL 1.1.4, May 2003) so I'm cautious about changing it.

Could c) also be considered a breaking change?

Now:

>>> from PIL import ImageColor
>>> ImageColor.colormap["whitesmoke"]
'#f5f5f5'
>>> ImageColor.getrgb("whitesmoke")
(245, 245, 245)
>>> ImageColor.colormap["whitesmoke"]
(245, 245, 245)

c)

>>> from PIL import ImageColor
>>> ImageColor.colormap["whitesmoke"]
'#f5f5f5'
>>> ImageColor.getrgb("whitesmoke")
(245, 245, 245)
>>> ImageColor.colormap["whitesmoke"]
'#f5f5f5'

This is arguably less of a problem that a), and maybe people don't rely on the value in colormap after calling getrgb() at least once?

Pro: not many hits for accessing colormap directly: https://grep.app/search?q=ImageColor.colormap

---

If efficiency is a problem, we could use from functools import lru_cache and add a lru_cache or @lru_cache(maxsize=None) decorator to getrgb().

Of course, this only helps for subsequent calls to the functions.

Now:

❯ python -m timeit -s "from PIL import ImageColor" "ImageColor.getrgb('whitesmoke'); ImageColor.getrgb('whitesmoke');"
1000000 loops, best of 5: 202 nsec per loop

This PR:

❯ python -m timeit -s "from PIL import ImageColor" "ImageColor.getrgb('whitesmoke'); ImageColor.getrgb('whitesmoke');"
2000000 loops, best of 5: 175 nsec per loop

lru_cache:

❯ python -m timeit -s "from PIL import ImageColor" "ImageColor.getrgb('whitesmoke'); ImageColor.getrgb('whitesmoke');"
5000000 loops, best of 5: 54.1 nsec per loop

It could also help getcolor().

Now:

❯ python -m timeit -s "from PIL import ImageColor" "ImageColor.getcolor('whitesmoke', 'RGB'); ImageColor.getcolor('whitesmoke', 'RGB');"
1000000 loops, best of 5: 317 nsec per loop

lru_cache:

❯ python -m timeit -s "from PIL import ImageColor" "ImageColor.getcolor('whitesmoke', 'RGB'); ImageColor.getcolor('whitesmoke', 'RGB');"
5000000 loops, best of 5: 73.8 nsec per loop

---

So: maybe we do c), possibly with lru_cache?

[radarhere]

I'm reluctant to add a new caching strategy here as a once-off, or to open the door to further caching strategies without careful consideration, so I'll just close this.