Skip to content

ImageInfo adds a new getter named sizeBytes to decouple ImageCache and ui.Image #86555

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

Merged
merged 3 commits into from
Aug 18, 2021

Conversation

ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Jul 16, 2021

We used external texture to display the images in our flutter app. On the Android platform, we draw the Bitmap onto the SurfaceTexture, and on the iOS platform, we convert the UIImage to CVPixelBufferRef, and finally use the Texture Widget to display the image.
We want to let the external texture can benefit from the ImageCache logic.We hope that ImageCache can no longer dependent on ui.Image, so that we can customize ImageInfo and ImageProvier to cache external texture images in ImageCache.

List which issues are fixed by this PR. You must list at least one issue.
Fixes #86402

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Sorry, something went wrong.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Jul 16, 2021
@google-cla google-cla bot added the cla: yes label Jul 16, 2021
@ColdPaleLight ColdPaleLight force-pushed the adds_size_bytes branch 2 times, most recently from 19f45ef to ca0ca43 Compare July 16, 2021 14:16
@wongkoo
Copy link

wongkoo commented Jul 19, 2021

👏🏿 This small change can bring a lot of scalability benefits, awesome

@ColdPaleLight
Copy link
Member Author

Hi,@dnfield
Would you mind reviewing this PR?
Thanks

@goderbauer
Copy link
Member

Can you please add a test to this PR?

@goderbauer goderbauer requested a review from dnfield July 20, 2021 21:25
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a test.

It's also not entirely clear to me what the use case is here. ImageInfo must have an image, which must have a width and height.

It would probably be better to expose sizeBytes on dart:ui.Image instead.

@ColdPaleLight
Copy link
Member Author

ColdPaleLight commented Jul 21, 2021

Hi, @dnfield
I have added the test.
The following is the reason why I hope this PR can be merged

Background

We used external texture to display the images in our flutter app. On the Android platform, we draw the Bitmap onto the SurfaceTexture, and on the iOS platform, we convert the UIImage to CVPixelBufferRef, and finally use the Texture Widget to display the image. We do this mainly based on the following considerations:

  1. We hope that any Bitmap or UIImage can be efficiently displayed to flutter app. For example, we need to customize a photo list page, but the native platform API only provide us Bitmap or UIImage as photo.
  2. Reuse the native platform image loader. the native image loader, such as Glide on the Android platform and SDWebImage on the iOS platform, have implemented a complete set of caching solutions (memory cache, disk cache) and image decoding solutions. In addition, our CDN cropping optimization is also implemented in the native image loader. We hope that flutter app can directly reuse these capabilities.
  3. Compared with ui.Image, the memory of external texture is not managed by GC, we can precisely control the timing of memory release

Detail

Use Texture Widget to display the image, we only need textureId, width, height, so we have customized a TextureImageInfo to represent this image. but ImageInfo must have an ui.Image,so we had to give ImageInfo a dummy

ui.Image _dummy;

FutureOr<TextureImageInfo> createTextureImageInfo(
    {int textureId, int width, int height}) async {
  if (_dummy != null) {
    return TextureImageInfo(
        textureId: textureId, width: width, height: height, image: _dummy);
  }

  _dummy = await _createImage(1, 1);
  return TextureImageInfo(
      textureId: textureId, width: width, height: height, image: _dummy);
}

Future<ui.Image> _createImage(int width, int height) async {
  final Completer<ui.Image> completer = Completer<ui.Image>();
  ui.decodeImageFromPixels(
    Uint8List.fromList(
        List<int>.filled(width * height * 4, 0, growable: false)),
    width,
    height,
    ui.PixelFormat.rgba8888,
        (ui.Image image) {
      completer.complete(image);
    },
  );
  return completer.future;
}

// ignore: must_be_immutable
class TextureImageInfo extends ImageInfo {
  TextureImageInfo({
    this.textureId,
    this.width,
    this.height,
    ui.Image image,
    double scale = 1.0,
    String debugLabel,
  }) : super(
    image: image,
    scale: scale,
    debugLabel: debugLabel,
  );

  int textureId;

  int width;

  int height;

  int get sizeBytes => height *  width * 4;

  ......

}

Now, we can create TextureImageInfo in the following way

TextureImageInfo imageInfo = await createTextureImageInfo(textureId:textureId,width:width,height:height)

Then we customized an ImageProvider to provide TextureImageInfo,and copy the code of ImageCache and Image Widget and modify their dependence on ui.Image. It works well.

Expectation

At present, our solution is create a ImageCacheExt, let it extends ImageCache, then copy all the code of ImageCache to ImageCacheExt and modify it. The reason is that the image in our TextureImageInfo is dummy, so the correct sizeBytes cannot be calculated. If ImageInfo has a sizeBytes method, we can reuse ImageCache directly.

Questions

By the way,would you mind answering the following two questions:

  1. Does Flutter have plans to provide a more efficient way to display Bitmap or UIImage from any source in the future?
  2. Does Flutter have plans to extend ImageCache to manage other types of images, such as the texture type mentioned above?

Thank you

@ColdPaleLight ColdPaleLight force-pushed the adds_size_bytes branch 2 times, most recently from f6b3159 to 7bd1a5e Compare July 21, 2021 10:39
@ColdPaleLight ColdPaleLight requested a review from dnfield July 21, 2021 11:17
@dnfield
Copy link
Contributor

dnfield commented Jul 21, 2021

Supporting texture based images is an interesting idea. There's also a bug open for supporting other graphics related things in the cache - #83801

Let me think about this one a little bit.

@dnfield
Copy link
Contributor

dnfield commented Jul 21, 2021

Can you explain a bit more about how you're getting the size of the texture?

@ColdPaleLight
Copy link
Member Author

ColdPaleLight commented Jul 22, 2021

Hi, @dnfield

Usually we use the size of Bitmap/CGImage as the size of the texture, but sometimes we can also specify the size of the texture

Android:

Bitmap bitmap = getBitmapFromSomeWhere();

int textureWidth = bitmap.width();
int textureHeight = bitmap.height();
// we can also specify the size of the texture like this:
// int textureWidth = expectTextureWidthFromSomeWhere;
// int textureHeight = expectTextureHeightFromSomeWhere;

TextureRegistry.SurfaceTextureEntry entry =  textureRegistry.createSurfaceTexture();
Surface surface = new Surface(textureEntry.surfaceTexture());
textureEntry.surfaceTexture().setDefaultBufferSize(textureWidth, textureHeight);
if (surface != null && surface.isValid()) {
    try {
        Canvas canvas = surface.lockCanvas(null);
        Rect mSrcRect = new Rect(0, 0, bitmap.getWidth(), bitmap.getHeight());
        Rect mDestRect = new Rect(0, 0, textureWidth, textureHeight);
        canvas.drawBitmap(imageBitmap, mSrcRect, mDestRect, null);
        surface.unlockCanvasAndPost(canvas);
    } catch (Exception e) {
        e.printStackTrace();
        onLoadFailed(imageBitmap, needRecycle);
    }
}

Map<String,Object> result = new HashMap();
result.put("textureId", textureEntry.id());
result.put("width", textureWidth);
result.put("height", textureHeight);

// then send result to flutter

The logic of iOS is basically the same as Android,we convert CGImage to CVPixelBuffer,and size of the texture is the size of the CVPixelBuffer.

- (CVPixelBufferRef)createPixelBufferFromImage:(CGImageRef)image andSize:(CGSize)size {
    NSDictionary *options = @{
                              (__bridge NSString *)kCVPixelBufferIOSurfacePropertiesKey : @{},
                              (__bridge NSString *)kCVPixelBufferCGImageCompatibilityKey: @NO,
                              (__bridge NSString *)kCVPixelBufferCGBitmapContextCompatibilityKey: @NO
                              };
    
    CVPixelBufferRef pxbuffer = NULL;
    
    // imageSize is the size of the texture 
    CGSize imageSize = size;
    if (CGSizeEqualToSize(size, CGSizeZero)) {
        imageSize = CGSizeMake(CGImageGetWidth(image),  CGImageGetHeight(image));
    }
    
    CVReturn status = CVPixelBufferCreate(kCFAllocatorDefault,
                                          imageSize.width,
                                          imageSize.height,
                                          kCVPixelFormatType_32BGRA,
                                          (__bridge CFDictionaryRef) options,
                                          &pxbuffer);
    if (status!=kCVReturnSuccess) {
        NSLog(@"Operation failed");
    }
    NSParameterAssert(status == kCVReturnSuccess && pxbuffer != NULL);
    
    CVPixelBufferLockBaseAddress(pxbuffer, 0);
    void *pxdata = CVPixelBufferGetBaseAddress(pxbuffer);
    
    CGColorSpaceRef rgbColorSpace = CGColorSpaceCreateDeviceRGB();
    CGContextRef context = CGBitmapContextCreate(pxdata, imageSize.width,
                                                 imageSize.height, 8, CVPixelBufferGetBytesPerRow(pxbuffer), rgbColorSpace,
                                                 kCGBitmapByteOrder32Little | kCGImageAlphaPremultipliedFirst);
    
    
    NSParameterAssert(context);
    
    CGContextDrawImage(context, CGRectMake(0, 0, imageSize.width,
                                           imageSize.height), image);
    CGColorSpaceRelease(rgbColorSpace);
    CGContextRelease(context);
    
    CVPixelBufferUnlockBaseAddress(pxbuffer, 0);
    return pxbuffer;
}

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I can't think of a great reason this API is harmful, and subclassing ui.Image could do bad things.

@Piinks Piinks added a: images Loading, displaying, rendering images c: new feature Nothing broken; request for a new capability labels Aug 17, 2021
@skia-gold
Copy link

Gold has detected about 4 new digest(s) on patchset 3.
View them at https://flutter-gold.skia.org/cl/github/86555

@goderbauer
Copy link
Member

@ColdPaleLight Can you rebase this PR to make the checks pass?

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux web_long_running_tests_3_5 has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite analyze-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

@google-cla
Copy link

google-cla bot commented Aug 18, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Aug 18, 2021
@ColdPaleLight
Copy link
Member Author

@googlebot I consent.

@google-cla
Copy link

google-cla bot commented Aug 18, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@ColdPaleLight
Copy link
Member Author

@dnfield @goderbauer All checks have passed. Can you add the label "waiting for tree to go green" again?

@dnfield dnfield merged commit 33f5ac6 into flutter:master Aug 18, 2021
blasten pushed a commit to blasten/flutter that referenced this pull request Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: images Loading, displaying, rendering images c: new feature Nothing broken; request for a new capability customer: alibaba framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend ImageCache to handle Custom ImageInfo
7 participants