Add test for external textures on Android #26109

ds84182 · 2021-05-12T21:54:01Z

This PR adds three tiers of tests for external texture rendering, along with comparisons for Android's SurfaceView.

Identity transforms
Surface rotations. This requires decoding a video through MediaCodec with the rotation parameter set. Android framework source basically passes through the rotation parameter to the underlying Surface.
Surface crops, and surface rotations + crops. This requires the use of ImageReader (21+) and ImageWriter (23+). MediaCodec presents directly into the ImageReader, the crop rect is set on the acquired Images, and then the images are sent to ImageWriter which presents to the screen. The crop rect (which we now control) and rotation (passed from MediaCodec) are applied directly to the underlying Surface. Note that "Image" is a bit of a misnomer, they're actually just presentable hardware buffers.

Part of #24888

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.

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

chinmaygarde · 2021-05-21T18:36:46Z

Ping @gaaclarke and @blasten for routing.

...ario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java

testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/TestActivity.java

gaaclarke · 2021-05-22T01:48:06Z

I'm sorry, I skipped over this because rarely does anything Android have to do with me. The code LGTM and tests seem representative. I honestly don't know the Android integration tests system. It would be worth checking with @blasten there, @blasten ping me Monday if you want me to talk you through the context of the test.

Let's get this guy merged asap, if it gets stuck somehow message me on chat.

gaaclarke · 2021-06-02T22:06:12Z

friendly ping @blasten

blasten · 2021-06-03T01:54:14Z

The current process for checking in the golden files isn't accurately documented. I need to fix this issue.

For the time being, you could go to the "Linux Android Scenarios" check page. Check the output of Scenario App Integration Tests to see which tests have failed. The latest ones are these.

Find the "gsutil upload "flutter/{something}/diff_failures.zip". The latest one is this zip.

Replace the .png files in the engine repo that are different from the ones that end with *_actual.png within the zip file. This means removing the *_actual.png prefix and then copying the files over.

blasten · 2021-06-03T02:02:13Z

...ario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java

+      return atomicCanProduceFrames.get();
+    }
+
+    private void decodeThreadMain() {


Is the logic below strictly necessary? Ideally, the test would be as simple as possible if it can capture the behavior you want to make sure doesn't break.

blasten · 2021-06-03T02:04:34Z

testing/scenario_app/lib/src/texture.dart

+class DisplayTexture extends Scenario {
+  /// Creates the DisplayTexture scenario.
+  ///
+  /// The [dispatcher] parameter must not be null.


our style docs mentions that it's preferred to avoid comments that can otherwise be communicated through the type system or the method signature.

Omitting the comment fails a lint check, so I don't think this can be removed. This matches whats done in other test scenarios.

blasten · 2021-06-03T02:13:43Z

@ds84182 It doesn't appear that #24888 deals with video although plugins use textures to display video. What do you think about faking a frame? Maybe using android.graphics.Canvas so we don't need the video.

ds84182 · 2021-06-03T04:07:17Z

The problem with using andorid.graphics.Canvas is that you can't specify surface rotations with it. Video decoding allows you to specify arbitrary rotations, and surface crops didn't seem to work correctly when sourced from a Canvas.

Most of the issues with external textures are from Flutter not supporting rotations and crops correctly, which is what #24888 fixes. If Android made it easy to specify rotations and crops, this PR would be simple.

blasten · 2021-06-03T21:14:29Z

I see. Thanks for the explanation @ds84182.

My only last comment would be to add more comments to test code.

ds84182 · 2021-06-04T22:04:36Z

@blasten Looks like the diff failures are for other scenario tests. Should I still add them to the PR?

blasten · 2021-06-04T22:08:44Z

I saw that. Yes. That sounds good

chinmaygarde · 2021-06-10T20:15:22Z

@blasten Another review after the latest updates?

blasten · 2021-06-10T20:16:43Z

This is waiting on #24888. Other than that LGTM

ds84182 · 2021-06-11T17:16:34Z

Actually the opposite, #24888 is waiting on this CL since it's the only way to verify the changes are correct.

blasten

LGTM

chinmaygarde · 2021-06-17T20:04:24Z

The Firebase Test Lab failures look related to this patch. Can we ensure presubmits pass please?

chinmaygarde · 2021-06-24T20:04:40Z

The linux presubmit failure is just a minor formatting fix. I am not sure how to troubleshoot the Firebase Test Lab failure. Perhaps @blasten can help?

blasten · 2021-06-24T21:17:41Z

testing/scenario_app/android/app/src/main/AndroidManifest.xml

+            android:windowSoftInputMode="adjustResize"
+            android:theme="@style/FullScreenScreenshot">
+            <intent-filter>
+                <action android:name="com.google.intent.action.TEST_LOOP" />


this seems correct. I wonder if the issue is that only one activity can have this action. What happens if you remove the content of the outer <intent-filter>?

chinmaygarde · 2021-07-01T20:05:26Z

Any updates here?

chinmaygarde · 2021-07-15T20:02:26Z

Closing this as stale. Please reopen if progress is possible and once the conflicts are solved.

blasten · 2021-07-15T20:44:21Z

@ds84182 feel free to reach out to me if you'd like to try to land your change. I know it's very painful to contribute to some of our repos, but we are trying to improve that.

google-cla bot added the cla: yes label May 12, 2021

ds84182 marked this pull request as ready for review May 12, 2021 22:22

ds84182 requested a review from gaaclarke May 12, 2021 22:22

gaaclarke reviewed May 22, 2021

View reviewed changes

...ario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java Outdated Show resolved Hide resolved

gaaclarke reviewed May 22, 2021

View reviewed changes

testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/TestActivity.java Show resolved Hide resolved

ds84182 requested a review from blasten May 26, 2021 01:01

blasten reviewed Jun 3, 2021

View reviewed changes

ds84182 mentioned this pull request Jun 11, 2021

Fix handling of external texture transforms on Android #24888

Merged

9 tasks

blasten approved these changes Jun 11, 2021

View reviewed changes

ds84182 added 4 commits June 23, 2021 14:37

Add test for external textures on Android

192decd

Goldens and fix TODO

6b08e27

Address review comments

c61332a

Update goldens and add comments

5a96a00

ds84182 force-pushed the ext_tex_test branch from bc9e34f to 5a96a00 Compare June 23, 2021 21:37

blasten reviewed Jun 24, 2021

View reviewed changes

chinmaygarde closed this Jul 15, 2021

ds84182 mentioned this pull request Feb 24, 2022

Add test for external textures on Android #31642

Merged

8 tasks

Add test for external textures on Android #26109

Add test for external textures on Android #26109

Uh oh!

Conversation

ds84182 commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

chinmaygarde commented May 21, 2021

Uh oh!

Uh oh!

Uh oh!

gaaclarke commented May 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaaclarke commented Jun 2, 2021

Uh oh!

blasten commented Jun 3, 2021

Uh oh!

blasten Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

blasten Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

ds84182 Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

blasten commented Jun 3, 2021

Uh oh!

ds84182 commented Jun 3, 2021

Uh oh!

blasten commented Jun 3, 2021

Uh oh!

ds84182 commented Jun 4, 2021

Uh oh!

blasten commented Jun 4, 2021

Uh oh!

chinmaygarde commented Jun 10, 2021

Uh oh!

blasten commented Jun 10, 2021

Uh oh!

ds84182 commented Jun 11, 2021

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Jun 17, 2021

Uh oh!

chinmaygarde commented Jun 24, 2021

Uh oh!

blasten Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Jul 1, 2021

Uh oh!

chinmaygarde commented Jul 15, 2021

Uh oh!

blasten commented Jul 15, 2021

Uh oh!

Uh oh!

ds84182 commented May 12, 2021 •

edited

Loading

gaaclarke commented May 22, 2021 •

edited

Loading