From c6783daaeca4610eeb8fa14d2bbed1f530dfeba4 Mon Sep 17 00:00:00 2001
From: Sam Wilson <sam@samwilson.id.au>
Date: Wed, 10 Apr 2019 09:21:44 +0800
Subject: [PATCH] Render PNGs more efficiently

Use file redirection in the OS shell to render PNG file rather
than reading them into memory. This also makes the rendering
an atomic operation from the application's point of view, so
perhaps will fix the issue with returning incomplete PNGs to the
client.

Bug: T219335
---
 src/Controller/ApiController.php | 20 +++++++++++---------
 src/Service/Renderer.php         | 19 ++++++++++++-------
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/src/Controller/ApiController.php b/src/Controller/ApiController.php
index 94fd15fd..b8d328eb 100644
--- a/src/Controller/ApiController.php
+++ b/src/Controller/ApiController.php
@@ -54,14 +54,16 @@ public function getFile(string $filename, string $lang): Response
     }
 
     /**
-     * Get a full filesystem path to a temporary PNG file.
+     * Get a full filesystem path to a temporary file.
      * @param string $filename The base SVG filename.
      * @param string $key The unique key to append to the filename.
+     * @param string $ext The file extension to use for the returned filename.
      * @return string
      */
-    protected function getTempPngFilename(string $filename, string $key): string
+    protected function getTempFilename(string $filename, string $key, string $ext): string
     {
-        return $this->cache->fullPath($filename.'_'.$key.'.png');
+        $name = pathinfo($filename, PATHINFO_FILENAME);
+        return $this->cache->fullPath($name.'_'.$key.'.'.$ext);
     }
 
     /**
@@ -85,14 +87,14 @@ public function requestFileWithTranslations(string $filename, string $lang, Requ
         // Write the modified SVG out to the filesystem, named with a unique key. This is necessary
         // both because multiple people could be translating the same file at the same time, and
         // also means we can use the same key for the rendered PNG file. The key is generated from
-        // the translation set so that the
+        // the translation set in order to not generate redundant cache files.
         $fileKey = md5(serialize($translations));
-        $tempPngFilename = $this->getTempPngFilename($filename, $fileKey);
-        $file->saveToPath($tempPngFilename);
+        $tempPngFilename = $this->getTempFilename($filename, $fileKey, 'png');
+        $tempSvgFilename = $this->getTempFilename($filename, $fileKey, 'svg');
+        $file->saveToPath($tempSvgFilename);
 
         // Render the modified SVG to PNG, and return it's URL.
-        $renderedPngContents = $this->svgRenderer->render($tempPngFilename, $lang);
-        file_put_contents($tempPngFilename, $renderedPngContents);
+        $this->svgRenderer->render($tempSvgFilename, $lang, $tempPngFilename);
         $relativeUrl = $this->generateUrl('api_file_translated', [
             'filename' => $filename,
             'key' => $fileKey,
@@ -113,7 +115,7 @@ public function requestFileWithTranslations(string $filename, string $lang, Requ
      */
     public function getFileWithTranslations(string $filename, string $lang, string $key, Request $request): Response
     {
-        $tempPngFilename = $this->getTempPngFilename($filename, $key);
+        $tempPngFilename = $this->getTempFilename($filename, $key, 'png');
         if (file_exists($tempPngFilename)) {
             return new BinaryFileResponse($tempPngFilename, 200, ['X-Accel-Buffering' => 'no']);
         }
diff --git a/src/Service/Renderer.php b/src/Service/Renderer.php
index c0a6e423..0123e9f6 100644
--- a/src/Service/Renderer.php
+++ b/src/Service/Renderer.php
@@ -24,24 +24,29 @@ public function __construct(string $rsvgCommand)
     }
 
     /**
+     * Render a SVG file to PNG and either save a file or return the image.
      * @param string $file Full filesystem path to the SVG file to render.
      * @param string $lang Code of the language in which to render the image.
+     * @param string $outFile Full filesystem path to the file to write the PNG to.
      * @throws ProcessFailedException If the PNG conversion failed.
-     * @return string The PNG image contents.
+     * @return string The PNG image contents, or nothing if an $outFile was provided.
      */
-    public function render(string $file, string $lang) : string
+    public function render(string $file, string $lang, ?string $outFile = null) : string
     {
-        $process = new Process([$this->rsvgCommand, $file]);
+        // Construct the command, using variables that will be escaped when it's run.
+        $command = $this->rsvgCommand.' "$SVG"';
+        if ($outFile) {
+            // Redirect to output file if required.
+            $command .= ' > "$PNG"';
+        }
+        $process = Process::fromShellCommandline($command);
         if ('fallback' !== $lang) {
             // Set the LANG environment variable, which will be interpreted as the SVG
             // systemLanguage. If the fallback language is being requested, the OS's default will be
             // used instead (as is done in MediaWiki).
             $process->setEnv(['LANG' => $lang]);
         }
-        $process->run();
-        if (!$process->isSuccessful()) {
-            throw new ProcessFailedException($process);
-        }
+        $process->mustRun(null, ['SVG' => $file, 'PNG' => $outFile]);
         return $process->getOutput();
     }
 }