timpushkin / keypointdetectandroid Goto Github PK

Libraries receive an RGB image as input and convert them to the required format themselves, so there is no need for restrictions on the output of a grayscale image.

Currently in lib module Kotlin build frequently starts before CMake build finishes for some reason, so Scapix bridge is not yet ready when it is required for building Kotlin code. This can be fixed by manually adding dependencies between these tasks.

Now, for small camera resolutions, an image of the same size is displayed, but it is preferable to have an image with keypoints scaled according to the screen parameters.

Actually, exactly the same code is used here twice. You can create a function for this -- something like function(get_opencv repo) where repo argument will be either opencv or opencv_contrib (this one argument seems to be enough).

Originally posted by @TimPushkin in #13 (comment)

SuperPoint seems to either run on main thread or use smartphone so heavily that it makes the main thread freeze. When the image is large, it is enough to get app's process killed.

Move keypoint detection algorithms' names to resources and add localization support.

Currently SuperPoint accepts Context in its constructor but only uses it for initialization and doesn't save links to it. This could be replaced with a builder that will accept Context and provide SuperPoint with the assets it needs.

Currently we are using RGBA_8888 images from CameraX and then convert them to the formats we need. The problem is that CameraX actually captures the images in YUV_420_888 and then coverts them RGBA_8888. We can reduce the computational overhead by accepting YUV_420_888 instead and converting it straight to our formats.

Currently native library has a single header with all of the classes put together. This is because Scapix currently requires structs to be defined in the same header where they are used in a ported class and all of the library's classes use the same structs.

This is not a good thing as the header might grow too large with time, so it would be great to figure something out. Or just wait for Scapix to get rid of this requirement.

At the moment, the application handles a camera only in onCreate, onResume and onDestroy.

Currently when phone is rotated the camera resolution may change because the previously picked one may be no longer supported (because it's been also rotated). It can be solved by remembering display mode (landscape/portrait) and rotating the saved resolution if the mode's been changed.

• Change native code so that OpenCV makes use of OpenCL
• Configure OpenVX downloading and installation
• Investigate and fix OpenCV's JPEG lib conflict with LTO and Thin LTO

Would be great to have some logging in each public method both for standard C++ and for Android. Something like what's done here.

Would be great to add the following CI for native library:

• Linting (cpplint, clang-tidy)
• Build with examples
• Build with language bridge generation

Лучше обзывать всё Keypoints -- features слишком многозначное слово

Originally posted by @anastasiia-kornilova in #8 (comment)

Maybe we don't even need to print descriptors as it looks like a mess in the output -- just put descriptor size for each keypoint (in some algorithms the size may be dynamic, though, not in ours).

Originally posted by @TimPushkin in #13 (review)

At the moment, 180 and 270 degree turns of incorrectly oriented images are performed by 2 and 3 turns by 90 degrees, respectively. This can be optimized.

You probably can move a lot of duplicated code from the detectors in their common abstract parent.

Originally posted by @TimPushkin in #13 (review)

The current example can be modified to draw keypoints on the input image and save it using OpenCV

For some reason I now get ClassCastException here where a lambda is being cast to LifecycleEventObserver.

In keypoint detection from from file system, keypoints are not removed when the image changes which should be fixed.

• Add a graphical hint for a user about the existence of the side menu
• Enlarge measurement time text description
• Add current algorithm name

In the example it would be better to accept a command line argument with the algorithm name and run only the selected one because running all three is pretty messy.

Originally posted by @TimPushkin in #13 (review)

Would be great to add the following Android CI:

• Linting (detekt and maybe ktlint) for both Gradle modules
• Building of app which includes building lib

When Detekt is configured in multiple modules, it produces warnings like this

SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/home/runner/.gradle/caches/modules-2/files-2.1/io.gitlab.arturbosch.detekt/detekt-formatting/1.21.0/b7e14d2040b8c60922[27](https://github.com/TimPushkin/KeypointDetectAndroid/runs/7728872264?check_suite_focus=true#step:5:28)a0056223e9f36cc54a6/detekt-formatting-1.21.0.jar!/org/slf4j/impl/StaticLoggerBinder.class]
> Task :app:detekt
SLF4J: Found binding in [jar:file:/home/runner/.gradle/caches/modules-2/files-2.1/org.slf4j/slf4j-nop/1.7.36/a3c1eb685d59414527faa93623acae311c184032/slf4j-nop-1.7.36.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [org.slf4j.helpers.NOPLoggerFactory]

It doesn't fail runs in CI as these are warnings, not errors.

Currently the main README has outdated GUI screenshots which need to be updated