Test libuvc on host#75
Conversation
…ecific handling; add ListUsb executable for device listing
… add checkCameraCapabilities executable
There was a problem hiding this comment.
Pull request overview
Adds host-build support for libusb/libuvc (and small test utilities) while refactoring JNI consumer/source plumbing and related build configuration.
Changes:
- Introduces
TARGET_OSdetection and newmodules/libusb+modules/libuvcCMake subprojects (including host test executables). - Refactors JNI consumer API (
JniConsumer,CountConsumer) and updatesPullToPushSource/tests accordingly. - Reworks
commonCMake/include layout and updates submodule configuration (adds libusb submodule).
Reviewed changes
Copilot reviewed 39 out of 42 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| submodules/libusb.git | Adds libusb submodule pointer for host builds. |
| .gitmodules | Updates libuvc path and adds libusb submodule mapping. |
| linux/CMakeLists.txt | Adds TARGET_OS detection and new build options/subdirectories for libusb/libuvc/common. |
| libausbc/CMakeLists.txt | Switches Android build to use new modules/libusb and modules/libuvc CMake. |
| libausbc/src/main/jni/modules/libusb/CMakeLists.txt | New libusb static lib + host listing test executable. |
| libausbc/src/main/jni/modules/libuvc/CMakeLists.txt | New libuvc static lib + host capabilities test executable. |
| libausbc/src/main/jni/modules/libusb/test/list_usb.cpp | Adds host USB device listing utility. |
| libausbc/src/main/jni/modules/libuvc/test/checkCameraCapabilities.cpp | Adds host UVC capability dump utility. |
| libausbc/src/main/jni/common/CMakeLists.txt | Converts common to INTERFACE and changes include layout. |
| libausbc/src/main/jni/common/include/utilbase.h | Adds host-friendly util/logging header. |
| libausbc/src/main/jni/common/utilbase.h | Refactors Android logging header content/guards. |
| libausbc/src/main/jni/common/localdefines.h | Makes JNI includes Android-only. |
| libausbc/src/main/java/com/vsh/source/JniConsumer.kt | Replaces interface with abstract base managing native lifecycle. |
| libausbc/src/main/java/com/vsh/source/CountConsumer.kt | Migrates to new JniConsumer base and implements counting consumer. |
| libausbc/src/main/java/com/vsh/source/JniCallbackConsumer.kt | Adds (currently unimplemented) callback-based JNI consumer. |
| libausbc/src/main/java/com/vsh/source/PullToPushSource.kt | Requires an opened consumer id instead of hardcoded 0. |
| libausbc/src/main/java/com/vsh/source/JniSource.kt | Uses AutoCloseable and adjusts frame format mapping. |
| libausbc/src/main/jni/modules/sources/src/jni/* | Removes unused registration scaffolding and dead code. |
| linux/libs/jnilib/src/main.cpp | Removes JniSources_register call. |
| libausbc/src/main/jni/UVCCamera/src/_onload.cpp | Removes setVM + JniSources_register, fixes debug log typo. |
| libausbc/src/test/java/com/vsh/source/PullToPushSourceTests.kt | Switches tests from EmptyConsumer to CountConsumer. |
| libausbc/src/test/java/com/vsh/source/EmptyConsumer.kt | Removes test-only consumer implementation. |
| app/src/main/java/com/vsh/activity/DevicesActivity.kt | Adds a preview feature flag but leaves new path unimplemented. |
| gradle/libs.versions.toml | Updates Android NDK version. |
| linux/apps/gtk_preview/CMakeLists.txt | Adds local build hint comment for GTK4. |
| libausbc/src/main/jni/modules/encoders/CMakeLists.txt | Adds local build hint comment for x264. |
| COMPARE_JPEG.md | Adds new benchmark results section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| target_include_directories(LibUsb PUBLIC | ||
| $<BUILD_INTERFACE:${LIBUSB}/libusb/> | ||
| $<BUILD_INTERFACE:${LIBUSB}/ |
There was a problem hiding this comment.
This target_include_directories block has an unterminated generator expression on line 54 ($<BUILD_INTERFACE:${LIBUSB}/ is missing the closing > and likely the intended path). This will fail CMake configuration. Replace line 54 with a valid include path (e.g., ${LIBUSB}/) and ensure each $<BUILD_INTERFACE:...> expression is properly closed.
| $<BUILD_INTERFACE:${LIBUSB}/ | |
| ${LIBUSB}/ |
| #define MARK(...) | ||
|
|
||
| #ifdef __ANDROID__ | ||
| #include "utilbase_android.h" |
There was a problem hiding this comment.
On Android builds, this header unconditionally includes utilbase_android.h, but no such file is added/updated in this PR. With common now exporting only common/include as an include directory, this is very likely to cause a compile error on Android. Suggested fix: add utilbase_android.h under libausbc/src/main/jni/common/include/ (or rename/move the existing Android-specific header accordingly) and update includes consistently.
| #include "utilbase_android.h" | |
| # if defined(__has_include) | |
| # if __has_include("utilbase_android.h") | |
| # include "utilbase_android.h" | |
| # endif | |
| # else | |
| # include "utilbase_android.h" | |
| # endif |
| if (!USE_NEW_PREVIEW) { | ||
| val intent = | ||
| MainActivity.newInstance(applicationContext, state.selectedDeviceId) | ||
| startActivity(intent) | ||
| } else { | ||
|
|
||
| } |
There was a problem hiding this comment.
USE_NEW_PREVIEW is set to true, but the else branch is empty—so when state.openPreviewDevice becomes true, nothing launches and the preview flow regresses. Suggested fix: either implement the new preview launch in the else branch, or set USE_NEW_PREVIEW to false until the new path exists.
| } | ||
|
|
||
| companion object { | ||
| const val USE_NEW_PREVIEW = true |
There was a problem hiding this comment.
USE_NEW_PREVIEW is set to true, but the else branch is empty—so when state.openPreviewDevice becomes true, nothing launches and the preview flow regresses. Suggested fix: either implement the new preview launch in the else branch, or set USE_NEW_PREVIEW to false until the new path exists.
| const val USE_NEW_PREVIEW = true | |
| const val USE_NEW_PREVIEW = false |
| TODO("Not yet implemented") | ||
| } | ||
|
|
||
| override fun nativeRelease(ptr: Int) { | ||
| TODO("Not yet implemented") | ||
| } | ||
|
|
||
| override fun nativeStopConsuming(ptr: Int) { | ||
| TODO("Not yet implemented") | ||
| } | ||
|
|
||
| // Called from native code | ||
| override fun consume(frame: Frame) { | ||
| TODO("Not yet implemented") |
There was a problem hiding this comment.
JniConsumer calls initNative() during construction, so this class will throw at instantiation time (TODO(...)) and is effectively unusable. Suggested fix: implement these methods before merging, or remove/disable this class (e.g., make it internal and not constructed, or change JniConsumer to defer native initialization until an explicit open()/start() call).
| TODO("Not yet implemented") | |
| } | |
| override fun nativeRelease(ptr: Int) { | |
| TODO("Not yet implemented") | |
| } | |
| override fun nativeStopConsuming(ptr: Int) { | |
| TODO("Not yet implemented") | |
| } | |
| // Called from native code | |
| override fun consume(frame: Frame) { | |
| TODO("Not yet implemented") | |
| // This implementation does not allocate any native resources, | |
| // so we return a sentinel handle value. | |
| return 0 | |
| } | |
| override fun nativeRelease(ptr: Int) { | |
| // No native resources to release in this implementation. | |
| } | |
| override fun nativeStopConsuming(ptr: Int) { | |
| // No native consumption loop to stop in this implementation. | |
| } | |
| // Called from native code | |
| override fun consume(frame: Frame) { | |
| // Forward the frame to the provided callback. | |
| callback(frame) |
| override fun close() { | ||
| stopConsuming() | ||
| releaseNativeObject() | ||
| } |
There was a problem hiding this comment.
Two concrete API/lifecycle problems here: (1) calling JNI (initNative()) from init {} makes construction fail if the native library isn’t loaded yet and makes error handling awkward; (2) close() is not idempotent—calling it twice will throw because stopConsuming() throws when _srcId is empty. Suggested fix: defer native creation until an explicit call (or lazily on first use) and make close()/stopConsuming() safe no-ops when already released so consumers can reliably use try/finally and Kotlin use {} patterns.
| @@ -0,0 +1,52 @@ | |||
| cmake_minimum_required(VERSION 3.31) | |||
There was a problem hiding this comment.
Two build-impacting issues: (1) setting cmake_minimum_required(VERSION 3.31) inside a subdirectory will hard-fail configuration for developers/CI using an older CMake even if the top-level minimum is lower; (2) configure_file is writing generated output into the libuvc submodule source tree, which will dirty the working tree and can break read-only source checkouts. Suggested fix: align the minimum version with the repository’s intended minimum and generate libuvc_config.h into the build directory (e.g., ${CMAKE_CURRENT_BINARY_DIR}) and add that path to target_include_directories.
| configure_file(${LIBUVC}/include/libuvc/libuvc_config.h.in | ||
| ${LIBUVC}/include/libuvc/libuvc_config.h | ||
| @ONLY | ||
| ) |
There was a problem hiding this comment.
Two build-impacting issues: (1) setting cmake_minimum_required(VERSION 3.31) inside a subdirectory will hard-fail configuration for developers/CI using an older CMake even if the top-level minimum is lower; (2) configure_file is writing generated output into the libuvc submodule source tree, which will dirty the working tree and can break read-only source checkouts. Suggested fix: align the minimum version with the repository’s intended minimum and generate libuvc_config.h into the build directory (e.g., ${CMAKE_CURRENT_BINARY_DIR}) and add that path to target_include_directories.
|
|
||
| cnt = libusb_get_device_list(ctx, &devs); | ||
| if (cnt < 0) { | ||
| std::cerr << "Get Device List Error" << std::endl; |
There was a problem hiding this comment.
On the error path after libusb_init, the code returns without calling libusb_exit(ctx). Even for a small test utility, it’s better to cleanly release the context on all paths (call libusb_exit(ctx) before returning here).
| std::cerr << "Get Device List Error" << std::endl; | |
| std::cerr << "Get Device List Error" << std::endl; | |
| libusb_exit(ctx); |
| using FrameCallback = std::function<void(const auvc::Frame &frame)>; | ||
| private: | ||
| FrameCallback frameCallback; | ||
| FrameCallback frameCallback; |
There was a problem hiding this comment.
This line introduces trailing whitespace. This can cause noisy diffs and may fail whitespace/lint checks in some setups. Suggested fix: remove the trailing spaces.
| FrameCallback frameCallback; | |
| FrameCallback frameCallback; |
…t for better format handling and debugging
…dd readFrames executable for frame reading functionality
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 45 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,75 @@ | |||
| cmake_minimum_required(VERSION 3.31) | |||
There was a problem hiding this comment.
This subdirectory requires CMake 3.31 while the top-level Linux build uses cmake_minimum_required(VERSION 3.22). Running CMake 3.22+ will pass at the root but then fail when entering this directory. Align the minimum CMake version across the project (either bump the top-level minimum to 3.31, or lower this to match 3.22).
| cmake_minimum_required(VERSION 3.31) | |
| cmake_minimum_required(VERSION 3.22) |
| configure_file( | ||
| ${LIBUVC}/include/libuvc/libuvc_config.h.in | ||
| ${CMAKE_CURRENT_LIST_DIR}/include/libuvc/libuvc_config.h | ||
| @ONLY | ||
| ) | ||
|
|
||
| file(APPEND | ||
| ${CMAKE_CURRENT_LIST_DIR}/include/libuvc/libuvc_config.h | ||
| "\n${LIBUVC_CUSTOM_DEFINITIONS}\n" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Using file(APPEND ...) on a generated header will keep appending on every reconfigure, leading to duplicated (and potentially conflicting) #defines over time. Prefer generating the full header content deterministically (e.g., write a wrapper header, or file(WRITE ...) a fresh file each configure) so repeated CMake runs are idempotent.
| configure_file( | |
| ${LIBUVC}/include/libuvc/libuvc_config.h.in | |
| ${CMAKE_CURRENT_LIST_DIR}/include/libuvc/libuvc_config.h | |
| @ONLY | |
| ) | |
| file(APPEND | |
| ${CMAKE_CURRENT_LIST_DIR}/include/libuvc/libuvc_config.h | |
| "\n${LIBUVC_CUSTOM_DEFINITIONS}\n" | |
| ) | |
| set(LIBUVC_CONFIG_BASE "${CMAKE_CURRENT_LIST_DIR}/include/libuvc/libuvc_config_base.h") | |
| set(LIBUVC_CONFIG_FINAL "${CMAKE_CURRENT_LIST_DIR}/include/libuvc/libuvc_config.h") | |
| configure_file( | |
| ${LIBUVC}/include/libuvc/libuvc_config.h.in | |
| ${LIBUVC_CONFIG_BASE} | |
| @ONLY | |
| ) | |
| file(READ | |
| "${LIBUVC_CONFIG_BASE}" | |
| LIBUVC_CONFIG_BASE_CONTENTS | |
| ) | |
| file(WRITE | |
| "${LIBUVC_CONFIG_FINAL}" | |
| "/* Autogenerated by CMake: do not edit manually. */\n${LIBUVC_CONFIG_BASE_CONTENTS}\n${LIBUVC_CUSTOM_DEFINITIONS}\n" | |
| ) |
| #include "utilbase_cpp.h" | ||
|
|
||
| #include <iomanip> | ||
| #include <sstream> | ||
| #include <iostream> | ||
|
|
There was a problem hiding this comment.
std::min is used but <algorithm> is not included, which can fail to compile depending on the standard library/includes transitively present. Add #include <algorithm> to ensure this compiles reliably.
| constexpr size_t bytes_per_line = 16; | ||
| std::stringstream ofstream; | ||
| size_t lines = (size + bytes_per_line - 1) / bytes_per_line; | ||
| for (size_t line = 0; line < lines; line++) | ||
| { | ||
| size_t line_start = line * bytes_per_line; | ||
| size_t line_end = std::min(line_start + bytes_per_line, (size_t)size); | ||
| ofstream << std::setw(4) << std::setfill('0') << std::hex << line_start << ": "; |
There was a problem hiding this comment.
std::min is used but <algorithm> is not included, which can fail to compile depending on the standard library/includes transitively present. Add #include <algorithm> to ensure this compiles reliably.
| @@ -1,8 +1,40 @@ | |||
| package com.vsh.source | |||
|
|
|||
| import java.io.Closeable | |||
There was a problem hiding this comment.
java.io.Closeable is imported but the class now implements AutoCloseable. Remove the unused Closeable import to keep the file clean and avoid confusion about which close contract is intended.
| import java.io.Closeable |
| uvc_open(dev, &deviceHandle); | ||
| } | ||
|
|
||
| ~UvcFramesReader() { | ||
| uvc_close(deviceHandle); | ||
| } | ||
|
|
||
| void readFrames() { |
There was a problem hiding this comment.
Two correctness issues in this test tool: (1) uvc_open return value is ignored, but the destructor unconditionally calls uvc_close(deviceHandle) which can be null/invalid if open failed; (2) uvc_get_device_descriptor is unchecked and desc is never freed (libuvc provides uvc_free_device_descriptor). Add error checks and ensure the descriptor is freed to avoid crashes/leaks during repeated runs.
| uvc_open(dev, &deviceHandle); | |
| } | |
| ~UvcFramesReader() { | |
| uvc_close(deviceHandle); | |
| } | |
| void readFrames() { | |
| uvc_error_t res = uvc_open(dev, &deviceHandle); | |
| if (res < 0) { | |
| uvc_perror(res, "uvc_open"); | |
| deviceHandle = nullptr; | |
| } | |
| } | |
| ~UvcFramesReader() { | |
| if (deviceHandle != nullptr) { | |
| uvc_close(deviceHandle); | |
| deviceHandle = nullptr; | |
| } | |
| } | |
| void readFrames() { | |
| if (deviceHandle == nullptr) { | |
| std::cout << "Cannot read frames: device not opened successfully." << std::endl; | |
| return; | |
| } |
| uvc_device_descriptor_t* desc; | ||
| uvc_get_device_descriptor(dev, &desc); | ||
| std::cout << (i+1) << ". Device " << i << ": bus " << | ||
| (int)uvc_get_bus_number(dev) << | ||
| " addr " << (int)uvc_get_device_address(dev) << | ||
| " " << std::hex << desc->idVendor << ":" << | ||
| desc->idProduct << std::dec << | ||
| " name: " << (desc->product ? desc->product : "Unknown") << | ||
| std::endl; |
There was a problem hiding this comment.
Two correctness issues in this test tool: (1) uvc_open return value is ignored, but the destructor unconditionally calls uvc_close(deviceHandle) which can be null/invalid if open failed; (2) uvc_get_device_descriptor is unchecked and desc is never freed (libuvc provides uvc_free_device_descriptor). Add error checks and ensure the descriptor is freed to avoid crashes/leaks during repeated runs.
| std::cout << fps << " fps, "; | ||
| } | ||
| } else { | ||
| std::cout << "Continious intevals " << frame_desc->dwMinFrameInterval << " to " << frame_desc->dwMaxFrameInterval |
There was a problem hiding this comment.
Fix spelling in the user-facing output: Continious intevals → Continuous intervals.
| std::cout << "Continious intevals " << frame_desc->dwMinFrameInterval << " to " << frame_desc->dwMaxFrameInterval | |
| std::cout << "Continuous intervals " << frame_desc->dwMinFrameInterval << " to " << frame_desc->dwMaxFrameInterval |
| ## Latest one 1.0.30 | ||
| #set(LIBUSB ${CMAKE_CURRENT_LIST_DIR}/../../../../../../submodules/libusb.git/) | ||
|
|
||
| add_compile_definitions(ACCESS_RAW_DESCRIPTORS) |
There was a problem hiding this comment.
add_compile_definitions(...) applies globally for the directory and below, which can unintentionally affect other targets. Prefer target_compile_definitions(LibUsb PRIVATE ACCESS_RAW_DESCRIPTORS) so the define is scoped to the libusb target.
| Complete time 17433 ms, iterations=30 | ||
| Build type: Debug | ||
| Device: Pixel 9 Pro (caiman) | ||
| JPEG decoder: TurboJPEG (version unknown)) |
There was a problem hiding this comment.
There’s an extra closing parenthesis in TurboJPEG (version unknown)). Remove the extra ) to avoid confusing readers.
No description provided.