[PlatformView][Android]VirtualDisplay resize on Android31 and above #47946

designDo · 2023-11-11T04:13:14Z

…128920) Resolves flutter/flutter#128920

flutter-dashboard · 2023-11-11T04:13:18Z

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 or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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.

johnmccutchan · 2023-11-14T15:21:19Z

Hi,

Can you please update your branch so it can be merged?

Also, how can write a test for this scenario?

johnmccutchan · 2023-11-14T15:32:03Z

shell/platform/android/io/flutter/plugin/platform/VirtualDisplayController.java

+    // When 'hot reload', although the resize method is triggered, the size of the native View has
+    // not changed.
+    if (width == getRenderTargetWidth() && height == getRenderTargetHeight()) {
+      getView().postDelayed(onNewSizeFrameAvailable, 0);


What is the contract around onNewSizeFrameAvailable? In the case that the size doesn't change do we need to run it?

yes, postDelayed can be removed

I don't think this is true, it must be called.

johnmccutchan · 2023-11-14T15:33:14Z

shell/platform/android/io/flutter/plugin/platform/VirtualDisplayController.java

+      return;
+    }
+    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {
+      resizeAboveAndroidS(getView(), width, height, onNewSizeFrameAvailable);


Please use the numeric version instead of S:

resize31(...);

johnmccutchan · 2023-11-14T15:34:11Z

shell/platform/android/io/flutter/plugin/platform/VirtualDisplayController.java

    renderTarget.release();
  }

+  @TargetApi(21)


This should be 31

johnmccutchan · 2023-11-14T15:34:54Z

shell/platform/android/io/flutter/plugin/platform/VirtualDisplayController.java

+  private void resizeAboveAndroidS(
+      View embeddedView, int width, int height, final Runnable onNewSizeFrameAvailable) {
+    renderTarget.resize(width, height);
+    // https://android.googlesource.com/platform/prebuilts/fullsdk/sources/android-30/+/refs/heads/master/android/app/Presentation.java#293


Rewrite this comment block as:

// On Android versions 31+ resizing of a Virtual Display's Presentation is natively supported.

Please address this comment.

designDo · 2023-11-15T02:53:55Z

@johnmccutchan hi, thanks for code review. I have merge main branch to pre/128920.
https://github.com/designDo/video_player_surface , this repo can be used to test SurfaceView play on Android 31+.

designDo · 2023-11-15T05:19:37Z

something wrong with build windows, https://github.com/flutter/engine/pull/47946/checks?check_run_id=18690556117
I see the build log, It doesn’t seem to be a code problem.

johnmccutchan · 2023-11-15T22:32:36Z

@johnmccutchan hi, thanks for code review. I have merge main branch to pre/128920. https://github.com/designDo/video_player_surface , this repo can be used to test SurfaceView play on Android 31+.

By a test I mean an automated test that is part of this PR.

johnmccutchan · 2023-11-15T22:33:00Z

@designDo Are you going to address my other comments?

designDo · 2023-11-16T01:01:20Z

@designDo Are you going to address my other comments?

I modified everything mentioned in the code review

designDo · 2023-11-16T01:03:57Z

@johnmccutchan hi, thanks for code review. I have merge main branch to pre/128920. https://github.com/designDo/video_player_surface , this repo can be used to test SurfaceView play on Android 31+.

By a test I mean an automated test that is part of this PR.

@johnmccutchan hi, thanks for code review. I have merge main branch to pre/128920. https://github.com/designDo/video_player_surface , this repo can be used to test SurfaceView play on Android 31+.

By a test I mean an automated test that is part of this PR.
how to do this

johnmccutchan

Just a couple nits left and then I will approve. Thanks for the contribution!

johnmccutchan · 2023-11-16T15:30:06Z

shell/platform/android/io/flutter/plugin/platform/VirtualDisplayController.java

+  private void resizeAboveAndroidS(
+      View embeddedView, int width, int height, final Runnable onNewSizeFrameAvailable) {
+    renderTarget.resize(width, height);
+    // https://android.googlesource.com/platform/prebuilts/fullsdk/sources/android-30/+/refs/heads/master/android/app/Presentation.java#293


Please address this comment.

johnmccutchan · 2023-11-16T15:30:27Z

shell/platform/android/io/flutter/plugin/platform/VirtualDisplayController.java

+    // When 'hot reload', although the resize method is triggered, the size of the native View has
+    // not changed.
+    if (width == getRenderTargetWidth() && height == getRenderTargetHeight()) {
+      return;


After reading the calling code, we do need to run the onNewSizeFrameAvailable in this case.

Meaningless comments have been removed.
In this case, added onNewSizeFrameAvailable callback.
Thank you for your guidance and help!

…ack.

johnmccutchan · 2023-11-20T19:02:16Z

shell/platform/android/io/flutter/plugin/platform/VirtualDisplayController.java

+      View embeddedView, int width, int height, final Runnable onNewSizeFrameAvailable) {
+    renderTarget.resize(width, height);
+    // On Android versions 31+ resizing of a Virtual Display's Presentation is natively supported.
+    // Fix in: https://github.com/flutter/flutter/issues/128920


Please remove the comment "// Fix in: ..." as it isn't needed in the code.

OK, this line has removed. I have merged main into pre/128920.

johnmccutchan · 2023-11-21T03:16:11Z

Thanks for your contribution!

…nd above (flutter/engine#47946)

…138774) flutter/engine@3348fb0...70b1c73 2023-11-21 [email protected] [PlatformView][Android]VirtualDisplay resize on Android31 and above (flutter/engine#47946) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

designDo added 4 commits November 11, 2023 10:17

[PlatformView][Android]VirtualDisplay resize on Android31 and above(#…

75d1fe7

…128920) Resolves flutter/flutter#128920

add import

d1c12da

Code format

7a1c649

Code format

950afdd

github-actions bot added the platform-android label Nov 11, 2023

designDo closed this Nov 11, 2023

Code format

228be10

designDo reopened this Nov 11, 2023

add TargetAPI 21

afe1b13

designDo closed this Nov 13, 2023

designDo reopened this Nov 13, 2023

Merge branch 'main' into pre/128920

ccda5a7

johnmccutchan self-requested a review November 14, 2023 15:27

johnmccutchan reviewed Nov 14, 2023

View reviewed changes

Merge branch 'main' into pre/128920

5e10d6e

designDo closed this Nov 15, 2023

designDo added 2 commits November 15, 2023 10:37

Merge branch 'main' into pre/128920

f29bcf9

Fixed review code

f5abcd3

designDo reopened this Nov 15, 2023

Merge branch 'main' into pre/128920

f6c61ee

Merge branch 'main' into pre/128920

5391593

johnmccutchan reviewed Nov 16, 2023

View reviewed changes

designDo added 2 commits November 17, 2023 08:56

Merge branch 'main' into pre/128920

ba38dca

remove meaningless comments and add add onNewSizeFrameAvailable callb…

f000fe0

…ack.

johnmccutchan approved these changes Nov 20, 2023

View reviewed changes

designDo added 2 commits November 21, 2023 09:00

Merge branch 'main' into pre/128920

8b2f684

remove meaningless comments

502bab9

johnmccutchan merged commit 70b1c73 into flutter:main Nov 21, 2023

engine-flutter-autoroll mentioned this pull request Nov 21, 2023

Roll Flutter Engine from 3348fb0ca302 to 70b1c7341255 (1 revision) flutter/flutter#138774

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 21, 2023

70b1c7341 [PlatformView][Android]VirtualDisplay resize on Android31 a…

8f35b97

…nd above (flutter/engine#47946)

designDo deleted the pre/128920 branch November 23, 2023 12:09

This was referenced Feb 4, 2024

[Android] Flutter 3.19 beta - Widget freezes on resize learntoflutter/flutter_embed_unity#14

Closed

[Android] Virtual display - Buggy resize with Android 12+ flutter/flutter#142952

Closed

[PlatformView][Android]VirtualDisplay resize on Android31 and above #47946

[PlatformView][Android]VirtualDisplay resize on Android31 and above #47946

Uh oh!

Conversation

designDo commented Nov 11, 2023

Uh oh!

flutter-dashboard bot commented Nov 11, 2023

Uh oh!

johnmccutchan commented Nov 14, 2023

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!

designDo commented Nov 15, 2023

Uh oh!

designDo commented Nov 15, 2023

Uh oh!

johnmccutchan commented Nov 15, 2023

Uh oh!

johnmccutchan commented Nov 15, 2023

Uh oh!

designDo commented Nov 16, 2023

Uh oh!

designDo commented Nov 16, 2023

Uh oh!

johnmccutchan left a comment

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!

johnmccutchan commented Nov 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants