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

[ignatz]

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.

[mehmetf]

@chinmaygarde This is re: memory pressure that customer:dream is experiencing so they want us to review it asap. Thanks!

[dnfield]

We should add some tests for this, particularly around where the codec can support efficient sub-sampling and what happens when we request different aspect ratios (whether we decide to preserve aspect ratio or not).

[dnfield]

The infra failures on this patch are not related to changes here.

@godofredoc have we added mac try builders? If so we have to configure them to have the right credentials to access infra_internal in CIPD.

[dnfield]

Ah sorry, looks like the permissions issue got fixed since this PR was added. It should succeed on a rerun/new commit.

[godofredoc]

The permission problems were fixed this morning at ~12:00PM PST. If you retry the mac builders should not fail the installation of xcode.

[ignatz]

We should add some tests for this, particularly around where the codec can support efficient sub-sampling and what happens when we request different aspect ratios (whether we decide to preserve aspect ratio or not).

Do you have a pointer to pre-existing tests and whether they can already provide coverage? Right now, the change is a semantic noop. On a tangent, writing unit-tests is also non-trivial since the functions in questions have internal linkage.

[dnfield]

The tests for this are in lib/ui/painting/image_decoder_unittests.cc

We should add a test to that which would have failed before this change.

There are a few ways we could make this code a bit more testable. One is to move all of the resize logic into some other class that we properly test, and pass in an instance of it to ImageDecoder. This would mean getting rid of all the private statics on the class. Another would be to make some of those private statics public (or at least friends of the test) so that we could more directly test them.

I would lean towards refactoring at least some of the private static logic into another object that can just be given to the ImageDecoder, but I'm certainly open to other ideas :)

[ignatz]

engine/lib/ui/painting/image_decoder_unittests.cc

Line 317 in 5ef2c79

TEST_F(ImageDecoderFixtureTest, CanDecodeWithResizes) {

seems to cover the respective code path, i.e. makes sure that decoding works.

We should add a test to that which would have failed before this change.

I'm taking suggestions. My implementation deliberately doesn't change the observable behavior other than saving resources. W/o e.g. a mock skia backend that let's us assert backend API calls, it's not clear to me how we would test it. Maybe you know a SkImage API to test if a SkImage_Lazy implementation is expanded?

[dnfield]

I think it would make sense to just test that our call to ResizeImage came in with an image decoder, and that the implementation tried to use that decoder to resize the image. We could refactor the static method you're updating so that it's no static and it's testable for that, right?

@chinmaygarde may have better ideas.

[chinmaygarde]

This optimization was present in the engine for a hot minute before we realized that SkCodec::MakeFromData does not return dimensions after account for EXIF orientation while SkImage::MakeFromEncoded does apply EXIF related rotation (introduced in ad582b5#diff-5c1f6e0e5e35e3a93e44256d86f61627R167 and reverted in fd2cb81#diff-5c1f6e0e5e35e3a93e44256d86f61627R136). It looks to me that the same issue is present here as well in that the decoded_image will return dimensions with EXIF rotation information applied while the codec will not. TBH, I am finding it a bit hard to reason about what happens when something like fixtures/Horizontal.jpg is scaled down. To make this clearer and easier to test, we could modify ImageDecoder::ImageResult to return more information about the intermediates during image decompression. That would make it easier to write unit-tests for this in image_decode_unittests.cc.

Another thing that initially tripped me up was the fact that this patch relies on SkImage::MakeFromEncoded relying on a lazy image being created. This seems like an implementation detail in Skia that makes the code here a bit hard to follow. A suggestion would be to create the codec once, apply any EXIF related orientation corrections, resize down to a size supported by the codec and then perform the final resize to the correct size if necessary. In this model, each step is clear and the overall rubric easier to follow.

[ignatz]

This optimization was present in the engine for a hot minute before we realized that SkCodec::MakeFromData does not return dimensions after account for EXIF orientation while SkImage::MakeFromEncoded does apply EXIF related rotation (introduced in ad582b5#diff-5c1f6e0e5e35e3a93e44256d86f61627R167 and reverted in fd2cb81#diff-5c1f6e0e5e35e3a93e44256d86f61627R136). It looks to me that the same issue is present here as well in that the decoded_image will return dimensions with EXIF rotation information applied while the codec will not.

Good call. You're right that the codec doesn't respect the orientation. I'm now using a slightly higher level abstraction: ImageGenerator, which does support orientation. Note that ImageGenerator is in skia's sources so might be considered a private API. Unfortunately, there's really no other API to use, i.e. images don't support efficient sub-pixel decoding. That said, I'm reasonably confident that we can talk to skia folks especially since SkCodecImageGenerator::MakeFromEncodedCodec is already exposed.

I added a unit-test to ensure we respect exif orientation.

TBH, I am finding it a bit hard to reason about what happens when something like fixtures/Horizontal.jpg is scaled down. To make this clearer and easier to test, we could modify ImageDecoder::ImageResult to return more information about the intermediates during image decompression. That would make it easier to write unit-tests for this in image_decode_unittests.cc.

I moved the decoding bits now out of ResizeRasterImage and into ImageFromCompressedData, I think this separates the concerns more clearly and makes it easier to digest.

Another thing that initially tripped me up was the fact that this patch relies on SkImage::MakeFromEncoded relying on a lazy image being created. This seems like an implementation detail in Skia that makes the code here a bit hard to follow.

SkImage::MakeFromEncoded's documentation states that it is best-effort lazy. Either way, I tried to avoid it in my rewrite.

A suggestion would be to create the codec once, apply any EXIF related orientation corrections, resize down to a size supported by the codec and then perform the final resize to the correct size if necessary. In this model, each step is clear and the overall rubric easier to follow.

This should be the case now.

[chinmaygarde]

A few nits but this is looking good.