Add methods to make subclass extension easier

Author: fdintino

src/thumbor_video_engine/engines/ffmpeg.py

@@ -186,6 +186,12 @@ def load(self, buffer, extension):
 
         self.probe()
 
+    def _route(self, label, method, src_file, *args):
+        """Dispatch to a gif-transcode route (``method``), labelled for
+        observability. Override to record per-route metrics/traces. Defaults
+        to simply calling the method."""
+        return method(src_file, *args)
+
     def has_transparency(self):
         if self.image:
             return has_transparency(self.image)
@@ -311,7 +317,7 @@ def transcode_to_gif(self, src_file):
         if (self.context.config.FFMPEG_GIF_PIPELINE == 'gifski'
                 and self._gifski_path() is not None):
             return self._transcode_to_gif_gifski(src_file)
-        return self._gif_legacy(src_file)
+        return self._route('legacy', self._gif_legacy, src_file)
 
     def _gifski_path(self):
         configured = self.context.config.GIFSKI_PATH
@@ -329,27 +335,37 @@ def _transcode_to_gif_gifski(self, src_file):
         width, height = self.image_size
         max_target = self.context.config.GIFSKI_MAX_TARGET_PIXELS
         if max_target and width * height > max_target:
-            return self._gif_legacy(src_file)
+            return self._gifski_oversized_target(src_file)
 
         info = self.gif_info
         if info is None:
             if self.image:
                 # Animated webp input (or an unparseable gif that PIL could
                 # still open): per-frame timing is unknown to us, so use the
                 # timing-exact legacy path.
-                return self._gif_legacy(src_file)
+                return self._route('legacy', self._gif_legacy, src_file)
             # Video source: constant frame rate from ffprobe
-            return self._gifski_y4m(src_file, self._video_fps(), "0")
+            return self._route(
+                'y4m', self._gifski_y4m, src_file, self._video_fps(), "0")
 
         if not info.is_uniform_delay:
             # Variable frame delays can't be represented in a constant frame
             # rate y4m stream; the legacy path preserves them exactly.
-            return self._gif_legacy(src_file)
+            return self._route('legacy', self._gif_legacy, src_file)
 
         repeat = "-1" if info.loop_count is None else str(info.loop_count)
         if self._gif_visibly_transparent(info):
-            return self._gifski_png_frames(src_file, info.uniform_fps, repeat)
-        return self._gifski_y4m(src_file, info.uniform_fps, repeat)
+            return self._route(
+                'png', self._gifski_png_frames, src_file, info.uniform_fps, repeat)
+        return self._route(
+            'y4m', self._gifski_y4m, src_file, info.uniform_fps, repeat)
+
+    def _gifski_oversized_target(self, src_file):
+        """Chosen when the target output exceeds ``GIFSKI_MAX_TARGET_PIXELS``
+        (gifski's quantizer memory grows with output size). Override to serve
+        something faster (e.g. a quick low-quality pass with a background
+        re-encode). Defaults to the bounded-memory legacy path."""
+        return self._route('legacy_large', self._gif_legacy, src_file)
 
     def _gif_visibly_transparent(self, info):
         """GCE transparency flags over-approximate: optimized opaque GIFs
@@ -375,13 +391,30 @@ def _video_fps(self):
             fps = DEFAULT_VIDEO_GIF_FPS
         return min(fps, MAX_VIDEO_GIF_FPS)
 
+    def _gifski_quality(self):
+        """Quality (1-100) passed to gifski. Override to vary it per request
+        (e.g. a low-quality fast pass)."""
+        return self.context.config.GIFSKI_QUALITY
+
+    def _gifski_extra_args(self):
+        """Extra flags for the gifski command (e.g. ``['--fast']``). Override
+        to add them; defaults to none."""
+        return []
+
+    def _gifski_gifsicle_pass(self):
+        """Whether to run a final geometry-free ``gifsicle -O3`` pass over
+        gifski's output. Override to vary it per request (e.g. skip it for a
+        quick low-quality pass). Defaults to the ``GIFSKI_GIFSICLE_PASS``
+        config."""
+        return self.context.config.GIFSKI_GIFSICLE_PASS
+
     def _gifski_cmd(self, out_file, fps):
-        config = self.context.config
         width, height = self.image_size
         return [
             self._gifski_path(),
             "--quiet",
-            "--quality", "%s" % config.GIFSKI_QUALITY,
+            "--quality", "%s" % self._gifski_quality(),
+        ] + self._gifski_extra_args() + [
             "--fps", "%g" % float(fps),
             # gifski caps output at ~800x600 unless explicitly sized; frames
             # are already scaled to exactly this size by ffmpeg
@@ -407,7 +440,7 @@ def _gifski_y4m(self, src_file, fps, repeat):
         with named_tmp_file(suffix=".gif") as out_file:
             gifski_cmd = self._gifski_cmd(out_file, fps) + ["--repeat", repeat, "-"]
             self._run_pipeline(ffmpeg_cmd, gifski_cmd)
-            if self.context.config.GIFSKI_GIFSICLE_PASS:
+            if self._gifski_gifsicle_pass():
                 return self._gifsicle_optimize_file(out_file)
             with open(out_file, mode="rb") as f:
                 return f.read()
@@ -441,7 +474,7 @@ def _gifski_png_frames(self, src_file, fps, repeat):
                     + frame_files
                 )
                 self.run_cmd(gifski_cmd)
-                if self.context.config.GIFSKI_GIFSICLE_PASS:
+                if self._gifski_gifsicle_pass():
                     return self._gifsicle_optimize_file(out_file)
                 with open(out_file, mode="rb") as f:
                     return f.read()

tests/engines/test_extension_hooks.py

@@ -0,0 +1,113 @@
+import os
+
+from thumbor_video_engine.engines.ffmpeg import Engine as FFmpegEngine
+
+
+def load_gif(context, storage_path, name="hotdog.gif", **config):
+    for k, v in config.items():
+        setattr(context.config, k, v)
+    with open(os.path.join(storage_path, name), mode="rb") as f:
+        buf = f.read()
+    engine = FFmpegEngine(context)
+    engine.load(buf, ".gif")
+    return engine, buf
+
+
+def test_route_hook_labels_each_gifski_route(context, storage_path, mocker):
+    routes = []
+
+    class RecordingEngine(FFmpegEngine):
+        def _route(self, label, method, src_file, *args):
+            routes.append(label)
+            return super()._route(label, method, src_file, *args)
+
+    context.config.FFMPEG_GIF_PIPELINE = "gifski"
+    engine = RecordingEngine(context)
+    with open(os.path.join(storage_path, "hotdog.gif"), mode="rb") as f:
+        engine.load(f.read(), ".gif")
+    # don't actually shell out; just confirm the route is labelled
+    mocker.patch.object(engine, "_gifski_y4m", return_value=b"gif")
+
+    assert engine._transcode_to_gif_gifski("/tmp/x.gif") == b"gif"
+    assert routes == ["y4m"]
+
+
+def test_route_hook_used_for_legacy(context, storage_path, mocker):
+    routes = []
+
+    class RecordingEngine(FFmpegEngine):
+        def _route(self, label, method, src_file, *args):
+            routes.append(label)
+            return b"gif"
+
+    engine = RecordingEngine(context)  # legacy is the default pipeline
+    engine.extension = ".gif"
+    assert engine.transcode_to_gif("/tmp/x.gif") == b"gif"
+    assert routes == ["legacy"]
+
+
+def test_oversized_target_hook(context, storage_path, mocker):
+    class OversizedEngine(FFmpegEngine):
+        def _gifski_oversized_target(self, src_file):
+            return b"fast-first"
+
+    context.config.FFMPEG_GIF_PIPELINE = "gifski"
+    context.config.GIFSKI_MAX_TARGET_PIXELS = 1  # everything is "oversized"
+    with open(os.path.join(storage_path, "hotdog.gif"), mode="rb") as f:
+        engine = OversizedEngine(context)
+        engine.load(f.read(), ".gif")
+
+    assert engine._transcode_to_gif_gifski("/tmp/x.gif") == b"fast-first"
+
+
+def test_gifski_quality_and_extra_args_hooks(context):
+    class FastEngine(FFmpegEngine):
+        def _gifski_path(self):
+            return "/usr/bin/gifski"
+
+        def _gifski_quality(self):
+            return 40
+
+        def _gifski_extra_args(self):
+            return ["--fast"]
+
+    engine = FastEngine(context)
+    engine.image_size = (100, 75)
+    cmd = engine._gifski_cmd("/tmp/out.gif", 25)
+
+    assert "--fast" in cmd
+    assert cmd[cmd.index("--quality") + 1] == "40"
+    assert cmd[cmd.index("--width") + 1] == "100"
+
+
+def test_gifski_quality_default(context):
+    context.config.GIFSKI_QUALITY = 90
+    engine = FFmpegEngine(context)
+    assert engine._gifski_quality() == 90
+    assert engine._gifski_extra_args() == []
+
+
+def test_gifski_gifsicle_pass_hook_gates_the_pass(context, storage_path, mocker):
+    # a subclass can suppress the gifsicle -O3 pass per request even when
+    # GIFSKI_GIFSICLE_PASS is configured on
+    class NoPassEngine(FFmpegEngine):
+        def _gifski_gifsicle_pass(self):
+            return False
+
+    context.config.FFMPEG_GIF_PIPELINE = "gifski"
+    context.config.GIFSKI_GIFSICLE_PASS = True
+    with open(os.path.join(storage_path, "hotdog.gif"), mode="rb") as f:
+        engine = NoPassEngine(context)
+        engine.load(f.read(), ".gif")
+    engine.resize(100, 75)
+    gifsicle_spy = mocker.spy(NoPassEngine, "_gifsicle_optimize_file")
+
+    engine.read(".gif", quality=80)
+    assert gifsicle_spy.call_count == 0
+
+
+def test_gifski_gifsicle_pass_default_follows_config(context):
+    context.config.GIFSKI_GIFSICLE_PASS = True
+    assert FFmpegEngine(context)._gifski_gifsicle_pass() is True
+    context.config.GIFSKI_GIFSICLE_PASS = False
+    assert FFmpegEngine(context)._gifski_gifsicle_pass() is False