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

Deprecate crop/rotation support for Android plugins, consider Dart alternative #144407

Open
2 tasks
Tracked by #144352
johnmccutchan opened this issue Feb 29, 2024 · 5 comments
Open
2 tasks
Tracked by #144352
Assignees
Labels
fyi-android For the attention of Android platform team P2 Important issues not at the top of the work list platform-android Android applications specifically team-engine Owned by Engine team triaged-engine Triaged by Engine team

Comments

@johnmccutchan
Copy link
Contributor

The tests as written today are testing Android features and code paths that aren't consistently supported on Android and thus can't be used. Also of note, these functions are not used by our popular plugins.

There are three paths currently tested today:

  1. MediaRenderer (applies rotation) -> ImageRenderer (applies crop) -> SurfaceView [correctly handles crop and rotation]
  2. MediaRenderer (applies rotation) -> ImageRenderer (applies crop) -> SurfaceProducer(ST-backed) [correctly handles crop and rotation]
  3. MediaRenderer (applies rotation) -> ImageRenderer (applies crop) -> SurfaceProducer(IR-backed) [does not handle crop or rotation]

Due to an oversight by Android, ImageReader backed surfaces do not respect metadata applied to the surface (rotation & crop). Rotation information is not available at all and crop information is corrupted by the ImageReader (only the width/height is propagated the origin offset is not).

Two action items:

  • We need to replace the Android code with Dart based texture rotation and cropping.
  • We need to make the SurfaceTexture backend ignore the crop and rotation metadata when we render so that it is consistent with ImageReader. We can/should probably make this behaviour SurfaceProducer specific so we don't break any legacy apps.
@matanlurey matanlurey added platform-android Android applications specifically P1 High-priority issues at the top of the work list team-engine Owned by Engine team e: scenario-app The `testing/scenario_app` fixture in the engine fyi-android For the attention of Android platform team labels Feb 29, 2024
@jonahwilliams jonahwilliams added the triaged-engine Triaged by Engine team label Mar 4, 2024
@matanlurey matanlurey removed their assignment Mar 6, 2024
@matanlurey
Copy link
Contributor

I am not currently working on this, but @johnmccutchan I know you had something WIP so I'll keep it assigned to you?

@johnmccutchan
Copy link
Contributor Author

Yes, I'll get back to this soon. The action items I filed above are still the current status.

@matanlurey
Copy link
Contributor

@johnmccutchan @jonahwilliams and I chatted about whether or not this should be a blocker for Impeller on Android (Vulkan edition). We decided that this is rather rare functionality, this would only block a user migrating to Impeller if and only if:

  • They have a plugin that uses SurfaceTextures
  • That plugin migrates to SurfaceProducer

In other words, plugins could continue to just provide a SurfaceTexture API if they want native crop/rotation, of course that means the plugins would not be compatible with Android + Vulkan. We should have an answer for this eventually (including in docs) but it seems non-critical to have for the initial opt-out period and moving our own internal plugins (that don't use this feature) to SurfaceProducer (unless we find that we indeed are using this functionality, which is unlikely).

Let us know if you disagree strongly, and we can re-evaluate, but for now I'm removing as a blocker/moving to P2.

@matanlurey matanlurey added P2 Important issues not at the top of the work list and removed P1 High-priority issues at the top of the work list labels Mar 28, 2024
@johnmccutchan
Copy link
Contributor Author

The crop/rotation parts of the scenario_app tests are actually testing Android functionality and not Flutter functionality.

The action items in my first comment still stand however I don't think they are priorities as I don't think any plugins are actually using this functionality in practice.

@matanlurey matanlurey changed the title Issues with Scenarios App External Texture Tests on Android Deprecate crop/rotation support for Android plugins, consider Dart alternative Mar 29, 2024
@matanlurey matanlurey removed the e: scenario-app The `testing/scenario_app` fixture in the engine label Mar 29, 2024
@matanlurey
Copy link
Contributor

Removing e: scenario-app as this is no longer in scope for the e2e test suite.

auto-submit bot pushed a commit to flutter/engine that referenced this issue Mar 29, 2024
…pp`. (#51769)

Closes flutter/flutter#145957.

As @jonahwilliams and @johnmccutchan and I discussed (flutter/flutter#144407), the functionality that was being tested was actually _Android's_ ability to rotate and crop `SurfaceTexture`-backed textures. This same functionality doesn't even exist in the `ImageReader`-based textures (read: modern Android devices):

> Due to an oversight by Android, ImageReader backed surfaces do not respect metadata applied to the surface (rotation & crop). Rotation information is not available at all and crop information is corrupted by the ImageReader (only the width/height is propagated the origin offset is not).

We might decide to re-add this functionality in the Dart `Texture` widget, but given we'll be migrating our plugins to `SurfaceProducer` (again, read: using `ImageTexture` for most Android phones), it's pointless to test this (and isn't even testing Flutter's code).

This reduces our test suite significantly (8 tests down to 2), which should also help with runtime and flakiness.

/cc @zanderso who I'm sure will be stoked.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fyi-android For the attention of Android platform team P2 Important issues not at the top of the work list platform-android Android applications specifically team-engine Owned by Engine team triaged-engine Triaged by Engine team
Projects
None yet
Development

No branches or pull requests

3 participants