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

android: bugs and differences between HW/SW rotation #6771

Closed
pazos opened this issue Oct 8, 2020 · 24 comments · Fixed by #6788
Closed

android: bugs and differences between HW/SW rotation #6771

pazos opened this issue Oct 8, 2020 · 24 comments · Fixed by #6788
Labels
Android bug firmware help-wanted We'd like help with this issue

Comments

@pazos
Copy link
Member

pazos commented Oct 8, 2020

  • KOReader version: 2020.7+
  • Device: Android

background

Support for Android native rotation was added in 7094519.
The main benefit is no overhead in rotated modes (specially notable with keyboard)

It didn't work fine on pre-kitkat emulators so I left them with SW rotation. Because android is usually bad implemented or very broken I also added code to handle 4.4+ that don't play nice with surface changes

Issue 1

  • Notched devices w/ HW rotation could have a bad viewport when rotated (confirmed).

issue 2

  • 90º and 270º rotas are swapped between HW and SW devices (or at least rota is inverted between my phone and my Cervantes). (confirmed)

issue 3

  • Devices w/ HW rotation that use landscape as default orientation have a bug in rotation code [ 0 -> 270, 90 -> 0, 180 -> 90, 270 -> 180] (reported by mobileread's u/Xwang

Steps to reproduce

  • issue 1 requires a device with a notch. Just set a 90º/270º rota. It is the display fine or some part of the UI is out of bounds?

  • issue 2 requires a device with HW rotation and one w/SW rotation. Do them rotate equal or values are swaped?

  • issue 3 requires a very special snowflake device. Sadly most devices I'm aware use portrait as native orientation (even if they're emulators and are presented to you as landscape)

Anyways, all the three issues are easy to workaround. Issue 1 is hard to get done well as it involves messing with device viewport, but a quick workaround could be disable HW rotation. As they're all modern phones the change shouln't be that noticeable.

If you're experiencing some of these bugs and want to help debugging please post here.

@pazos pazos added bug Android firmware help-wanted We'd like help with this issue labels Oct 8, 2020
@ezdiy
Copy link
Member

ezdiy commented Oct 8, 2020

Yup, when doing my linuxfb rot stuff, what android did essentially amounted to dead road. There's bunch of rotation hooks for linuxfb that android should be technically doing, as it can account for most cases you present - rotation angle values being arbitrary, device starting at weird native rotation. I don't have the nerves of steel one needs to develop for android, but can probably sketch what needs to be done.

@NiLuJe
Copy link
Member

NiLuJe commented Oct 8, 2020

As far as issue 2 is concerned, I wouldn't necessarily trust anything an NTX board has to say about HW rotation either, their kernels do... insanely weird crap ^^.

@NiLuJe
Copy link
Member

NiLuJe commented Oct 8, 2020

I can definitely confirm 1. on a notched phone on P. I have an horizontal black band on the bottom.

And also that the FM rotation is borked like on that recent issue I can't find :D.

@NiLuJe
Copy link
Member

NiLuJe commented Oct 8, 2020

And that's also a yes on 2., they're indeed inverted ;).

@pazos
Copy link
Member Author

pazos commented Oct 8, 2020

@NiLuJe: cool, I guess :p

Issue 1 can be workarounded easily by disabling HW rotation if notch_size > 0 px. A proper handle would involve using notch_size and current rota to compute the viewport and apply it with Device:setViewport. Old phones use that to toggle the legacy "action bar" It is not fun and quite hackish but something like that should work.

Issue 2 is easier. Just Swap Landscape and reverse landscape in https://github.com/koreader/koreader-base/blob/master/ffi/framebuffer_android.lua#L95-L103

Issue 3 does involve the same function than issue 2. Just don't assume 0 is PORTRAIT and retrieve initial orientation to decide if we need to start with 0 == landscape (reversed or not)

@yparitcher
Copy link
Member

yparitcher commented Oct 8, 2020

related: #6704

as commented there, I think this is due to the fact that Android HW rotation was never fixed to handle the FM when we redid rotation a few months back

@pazos
Copy link
Member Author

pazos commented Oct 8, 2020

I think this is due to the fact that Android HW rotation was never fixed to handle the FM when we redid rotation a few months back

Indeed. In fact android HW rotation was made possible by your rota refactor :). I did tested CRe documents and there everything worked fine, but the bug in FM is there from the very begin :P

@Galunid
Copy link
Member

Galunid commented Oct 9, 2020

Notched devices w/ HW rotation could have a bad viewport when rotated (needs to be confirmed).

I confirm, bottom menu is moved up by notch height and notch doesn't reduce width (phone's height in this case)

Issue 2 I can't really test, since my device refuses to do any SW rotation in KOReader (I don't see an option to toggle this in any menu, tried with today's nightly)

I can't seem to select 180 degrees rotation though, it's always set to 0

@pazos
Copy link
Member Author

pazos commented Oct 11, 2020

https://github.com/pazos/android-luajit-launcher/tree/fix_hw_rota should fix the three issues.

But I cannot test issue 3 as I don't have a device that boots in landscape and I cannot emulate one :(

Also needs

diff --git a/ffi/framebuffer_android.lua b/ffi/framebuffer_android.lua
index 58115af..8cd70af 100644
--- a/ffi/framebuffer_android.lua
+++ b/ffi/framebuffer_android.lua
@@ -95,9 +95,9 @@ function framebuffer:setRotationMode(mode)
     if android.hasNativeRotation() then
         local key
         if mode == 0 then key = "PORTRAIT"
-        elseif mode == 1 then key = "LANDSCAPE"
+        elseif mode == 1 then key = "REVERSE_LANDSCAPE"
         elseif mode == 2 then key = "REVERSE_PORTRAIT"
-        elseif mode == 3 then key = "REVERSE_LANDSCAPE" end
+        elseif mode == 3 then key = "LANDSCAPE" end
         if key then
             android.orientation.set(C["ASCREEN_ORIENTATION_" .. key])
         end

Because what android calls REVERSE_LANDSCAPE it's landscape for us.

@NiLuJe
Copy link
Member

NiLuJe commented Oct 11, 2020

@pazos: I don't usually have an Android TC on hand, so, if you happen to have a test build... :o)

@Galunid
Copy link
Member

Galunid commented Oct 11, 2020

I tried building using docker image, got warning, that's probably safe to ignore

This app only has 32-bit [armeabi-v7a] native libraries. Beginning August 1, 2019 Google Play store requires that all apps that include native libraries must provide 64-bit versions. For more information, visit https://g.co/64-bit-requirement

and failure with:

Task :app:stripArmFullStorageReleaseDebugSymbols FAILED
No toolchains found in the NDK toolchains folder for ABI with prefix: aarch64-linux-android

Not sure if it's something on my end, I report it just in case. I can build master, can't build with your commits (pazos:hw_rota_fix).

Alias I use to build, in case it helps.

alias koreader_build_android='docker run -v "$(pwd):/home/ko/koreader" -v /home/kris/.gradle:/home/ko/.gradle -it koreader/koandroid:0.3.1 bash -c "source ~/.bashrc && pushd koreader && ./kodev release android"; pushd platform/android/luajit-launcher && rm -rf jni/luajit/luajit-2.0/.git && git reset --hard HEAD && popd && /opt/android-sdk/build-tools/30.0.2/apksigner sign --ks my.keystore *.apk'

@pazos
Copy link
Member Author

pazos commented Oct 11, 2020

@Galunid: it is a problem in the docker image. It has a stripped TC.

The problem is the new gradle distribution in: koreader/android-luajit-launcher@f2a19bb#diff-fd9d61e37d939c941e7a62077f38d62d required a new gradle plugin koreader/android-luajit-launcher@f2a19bb#diff-c197962302397baf3a4cc36463dce5ea

If you revert to gradle distribution 5.1.1 and gradle plugin 3.4.2 you can build just fine. You'll miss the new warnings too, which inform us which is the only requisite we don't match to publish on the play store :p

@pazos
Copy link
Member Author

pazos commented Oct 11, 2020

Note to @Frenzie: don't strip any ABI from the next Docker image (probably the original armeabi is it safe to remove as is isn't supported now). Anways, it looks like a fail on gradle itself as the error:

No toolchains found in the NDK toolchains folder for ABI with prefix: aarch64-linux-android doesn't make sense on a task named stripArmFullStorageReleaseDebugSymbols which is armeabi-v7a

Maybe it is fixed on a even newer gradle version. I will check that.

@NiLuJe: I don't have any APK but can build one if you want to test. FWIW Issues 1 and 2 were barely tested using emulators.

@pazos
Copy link
Member Author

pazos commented Oct 11, 2020

@Galunid @NiLuJe: test APK in https://www.dropbox.com/s/34zjxqocc5rtkqd/koreader-2020.10%2BRota.apk?dl=0. It uses an old tag but should match current master + changes above.

@Galunid
Copy link
Member

Galunid commented Oct 11, 2020

@pazos Just build it myself (after reverting the gradle stuff), and it works (at least issue 1) on android 10. 180 degrees rotation works too.
Issue 2 seems fixed too (my phone's rotation is the same as Kobo)

@pazos
Copy link
Member Author

pazos commented Oct 11, 2020

Issue 2 seems fixed too (my phone's rotation is the same as Kobo)

You can't test that on a device with a notch, because issue 2 requires a HW rotation and now notched devices are using SW rotation. If you still have an onyx reader you can test there, if you wish :)

@Galunid
Copy link
Member

Galunid commented Oct 11, 2020

@pazos Doesn't work. When you select 90 degrees counter clockwise it rotates 90 degrees clockwise, and makes 90 degrees clockwise button active. Same for the other one. Still can't select 180 degrees rotation (always stays portrait, doesn't "select" 180 degrees button)

@pazos
Copy link
Member Author

pazos commented Oct 11, 2020

@Galunid: to be sure we all speak the same language I'm going to talk based on image in https://github.com/koreader/koreader-base/blob/master/ffi/framebuffer.lua#L44-L58 :)

There's 4 rotation modes, on the UI they are 90º | 0º | 90º | 180º and should match 3 | 0 | 1 | 2 in the image. That's what I see on the emulator in both HW and SW rotas.

About reverse portrait not working I'm afraid that's a limitation of the device. We cannot fix that if the framework doesn't admit that orientation (no other app should be able to do that). About the button "doesn't select a specific rotation", it is a side effect of the code reporting the real rotation, which would be the last one if 180º doesn't work on a device.

@NiLuJe
Copy link
Member

NiLuJe commented Oct 11, 2020

@pazos: Thanks, all good on my end (notch, P) ;).

@Frenzie
Copy link
Member

Frenzie commented Oct 11, 2020

@pazos

Note to @Frenzie: don't strip any ABI from the next Docker image (probably the original armeabi is it safe to remove as is isn't supported now). Anways, it looks like a fail on gradle itself as the error:

No toolchains found in the NDK toolchains folder for ABI with prefix: aarch64-linux-android doesn't make sense on a task named stripArmFullStorageReleaseDebugSymbols which is armeabi-v7a

O-o

The image is already awfully close to 50 gigabyte. >_>

@Galunid
Copy link
Member

Galunid commented Oct 11, 2020

@pazos When I select 3, it gives me 1, and makes button 1 active, when I select 1, it rotates 3 and makes button 3 active. About the 180 thingy I reported just in case, as long as you're aware it's fine by me

@pazos
Copy link
Member Author

pazos commented Oct 11, 2020

When I select 3, it gives me 1, and makes button 1 active, when I select 1, it rotates 3 and makes button 3 active.

It doesn't make sense unless your device wasn't affected by issue 2 in the first place (ie: HW rota was ok and the same as emulators/kobos..).

Anyways, I uploaded a new version which should fix logic issues in landscape and doesn't need changes in koreader-base. Please test: https://www.dropbox.com/s/34zjxqocc5rtkqd/koreader-2020.10%2BRota.apk?dl=0

If you still have the same bug on the new version I guess we need to workaround it for that device in particular, as both my tablet, my phone and every working emulator now behave as expected :)

@Galunid
Copy link
Member

Galunid commented Oct 11, 2020

@pazos This one works

@NiLuJe
Copy link
Member

NiLuJe commented Oct 11, 2020

Still good on my end, too ;).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android bug firmware help-wanted We'd like help with this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants