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

Rotation not handled correctly with apk #17

Open
pcrepieux opened this issue Jul 8, 2021 · 12 comments
Open

Rotation not handled correctly with apk #17

pcrepieux opened this issue Jul 8, 2021 · 12 comments
Assignees

Comments

@pcrepieux
Copy link

Only portrait mode is properly managed at the moment.

As reported in DeviceFarmer/STFService.apk#37 users who need to have a landscape orientation when using the apk get a badly resized screen. This is also true with STF.

@sudhalucky
Copy link

Hi @pcrepieux Seems the same issue is happening on Android devices even below 12 when a screenshot is taken in landscape mode, the device rendering itself does not have any issue. I have tried both minicap-bin and minicap-apk as screen grabber got the same results however.

@mark-omarov
Copy link

Hi @pcrepieux.

I'm wondering if this issue was fixed or not. I can see possibly-related changes to this problem, but I don't see any publishes after version 2.7.0, which itself doesn't seem to include the fix.

Building and replacing directly in node_modules (just for testing purposes) didn't help to solve the problem.
I'd appreciate if you could take a couple minutes to get back to me on this regard. Cheers!

@pcrepieux
Copy link
Author

Hi @mark-omarov
Indeed, the latest released version doesn't provide this fix. I am a bit concerned if your manual updating of the node module didn't help. I'll double check on my side.

@mark-omarov
Copy link

Hi @pcrepieux
Thanks for getting back on this one. It's very nice of you to double check, I'd greatly appreciate.
If it works, shall we bump up the version and release it? It'd also be nice to update it for the STF lib.

There is at least one more problem related to this issue though, as @sudhalucky has mentioned - screenshots in the landscape mode aren't working, even for versions earlier than 12. I have some other problems with Android 12, but they seems to be related to a different lib, so I won't mention them here.

@pcrepieux
Copy link
Author

Here is a quick status. The built from master branch seems to work fine. There is maybe a little trick that can be misleading: when taking screenshot, the apk will be used rather than the binary (for the video stream, the binary is tried first and there is a fallback to the apk). As 2.7.0 doesn't come with latest apk fixes, that could explain the inconsistency that you are experiencing.
I just submitted a PR to properly manage secondary display and ensured to upgrade the version name. I suggest you try again with manually with master (or with #33) or wait a little bit that 2.7.1 comes online (which hopefully will be pretty soon).

@mark-omarov
Copy link

@pcrepieux I sincerely appreciate your help with this issue!
I'll give it a little bit of time and test once 2.7.1 release is published, then I'mma test it with STF and likely to update the version there as well. I'll hope for the best.
Thanks again my friend!

@sudhalucky
Copy link

Hi @pcrepieux
How are you?
Hope you are enjoying your holidays, sorry to trouble you at this time of the year, I see you sent a pull request for this fix.
Any idea on when this could be merged to master and publish 2.7.1?
I thought of compiling the changes myself but held on expecting this to be released sooner.

@pcrepieux
Copy link
Author

I submitted just before the holidays season (that was not the best timing). Let me ping @koral-- Even if I can merge #33, I'd prefer to have a review before.

@mark-omarov
Copy link

Hi @pcrepieux ,

Thanks for your fix, it was merged to the master, my local build now works perfectly, very much appreciated!
It'd be great to publish it as well, the npm's latest version is yet 2.7.0.
I'm kinda in the same boat with @sudhalucky, I could replace it with the local version from master, but figured publishing should happen anytime soon anyway.

@pcrepieux
Copy link
Author

Ohh, I see ... I've just fixed a small pipeline issue. Here you are:
https://www.npmjs.com/package/@devicefarmer/minicap-prebuilt/v/2.7.1
I guess we'll put it also in stf shortly.

@mark-omarov
Copy link

mark-omarov commented Jan 27, 2022

Hi @pcrepieux ,

Thanks for fixing the pipeline, I've tested and here's something interesting.
The fix worked perfectly for some devices, but there's a problem caught specifically for Pixel 4 XL (Android 12), this might be reproducible on other devices, but for me on Pixel 3A it isn't.

Initially, I thought it was a problem of minicap, you can find it in the collapsable list down below.
After some debugging, I'm positive that minicap is working as expected, and the actual problem is in the stf itself.
There's a running promise that has a 2s timeout window to retrieve a pid, for most devices it's enough, but in some edge cases, like the one I've experienced, 2s isn't enough, I've increased the timeout and tested - it seems to work
I'll spend a little bit more time on testing on Monday, and if it actually fixes the problem I'll prepare PR with the fix for STF repo.

The original idea is here There's an error popping up on the first connection (sometimes following connections as well) in minicap, that eventually forces stf to stop the service, therefore, the screen is not showing up on the web-ui.

It's possible to bypass the problem by re-connecting a number of times or trying to enable/disable screen streaming, it's like rolling a dice though.

This is the error I caught with logcat:

01-27 12:37:25.450   675   675 E CompositionEngine: ANativeWindow::dequeueBuffer failed for display [minicap] with error: -32
01-27 12:37:25.450   675   675 W CompositionEngine: Dequeuing buffer for display [minicap] failed, bailing out of client composition for this frame

I'm not completely sure due to the lack of android development experience, but if I were to guess it seems like minicap fails to start the capturing because it tries to access the display id while it's still locked? I'm speculating here. :)

I thought you should know about it. I'll update you if any new details come up.

Edit: tested in combination w/ stf.

@mark-omarov
Copy link

Hey @pcrepieux @koral-- , sorry for my ignorance and not updating you after the PR got merged, this issue has been fixed as far as I'm concerned. Maybe you can consider closing it. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants