Add support for image loading progress by tvolkert · Pull Request #32465 · flutter/flutter

tvolkert · 2019-05-10T06:25:03Z

Description

Add support for image loading progress.

This PR adds a imageLoadingBuilder property to the Image class. This is useful for images that are incrementally loaded (e.g. over a local file system or a network) and you want to give the user an indication of when the image will be displayed.

Related Issues

Fixes #32374

Tests

I added the following tests:

consolidateHttpClientResponseBytes notifies onBytesReceived for every chunk of bytes
NetworkImage notifies listeners of chunk events
ImageStreamCompleter delivers chunk events to listeners
ImageStreamCompleter drops chunks events on the floor (does not buffer the events) if there are no listeners
ImageStreamCompleter reports errors in the chunk event stream
Image invokes imageLoadingBuilder as the image loads
Image ignores chunk events if imageLoadingBuilder is null

Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

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.

Fixes #32374

gspencergoog · 2019-05-14T00:02:20Z

+/// The `total` parameter will contain the _expected_ total number of bytes to
+/// be received (extracted from the value of the `Content-Length` HTTP response
+/// header), or -1 if the size of the response body is not known in advance.
+/// Even if the parameter reports a non-negative value, it is not 100%


The Content-Length is the size of the transferred data, whether or not it is compressed, at least according to the spec. So this should be a good proxy for the progress, and I'm not sure that the comment is needed. Do ask @Hixie, however, because he probably knows if the spec is implemented properly by most servers.

I had added this comment to fix a bug previously.

Some servers in the wild won't set this properly. And ones that do set it properly will give you the compressed size, but dart:io automagically decompresses for you without telling you whether it's compressed. At one point I had patched this so that it would try to be smart and detect that, but that was abandoned since the header may not even be reliable.

It may be worth documenting here that total could be less than cumulative when GZIP is used.

Do you think we could use the content-type header to determine if the response is one of our supported image formats or a likely compression type?

I updated the code to better handle gzipped content. Now, consolidateHttpClientResponseBytes() will unzip the raw bytes itself if needed, which allowed us to turn off autoUncompress in the http client used by NetworkImage.

I also updated the docs.

gspencergoog · 2019-05-14T00:10:54Z

 import 'mocks_for_image_cache.dart';

+/// A valid encoding of a 1x1 GIF.
+final Uint8List _smallGif = Uint8List.fromList(<int>[


Don't add your own, just use one from image_data.dart

dnfield · 2019-05-14T17:35:20Z

+  // to the stream, so we manually count the bytes as they cross the wire.
  final Completer<Uint8List> completer = Completer<Uint8List>.sync();
  final List<List<int>> chunks = <List<int>>[];
+  final int expectedContentLength = response.contentLength;


This will almost always be less than the actual content length - your progress bar will end up loading faster because the contentLength will usually be the gzip compressed length. You should be able to notice this if you test a network download of this with something from Wikimedia for example.

the bytes manually, rather than relying on HttpClient.autoUncompress. Correspondingly, change NetworkImage's HttpClient to turn off autoUncompress, so as to get a more valid reading of the expected content length. (also, some minor doc updates)

tvolkert · 2019-05-15T02:08:27Z

@gspencergoog @dnfield PTAL

gspencergoog · 2019-05-15T02:45:40Z

+/// progress.
+///
+/// This is useful for images that are incrementally loaded (e.g. over a local
+/// file system or a network) and you want to give the user an indication of


Avoid 'you'.

I can change it, but pourquoi (I see 152 files in flutter/lib and flutter_test/lib with "you" in the comment)?

Either way, done 😄

gspencergoog · 2019-05-15T02:46:23Z

+  ///
+  /// Once an image has fully loaded, this builder will no longer be consulted.
+  ///
+  /// {@tool sample}


You could make this a snippet pretty easily...

Co-Authored-By: Greg Spencer <[email protected]>

dnfield

LGTM

… optional named parameter. It turns out a lot of code outside Flutter is using that method, so it needs to be a non-breaking change to the signature.

Hixie · 2019-05-15T18:31:42Z

+/// The `total` parameter will contain the _expected_ total number of bytes to
+/// be received across the wire (extracted from the value of the
+/// `Content-Length` HTTP response header), or -1 if the size of the response
+/// body is not known in advance.


not known in advance (this is common for HTTP chunked transfer encoding, which itself is common when dealing with CGI scripts on HTTP servers) (or something. "CGI script" is 90s terminology, I don't know what the cool kids are calling it these days.)

Hixie · 2019-05-15T18:32:36Z

+/// Efficiently converts the response body of an [HttpClientResponse] into a
+/// [Uint8List].
 ///
 /// The future returned will forward all errors emitted by [response].


it can only forward one error... maybe s/all errors/any error/?

Hixie · 2019-05-15T18:33:14Z

-  // to the stream.
+///
+/// The [onBytesReceived] callback, if specified, will be invoked for every
+/// chunk of bytes that are received while consolidating the response bytes.


Hixie · 2019-05-15T19:14:26Z

+      expectedContentLength = -1;
+    } else {
+      // We need to un-compress the bytes as they come in.
+      sink = gzip.decoder.startChunkedConversion(output);


why do we want to override autoUncompress?

A search yielded no usages of people calling this method with a client that has autoUncompress off, so we have a chance to define the behavior, so I just defined it as "this method will always return uncompressed bytes". The use case is setting the client to not auto-uncompress so that we can get an accurate (or at least, more likely to be accurate) accounting of the expected content length. When the client has auto-uncompressed, we have no chance of knowing the expected content length.

An alternative would be to add a new autoUncompress parameter to this method so callers could control the behavior (it'd be a no-op if the client had already uncompressed). But since it looked like no one was using this with clients that kept the response bytes compressed, it seemed simpler to just define the method as always uncompressing.

How do you download a gzip with this API?

heh fair enough - I'll add a parameter 😄

Hixie · 2019-05-15T19:17:18Z

+  final _OutputBuffer output = _OutputBuffer();
+  ByteConversionSink sink = output;
+  int expectedContentLength = response.contentLength;
+  if (response.headers?.value(HttpHeaders.contentEncodingHeader) == 'gzip') {


what if it's "identity, gzip"?

I wondered about making this more robust, but I opted for copying the exact behavior from HttpClient so as to not add confusion as to how we determine that unzipping is necessary.

Please at a minimum file a bug and add a TODO

According to the spec, "identity" is an invalid value in the Content-Encoding header.

identity The default (identity) encoding; the use of no transformation whatsoever. This content-coding is used only in the Accept- Encoding header, and SHOULD NOT be used in the Content-Encoding header.

So I think the current impl here is correct.

Hixie · 2019-05-15T19:18:12Z

+    sink.add(chunk);
+    if (onBytesReceived != null) {
+      bytesReceived += chunk.length;
+      onBytesReceived(bytesReceived, expectedContentLength);


should we document that exceptions from this callback will be uncaught and will cause the future never to complete?

(actually i guess they will be caught by listen?)

i guess since i'm confused about this, we should document it, whatever the behaviour is.

Hixie · 2019-05-15T19:19:36Z

+/// mock client that hasn't been affected by other tests.
+///
+/// This value is ignored in non-debug builds.
+@visibleForTesting


why is this one @visibleForTesting but not the others?

Seems like it would be reasonable for this one to be usable from non-test debug code too?

I wondered about that - I couldn't think of a concrete use case during debugging. But happy to remove the annotation - will do.

Hixie · 2019-05-15T19:22:24Z

+///  * [Image.imageLoadingBuilder], which allows callers to visually represent
+///    the loading progress of an image at the widget layer.
+class ImageChunkEvent extends Diagnosticable {
+  /// Creates a new chunk event.


can the arguments be null?

Hixie · 2019-05-15T19:22:43Z

+  /// This value is not necessarily equal to the expected _size_ of the image
+  /// in bytes, as the bytes required to load the image may be compressed.
+  ///
+  /// This value will be -1 if the number is not known in advance. When this is


why -1 rather than null?

'cause it's a passthrough from the response.contentLength property, which uses -1. I could certainly see the argument for making this be null - wdyt?

ah if it's just passing through an existing API, sounds good to me.

if you haven't already, please reference the api you're forwarding in the docs

Hixie · 2019-05-15T19:23:35Z

+  ///
+  /// When used with [Image.network], this value comes from the `Content-Length`
+  /// HTTP response header, and as such is only as trustworthy as the server
+  /// that reported it. Callers are correspondingly advised to be defensive


Content-Length is always accurate, if present. It has to be, there's no other signal of the end of the resource.

Hixie · 2019-05-15T19:28:46Z

  /// was added, even if it was added with a different `onError` than the
  /// intended paired `addListener` call.
-  void addListener(ImageListener listener, { ImageErrorListener onError }) {
+  void addListener(


this api is already broken and this makes it more broken. i don't think we should go down this road.

See the docs for removeListener, for example. We'd have to update those if we went this path, and I don't think I could do that without shame.

Hixie · 2019-05-15T19:31:14Z

      filterQuality: widget.filterQuality,
    );
+    if (widget.imageLoadingBuilder != null && _chunkEvent != null)
+      result = widget.imageLoadingBuilder(context, _chunkEvent, _imageInfo == null ? null : result);


we shouldn't create the RawImage for the cases where we don't use it

Hixie · 2019-05-15T19:34:12Z

-      _imageStream.addListener(_handleImageChanged);
+      _imageStream.addListener(
+        _handleImageChanged,
+        chunkListener: widget.imageLoadingBuilder != null ? _handleImageChunkLoaded : null,


as far as i can tell you don't handle widget.imageLoadingBuilder changing over time

Hixie · 2019-05-15T19:34:59Z

+  void _handleImageChunkLoaded(ImageChunkEvent event) {
+    assert(widget.imageLoadingBuilder != null);
+    setState(() {
+      _chunkEvent = event;


this is REALLY expensive. It forces a rebuild for every frame even if the builder doesn't actually return anything that is affected by this information.

But why provide a builder at all if you're not affected by this information? This information is the essence of why you provided a builder. And yes, it'll mean that you rebuild every time bytes come across the wire while the image is loading, which is likely every frame.

you may wish to show an indeterminate progress indicator while downloading. Or the word "LOADING". Or alternative text in an inline widget.

Hixie · 2019-05-15T19:36:41Z

+    HttpClient client = _sharedHttpClient;
+    assert(() {
+      if (debugNetworkImageUseFreshHttpClient)
+        client = HttpClient();


you don't set autoUncompress to false on this one

Hixie · 2019-05-15T19:37:12Z

-    if (response.statusCode != HttpStatus.ok)
-      throw Exception('HTTP request failed, statusCode: ${response?.statusCode}, $resolved');
+  /// Do not access this field directly; use [_httpClient] instead.
+  static final HttpClient _sharedHttpClient = HttpClient()..autoUncompress = false;


you turn off autoUncompress here, but you only support gzip on the other side. HTTP servers can use a variety of compression algorithms other than gzip.

(You should make sure to set the TE header to whatever it is you support. Also, consider Accept-Encoding and Content-Encoding, which are different than TE and Transfer-Encoding.)

HttpClient documents this property as only respecting gzip -- https://api.dartlang.org/stable/2.3.0/dart-io/HttpClient/autoUncompress.html

Hixie · 2019-05-15T23:29:10Z

Let's see if we can make FadeInImage a client of this.

tvolkert · 2019-05-16T23:53:58Z

I split the consolidateHttpClientResponseBytes() changes off into a separate PR - #32853 since they're isolated.

tvolkert · 2019-05-18T01:37:39Z

This is being broken off into multiple sub-PRs. Here are the ones that have been sent for review thus far:

goderbauer · 2019-05-20T17:44:04Z

Should this PR be closed since it split off into multiple new PRs?

tvolkert · 2019-05-20T18:19:36Z

Yep, done.

googlebot added the cla: yes label May 10, 2019

tvolkert added work in progress; do not review and removed cla: yes labels May 10, 2019

googlebot added the cla: yes label May 10, 2019

tvolkert mentioned this pull request May 10, 2019

Ability to display loading progress of an image #32374

Closed

goderbauer added the framework flutter/packages/flutter repository. See also f: labels. label May 10, 2019

tvolkert changed the title ~~WIP: proof of concept image loading progress notification~~ Add support for image loading progress May 11, 2019

Add support for image loading progress

522f805

Fixes #32374

gspencergoog reviewed May 14, 2019

View reviewed changes

tvolkert added 2 commits May 13, 2019 17:41

Review comments

e1af594

Finish adding tests

604f1cf

tvolkert removed the work in progress; do not review label May 14, 2019

tvolkert requested review from Hixie and goderbauer May 14, 2019 17:22

dnfield reviewed May 14, 2019

View reviewed changes

gspencergoog reviewed May 15, 2019

View reviewed changes

tvolkert and others added 2 commits May 14, 2019 20:16

Apply suggestions from code review

3e59ef0

Co-Authored-By: Greg Spencer <[email protected]>

Convert sample to snippet

b5724c4

dnfield approved these changes May 15, 2019

View reviewed changes

tvolkert added 3 commits May 14, 2019 21:46

Review comments

289de3d

Changed client param to consolidateHttpClientResponseBytes to be an…

50db71c

… optional named parameter. It turns out a lot of code outside Flutter is using that method, so it needs to be a non-breaking change to the signature.

Self-review

f0752af

Hixie reviewed May 15, 2019

View reviewed changes

Comment thread packages/flutter/lib/src/foundation/consolidate_response.dart

Hixie reviewed May 15, 2019

View reviewed changes

Comment thread packages/flutter/lib/src/foundation/consolidate_response.dart

Hixie reviewed May 15, 2019

View reviewed changes

tvolkert closed this May 20, 2019

tvolkert deleted the images branch May 21, 2019 00:57

github-actions Bot locked as resolved and limited conversation to collaborators Aug 6, 2021

Conversation

tvolkert commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert commented May 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tvolkert commented May 10, 2019 •

edited

Loading

tvolkert May 15, 2019 •

edited

Loading