koreader / android-luajit-launcher

Android NativeActivity based launcher for LuaJIT, implementing the main loop within Lua land via FFI
MIT License
132 stars 84 forks source link

Fix Refresh Issue on Some Onyx Devices #435

Closed hugleo closed 10 months ago

hugleo commented 1 year ago

Fix Black/White refresh problem for Regal and HD Mode I'm considering using these changes for ULTRAC. If it doesn't work for other devices, perhaps a condition could be used to distinguish. Added comments in the code explaining the problem.


This change is Reviewable

Galunid commented 12 months ago

Hi, I'm not thrilled about this. The main issue with this PR is that it seems to use repaintEverything anytime we want to update the screen. I'm pretty sure it'll cause it to refresh full screen when browsing menu. It takes away ability to use different refresh modes. The actual cause of switching to fast mode is preventSystemRefresh I think. You can give it a try without that method. I'll take a look at the update onyx documentation. It wasn't available 3-4 years ago and I had to manually reverse engineer their APKs and sdk.

hugleo commented 12 months ago

The modes are,

For A2 Mode devices: https://help.boox.com/hc/en-us/articles/8569262708372-Refresh-Modes

For HD Mode/Regal devices: https://help.boox.com/hc/en-us/articles/10701257029780-Refresh-Modes

I'm not sure if it will work for A2 Mode devices.

However, with the ultrac and this PR, I haven't noticed the browsing menu refresh problem.

Here, I use the Onyx configuration Fast Mode for text-only PDFs/EPUBs since I'm okay with seeing the white screen for only a couple of pages.

But for comics, since I need to refresh each page, it's really annoying.

I created a GIF to demonstrate the refresh modes:

HD/Regal Refresh Style:

normal

pleasant

Fast Mode Refresh Style:

fast

blackwhite

The issue with the refreshScreen method is that when I switch to Balanced/Fast Mode/Ultra Fast it enters in Fast mode refresh style. However, when I switch back to HD/Regal it continues using the Fast mode refresh style indefinitely. The only way to revert to the HD/Regal refresh style is to reboot the device.

Galunid commented 12 months ago

Could you try building with preventSystemRefresh call commented out, but the rest of the code unchanged? Does that make the problem go away? If you want to set refresh every page, you can do that in Top menu -> 3rd tab (gear icon) -> Screen -> E-ink settings -> Full refresh rate

hugleo commented 12 months ago

The preventSystemRefresh simply causes the error method to be nonexistent in logcat. The remainder of the code continues, and the refresh is performed normally. I could use that one but I don't want the white screen style as explained in the previous comment.

Methods:

It seems to be the same behavior for Ultrac but refreshScreen doesn't reset the style to default. repaintEverything does the trick.

I keep getting this if I change back to High Mode or Regal: https://user-images.githubusercontent.com/4573505/268297071-cdbbe6ab-5010-4b22-b66e-5bd2c2ffb8e8.gif

But I want this: https://user-images.githubusercontent.com/4573505/268296999-fc0b63c7-87e2-4777-8c4e-a8ece6882336.gif

;-)

Galunid commented 12 months ago

In that case it seems it's better to create separate epd controller for UltraC model, so we don't break the other ones ;)

hugleo commented 12 months ago

I have two more questions:

1) From what I understand, you used refreshScreen instead of repaintEverything because the first time it's executed, it enters a mode that fixes the problem of koreader refreshing in the menu browser, right?

Because I see that currently, the other modes getWaveformPartial and getWaveformFullUi are implemented but not used since full-only is enabled. For future possible use I guess.

If it doesn't raise any problems we can refresh with repaintEverything when the mode is full, and otherwise, fall back to refreshScreen.

We can also set modes with:

Class.forName("android.view.View").getMethod("applyAppScopeUpdate", String::class.java, Boolean::class.javaPrimitiveType, Boolean::class.javaPrimitiveType,
                                          Integer.TYPE, Integer.TYPE).invoke(null, TAG, true, true, 8, Integer.MAX_VALUE)

But I think it's not worth the pain.

2) https://github.com/koreader/koreader-base/blob/master/ffi/framebuffer_android.lua

-- update the entire screen
function framebuffer:_updateFull()
    -- freescale ntx platform
    if has_eink_screen and (eink_platform == "freescale" or eink_platform == "qualcomm") then
        if has_eink_full_support then
            -- we handle the screen entirely
            self:_updatePartial(full, delay_page)
        else
            -- we're racing against system driver. Let the system win and apply
            -- a full update after it.
            self:_updatePartial(full, 500)
        end
    -- rockchip rk3x platform
    elseif has_eink_screen and (eink_platform == "rockchip") then
        android.einkUpdate(full)
    end
end

Regarding the method framebuffer_android.lua, qualcomm is falling to a 500 ms condition.

It was used for Nook as mentioned here.

Are you sure that delay_page isn't enough for Onyx devices?

Because if needed I think it should be increased or dealt with directly in the controller. If I create another qualcomm driver file, then I wouldn't need to override the behavior like I do with sleep(EINK_WAVEFORM_DELAY.toLong()).

Galunid commented 12 months ago

From what I understand, you used refreshScreen instead of repaintEverything because the first time it's executed, it enters a mode that fixes the problem of koreader refreshing in the menu browser, right?

No, it's because refreshScreen can be used to refresh any part of the screen (e.g. menu), using any mode we want (Fast/Partial/Full). As far as I understand repaintEverything is used for refreshing whole screen.

It seems there may be a bug https://github.com/koreader/android-luajit-launcher/blob/858573cc8808942a32f258fcfb1691c198887c3b/app/src/main/java/org/koreader/launcher/device/epd/OnyxEPDController.kt#L15

Should return "all" as far as I know, since onyx supports partial screen refreshes @pazos Could you check if it was simply by accident while refactoring the EPD components, or if there was some reason it was changed?

If it was indeed a bug, then we should be using delay_page.

hugleo commented 12 months ago

Now things make sense for me.

There are two bugs then.

I think it could be a regression that breaks things.

https://github.com/koreader/android-luajit-launcher/compare/f2d946b3b49e728272df4cb56185a2fe57cdb4ff...b3b3c3c14d81697097d2160f7a322c9700ba0015#diff-820e361a75897cc66e8cd9904645b9e6f4a2647f169ed31193cf56c341b79735

Analyzing your commit at that time I see that you implemented the modes in assets/android.lua. However now the modes are implemented in the Kotlin class files.

I didn't check further, but I believe the file QualcommOnyxEPDController.kt was replaced with OnyxEPDController.kt in other commits, and it didn't incorporate the required partial refresh

pazos commented 12 months ago

Could you check if it was simply by accident while refactoring the EPD components, or if there was some reason it was changed?

I've tried but the mess with submodules makes impossible to figure out things without spending a lot of time triaging pairs of commits.

Please double check yourself, better if on device, what happens if you make it to request an update on each partial refresh.

Should return "all" as far as I know, since onyx supports partial screen refreshes

I didn't intend to change the behaviour of any driver. AFAICT I copied the previous behaviour in https://github.com/koreader/android-luajit-launcher/pull/322, but maybe I missunderstand something.

It should be easily testable. Pick a release from late 2020 and compare partial refreshes with a 2023 release and tell me. You can grab the logcat and figure out if these partial refreshes are requested.

Also, any device with support for partial refreshes can be used as full update only. That's the behaviour on Nooks, for instance. AFAIK the only device where we do each update ourselves is in Tolinos.

Here's your original PR: https://github.com/koreader/koreader-base/pull/1221

Based on the conversation there it seems that we're talking about partial refreshes and how to implement them in Onyx. So probably you're right and I mess some stuff.

Oh, boy, this is hard to maintain :p

Anyhow, please keep in mind the onyx devices added during the last couple of years inherited the new behaviour. I cannot link right now but I do remember to see a lot of logs where the schim to prevent the screen refresh was triggering a catched exception.

So, if you check the behaviour isn't the intended one instead of moving all onyx devices to the full behaviour just create a new class with that slightly modification and move the devices where you can test that behaviour.

hugleo commented 12 months ago

Could you check if it was simply by accident while refactoring the EPD components, or if there was some reason it was changed?

I've tried but the mess with submodules makes impossible to figure out things without spending a lot of time triaging pairs of commits.

Please double check yourself, better if on device, what happens if you make it to request an update on each partial refresh.

Should return "all" as far as I know, since onyx supports partial screen refreshes

I didn't intend to change the behaviour of any driver. AFAICT I copied the previous behaviour in #322, but maybe I missunderstand something.

It should be easily testable. Pick a release from late 2020 and compare partial refreshes with a 2023 release and tell me. You can grab the logcat and figure out if these partial refreshes are requested.

Also, any device with support for partial refreshes can be used as full update only. That's the behaviour on Nooks, for instance. AFAIK the only device where we do each update ourselves is in Tolinos.

Here's your original PR: koreader/koreader-base#1221

Based on the conversation there it seems that we're talking about partial refreshes and how to implement them in Onyx. So probably you're right and I mess some stuff.

Oh, boy, this is hard to maintain :p

Anyhow, please keep in mind the onyx devices added during the last couple of years inherited the new behaviour. I cannot link right now but I do remember to see a lot of logs where the schim to prevent the screen refresh was triggering a catched exception.

The preventSystemRefresh should have a similar method for other devices. I think it's applyAppScopeUpdate, or I don't remember now, but I think I saved it in my annotations. When I have some time I'll try if partial refresh works on my device. The Onyx Ultrac (or Ultra, etc.) normal mode is very accepted for browsing the menu IMHO (slower than the fast modes of course). I can also enable system optimizations if I want, the good thing is that the device assumes Koreader behavior on full refresh (I like that because in some heavy images, Koreader eliminates 100% of ghosts, I think. Better than the default system refresh or Onyx default ereader). It's hard to say for other devices since the "GPU" thing could mean something more.

So, if you check the behaviour isn't the intended one instead of moving all onyx devices to the full behaviour just create a new class with that slightly modification and move the devices where you can test that behaviour. I don't know if Galunid still has the device. It's been some years :D.

Galunid commented 11 months ago

Great, so it seems it was unintended. Sadly I don't have device on hand so I cannot test it myself. I borrowed it to my friend. I'll test it next time I see her. @hugleo Do let me know when you have results of your tests. Partial refreshes should offer a better experience ;)

hugleo commented 11 months ago

I've configured this driver for 'all' and tested it. This causes a full refresh with any menu action. As a result it will cause serious problems for any Onyx device that does not support this modes. I suggest keeping this driver as it is and creating two additional drivers: one with the 'repaintEverything' thing and one with the 'all modes refresh' enabled. With time people will migrate to the driver that is more suitable for them.

hugleo commented 10 months ago

Closing this one because onyx fixed the black/white overlay in the currently firmware. Both refresh functions seems to work similar for ultrac now.

Will not change self:_updatePartial(full, 500) for now since 500 ms seems not too noticeable, maybe in some future. framebuffer_android.lua