More efficient decoding for down-sampled Flutter images using cache(Width|Height) (#15372)

ignatz · cbracken · commit c7d0fb787922 · 2020-01-16T10:13:04.000-08:00
When down-scaling images, decode encoded images into smaller images
closer to the target size before finally down-scaling them to their
target size. For very large images this can avoid inflating the image
into its full size first before throwing it away. This can help to
significantly reduce peak memory utilization.

On a tangent, we could be even more efficient, if we'd interpret the
cache(Width|Height) as sizing hints.

I also opportunistically added warnings, I don't think a "caching" API
should support scaling images up or changing their aspect ratio.
diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter
@@ -272,6 +272,7 @@ FILE: ../../../flutter/lib/ui/dart_ui.h
 FILE: ../../../flutter/lib/ui/dart_wrapper.h
 FILE: ../../../flutter/lib/ui/fixtures/DashInNooglerHat.jpg
 FILE: ../../../flutter/lib/ui/fixtures/Horizontal.jpg
+FILE: ../../../flutter/lib/ui/fixtures/Horizontal.png
 FILE: ../../../flutter/lib/ui/fixtures/hello_loop_2.gif
 FILE: ../../../flutter/lib/ui/fixtures/hello_loop_2.webp
 FILE: ../../../flutter/lib/ui/fixtures/ui_test.dart
diff --git a/lib/ui/BUILD.gn b/lib/ui/BUILD.gn
@@ -155,6 +155,7 @@ if (current_toolchain == host_toolchain) {
     fixtures = [
       "fixtures/DashInNooglerHat.jpg",
       "fixtures/Horizontal.jpg",
+      "fixtures/Horizontal.png",
       "fixtures/hello_loop_2.gif",
       "fixtures/hello_loop_2.webp",
     ]
diff --git a/lib/ui/fixtures/Horizontal.png b/lib/ui/fixtures/Horizontal.png
diff --git a/lib/ui/painting/image_decoder.cc b/lib/ui/painting/image_decoder.cc
@@ -4,11 +4,18 @@
 
 #include "flutter/lib/ui/painting/image_decoder.h"
 
+#include <algorithm>
+
 #include "flutter/fml/make_copyable.h"
-#include "flutter/fml/trace_event.h"
 #include "third_party/skia/include/codec/SkCodec.h"
+#include "third_party/skia/src/codec/SkCodecImageGenerator.h"
 
 namespace flutter {
+namespace {
+
+constexpr double kAspectRatioChangedThreshold = 0.01;
+
+}  // namespace
 
 ImageDecoder::ImageDecoder(
     TaskRunners runners,
@@ -25,6 +32,10 @@ ImageDecoder::ImageDecoder(
 
 ImageDecoder::~ImageDecoder() = default;
 
+static double AspectRatio(const SkISize& size) {
+  return static_cast<double>(size.width()) / size.height();
+}
+
 // Get the updated dimensions of the image. If both dimensions are specified,
 // use them. If one of them is specified, respect the one that is and use the
 // aspect ratio to calculate the other. If neither dimension is specified, use
@@ -40,8 +51,7 @@ static SkISize GetResizedDimensions(SkISize current_size,
     return SkISize::Make(target_width.value(), target_height.value());
   }
 
-  const auto aspect_ratio =
-      static_cast<double>(current_size.width()) / current_size.height();
+  const auto aspect_ratio = AspectRatio(current_size);
 
   if (target_width) {
     return SkISize::Make(target_width.value(),
@@ -57,34 +67,52 @@ static SkISize GetResizedDimensions(SkISize current_size,
 }
 
 static sk_sp<SkImage> ResizeRasterImage(sk_sp<SkImage> image,
-                                        std::optional<uint32_t> target_width,
-                                        std::optional<uint32_t> target_height,
+                                        const SkISize& resized_dimensions,
                                         const fml::tracing::TraceFlow& flow) {
   FML_DCHECK(!image->isTextureBacked());
 
-  const auto resized_dimensions =
-      GetResizedDimensions(image->dimensions(), target_width, target_height);
+  TRACE_EVENT0("flutter", __FUNCTION__);
+  flow.Step(__FUNCTION__);
 
   if (resized_dimensions.isEmpty()) {
     FML_LOG(ERROR) << "Could not resize to empty dimensions.";
     return nullptr;
   }
 
-  if (resized_dimensions == image->dimensions()) {
-    // The resized dimesions are the same as the intrinsic dimensions of the
-    // image. There is nothing to do.
-    return image;
+  if (image->dimensions() == resized_dimensions) {
+    return image->makeRasterImage();
   }
 
-  TRACE_EVENT0("flutter", __FUNCTION__);
-  flow.Step(__FUNCTION__);
+  if (resized_dimensions.width() > image->dimensions().width() ||
+      resized_dimensions.height() > image->dimensions().height()) {
+    FML_LOG(WARNING) << "Image is being upsized from "
+                     << image->dimensions().width() << "x"
+                     << image->dimensions().height() << " to "
+                     << resized_dimensions.width() << "x"
+                     << resized_dimensions.height()
+                     << ". Are cache(Height|Width) used correctly?";
+    // TOOD(48885): consider exiting here, there's no good reason to support
+    // upsampling in a "caching"-optimization context..
+  }
 
-  const auto scaled_image_info = image->imageInfo().makeWH(
-      resized_dimensions.width(), resized_dimensions.height());
+  const bool aspect_ratio_changed =
+      std::abs(AspectRatio(resized_dimensions) -
+               AspectRatio(image->dimensions())) > kAspectRatioChangedThreshold;
+  if (aspect_ratio_changed) {
+    // This is probably a bug. If a user passes dimensions that change the
+    // aspect ratio in a "caching" context that's probably not working as
+    // intended and rather a signal that the API is hard to use.
+    FML_LOG(WARNING)
+        << "Aspect ratio changes. Are cache(Height|Width) used correctly?";
+  }
+
+  const auto scaled_image_info =
+      image->imageInfo().makeDimensions(resized_dimensions);
 
   SkBitmap scaled_bitmap;
   if (!scaled_bitmap.tryAllocPixels(scaled_image_info)) {
-    FML_LOG(ERROR) << "Could not allocate bitmap when attempting to scale.";
+    FML_LOG(ERROR) << "Failed to allocate memory for bitmap of size "
+                   << scaled_image_info.computeMinByteSize() << "B";
     return nullptr;
   }
 
@@ -122,31 +150,98 @@ static sk_sp<SkImage> ImageFromDecompressedData(
     return nullptr;
   }
 
-  return ResizeRasterImage(std::move(image), target_width, target_height, flow);
+  if (!target_width && !target_height) {
+    // No resizing requested. Just rasterize the image.
+    return image->makeRasterImage();
+  }
+
+  auto resized_dimensions =
+      GetResizedDimensions(image->dimensions(), target_width, target_height);
+
+  return ResizeRasterImage(std::move(image), resized_dimensions, flow);
 }
 
-static sk_sp<SkImage> ImageFromCompressedData(
-    sk_sp<SkData> data,
-    std::optional<uint32_t> target_width,
-    std::optional<uint32_t> target_height,
-    const fml::tracing::TraceFlow& flow) {
+sk_sp<SkImage> ImageFromCompressedData(sk_sp<SkData> data,
+                                       std::optional<uint32_t> target_width,
+                                       std::optional<uint32_t> target_height,
+                                       const fml::tracing::TraceFlow& flow) {
   TRACE_EVENT0("flutter", __FUNCTION__);
   flow.Step(__FUNCTION__);
 
-  auto decoded_image = SkImage::MakeFromEncoded(data);
+  if (!target_width && !target_height) {
+    // No resizing requested. Just decode & rasterize the image.
+    return SkImage::MakeFromEncoded(data)->makeRasterImage();
+  }
 
-  if (!decoded_image) {
+  auto codec = SkCodec::MakeFromData(data);
+  if (codec == nullptr) {
     return nullptr;
   }
 
-  // Make sure to resolve all lazy images.
-  decoded_image = decoded_image->makeRasterImage();
+  const auto* codec_ptr = codec.get();
 
-  if (!decoded_image) {
+  // Note that we cannot read the dimensions from the codec since they don't
+  // respect image orientation provided e.g. in EXIF data.
+  auto image_generator = SkCodecImageGenerator::MakeFromCodec(std::move(codec));
+  const auto& source_dimensions = image_generator->getInfo().dimensions();
+
+  auto resized_dimensions =
+      GetResizedDimensions(source_dimensions, target_width, target_height);
+
+  // No resize needed.
+  if (resized_dimensions == source_dimensions) {
+    return SkImage::MakeFromEncoded(data)->makeRasterImage();
+  }
+
+  auto decode_dimensions = codec_ptr->getScaledDimensions(
+      std::max(static_cast<double>(resized_dimensions.width()) /
+                   source_dimensions.width(),
+               static_cast<double>(resized_dimensions.height()) /
+                   source_dimensions.height()));
+
+  // If the codec supports efficient sub-pixel decoding, decoded at a resolution
+  // close to the target resolution before resizing.
+  if (decode_dimensions != codec_ptr->dimensions()) {
+    if (source_dimensions != codec_ptr->dimensions()) {
+      decode_dimensions =
+          SkISize::Make(decode_dimensions.height(), decode_dimensions.width());
+    }
+
+    auto scaled_image_info =
+        image_generator->getInfo().makeDimensions(decode_dimensions);
+
+    SkBitmap scaled_bitmap;
+    if (!scaled_bitmap.tryAllocPixels(scaled_image_info)) {
+      FML_LOG(ERROR) << "Failed to allocate memory for bitmap of size "
+                     << scaled_image_info.computeMinByteSize() << "B";
+      return nullptr;
+    }
+
+    const auto& pixmap = scaled_bitmap.pixmap();
+    if (image_generator->getPixels(pixmap.info(), pixmap.writable_addr(),
+                                   pixmap.rowBytes())) {
+      // Marking this as immutable makes the MakeFromBitmap call share
+      // the pixels instead of copying.
+      scaled_bitmap.setImmutable();
+
+      auto decoded_image = SkImage::MakeFromBitmap(scaled_bitmap);
+      FML_DCHECK(decoded_image);
+      if (!decoded_image) {
+        FML_LOG(ERROR)
+            << "Could not create a scaled image from a scaled bitmap.";
+        return nullptr;
+      }
+      return ResizeRasterImage(std::move(decoded_image), resized_dimensions,
+                               flow);
+    }
+  }
+
+  auto image = SkImage::MakeFromEncoded(data);
+  if (!image) {
     return nullptr;
   }
 
-  return ResizeRasterImage(decoded_image, target_width, target_height, flow);
+  return ResizeRasterImage(std::move(image), resized_dimensions, flow);
 }
 
 static SkiaGPUObject<SkImage> UploadRasterImage(
diff --git a/lib/ui/painting/image_decoder.h b/lib/ui/painting/image_decoder.h
@@ -13,6 +13,7 @@
 #include "flutter/fml/concurrent_message_loop.h"
 #include "flutter/fml/macros.h"
 #include "flutter/fml/mapping.h"
+#include "flutter/fml/trace_event.h"
 #include "flutter/lib/ui/io_manager.h"
 #include "third_party/skia/include/core/SkData.h"
 #include "third_party/skia/include/core/SkImage.h"
@@ -68,6 +69,11 @@ class ImageDecoder {
   FML_DISALLOW_COPY_AND_ASSIGN(ImageDecoder);
 };
 
+sk_sp<SkImage> ImageFromCompressedData(sk_sp<SkData> data,
+                                       std::optional<uint32_t> target_width,
+                                       std::optional<uint32_t> target_height,
+                                       const fml::tracing::TraceFlow& flow);
+
 }  // namespace flutter
 
 #endif  // FLUTTER_LIB_UI_PAINTING_IMAGE_DECODER_H_
diff --git a/lib/ui/painting/image_decoder_unittests.cc b/lib/ui/painting/image_decoder_unittests.cc
@@ -519,5 +519,43 @@ TEST(ImageDecoderTest,
   ASSERT_EQ(webp_codec->getRepetitionCount(), 1);
 }
 
+TEST(ImageDecoderTest, VerifySimpleDecoding) {
+  auto data = OpenFixtureAsSkData("Horizontal.jpg");
+  auto image = SkImage::MakeFromEncoded(data);
+  ASSERT_TRUE(image != nullptr);
+  ASSERT_EQ(SkISize::Make(600, 200), image->dimensions());
+
+  ASSERT_EQ(ImageFromCompressedData(data, 6, 2, fml::tracing::TraceFlow(""))
+                ->dimensions(),
+            SkISize::Make(6, 2));
+}
+
+TEST(ImageDecoderTest, VerifySubpixelDecodingPreservesExifOrientation) {
+  auto data = OpenFixtureAsSkData("Horizontal.jpg");
+  auto image = SkImage::MakeFromEncoded(data);
+  ASSERT_TRUE(image != nullptr);
+  ASSERT_EQ(SkISize::Make(600, 200), image->dimensions());
+
+  auto decode = [data](std::optional<uint32_t> target_width,
+                       std::optional<uint32_t> target_height) {
+    return ImageFromCompressedData(data, target_width, target_height,
+                                   fml::tracing::TraceFlow(""));
+  };
+
+  auto expected_data = OpenFixtureAsSkData("Horizontal.png");
+  ASSERT_TRUE(expected_data != nullptr);
+  ASSERT_FALSE(expected_data->isEmpty());
+
+  auto assert_image = [&](auto decoded_image) {
+    ASSERT_EQ(decoded_image->dimensions(), SkISize::Make(300, 100));
+    ASSERT_TRUE(decoded_image->encodeToData(SkEncodedImageFormat::kPNG, 100)
+                    ->equals(expected_data.get()));
+  };
+
+  assert_image(decode(300, 100));
+  assert_image(decode(300, {}));
+  assert_image(decode({}, 100));
+}
+
 }  // namespace testing
 }  // namespace flutter

Original file line number	Diff line number	Diff line change
`@@ -155,6 +155,7 @@ if (current_toolchain == host_toolchain) {`
`155`	`155`	`fixtures = [`
`156`	`156`	`"fixtures/DashInNooglerHat.jpg",`
`157`	`157`	`"fixtures/Horizontal.jpg",`
	`158`	`+ "fixtures/Horizontal.png",`
`158`	`159`	`"fixtures/hello_loop_2.gif",`
`159`	`160`	`"fixtures/hello_loop_2.webp",`
`160`	`161`	`]`