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.

Sign up for GitHub

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

Jump to bottom

Implement VNC backend #63

Merged
merged 2 commits into from
Nov 4, 2024
Merged

Implement VNC backend #63

merged 2 commits into from
Nov 4, 2024

Conversation

ndsl7109256
Copy link
Collaborator

@ndsl7109256 ndsl7109256 commented Oct 24, 2024 •
edited
Loading

Add VNC backend using neatvnc1. Allow users can view and interact with applications in Mado through VNC-compatible client software.

To test the new feature:

  1. Choose VNC server output support as backend selection
image
  1. Build and run
$ make
$ ./demo-vnc
  1. Use VNC client software to connect to Mado VNC server
2024-10-24.10.34.16.mov

jserv
jserv reviewed Oct 24, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
@jserv jserv requested a review from Bennctu October 24, 2024 04:21
shengwen-tw
shengwen-tw reviewed Oct 24, 2024
View reviewed changes
backend/vnc.c Show resolved Hide resolved
backend/vnc.c Show resolved Hide resolved
backend/vnc.c Outdated Show resolved Hide resolved
backend/vnc.c Show resolved Hide resolved
Bennctu
Bennctu reviewed Oct 24, 2024
View reviewed changes
Makefile
ifeq ($(CONFIG_BACKEND_VNC), y)
BACKEND = vnc
libtwin.a_files-y += backend/vnc.c
libtwin.a_cflags-y += $(shell pkg-config --cflags neatvnc aml pixman-1)
Copy link
Collaborator

@Bennctu Bennctu Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In #55, it also uses pixman features. Therefore, whoever merges first, another person needs to pay attention to the adjustment of pixman patch in the Makefile.

jserv
jserv reviewed Oct 24, 2024
View reviewed changes
build-neatvnc.sh Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 25, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 25, 2024
View reviewed changes
tools/build-neatvnc.sh Outdated Show resolved Hide resolved
@ndsl7109256 ndsl7109256 force-pushed the vncbackend branch 3 times, most recently from 5e4fd44 to 5920459 Compare October 25, 2024 06:45
jserv
jserv reviewed Oct 25, 2024
View reviewed changes
tools/build-neatvnc.sh Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 25, 2024
View reviewed changes
README.md Outdated
For those who want to run demo program with VNC as the backend, it is necessary to install the [aml](https://github.com/any1/aml) and [neatvnc](https://github.com/any1/neatvnc) libraries. Please note that using package managers like apt might install outdated versions of these libraries. To ensure you have the latest versions, you can build them from source by running the provided script:

```bash
$sudo ./tools/build-neatvnc.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't run with sudo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since ninja -C build install requires root access, may I know the reason for removing sudo here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The way to pass an option to Meson is using the -D option. So to set the prefix, you should use meson -Dprefix=$(PWD)/_statatic build.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The current build script also installs some dependencies specifically for neatvnc and aml. Should these be moved to the Prerequisites section in the README?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the primary purpose of this build script is to simplify the installation of build-time dependencies. While users can manually handle the dependencies themselves, the script provides a convenient alternative. We specifically chose to install the dependencies as statically-linked libraries to minimize required system modifications, ensuring the least intrusive setup process possible.

jserv
jserv reviewed Oct 28, 2024
View reviewed changes
tools/build-neatvnc.sh Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 28, 2024
View reviewed changes
tools/build-neatvnc.sh Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 28, 2024
View reviewed changes
tools/build-neatvnc.sh Outdated Show resolved Hide resolved
@ndsl7109256 ndsl7109256 force-pushed the vncbackend branch 2 times, most recently from ca1e4e9 to f16b17c Compare October 28, 2024 08:52
jserv
jserv reviewed Oct 28, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 28, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 28, 2024
View reviewed changes
backend/vnc.c Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 28, 2024
View reviewed changes
tools/build-neatvnc.sh Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 28, 2024
View reviewed changes
tools/build-neatvnc.sh Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 28, 2024
View reviewed changes
tools/build-neatvnc.sh Outdated Show resolved Hide resolved
@ndsl7109256 ndsl7109256 force-pushed the vncbackend branch 3 times, most recently from 1e02939 to 6b08f69 Compare October 28, 2024 10:02
jserv
jserv reviewed Oct 28, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 28, 2024
View reviewed changes
tools/build-neatvnc.sh Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 28, 2024
View reviewed changes
tools/build-neatvnc.sh Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 28, 2024
View reviewed changes
backend/vnc.c Show resolved Hide resolved
@jserv

This comment was marked as outdated.

Sign in to view
@jserv
Copy link
Contributor

jserv commented Oct 28, 2024 •
edited
Loading

I successfully tested the VNC backend with cursor functionality on Ubuntu Linux. The VNC server started correctly, as shown in the logs:

00:28:38 INFO  backend/vnc.c:177: NeatVNC server IP 127.0.0.1 PORT 5900
DEBUG: ../src/server.c: 1939: Trying address: 127.0.0.1
DEBUG: ../src/server.c: 1959: Successfully bound to address

However, when connecting via TigerVNC client, I encountered two issues:

  1. The display dimensions were incorrect.
  2. Mouse movement was not smooth, with the cursor icon leaving behind visual artifacts.
VNC

@jserv jserv requested a review from Bennctu October 28, 2024 16:38
@ndsl7109256
Copy link
Collaborator Author

ndsl7109256 commented Oct 29, 2024 •
edited by jserv
Loading

However, when connecting via TigerVNC client, I encountered two issues:

  1. The display dimensions were incorrect.
  2. Mouse movement was not smooth, with the cursor icon leaving behind visual artifacts.
  1. I couldn't reproduce the issue via TigerVNC, could you share more about your settings and information?

image
image

  1. I have noticed the screen tearing problem here. I think one quick solution might be to use the cursor feature directly from neatvnc (example). However, this would create duplicate work regarding the cursor feature. I am also considering whether using nvnc_fb_pool would improve performance here, though it may take some time to evaluate.

jserv
jserv reviewed Oct 30, 2024
View reviewed changes
backend/vnc.c Outdated Show resolved Hide resolved
jserv
jserv reviewed Oct 30, 2024
View reviewed changes
backend/vnc.c Outdated Show resolved Hide resolved
jserv
jserv reviewed Nov 1, 2024
View reviewed changes
backend/vnc.c Outdated Show resolved Hide resolved
jserv
jserv reviewed Nov 1, 2024
View reviewed changes
backend/vnc.c
twin_screen_dispatch(tx->screen, &tev);
}

static struct nvnc_fb *_twin_vnc_create_cursor()
Copy link
Contributor

Choose a reason for hiding this comment

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

You should have a strong reason to create cursor here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since neatvnc provides a cursor feature, we could use it to render the cursor directly in the VNC framebuffer which makes smoother cursor movement.

jserv
jserv reviewed Nov 1, 2024
View reviewed changes
backend/vnc.c Outdated Show resolved Hide resolved
@ndsl7109256
Implement VNC backend
9be6240 
Add VNC backend using `neatvnc`[1]. Allow users can view and interact
with applications in Mado through VNC-compatible client software.

[1]: https://github.com/any1/neatvnc

Close sysprog21#32
jserv
jserv reviewed Nov 4, 2024
View reviewed changes
include/twin.h Outdated Show resolved Hide resolved
@ndsl7109256
Reduce cursor latency by rendering to VNC framebuf
6cbe4ae 
Previously, cursor movement was handled by passing pointer events into
twin screen, and then calculated the damage region for the cursor's new
position and transmitted it to the VNC framebuffer. This process
introduced latency in cursor display due to the multiple steps involved.
To address this delay, the cursor is now rendered directly within the
VNC framebuffer by leveraging neatvnc's internal cursor API. This change
bypasses the intermediate steps and provides a smoother and more
responsive cursor movement experience in the VNC backend.
@jserv
Copy link
Contributor

jserv commented Nov 4, 2024

Tested with TigerVNC and gvncviewer on Ubuntu Linux 20.04-LTS.

@jserv jserv merged commit 0c898a0 into sysprog21:main Nov 4, 2024
3 checks passed
@jserv
Copy link
Contributor

jserv commented Nov 4, 2024

Thank @ndsl7109256 for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jserv jserv jserv left review comments

@shengwen-tw shengwen-tw Awaiting requested review from shengwen-tw

@alanjian85 alanjian85 Awaiting requested review from alanjian85

@Bennctu Bennctu Awaiting requested review from Bennctu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ndsl7109256 @jserv @shengwen-tw @Bennctu