mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-01 16:58:06 +00:00
fix(layout): bound the fit cache; never alias the source image in fits
Two latent issues found in a self-review pass: - LayoutContext._fit_cache was an unbounded dict (the image cache got an LRU cap, the text-fit cache didn't). Cache keys embed the fitted TEXT, so a plugin fitting changing strings — a live game clock, a ticker — on a 24/7 service grows it forever. Now LRU-bounded at 512 entries via the same pattern as the image cache. - fit_image returned the caller's ORIGINAL image object when the source was already RGBA at target size (contain/fill_height, no ink crop). ImageFitResult is documented as an independent copy, and LayoutContext caches results — an aliased image lets later mutations of the source corrupt cached fits (or vice versa). Copy in that branch. Both covered by new regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam
This commit is contained in:
@@ -136,7 +136,15 @@ def fit_image(img: Image.Image, box: Any, *, mode: str = "contain",
|
|||||||
scale = min(scale, 1.0)
|
scale = min(scale, 1.0)
|
||||||
out_w = max(1, round(src_w * scale))
|
out_w = max(1, round(src_w * scale))
|
||||||
out_h = max(1, round(src_h * scale))
|
out_h = max(1, round(src_h * scale))
|
||||||
out = work if (out_w, out_h) == (src_w, src_h) else work.resize((out_w, out_h), resample)
|
if (out_w, out_h) == (src_w, src_h):
|
||||||
|
# No resize needed — but `work` may still BE the caller's original
|
||||||
|
# image (RGBA source, no ink crop). The result must always be an
|
||||||
|
# independent copy: LayoutContext caches ImageFitResults, and an
|
||||||
|
# aliased image would let later mutations of the source corrupt
|
||||||
|
# cached fits (or vice versa).
|
||||||
|
out = work.copy() if work is img else work
|
||||||
|
else:
|
||||||
|
out = work.resize((out_w, out_h), resample)
|
||||||
return ImageFitResult(out, out_w, out_h, scale, mode, (src_w, src_h))
|
return ImageFitResult(out, out_w, out_h, scale, mode, (src_w, src_h))
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
+27
-12
@@ -328,13 +328,28 @@ class LayoutContext:
|
|||||||
# fonts, which step between crisp ladder rungs instead.
|
# fonts, which step between crisp ladder rungs instead.
|
||||||
self.scale = min(self.width / max(1, design_w),
|
self.scale = min(self.width / max(1, design_w),
|
||||||
self.height / max(1, design_h))
|
self.height / max(1, design_h))
|
||||||
self._fit_cache: Dict[Any, FitResult] = {}
|
# LRU-bounded: entries are small, but keys embed the fitted TEXT —
|
||||||
# LRU-bounded (images are big, unlike text fits). Entries hold a
|
# a plugin fitting changing text (a live game clock, a ticker) on a
|
||||||
# strong reference to the source image when keyed by id() so the id
|
# 24/7 service would otherwise grow this without bound.
|
||||||
# can't be recycled out from under the cache.
|
self._fit_cache: "OrderedDict[Any, FitResult]" = OrderedDict()
|
||||||
|
# LRU-bounded (images are big). Entries hold a strong reference to
|
||||||
|
# the source image when keyed by id() so the id can't be recycled
|
||||||
|
# out from under the cache.
|
||||||
self._image_cache: "OrderedDict[Any, Tuple[Any, Any]]" = OrderedDict()
|
self._image_cache: "OrderedDict[Any, Tuple[Any, Any]]" = OrderedDict()
|
||||||
|
|
||||||
_IMAGE_CACHE_MAX = 64
|
_IMAGE_CACHE_MAX = 64
|
||||||
|
_FIT_CACHE_MAX = 512
|
||||||
|
|
||||||
|
def _fit_cache_get(self, key: Any) -> Optional["FitResult"]:
|
||||||
|
cached = self._fit_cache.get(key)
|
||||||
|
if cached is not None:
|
||||||
|
self._fit_cache.move_to_end(key)
|
||||||
|
return cached
|
||||||
|
|
||||||
|
def _fit_cache_put(self, key: Any, result: "FitResult") -> None:
|
||||||
|
self._fit_cache[key] = result
|
||||||
|
while len(self._fit_cache) > self._FIT_CACHE_MAX:
|
||||||
|
self._fit_cache.popitem(last=False)
|
||||||
|
|
||||||
# ---- the three adaptation patterns --------------------------------
|
# ---- the three adaptation patterns --------------------------------
|
||||||
|
|
||||||
@@ -373,11 +388,11 @@ class LayoutContext:
|
|||||||
acceptable rendering exists."""
|
acceptable rendering exists."""
|
||||||
box_w, box_h = _box_dims(box)
|
box_w, box_h = _box_dims(box)
|
||||||
key = ("text", text, box_w, box_h, ladder, ellipsis)
|
key = ("text", text, box_w, box_h, ladder, ellipsis)
|
||||||
cached = self._fit_cache.get(key)
|
cached = self._fit_cache_get(key)
|
||||||
if cached is not None:
|
if cached is not None:
|
||||||
return cached
|
return cached
|
||||||
result = self._walk_ladder(text, ladder, box_w, box_h, ellipsis)
|
result = self._walk_ladder(text, ladder, box_w, box_h, ellipsis)
|
||||||
self._fit_cache[key] = result
|
self._fit_cache_put(key, result)
|
||||||
return result
|
return result
|
||||||
|
|
||||||
def fit_text_proportional(self, text: str, box: Union[Region, Tuple[int, int]],
|
def fit_text_proportional(self, text: str, box: Union[Region, Tuple[int, int]],
|
||||||
@@ -416,14 +431,14 @@ class LayoutContext:
|
|||||||
box_w, box_h = _box_dims(box)
|
box_w, box_h = _box_dims(box)
|
||||||
effective_scale = self.scale if scale is None else scale
|
effective_scale = self.scale if scale is None else scale
|
||||||
key = ("text_prop", text, box_w, box_h, ladder, base_size_px, ellipsis, effective_scale)
|
key = ("text_prop", text, box_w, box_h, ladder, base_size_px, ellipsis, effective_scale)
|
||||||
cached = self._fit_cache.get(key)
|
cached = self._fit_cache_get(key)
|
||||||
if cached is not None:
|
if cached is not None:
|
||||||
return cached
|
return cached
|
||||||
target = base_size_px * effective_scale
|
target = base_size_px * effective_scale
|
||||||
eligible = [step for step in ladder if step.size_px <= target]
|
eligible = [step for step in ladder if step.size_px <= target]
|
||||||
candidates = eligible if eligible else (min(ladder, key=lambda s: s.size_px),)
|
candidates = eligible if eligible else (min(ladder, key=lambda s: s.size_px),)
|
||||||
result = self._walk_ladder(text, candidates, box_w, box_h, ellipsis)
|
result = self._walk_ladder(text, candidates, box_w, box_h, ellipsis)
|
||||||
self._fit_cache[key] = result
|
self._fit_cache_put(key, result)
|
||||||
return result
|
return result
|
||||||
|
|
||||||
def _walk_ladder(self, text: str, ladder: Sequence[FontStep],
|
def _walk_ladder(self, text: str, ladder: Sequence[FontStep],
|
||||||
@@ -460,7 +475,7 @@ class LayoutContext:
|
|||||||
one wouldn't (baseball's multiline pattern). Text is the widest line."""
|
one wouldn't (baseball's multiline pattern). Text is the widest line."""
|
||||||
box_w, box_h = _box_dims(box)
|
box_w, box_h = _box_dims(box)
|
||||||
key = ("lines", tuple(lines), box_w, box_h, ladder, spacing)
|
key = ("lines", tuple(lines), box_w, box_h, ladder, spacing)
|
||||||
cached = self._fit_cache.get(key)
|
cached = self._fit_cache_get(key)
|
||||||
if cached is not None:
|
if cached is not None:
|
||||||
return cached
|
return cached
|
||||||
|
|
||||||
@@ -482,7 +497,7 @@ class LayoutContext:
|
|||||||
if result.fits:
|
if result.fits:
|
||||||
break
|
break
|
||||||
|
|
||||||
self._fit_cache[key] = result
|
self._fit_cache_put(key, result)
|
||||||
return result
|
return result
|
||||||
|
|
||||||
def font_for_rows(self, rows: int, box_h: int,
|
def font_for_rows(self, rows: int, box_h: int,
|
||||||
@@ -491,7 +506,7 @@ class LayoutContext:
|
|||||||
(baseball's traditional-scoreboard pattern). Measures a digit/cap
|
(baseball's traditional-scoreboard pattern). Measures a digit/cap
|
||||||
sample rather than specific strings."""
|
sample rather than specific strings."""
|
||||||
key = ("rows", rows, box_h, ladder)
|
key = ("rows", rows, box_h, ladder)
|
||||||
cached = self._fit_cache.get(key)
|
cached = self._fit_cache_get(key)
|
||||||
if cached is not None:
|
if cached is not None:
|
||||||
return cached
|
return cached
|
||||||
|
|
||||||
@@ -508,7 +523,7 @@ class LayoutContext:
|
|||||||
if result.fits:
|
if result.fits:
|
||||||
break
|
break
|
||||||
|
|
||||||
self._fit_cache[key] = result
|
self._fit_cache_put(key, result)
|
||||||
return result
|
return result
|
||||||
|
|
||||||
# ---- images ---------------------------------------------------------
|
# ---- images ---------------------------------------------------------
|
||||||
|
|||||||
@@ -194,3 +194,15 @@ class TestBasePluginDrawImage:
|
|||||||
assert ifit.height == 32
|
assert ifit.height == 32
|
||||||
# pasted onto the mock's canvas
|
# pasted onto the mock's canvas
|
||||||
assert plugin.display_manager.image.getpixel((16, 16)) != (0, 0, 0)
|
assert plugin.display_manager.image.getpixel((16, 16)) != (0, 0, 0)
|
||||||
|
|
||||||
|
|
||||||
|
class TestResultIndependence:
|
||||||
|
def test_same_size_fit_never_aliases_the_source(self):
|
||||||
|
"""LayoutContext caches ImageFitResults — an aliased image would let
|
||||||
|
later mutations of the source corrupt cached fits (or vice versa)."""
|
||||||
|
from PIL import ImageDraw
|
||||||
|
src = Image.new("RGBA", (20, 20), (255, 0, 0, 255))
|
||||||
|
fit = fit_image(src, (20, 20))
|
||||||
|
assert fit.image is not src
|
||||||
|
ImageDraw.Draw(src).rectangle([0, 0, 19, 19], fill=(0, 255, 0, 255))
|
||||||
|
assert fit.image.getpixel((5, 5)) == (255, 0, 0, 255)
|
||||||
|
|||||||
@@ -436,3 +436,19 @@ class TestBasePluginIntegration:
|
|||||||
MockCacheManager(), pm)
|
MockCacheManager(), pm)
|
||||||
assert plugin.layout.design_size == (64, 32)
|
assert plugin.layout.design_size == (64, 32)
|
||||||
assert plugin.layout.scale == 2.0
|
assert plugin.layout.scale == 2.0
|
||||||
|
|
||||||
|
|
||||||
|
class TestFitCacheBound:
|
||||||
|
def test_fit_cache_is_lru_bounded(self, ctx):
|
||||||
|
"""A plugin fitting changing text (live game clock, ticker) on a
|
||||||
|
24/7 service must not grow the fit cache without bound."""
|
||||||
|
for i in range(ctx._FIT_CACHE_MAX + 100):
|
||||||
|
ctx.fit_text(f"tick {i}", Region(0, 0, 100, 20))
|
||||||
|
assert len(ctx._fit_cache) <= ctx._FIT_CACHE_MAX
|
||||||
|
|
||||||
|
def test_lru_keeps_recent_entries_hot(self, ctx):
|
||||||
|
hot = ctx.fit_text("stay hot", Region(0, 0, 100, 20))
|
||||||
|
for i in range(ctx._FIT_CACHE_MAX - 1):
|
||||||
|
ctx.fit_text(f"cold {i}", Region(0, 0, 100, 20))
|
||||||
|
ctx.fit_text("stay hot", Region(0, 0, 100, 20)) # keep touching it
|
||||||
|
assert ctx.fit_text("stay hot", Region(0, 0, 100, 20)) is hot
|
||||||
|
|||||||
Reference in New Issue
Block a user