Fix ImageReader may leak images when onDraw() not called

[eggfly]

Description

The onDraw() may not called after invalidate(), it makes some flash or flickering of flutter elements. This can be reproduced on some Android devices. (For example: HUAWEI P30 or HUAWEI Mate 30 PRO.)

And as the screen recording below:

Screenrecorder-2021-02-07-18-19-48-960.mp4

I made a demo code in main.dart:

import 'dart:io';

import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';
import 'package:webview_flutter/webview_flutter.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
        visualDensity: VisualDensity.adaptivePlatformDensity,
      ),
      home: MyHomePage(),
    );
  }
}

class MyHomePage extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text('HomePage'),
      ),
      body: Column(
        children: [
          _button(context),
        ],
      ),
    );
  }
}

Widget _button(BuildContext context) {
  return RaisedButton(
    child: Text('WebView'),
    onPressed: () {
      Navigator.of(context).push(CupertinoPageRoute(
        builder: (context) {
          return WebViewPage();
        },
      ));
    },
  );
}

class WebViewPage extends StatefulWidget {
  @override
  State<StatefulWidget> createState() {
    return WebViewPageState();
  }
}

class WebViewPageState extends State<WebViewPage> {
  @override
  void initState() {
    super.initState();
    if (Platform.isAndroid) WebView.platform = SurfaceAndroidWebView();
  }

  @override
  Widget build(BuildContext context) {
    return Material(
      child: SafeArea(
        child: Stack(
          children: [
            WebView(
              initialUrl: 'https://github.com',
              javascriptMode: JavascriptMode.unrestricted,
            ),
            _button(context),
            // Positioned(
            //   child: _button(context),
            //   top: 20,
            //   left: 20,
            // ),
          ],
        ),
      ),
    );
  }
}

and using webview_flutter 1.0.7 version in pubspec.yaml:

dependencies:
  flutter:
    sdk: flutter
  webview_flutter: 1.0.7

And also there's a log: "unable to acquire a buffer item, very likely client tried to acquire more than maximages buffers".

The imageQueue in old implementation may leaves two or more items after acquireLatestImage() and occure the native level log:
"unable to acquire a buffer item, very likely client tried to acquire more than maximages buffers" in https://android.googlesource.com/platform/frameworks/base/+/master/media/jni/android_media_ImageReader.cpp#517

ROOT CAUSE

Because there's no guarantee that onDraw() must be called after invalidate() when device sleeping or on some special devices.

Not all devices are reproduced. So we can add a modification to the flutter engine to simulate this problem on most Android phones.

shell/platform/android/io/flutter/embedding/android/FlutterImageView.java:

@@ -227,8 +222,17 @@ public class FlutterImageView extends View implements RenderSurface {
     pendingImages = 0;
   }
 
+  private int onDrawCounter = 0;
+
   @Override
   protected void onDraw(Canvas canvas) {
+    onDrawCounter++;
+    if (onDrawCounter > 10) {
+      onDrawCounter = 0;
+      Log.e("FlutterImageView", "mock onDraw() calling lost");
+      // mock onDraw() calling lost
+      return;
+    }
     super.onDraw(canvas);
 
     if (!imageQueue.isEmpty()) {

This can simulate the missing call of onDraw() once every 10 times. And the flashing or widgets can be observed as well as video above.

Solution

So after all, I made this change and I also think ImageReader related logics can be simplified:

• The new code closes current image every time when imageReader.acquireLatestImage() returns a new image, and no need to record a queue and no need to check imageReader.getMaxImages()

It fixes:
flutter/flutter#63897
flutter/flutter#70290

Test

The test code:

• Remove the test flutterImageView_onDrawClosesAllImages() with old logic.
• Merged acquiresMaxImagesAtMost and acquireLatestImageReturnsFalse() to the test acquiresImageClosesPreviousImageUnlessNoNewImage, it can cover mockReader.acquireLatestImage() returns null making the method returns false when no latest image, and the onDraw() missing cases.

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] and the [C++, Objective-C, Java style guides].
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
  writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the [CLA].
• All existing and new tests are passing.
• The reviewer has submitted any presubmit flakes in this PR using the [engine presubmit flakes form] before re-triggering the failure.

[blasten]

I feel like there is some misunderstanding from my side. The error message " Unable to acquire a buffer item, very likely client tried to acquire more than maxImages buffers" suggests that imageReader.acquireLatestImage() was called even though imageOpenedCount is less than imageReader.getMaxImages(). Unless there's a bug in this logic, shouldn't it be preventing the acquireLatestImage() call?

[eggfly]

@blasten I guess maybe the newest available image will be also counted by the code in ImageReader.cpp.
Let me have a deeper view in ImageReader.cpp

[eggfly]

@blasten I read the code and document of acquireLatestImage() in ImageReader.java carefully and I finally know why the error message comes even though the openedCount is less than maxImages.

The code:
https://cs.android.com/android/platform/superproject/+/master:frameworks/base/media/java/android/media/ImageReader.java;drc=master;l=410?q=two

The doc of acquireLatestImage() says:

     * Note that {@link #getMaxImages maxImages} should be at least 2 for
     * {@link #acquireLatestImage} to be any different than {@link #acquireNextImage} -
     * discarding all-but-the-newest {@link Image} requires temporarily acquiring two
     * {@link Image Images} at once. Or more generally, calling {@link #acquireLatestImage}
     * with less than two images of margin, that is
     * {@code (maxImages - currentAcquiredImages < 2)} will not discard as expected.
     * </p>

So it needs at least two reserved slots in buffer before one calling of acquireLatestImage(). And the implementation code is as same as it described:

    public Image acquireLatestImage() {
        Image image = acquireNextImage();
        if (image == null) {
            return null;
        }
        try {
            for (;;) {
                Image next = acquireNextImageNoThrowISE();
                if (next == null) {
                    Image result = image;
                    image = null;
                    return result;
                }
                image.close();
                image = next;
            }
        } finally {
            if (image != null) {
                image.close();
            }
        }
    }

Every nativeImageSetup() will need a new BufferItem in cpp, acquireNextSurfaceImage() calls nativeImageSetup(). then looking at the acquireLatestImage() logic, it calls acquireNextImage() and calls acquireNextImageNoThrowISE in the for-loop and close the last image unless the next is null. So It can really need two slots.

The code of this pull request fixs onDraw() may be lesser then invalidate(), and try to make the code logic neat and easy to maintain by closing the previous image if exists after reader.acquireLatestImages() and removing the queue and the counter.

[blasten]

would you mind trying to justify the comments, so the line width is a bit more even?

[eggfly]

@blasten OK, I will justify the comments.

[eggfly]

@blasten Done~

[blasten]

This is a great contribution @eggfly. Awesome work sharing your findings. LGTM

[eggfly]

@liyuqian @chinmaygarde Would you like to take a moment to have a review on this PR? Thank u 😄

[blasten]

@eggfly it will get merged once the tree becomes green

[ycv005]

Thanks @eggfly . Was waiting for this pr.

[eggfly]

flutter/flutter#83204