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

Separated unicapture #128

Merged
merged 2 commits into from
Feb 7, 2025
Merged

Separated unicapture #128

merged 2 commits into from
Feb 7, 2025

Conversation

mariotaku
Copy link
Member

Make unicapture (somewhat) separated. This library does great job capturing screen content, and can be used by webos-vncserver: Informatic/webos-vncserver#6 .

@mariotaku mariotaku requested a review from sundermann February 6, 2025 11:36
@sundermann sundermann requested a review from Copilot February 6, 2025 23:25

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 8 out of 12 changed files in this pull request and generated no comments.

Files not reviewed (4)
  • CMakeLists.txt: Language not supported
  • fbs/CMakeLists.txt: Language not supported
  • unicapture/CMakeLists.txt: Language not supported
  • unicapture/unicapture.c: Language not supported
Copy link
Contributor

@sundermann sundermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, though I'm wondering if ARGB is needed for VNC. Blending in the alpha channel saves 25% bandwidth at the cost of some computation but it could be worth it.

@@ -11,9 +11,11 @@ ExternalProject_Add(flatcc-native
URL_HASH SHA512=46ba5ca75facc7d3360dba797d24ae7bfe539a854a48831e1c7b96528cf9594d8bea22b267678fd7c6d742b6636d9e52930987119b4c6b2e38d4abe89b990cae
DOWNLOAD_EXTRACT_TIMESTAMP TRUE
CMAKE_ARGS
-DCMAKE_C_COMPILER=/usr/bin/cc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against changing this, but I'm just wondering why it's needed as in my testing the CMAKE_TOOLCHAIN_FILE from the main cmake was not passed automatically when using ExternalProject_Add.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my machine, environment-setup was sourced, so CC environment variable was set. Maybe I could try changing that on my computer instead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

environment-setup is not needed with cmake projects. It's enough to pass the toolchain file only.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried a few combination:

  1. Set -DCMAKE_TOOLCHAIN_FILE in configuration step: ExternalProject won't have toolchain specified
  2. Set CMAKE_TOOLCHAIN_FILE in environment variable (just like sourcing environment-setup): ExternalProject will have that toolchain too

This change seems to be out of scope for this time, so I'll revert the last commit :)

@mariotaku
Copy link
Member Author

@sundermann Yes, for VNC library, it only expects 1/2/4 bytes per pixel. We need to give it XRGB.

@mariotaku mariotaku force-pushed the feature/separated-unicapture branch from 27cde35 to 177e8cf Compare February 7, 2025 00:21
@mariotaku mariotaku merged commit 61e0611 into main Feb 7, 2025
4 checks passed
@mariotaku mariotaku deleted the feature/separated-unicapture branch February 7, 2025 00:22
@Syspoke
Copy link

Syspoke commented Feb 7, 2025

@mariotaku Hi, is it normal that we no longer have the .so libraries included in the zip artifacts build?
I think we need to copy them from the build/unicapture/ directory now by modifying the build.yml file.
Thanks.

@mariotaku
Copy link
Member Author

@Syspoke Sorry, I have introduced this build issue. I'll fix it in another commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants