Skip to content
This repository has been archived by the owner on Mar 14, 2018. It is now read-only.

Replace basic VNC client with TigerVNC #247

Merged
merged 1 commit into from
Mar 10, 2017
Merged

Conversation

pepijnve
Copy link
Contributor

@pepijnve pepijnve commented Mar 9, 2017

This PR replaces the original VNC client code with a third-party one that has more complete support for the VNC protocol.
In this PR TigerVNC has been used. This library is licensed under GPLv2 which may prove problematic since it would require SikuliX to be licensed under GPLv2 as well (or dual licensed MIT/GPLv2).
As an alternative I'm considering using https://github.com/comtel2000/jfxvnc instead which is licensed under the ASLv2. That library makes use of some JavaFX classes though meaning SikuliX would become dependent on that as well. In other words, Java SE 8 or higher would be required. What's the current Java SE requirement for SikuliX?

@pepijnve
Copy link
Contributor Author

pepijnve commented Mar 9, 2017

In the original code I could not understand what the intended usage of the VNC connection pooling (ConnectionController class) was. I removed all the code related to that and replaced it with something closer to how the ADBScreen class works. The idea is that you either use VNCScreen instances in a try/finally or try-with-resources block, or alternatively in Python you can use useVnc to make a VNC screen the default one.
I can restore the original connection pool code if necessary, but I'll need some help in understanding what it's intended purpose is.

@RaiMan
Copy link
Owner

RaiMan commented Mar 9, 2017

@pepijnve

In the original code I could not understand what the intended usage of the VNC connection pooling

LOL, then we are already 2 ;-)

When I made my experiments with ADBScreen, I did not find the time to get more into the details of the VNC solution. I only added the convenience start-up. So glad you did :-)

@RaiMan
Copy link
Owner

RaiMan commented Mar 9, 2017

Licensing:
In parallel with looking into your solution, I will check about switching to GPLv2 or even dual licensing as suggested.

Java 8:
I want the version 1.1.x to be satisfied with Java 7.
Version 2 will need Java 8 in the long run.
So TigerVNC is the right solution for 1.1.x.
Later this year I might come back to you, when I implement the non-local-screen support in version 2.

@RaiMan
Copy link
Owner

RaiMan commented Mar 9, 2017

Conclusion:
Great thanks for now.
In a few days I will come back with my findings and possible comments.

@RaiMan RaiMan merged commit 1a9d308 into RaiMan:master Mar 10, 2017
@RaiMan
Copy link
Owner

RaiMan commented Mar 11, 2017

Ok, I made some tests:

  • client script always on macOS 10.12
  • server on Windows 10-64 (TightVNC) or on macOS 10.12 (RealVNC)
  • Java 8 in all cases

I made my tests in the IntelliJ IDEA project environment and with a packed jar as it is distributed as SikuliX.

To get the TigerVNC classes available in the packed jar, I put the jar content unpacked into the resources, so that at jar runtime the com.... tree with the class files is at root.
BTW: the tigerVNC jar does not contain any information about licensing, so I decided to take the risk of packing the class files into the distribution until someone complains about it.

Some findings:

  • (to be fixed) the capture() in all cases gives black images and hence any find op does not work
  • mouse actions seem to work
  • password feature works
  • (to be fixed) sessions started in the IDE keep open and in the next rerun a new session is started (possible memory leak, sessions should be reused or all stopped when script ends (cleanup))

Hope you have some ideas. Until then I will not do any debugging, but adapt your solution into version 2.

@RaiMan
Copy link
Owner

RaiMan commented Mar 11, 2017

Take care: I made minor changes on top of your PR.

@RaiMan
Copy link
Owner

RaiMan commented Mar 12, 2017

ok, tested with the old version: black capture images too. So might be some other reason. I will try to find out next week.

@pepijnve
Copy link
Contributor Author

In which situation are you getting black images? FWIW, I've been using this code in production already to connect to the console of VMs running on an ESXi server. We're using the image capture functionality to detect prompts in an installation wizard.

@RaiMan
Copy link
Owner

RaiMan commented Mar 13, 2017

simply: vncScreen.capture() and then save the image.
since find also does not work in all situations (old and new version) when running the client on macOS 10.12 against Windows or Mac it looks like a more generic problem.
I will come back, when I have tested other combinations and done some debugging.
Thanks for taking care.

@pepijnve
Copy link
Contributor Author

pepijnve commented Mar 13, 2017

The issue here is that the constructor of VNCScreen returns before the initial framebuffer contents have been loaded. It sends a full frame buffer refresh request and then returns right away.
I played around with the code a bit to see if I could somehow wait for that request to finish, but there's no clear point where you know 100% sure that it's done.
It would probably be better to make OverlayCapturePrompt reload the screen contents periodically (or maybe manually).

@RaiMan
Copy link
Owner

RaiMan commented Mar 13, 2017

Ok, thanks for the hint. Makes sense. I will play around with it the next days a little bit to get nearer to a solution.

@pepijnve
Copy link
Contributor Author

Here's a quick patch I put together to test my hypothesis. Hitting F5 on the prompt screen will grab a new screen capture. With this in place I can see the screen loading part by part on my Mac.
refresh.patch.txt

@RaiMan
Copy link
Owner

RaiMan commented Mar 13, 2017

ok, I will check.

@RaiMan
Copy link
Owner

RaiMan commented Mar 14, 2017

ok, made some further tests with SikuliX version 2 (which gives me much more options for experimenting).

  • the frame buffer fill delay seems only to be a startup problem of the VNCClient
  • after the first capture successive captures show the correct content even with changes and no black areas immediately (even with screen sizes beyond 1600 x 1000)

In all cases I use the refreshFramebuffer with incremental false. Setting it to true does not change anything.

In SikuliX version 2 I do no longer work with buffered images internally, but with OpenCV Mat objects everywhere. So in the next weeks, I will check, wether it is possible, to use OpenCV Mat objects instead of buffered images in the VNCClient too, to access the FB content, which would make conversions from buffer data to buffered images and then to Mat obsolete (some 10 milliseconds saved ;-)

I will now check, wether my findings are true with SikuliX 1.1.1 and your VNC implementation. If yes, then the startup delay should be solved internally with some waiting/repeating based on a user setting.

Version 2 will have an automatic detection of the frame buffer filling based on OpenCV features to detect content changes (which I already used for my testing now).

@pepijnve
Copy link
Contributor Author

It's indeed an initialisation issue. On creation the VNCClient requests a full (incremental = false) frame buffer refresh from the server. This queries the initial frame buffer contents from the server.
After that, each frame buffer update triggers an incremental update of the entire frame buffer.

The first, non-incremental request forces the server to send the entire screen contents. The subsequent incremental requests ask the server to send any updates. Setting incremental to true gives the server the freedom to only send back the changed areas of the frame buffer. Whether it does that or just sends everything over the wire again depends on the server implementation.
Details are in section 7.5.3 of the RFB RFC.

Using OpenCV Mat objects should be feasible. Client-side framebuffer updates are processed in VNCFrameBuffer. Since those writes are performed on a background thread and the reads are done on the AWT event thread, access to the back buffer is guarded by a mutex. VNCFrameBuffer#getImage makes a copy of the requested region from the back buffer so that the requesting code is free to do whatever it wants with that. That seems like a good place to copy pixel data from the back buffer to an OpenCV Mat object.

@RaiMan
Copy link
Owner

RaiMan commented Mar 14, 2017

I now finalized the stuff. Will be available tomorrow.

I added a feature, so that open connections are closed on script run end in IDE and revised the start methods to be more backwards compatible.

I will not add the F5 patch, but instead put a note in the docs, about possible problems with the first access to the frame buffer (capture, userCapture and find ops), to add an appropriate wait() after the vncStart() in this case ( here in my local net it was 3 seconds before userCapture and 2 seconds before the first find op).

Thanks for your comment on OpenCV usage.

... and again great thanks for this valuable contribution. I learned a lot and got some new ideas about how to add such features in version 2 as extensions (which will also make the license problem obsolete).

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

Successfully merging this pull request may close these issues.

2 participants