Feature/ikvm support

[DirectXMan12]

Continues on from #408.

This adds support for color conversion as well as ATEN iKVM devices.

[DirectXMan12]

See also #385 and #396.

[DirectXMan12]

Changes made from #408:

• General JSHint cleanups
• Re-adding native support for 32-bit pixels (instead of converting them down to 24 bit and then back up to 32 bit later)
• Fix broken assumptions about native pixel format (noVNC's display.js generally expects BGRX when using 4-byte pixels, and RGB when using 3-byte pixels, but several changes were made in rfb.js that assumed RGBX for 4-byte pixels, leading to incorrect colors). The "native" pixel format is what was previously: 4-byte pixels use BGRX, while 3-byte pixels assume RGB (the difference between the former and the latter's order is due to the two formats display.js accepts natively).
• Preallocate data array with appropriate size in _convert_color (this should save a few allocs with JS engines that pay attention to those sorts of things)
• Applied change from the patch from @emmericp

[emmericp]

Changing the resolution on X9SCL/X9SCM boards still freezes the display as mentioned in #408.
I'm kind of stuck here debugging this, but I can provide anyone willing to debug this with full IPMI access to test servers.

[dudymas]

confirmed this works on 3.1.7 firmware, however newer firmware seems to break.

Any news or better sources I could go to for more tests? And is there anything I can do to accelerate this PR into acceptance?

[DirectXMan12]

@emmericp , @dudymas : if someone could figure out why the things that don't work have issues, that would be great.

I'm hesitant to merge code in that doesn't fully work, but I would like to get this merged at some point.

@kanaka , @samhed, @astrand : what do you think? One idea I've been toying with is merging the pixel conversion support into master, and merging the iKVM support into a separate branch until we're comfortable merging it into master.

[samhed]

Yes I do think that would be the best way forward, and I think @astrand agrees with me as well. Even back when #408 was first submitted we had a discussion at the office and that suggestion came up.

Both of these features would definitely be nice to have. The only one we can test or maintain currently feels like the color conversion one.

[emmericp]

I can still provide anyone willing to debug this on newer firmware with full IPMI access to a test system.
Just contact me.

[kanaka]

I agree the iKVM support should be split out to a branch until we have some long term way of testing it.

My main concern with the color conversion is to make sure that we are able to performance test the current default mode on at least Chrome and firefox to make sure there are no regressions (especially with tight encoding). From a brief look through the code it looks like the default case is only introducing a quick call or two to the main code, but I've been wrong about that before and discovered it only in testing (surprising things can trigger de-optimization or more GC in common code paths).

[DirectXMan12]

@kanaka: I did some basic performance testing (comparing what we have to the given code to tweaks to the given code), and there didn't seem too much of an impact. In this version, I also reintroduced an optimized path for one of the common cases, so no conversion is needed.

It would be nice to have other people do some performance tests to corroborate what I found, however.

[NiKiZe]

Just purchased an Aten IP8000 and nothing of the java stuff works (to put it mildly, or maybe Its just my hate for Java)
This card gives "Invalid server version 009.001" in noVNC
Will try to look into that myself.
I also have some Intel Server boards where I think the iKVM stuff might work that I will try out.
If any of this does work I'm very interested in getting this to be "production ready"

[NiKiZe]

running x11vnc as server and using this branch fails (screen is not displayed), but using master works fine. Would like to have a "known working state" before I start work, any chance that we can get this officially rebased on current master?

[sodabrew]

@DirectXMan12 Could you rebase this branch up to current master? I'd like to test this with my available set of SuperMicro equipment.

[NiKiZe]

@changetip give @DirectXMan12 a small incentive to rebase

[BashCo]

@changetip test @NiKiZe 1000 bits

[changetip]

Hi @NiKiZe, I've delivered a tip worth 1000 bits ($0.44) from @BashCo.

Learn about ChangeTip

[DirectXMan12]

Apologies for letting this lapse a bit -- I've been busy at work, and haven't had quite as much time as I'd like to work on noVNC. I've rebased the PR, but I can't test the changes myself to make sure that I didn't break any of the iKVM stuff. Please let me know if anything doesn't work.

P.S. While I appreciate the sentiment, I'm afraid I'm unable to accept any "tips" to work on features ;-)

[NiKiZe]

Thanks!
Just to get started I wanted to test normal VNC with this and it fails :(

• start x11vnc
• checkout this branch and run utils/launch.sh
• connect with browser
• gets disconnected immediately

Checking out master instead and doing the same works fine, so this branch breaks normal VNC somehow.

[DirectXMan12]

@NiKiZe Ok, that problem should be fixed -- there were two issues: bpp vs Bpp, as well as the fact that the boolean code was comparing to 1, whereas it should have been checking for any non-zero value instead (x11vnc, for instance, send 0xff)

[kelleyk]

@DirectXMan12 , I'm seeing several test failures when I run node tests/run_from_console.js against the code from this PR.

- failed: should support drawing RGBX blit images with true color via #blitImage ‣
- failed: should support drawing RGBX blit images with true color via #blitImage ‣
- failed: via old style method ‣
- failed: should handle 24bit depth (RGBX888) @ 32bpp [native] ‣
- failed: should handle 15bit depth (BGR555) @ 16bpp ‣
- failed: should handle 15bit depth (BGR555) @ 16bpp big-endian ‣
- failed: should handle subtype subrect encoding ‣
- failed: should handle subtype RAW encoding ‣

It looks like the output buffers are entirely filled with zeroes instead of with whatever image data is expected.

Am I doing something wrong?

[DirectXMan12]

@kelleyk looks like PhantomJS isn't very happy with one of the changes, but it seems to work fine in the browser (use -g to generate the HTML without running the tests, and additionally -o to open the generated page in your default browser). I'll try to investigate, but I might not have much time coming up.

[alantwentyseven]

Before adding Dell IDRAC support I rebased the pixelformat branch of jimdigriz's work back in September & October. During this, I uncovered and fixed numerous issues stemming from the merge/rebase originally, but also some previously undetected issues with colour conversion and the unit tests themselves.

The resulting branch was here: https://github.com/alantwentyseven/noVNC/tree/rebase-pixelformat

To which I added the Dell support, here: https://github.com/alantwentyseven/noVNC/tree/dell-encodings

However, I'm aware that this recently rebased PR has probably had fix-ups done by @DirectXMan12 a lot more efficiently, plus is a much more recent rebase. My intention now is to compare the 2 branches and see if any of the issues I originally resolved exist over here (it seems some do, from this thread), and re-do those fixes. From there, I should be able to apply the Dell support easily, and finally help @kelleyk get his iKVM support into a pixelformat-supported, aten-supported branch which is as close to master as possible.

[DirectXMan12]

Closing in favor of #614