Replace decoders with libvips, add tests and rename package #1
Conversation
Rodrigodd
force-pushed
the
integrate-libvips
branch
from
d92f53d to
a646d50
Compare
|
About the benchmarks (that was in the TODO of Rodrigodd#1) can you also include one for the old decoder. someone was interested in comparison. |
|
Also change the |
|
Also we don't want to allow every arbitrary format so there should be a whitelist (refer to the |
|
And for tests you can use https://github.com/ReactiveCircus/android-emulator-runner (there is a lit of production usage at the bottom) |
|
I added a benchmark module, but I could not retrieve the profile information. The gradlew task errors out in a adb command timeout, and I could not find the profile files in my phone. The benchmark still need to sample regions in a more suitable distribution, test more image formats, etc. And then rebase it to before making all changes, to compare the perfomance. I also cherry-pick @wwww-wwww fix for the color profile. But loading the test images he provide me on Mihon makes it crash. I still could not reproduce it in a unit-test and neither Debug Mihon directly. But I don't know if his changes is causing it, I only test the images after the changes. I don't have much time throughout the week, but I will continue to slowly work on those. |
Rodrigodd
force-pushed
the
integrate-libvips
branch
from
8489abb to
7394f33
Compare
Also only run assembleDebug for now, full assemble is too slow.
Also rely on the existing build tool of each external project, instead of patching CMake into them. Most `ExternalProject_Add` wrappers were copied from Komelia, at [https://github.com/Snd-R/Komelia/tree/8b938397e5c8743d4e995d2cc16203367de6f67d/image-decoder/native/cmake], but modified to make sure all libraries are staticaly linked.
Took me a while to figure out how the API work
Was trying to optimize by cropping the image to the desired region, and then resizing only that, but libvips seems to already be smart enough, so that is not necessary.
The intermediate buffer is not necessary, the `decode` function will already take care of outputing in the correct format.
Was looping to detect memory corruption. Not needed anymore.
And groupId/artifactId as `dev.mihon:image-decoder`
Rodrigodd
force-pushed
the
integrate-libvips
branch
from
7394f33 to
5866515
Compare
Only `bounds` were being used, so I only keeped that.
This folder is genrated by clangd
Mihon was crashing somethimes crashing when decoding the first image. I notice Mihon was decoding two images at the same time, which made me think it could be a threading issue. Reading libvips documentation, it states that it is safe, but VIPS_INIT must only be called in a single-thread. I was calling VIPS_INIT for every decoder created, which I believe was the issue. This commit moves VIPS_INIT to JNI_OnLoad, which is called only once when the library is loaded.
Rodrigodd
force-pushed
the
integrate-libvips
branch
from
5866515 to
863efc9
Compare
The emulator is failing to create due to lack of space. Maybe the previous build is using too much? I removed the build step, but it will still be built after the emulator is created, let's see if it will still break.
This will just check if the benchmark can run without errors.
Rodrigodd
force-pushed
the
integrate-libvips
branch
from
02ca207 to
a71fc01
Compare
I am not very confident that building with the cache will work, even more if we modify the CMakeLists.txt file between builds. But waiting >20min per build is very painful (and there are both Debug and Release builds).
Rodrigodd
force-pushed
the
integrate-libvips
branch
from
a1179a7 to
e78dbd8
Compare
The typo came from Androids documentation...
|
Got the tests to run on CI, and I got the benchmarks to work in both the old and new implementation. I could not get a reliable benchmark on my device, but it roughly appears that both implementations have the same performance: I think the only thing missing now is the resizing in linear color space. From some tests, I see that getting it to work would have a performance cost of 2.5x to 5x to 10x, depending on how it is implemented or measured. Here is the benchmark I used, but I only measure the performance, I didn't evaluated if the results are any good (I also do not yet know how to do that), so the implementations in that benchmark may be flawed. @AntsyLich, @wwww-wwww it is important to get linear color space resizing before merging this? I think it is important for manga, due to the heavy use of half-tone shading. But the previous implementation had that? That is, would merging this bring a regression? |
|
Did you mean the previous implementation had color space resizing? |
|
Yeah, I am asking if the previous implementation already does the resizing in a linear light color space, or it is just a new feature we want. |
This PR integrates libvips to get a better algorithm for downsampling images (mangas frequently use half-tone shading, which is very susceptible to aliasing when downsampling). As a bonus, it will allow moving all image format implementations upstream, decreasing the maintenance burden. Libvips also aims to efficiently partially decoding images, possibly decreasing CPU and memory usage (but this still need be measure).
Early discussions: Rodrigodd#1, tachiyomiorg#15
From my tests, the downsampling filter didn't help much.
subsampling-scale-image-viewwill still draw tiles up to 2x the screen resolution, and halftone patterns in manga are not small enough to be filtered out.This PR is still missing:
nativeFindInfo. It is still using the old code, because libvips does not have a proper way of querying image format info. Maybe we need to expand this function to include image formats that libvips supports but we didn't before.