Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Expose API that allows for network resizing images #31164

Closed

Conversation

Projects
None yet
6 participants
@iskakaushik
Copy link
Contributor

commented Apr 16, 2019

Description

This PR creates a new image provider ResizedImage that allows the user to specify the dimensions of the image that the engine can then use to resize to. This is the engine side PR accounting for the same changes: flutter/engine#8596

Related Issues

#26194

Tests

NEEDS TESTS!!

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read [Handling breaking changes]). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.
Kaushik Iska
Kaushik Iska

@iskakaushik iskakaushik force-pushed the iskakaushik:expose-resizing-image-api branch from d7034ad to 6330ffc Apr 17, 2019

final ByteData data = await key.bundle.load(key.name);
if (data == null)
throw 'Unable to read data';
return await PaintingBinding.instance.instantiateImageCodec(data.buffer.asUint8List());
return await PaintingBinding.instance.instantiateImageCodec(
data.buffer.asUint8List(), size: size);

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Apr 17, 2019

Contributor

One line


@override
Future<_SizeAwareCacheKey> obtainKey(
ImageConfiguration configuration) async {

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Apr 17, 2019

Contributor

The ImageConfiguration already has the size information, we should find a way to use that

/// set for resizing to take effect. This parameter indicates to the engine
/// that this image must be resized. The image will be sized to the constraints
/// of the layout or [width] and [height] regardless of this parameter. This
/// parameter is primarily intended for use to reduce the memory usage of

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Apr 17, 2019

Contributor
Suggested change
/// parameter is primarily intended for use to reduce the memory usage of
/// parameter is primarily intended to reduce the memory usage of
/// that this image must be resized. The image will be sized to the constraints
/// of the layout or [width] and [height] regardless of this parameter. This
/// parameter is primarily intended for use to reduce the memory usage of
/// [ImageCache].

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Apr 17, 2019

Contributor

We should gather some numbers to confirm this is true.

///
/// * [Image.network] for example usage when `resizeToFit` parameter is set.
class ResizedImage
extends ImageProvider<_SizeAwareCacheKey> {

This comment has been minimized.

Copy link
@Hixie

Hixie Apr 24, 2019

Contributor

nit: no need for newline here

@@ -215,7 +239,15 @@ class Image extends StatefulWidget {
this.gaplessPlayback = false,
this.filterQuality = FilterQuality.low,
Map<String, String> headers,
}) : image = NetworkImage(src, scale: scale, headers: headers),
bool resizeToFit = false,
}) : image = _resizeIfNeeded(

This comment has been minimized.

Copy link
@Hixie

Hixie Apr 24, 2019

Contributor

why do we do this for network image but not asset image or memory image?

This comment has been minimized.

Copy link
@iskakaushik

iskakaushik Apr 29, 2019

Author Contributor

@Hixie , i wanted to get feedback on this _resizeIfNeeded pattern before i go ahead and change the other ones.

This comment has been minimized.

Copy link
@Hixie

Hixie Apr 29, 2019

Contributor

I don't really understand the pattern as written. It seems very network-specific. I'm having trouble understanding how it generalizes.

@goderbauer

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@iskakaushik are you still working on this one or should we close it?

@iskakaushik

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@iskakaushik are you still working on this one or should we close it?

@goderbauer yes. I am waiting on the engine side to reach a good state first. Please do not close it.

Kaushik Iska added some commits May 3, 2019

@iskakaushik iskakaushik force-pushed the iskakaushik:expose-resizing-image-api branch from a7bf846 to 67aee54 May 3, 2019

@iskakaushik iskakaushik force-pushed the iskakaushik:expose-resizing-image-api branch from 67aee54 to 8690b95 May 3, 2019

Kaushik Iska
@jonahwilliams

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

We should be able to resize any image with this, but FileImage would be another usecase outlined in the original doc:

  • Take high resolution picture with phone camera
  • Save to filesystem
  • Load preview image in flutter app that is much smaller
@iskakaushik

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@jonahwilliams I added support for memory, file and network images with my latest changes. Also added some tests -- though these only test memory images for now. I will add some more to account for network and memory images.

Kaushik Iska
@goderbauer

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@iskakaushik Do you still have plans for this PR? It is getting a little stale. :)

@iskakaushik

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

closing this for now. will reopen after adding some benchmarks for current image decoding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.