Aten iKVM support #408

jimdigriz · 2014-10-29T18:16:07Z

Tested over multiple Supermicro boards (as detailed in the sourcecode) but with the most recent offereing from Supermicro I still need to reverse engineer their new encoding. This code is good to go though and was tested with a regular local TightVNC to make sure I did not poouch the RGB<->BGR stuff in the Pixel Format work I had to do first.

This solves #385 and should start helping out for the work needed in #396

One gotcha, IE now much be version 10+ due to the pixel endian handling.

samhed · 2014-10-30T12:39:59Z

It's a big PR, I feel that it might be easier to get a good overview for review if the pixelformat stuff was split to a different PR. Would that be possible?
I understand that these two are intertwined, but in my eyes the pixelformat part is something we are interested in no matter how the discussion regarding extending the RFB protocol goes. I should mention that I personally don't know much about this iKVM subject though.

samhed · 2014-10-30T12:42:18Z

One gotcha, IE now much be version 10+ due to the pixel endian handling.

This shouldn't be a problem, we are heading away from IE9 support in general, you can see the discussion in #375.

DirectXMan12 · 2014-10-30T19:26:13Z

There are a few issues with this PR:

There need to be unit tests for this, especially since it's a fairly major addition. Unit tests ensure that things don't get broken in the future.
Please clean up the commit history. You should move cleanup commits back into their original commits (you can edit git history with git rebase -i). There should be only a few commits in this pull request (I'd estimate between 2-4).
You make changes to the way the protocol works without fencing these decisions behind an "if (is_atem_ikvm)" (e.g. you no longer send the client encodings message). These changes may break functionality with other VNC servers. Can you make these conditional?
If possible, avoid object lookups (pixelFormat) According to @kanaka, these can cause performance issues. If possible, could you look and see if this._pixelFormat.xyz causes any performance drop (vs this._pf_xyz).

Thanks!

jimdigriz · 2014-10-30T20:05:59Z

Thanks this is something I can work with, however you will need to bear with me as I am not a JS coder. So

how do I write unit tests? What do you expect to see in these kind of unit tests? Am I to mock an entire ATEN iKVM server? Am I to figure out how to reply a raw PCAP file at this?
how do I run your unit tests?
how do I ahead of time get your Travis spiel without waiting for someone else to do it for me
"avoid use of objects for performance reasons" is meaningless to me, what should I use instead? Strange to me as this._FBU is being used in a similar manner. Where is there a list of recommendations on to gos and no gos of how I should be writing code for noVNC. When someone says 'performance', are we talking 1%, or 50%?
kind of would be surprised if someone titled their VNC session with "ATEN iKVM Server" or are you referring to the entry into _negotiate_aten_auth()? The latter is ugly, I was aiming for 'auto-detection' but I could wang in an explicit button for "iKVM" or something...seems wrong but it is your codebase
how do I benchmark my changes? What is acceptable?

A passing observation. I cannot see readily any details on what needs to be done to contribute code to this project. No description of what standards need to be met, how to meet them, where to get reviews, expectations, etc etc. Folks like me do need a way to measure how much an investment is needed to offload code onto you.

From my perspective it seems that following, the best I could, the existing coding style (with no ready documentation on a coding standard to hand, I am simple with how I tackle problems and "git grep -i '(coding|contrib)'" is my search tool!) was not good enough to meet the standards needed to make a pull request.

jimdigriz · 2014-10-30T20:14:59Z

Really, this is how simple I am:

aclouter@stevemcqueen:/usr/src/aten-ikvm/noVNC/tests$ node run_from_console.js

module.js:340
    throw err;
          ^
Error: Cannot find module 'ansi'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:280:25)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at Object.<anonymous> (/usr/src/aten-ikvm/noVNC/tests/run_from_console.js:2:12)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)

You really need to tell folks like me:

you need to run node version foo
you need to run "npm install" (not as root)
you need to go into the 'tests' directory
now run "node run_from_console.js"

I now get:

aclouter@stevemcqueen:/usr/src/aten-ikvm/noVNC/tests$ node run_from_console.js
using files /usr/src/aten-ikvm/noVNC/tests/test.base64.js,/usr/src/aten-ikvm/noVNC/tests/test.display.js,/usr/src/aten-ikvm/noVNC/tests/test.helper.js,/usr/src/aten-ikvm/noVNC/tests/test.keyboard.js,/usr/src/aten-ikvm/noVNC/tests/test.rfb.js,/usr/src/aten-ikvm/noVNC/tests/test.util.js,/usr/src/aten-ikvm/noVNC/tests/test.websock.js
Running tests /usr/src/aten-ikvm/noVNC/tests/test.base64.js, /usr/src/aten-ikvm/noVNC/tests/test.display.js, /usr/src/aten-ikvm/noVNC/tests/test.helper.js, /usr/src/aten-ikvm/noVNC/tests/test.keyboard.js, /usr/src/aten-ikvm/noVNC/tests/test.rfb.js, /usr/src/aten-ikvm/noVNC/tests/test.util.js, /usr/src/aten-ikvm/noVNC/tests/test.websock.js using provider SpookyJS (CapserJS on PhantomJS)

Results for /usr/src/aten-ikvm/noVNC/tests/test.base64.js:
==========================================================

3 tests run, 3 passed, 0 failed -- duration: 0.06s

all tests passed :-)

Results for /usr/src/aten-ikvm/noVNC/tests/test.display.js:
===========================================================

42 tests run, 40 passed, 2 failed -- duration: 0.17s

Display/Canvas Helper
---------------------

### drawing ###

#### (prefering native methods) ####

- failed: should support drawing BGRX blit images with true color via #blitImage ‣
          AssertionError: expected { Object (_drawCtx, _c_forceCanvas, ...) } to have displayed the image { '0': 0,

#### (prefering JavaScript) ####

- failed: should support drawing BGRX blit images with true color via #blitImage ‣
          AssertionError: expected { Object (_drawCtx, _c_forceCanvas, ...) } to have displayed the image { '0': 0,


Results for /usr/src/aten-ikvm/noVNC/tests/test.helper.js:
==========================================================

35 tests run, 35 passed, 0 failed -- duration: 0.08s

all tests passed :-)

Results for /usr/src/aten-ikvm/noVNC/tests/test.keyboard.js:
============================================================

55 tests run, 55 passed, 0 failed -- duration: 0.17s

all tests passed :-)
ERROR: Error: Child terminated with non-zero exit code 1

So according to the project, only the BGRX stuff failed, not surprising as I renamed the function. Easily fixed.

Really folks, you need to set up 'package.json' to let people do "npm runtests" and the lark, and you need to have something grepable in the project covering this. 'git grep -i tests' comes up with nothing useful.

DirectXMan12 · 2014-11-02T18:02:45Z

About the unit tests

Hey, sorry about the trouble you faced with that. I know at the moments it's a bit frustrating, but we just started asking for unit tests with pull requests, so the process is a bit new. Adding in a document about contribution is a good suggestion; I'll add something like that shortly.

For unit tests, there's no need to mock an entire ATEN iKVM server. That would be quite a bit work, I imagine. I suggest the following (I'll put something like this in the general contributing docs as well):

Add a basic test of the pixelFormat stuff. tell noVNC you want a pixelFormat of xyz, and make sure it converts correctly
For the encoding, if you can, push in a simple pattern and check to make sure the result is displayed correctly. You can see examples in message encoding handlers in tests/test.rfb.js.

P.S. You actually can run npm test (the standard npm command for running tests). That will run the tests using Karma and PhantomJS, but it might not give you as much information as tests/run_from_console.js

About not using objects

If you don't have time to test the performance impact, then it's fine to leave your code as it is. I suspect it will be fine, since like you said, _FBU is the same way. I think @kanaka has some numbers, though, so I can ask for his opinion (he's the one that gave the original recommendation).

About the detection of when iKVM is in use

The way you currently do it (checking the RFB name) is fine. I was just a bit concerned about this: https://github.com/kanaka/noVNC/pull/408/files#diff-c22afee41ed3afeb3bc9e5aeb1757e40L954 -- we really should be sending back the supported client encodings in normal VNC mode, regardless of whether or not we're converting between pixel formats.

In conclusion*: Thanks for all your work. I'm sorry if this was a frustrating process, and we'll try to improve the experience in the future.

jimdigriz · 2014-11-03T21:11:20Z

Thanks. I am busy this week so I doubt I will be able to find the time to jump back in on this, but hopefully a week on Monday, I will be able to.

Unit Tests

I'll give it my best shot.

I might try doing the old/new ATEN 'auto-detection' and make sure it does not stomp on a tunnel/non-tunnel 'vanilla' TightVNC server. This is really the only place I can imagine ever this breaking things badly.

The pixelformat code fortunately, once in, would probably just by its nature get regular exercising through day to day use. At the moment it is a NOOP, when the server supports noVNCs preference, but if it is not too much of a performance hit (how do we test this?) I recommend it is always run. This may though entail replacing _convert_color() with a conditional/spaghetti logic approach which may be faster than calling DataView/setUint8 for each pixel; I have no idea how to evaluate this.

Alternative Codepath for iKVM

I can trivially whack a conditionally around that, however maybe something more akin to this._encHandlers would be appropriate? Anyway, thought you were concerned about fallout from the auto-detect-o-matic action (I think its safe as obviously something is going off-pist from the spec).

Contributions

All simple folks like me need is a file at the top level of the project called "CONTRIBUTING" that details a checklist to run through before sending in pull requests and the expectations the project owners have before you are permitted to drop a pile of patches and the responsibility of maintaining them on them before disappearing into the ether.

DirectXMan12 · 2014-11-03T22:08:28Z

With regards the CONTRIBUTING file, does this pull request look good: #410 ?

DirectXMan12 · 2014-11-19T21:00:16Z

Did you get a chance to do any cleanup on this?

jimdigriz · 2014-11-22T15:06:06Z

Just started working on it all now. I have the fixed the existing unit tests and now just need to add:

ATEN specific wrappers around those unregistered message types
a test or two for the pixelFormat converstion
a test for an ATEN authentication
two tests for each of the ATEN frameUpdate format

Part of my work removed true_color altogether (replaced with "server will ignore noVNC preferred, so do convertColor").

I have to head out now, but should be able to work on this further tomorrow.

DirectXMan12 · 2014-11-24T21:12:57Z

Let me know when you're ready for me to merge this.

So people can follow the 'unit tests' instructions, we need to make sure PATH includes the karma bin directory otherwise we see the following: ---- aclouter@stevemcqueen:/usr/src/aten-ikvm/noVNC$ npm test > [email protected] test /usr/src/aten-ikvm/noVNC > karma start karma.conf.js sh: 1: karma: not found npm ERR! Test failed. See above for more details. npm ERR! not ok code 0 ----

jimdigriz · 2014-12-06T19:04:05Z

Okay, I think this is good, at least enough for folks to give it a whirl.

Really need others to give this a test to see if they see anything strange visually due to the pixelformat'ing patch, I have tried a lot of combinations (BGR/RGBxxx with and without the convertColor flag enabled). The ATEN patch is non-intrusive and would only affect those using ATEN kit (of which they couldn't connect to before anyway).

Pixelformat

N.B. side by side tested with Debian wheezy's tightvncserver and xtightvncviewer

has removed 'true_color' and made everything RGB only
cursor in non-{RGB,BGR}888 modes is usable but looks funny as Cursor() has no knowledge of other modes
'convertColor' means whether the client should do the pixel conversion, when unchecked, it assumes that the server can convert to the format the client has requested
that 'HACK? Xtightvnc' I have no real idea what is right or wrong here...my brain hurts

ATEN iKVM

I think the screen blanking works, ran it by someone who found a bug, but never got back to me if the fix was okay
TODO: the newer X10 Supermicro encoding type needs to be figured out
TODO: not too hard to add support for other messages (maybe power cycle, etc)

DirectXMan12 · 2014-12-12T16:26:06Z

Excellent. I will test this a bit on my own as well when I get a chance.

@kanaka , @samhed , @astrand : could you test out the pixelformat patches with whatever setups you have that use noVNC (standard VNC, VMs, etc)? If it works for multiple different people on several different target servers, I think it's safe to merge it.

DirectXMan12 · 2014-12-12T16:53:38Z

Hey @jimdigriz, I just had a couple comments on the pixelformat part. The first couple I can fix myself if you don't have time.

DirectXMan12 · 2014-12-12T16:56:37Z

The ATEN part LGTM 👍

jimdigriz · 2014-12-12T18:01:58Z

I have concerns about this line. Namely, performing a bunch of array operations that mutate the size of the array like this could be very slow.

Removing an element (tail end) seemed fast, it was when adding one that everything went down hill. Just my $0.02.

What would have been nice is to use the whole ByteArray do-thingy (and internally use RGBX everywhere) so it could be passed straight to PutImage. I tried but your tests all failed as phantomjs does not support it.

I'm just curious, why don't we just call blitImage here directly instead of pushing to the render queue?

Everything else looks to go via renderQ, so I just changed it to match.

Are we suddenly subtracting 7 from the max values here to test the color conversion, or for some other reason?

The -7 on those values is so the same test data can be recycled for both 888 and 555 true color mappings.

DirectXMan12 · 2014-12-12T18:40:54Z

Ok. Those explanations seem fine for me. I'd still like to double check with @kanaka, but it should be fine.

We plan to add support for just passing around a JS typed array, but that will have to wait until I have some free time ;-). Were you just planning on copying the input data 3 bytes at a time (for BGR data) into a BGRX ArrayBuffer?

jimdigriz · 2014-12-13T12:58:34Z

I toyed with .set(new Uint8ClampedArray(...)) which worked fine, but broke the tests in a way that would have involved (it seemed) tossing out phantomjs.

There I had _convert_color() outputting always RGBX so that it could be blitted directly onto the canvas with no byte copying and avoiding those slow as molasses loops. The code, as it is, always puts things into a RGB ArrayBuffer which then the blit function in display.js fixes up to an RGBX array; before passing it into PutImageData().

This answer your question, or have I missed the point?

DirectXMan12 · 2014-12-15T16:03:40Z

Nope, that sounds good.

MartinVerges · 2015-01-29T12:47:50Z

Hello,

we can provide some test systems for ATEN IPMI (Supermicro Servers) and would like to support your effort to implement it in noVNC. As you might know, IPMI is widely used on server systems and the java viewer is a pain in the a**. I personal want to get rid of that java viewer for better customer experience.

We are also willing to spend some money to get this solved and if somebody can do it, we would be glad to push noVNC to IPMI / KVM Support.

At the moment, it does not work in my test with X9SCM-F (other than stated) as it response "No supported sub-auth types!"

Best Regards
Martin

DirectXMan12 · 2015-01-29T16:23:34Z

@MartinVerges : Hi! More testing would be great.

I've been really busy over the past couple months, which is why I haven't gotten a chance to merge this yet. However, we are interested in getting the ATEN iKVM support in place, and I will endeavor to make some time in the next month to do one last set of checks before merging.

emmericp · 2015-01-30T15:20:57Z

I tested this with two supermicro X9SCL/X9SCM boards running IPMI firmware versions 3.15 and 3.19.

It works well, expect for turning the screen on and off which makes (re-)booting a system annoying. I fixed the most obvious problems (expecting aten_len to be 10 while it is 0 when the screen is off with this board/firmware), but it now just hangs when the screen turns off.
I also get the error "unsupported encoding -41877984" occasionaly when connecting during a machine during boot (i.e. lots of screen on/off and resolution changes).

I can give you guys full access to the servers for development, just send me an email for details.

DirectXMan12 · 2015-02-03T22:11:40Z

@emmericp : you said that you fixed some things. Do you have a patch? Thanks!

emmericp · 2015-02-03T23:08:27Z

@DirectXMan12 the only change that actually helped somewhat is the following.
The transition from screen off to on now works sometimes (e.g. connecting to a server that is currently idling and pressing a key).

I did not yet have time for a deeper dive into the code base, I'm still puzzled why it sometimes just seems to stop receiving data at all after changing the resolution.

diff --git a/include/rfb.js b/include/rfb.js
index 252345d..a02e217 100644
--- a/include/rfb.js
+++ b/include/rfb.js
@@ -2152,11 +2152,12 @@ var RFB;

                 if (this._FBU.width === 64896 && this._FBU.height === 65056) {
                     Util.Info("ATEN iKVM screen is probably off");
-                    if (this._FBU.aten_len !== 10) {
-                        Util.Debug(">> ATEN iKVM screen off (aten_len="+this._FBU.aten_len+")");
-                        this._fail('expected aten_len to be 10 when screen is off');
+                                       Util.Debug(">> screen off, aten_len = " + this._FBU.aten_len);
+                    if (this._FBU.aten_len !== 10 && this._FBU.aten_len !== 0) {
+                        Util.Debug(">> ATEN iKVM screen off (aten_len=" + this._FBU.aten_len + ")");
+                        this._fail("expected aten_len to be 10 or 0 when screen is off");
                     }
-                    this._FBU.aten_len -= 10;
+                    this._FBU.aten_len = 0;
                     return true;
                 }
                 if (this._fb_width !== this._FBU.width && this._fb_height !== this._FBU.height) {

DirectXMan12 · 2015-02-04T03:43:37Z

include/rfb.js

@@ -330,9 +334,12 @@ var RFB;
            this._FBU.lines        = 0;  // RAW
            this._FBU.tiles        = 0;  // HEXTILE
            this._FBU.zlibs        = []; // TIGHT zlib encoders
+            this._FBU.aten_len     = -1  // ATEN


missing ; here

DirectXMan12 · 2015-02-10T00:18:24Z

FYI, I've made some fixes to the code (which preserving attribution, of course), and pushed them to a new branch. There's a new PR at #445

jimdigriz added 3 commits December 4, 2014 21:18

added client side pixel format conversion support

d2c58ae

added aten-ikvm support

24439f5

DirectXMan12 reviewed Feb 4, 2015
View reviewed changes

DirectXMan12 mentioned this pull request Feb 10, 2015

Feature/ikvm support #445

Closed

DirectXMan12 closed this Feb 10, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aten iKVM support #408

Aten iKVM support #408

jimdigriz commented Oct 29, 2014

samhed commented Oct 30, 2014

samhed commented Oct 30, 2014

DirectXMan12 commented Oct 30, 2014

jimdigriz commented Oct 30, 2014

jimdigriz commented Oct 30, 2014

DirectXMan12 commented Nov 2, 2014

jimdigriz commented Nov 3, 2014

DirectXMan12 commented Nov 3, 2014

DirectXMan12 commented Nov 19, 2014

jimdigriz commented Nov 22, 2014

DirectXMan12 commented Nov 24, 2014

jimdigriz commented Dec 6, 2014

DirectXMan12 commented Dec 12, 2014

DirectXMan12 commented Dec 12, 2014

DirectXMan12 commented Dec 12, 2014

jimdigriz commented Dec 12, 2014

DirectXMan12 commented Dec 12, 2014

jimdigriz commented Dec 13, 2014

DirectXMan12 commented Dec 15, 2014

MartinVerges commented Jan 29, 2015

DirectXMan12 commented Jan 29, 2015

emmericp commented Jan 30, 2015

DirectXMan12 commented Feb 3, 2015

emmericp commented Feb 3, 2015

DirectXMan12 Feb 4, 2015

DirectXMan12 commented Feb 10, 2015

Aten iKVM support #408

Aten iKVM support #408

Conversation

jimdigriz commented Oct 29, 2014

samhed commented Oct 30, 2014

samhed commented Oct 30, 2014

DirectXMan12 commented Oct 30, 2014

jimdigriz commented Oct 30, 2014

jimdigriz commented Oct 30, 2014

DirectXMan12 commented Nov 2, 2014

About the unit tests

About not using objects

About the detection of when iKVM is in use

jimdigriz commented Nov 3, 2014

Unit Tests

Alternative Codepath for iKVM

Contributions

DirectXMan12 commented Nov 3, 2014

DirectXMan12 commented Nov 19, 2014

jimdigriz commented Nov 22, 2014

DirectXMan12 commented Nov 24, 2014

jimdigriz commented Dec 6, 2014

Pixelformat

ATEN iKVM

DirectXMan12 commented Dec 12, 2014

DirectXMan12 commented Dec 12, 2014

DirectXMan12 commented Dec 12, 2014

jimdigriz commented Dec 12, 2014

DirectXMan12 commented Dec 12, 2014

jimdigriz commented Dec 13, 2014

DirectXMan12 commented Dec 15, 2014

MartinVerges commented Jan 29, 2015

DirectXMan12 commented Jan 29, 2015

emmericp commented Jan 30, 2015

DirectXMan12 commented Feb 3, 2015

emmericp commented Feb 3, 2015

DirectXMan12 Feb 4, 2015

Choose a reason for hiding this comment

DirectXMan12 commented Feb 10, 2015