Fix incorrect capture when using DisplayManager API

[eiyooooo]

Fix the incorrect capture like this

[rom1v]

Thank you for your contribution.

Fix the same incorrect capture occur before 7e3b935 when using DisplayManager API

The commit 7e3b935 destroyed/created a new display on every rotation. On the contrary, your PR keeps the virtual display instance (instead of releasing/creating a new one).

Could you please explain a bit more the problem?

[eiyooooo]

Could you please explain a bit more the problem?

When execute SurfaceControl.createDisplay on Android 14, it essentially calls DisplayManager.createVirtualDisplay.
When execute SurfaceControl.setDisplayProjection on Android 14, it essentially calls DisplayManager.resizeVirtualDisplay.
When execute SurfaceControl.setDisplayLayerStack on Android 14, it essentially calls DisplayManager.setDisplayIdToMirror.

Instead of releasing/creating a new VirtualDisplay (avoiding displayId increase), I do the same job at

scrcpy/server/src/main/java/com/genymobile/scrcpy/ScreenCapture.java

Lines 55 to 57 in dcea343

	// Density doesn't matter since this virtual display is only used for mirroring.
	virtualDisplay.resize(videoRect.width(), videoRect.height(), 1);
	virtualDisplay.setSurface(surface);

When executing DisplayManager.setDisplayIdToMirror, it will copy the configuration of the original display to the created display (including the rotation). (It probably can only happen once to each created display, so you need to destroy the created display and create a new one to copy the rotation at 7e3b935.)

But if we use DisplayManager API directly create a VirtualDisplay and set it to VIRTUAL_DISPLAY_FLAG_AUTO_MIRROR, orientation and displayRect will automatically be computed based on configuration changes. 

Because Android 14 lock orientation doesn't work as expected <#4011>, so we use videoRect. But videoRect is already rotated according to the device rotation, so we need to freeze the VirtualDisplay's rotation to 0 to avoid a double rotation.

(It seems a little bit hard to understand while I am poor at English writing 😴)

And I suggest use DisplayManager API directly on Android 14 and above, instead of trying the SurfaceControl API first, because it basically does the same thing. If we use DisplayManager API, we could have a better control of the created VirtualDisplay.

[rom1v]

Thank you for the detailed answer. I will read in details a bit later.

Just a quick question:

Instead of releasing/creating a new VirtualDisplay (avoiding displayId increase)

Why is it a problem that an internal id is incremented? IIUC it's not the "display id" as in scrcpy --display-id=, is it?

[eiyooooo]

IIUC it's not the "display id" as in scrcpy --display-id=, is it?

Nope, it's the "display id" as in scrcpy --display-id=😉

[eiyooooo]

Why is it a problem that an internal id is incremented?

I remember someone mentioning that when the "display ID" is over 1000, the system may encounter issues and need a reboot to fix it.

[rom1v]

Oh, I had never noticed that mirroring the device created a new display id (listed by scrcpy --list-displays) with createVirtualDisplay() (be3f949).

Maybe there is another method which does not create a new display id (maybe refs #4848, but I did not understand the issue).

[eiyooooo]

Oh, I'm had never noticed that mirroring the device created a new display id (listed by scrcpy --list-displays) with createVirtualDisplay()

BTW, in Android 14

scrcpy/server/src/main/java/com/genymobile/scrcpy/ScreenCapture.java

Line 48 in be3f949

display = createDisplay();

also created a new display id

[rom1v]

Indeed (but I think this is true only for Android 14, on my older devices it does not create a new display id).

[rom1v]

With your commit from this PR, and this additional hack to force using the DisplayManager API:

diff --git a/server/src/main/java/com/genymobile/scrcpy/ScreenCapture.java b/server/src/main/java/com/genymobile/scrcpy/ScreenCapture.java
index 6f9c4d338..d04fad572 100644
--- a/server/src/main/java/com/genymobile/scrcpy/ScreenCapture.java
+++ b/server/src/main/java/com/genymobile/scrcpy/ScreenCapture.java
@@ -41,6 +41,7 @@ public class ScreenCapture extends SurfaceCapture implements Device.RotationList
         }
 
         try {
+            if (true) throw new Exception("fake");
             display = createDisplay();
             setDisplaySurface(display, surface, videoRotation, contentRect, unlockedVideoRect, layerStack);
             Ln.d("Display: using SurfaceControl API");

I can reproduce the problem that was fixed by 7e3b935 on a Pixel 8. 😞

[eiyooooo]

I can reproduce the problem that was fixed by 7e3b935 on a Pixel 8. 😞

Could you please post the screenshot of the incorrect capture?

[rom1v]

Could you please post the screenshot of the incorrect capture?

In fact, it is not a corrupted image: on device rotation, sometimes (not always, so there's probably a race condition) it just stops sending frames (so the physical device is portrait and the screen is updated, but the scrcpy window is landscape with the last landscape image, frozen), and the video stream is restored when I rotate the device screen one more time.

In practice, I can only reproduce if the current active application is the (default) camera app.

[eiyooooo]

on device rotation, sometimes (not always, so there's probably a race condition) it just stops sending frames (so the physical device is portrait and the screen is updated, but the scrcpy window is landscape with the last landscape image, frozen), and the video stream is restored when I rotate the device screen one more time.

scrcpy/server/src/main/java/com/genymobile/scrcpy/ScreenCapture.java

Lines 62 to 63 in dcea343

	int displayId = virtualDisplay.getDisplay().getDisplayId();
	ServiceManager.getWindowManager().freezeRotation(displayId, 0);

I think this should fix it 😑

[rom1v]

Nope, with or without these lines, I can reproduce the problem.

Besides, this is probably not an "incorrect" rotation value, it looks like a race condition (80% of times, it works correctly…).

[eiyooooo]

Fix the same incorrect capture occur before 7e3b935 when using DisplayManager API

So this pr is not about the incorrect capture occur before 7e3b935

Here are the incorrect capture I met

[rom1v]

😮 Do you encounter this problem with current dev branch on rotation, without passing --crop or --lock-video-orientation?

[eiyooooo]

😮 Do you encounter this problem with current dev branch on rotation, without passing --crop or --lock-video-orientation?

When using DisplayManager API without

scrcpy/server/src/main/java/com/genymobile/scrcpy/ScreenCapture.java

Lines 62 to 63 in dcea343

	int displayId = virtualDisplay.getDisplay().getDisplayId();
	ServiceManager.getWindowManager().freezeRotation(displayId, 0);

this problem occurs occasionally.

(without passing --crop or --lock-video-orientation)

[eiyooooo]

I can reproduce the problem that was fixed by 7e3b935 on a Pixel 8. 😞

With my commit from this PR, I can reproduce your bug fixed by 7e3b935 too. 😥

So I think releasing/creating a new one still necessary.

[eiyooooo]

😄

[rom1v]

For information, I tried to keep the mirroring virtual display on rotation on the current code base after all the changes related to virtual displays (on current dev branch): branch keep_existing_display.

But the problems fixed by 7e3b935 can still be reproduced, so this cannot be merged, we must re-create the mirroring virtual display every time.

[rom1v]

It seems I cannot reproduce the problem if I swap setSurface() and resize(): 6bd32b8

@eiyooooo Could you reproduce with this?

[eiyooooo]

Could you reproduce with this?

Are you referring to the issue shown in this image? If so, I’ll test it a bit later. 😄

[rom1v]

I'm referring to #4840 (comment) (7e3b935).

[eiyooooo]

I'm referring to #4840 (comment) (7e3b935).

Got it, I'll test it out shortly.

[rom1v]

It seems I cannot reproduce the problem if I swap setSurface() and resize(): 6bd32b8

Definitely causes a lot of issues: #5455 (comment)