Closed lobin-z0x50 closed 5 years ago
I think it would be better if I could implement this without using ImageMagic. Do you have any ideas?
Are there other implement to improve?
Seems like a neat feature to have. Might loop in @kaspergrubbe as he's been more active making changes to this than I. As you mention having the imagemagick dependency is a bit icky - from memory that one can be a pain to install. Maybe don't have it as a hard dependency - just try and require it and handle LoadError
with a warning, or do it even later in the screenshot method, so that it'll naturally throw an error if imagemagick isn't available and you try and take a screenshot...?
It's a fair chunk of code to import. Could we just depend on the vncrec gem for this functionality?
ImageMagick and RMagick are a constant pain to deal with, we could detect it and not make it a hard dependency as @aquasync mentions.
It's impressive what you managed to get just by copying code over from vncrec
, I did start some work on screenshotting here: https://github.com/kaspergrubbe/ruby-vnc/pull/1
It only works with raw, but it also adds a ScreenState
class that deals with keeping the screen state in memory, and that also allows us, at a later stage update smaller chunks of it so that we do not have to update the whole image but only the chunks that have changed on update.
Maybe we can reach some sort of middleway between our approaches (If it's worth it).
Thank you so much ! I see. Maybe I can remove install-time dependency to RMagick, and I think it can migrate to run-time loading.
For using vnc-rec
gem, I think it is not easy because I implemented call-back feature to copied source code.
The call-back feature is important in the cause of wait for response of a FramebufferUpdateRequest.
It is processing as below.
take_screenshot()
method called.take_screenshot()
method.My guess is it need define the class that inherits or extends Classes of vnc-rec
. I will try it.
@kaspergrubbe, I think so. I tried first, to write a class to handle some framebuffer data according to the RFB protocol. However, I found that there is already a OSS that saves the VNC screen data as a movie. And I thought it should be diverted. also, vnc-rec is supported the incremental update too. If you do not mind, could you tell me what you are approaches for?
I see some value in my screen-object because for what I use ruby-vnc
for is searching the screen for images. So I have a set of images loaded into memory, and I am asking ruby-vnc
to read the screen, and then I want to find out if my images are on the screen and where on the screen that they are.
But since your take_screenshot
method can take in any IO-object, I can probably work around it, and have the ScreenState
-object in my own application instead of it living inside of ruby-vnc
.
Hi @lobin-z0x50
Do you need any help with getting this feature finished? :)
Hi @kaspergrubbe
No problem. Iām little busy, because I have been to a business trip. I'm sorry to have kept you waiting but I have some project, please wait a few days. Thank you.
No worries, and no stress. I was just wondering if you needed a hand or were stuck.
@aquasync @kaspergrubbe My refactoring was done.
RMagick
, and it has been migrated to run-time on-demand loading.FrameBuffer
class has been changed to wrapper class for VNCRec::RFB::Proxy
, and removed some copied source code from vnc-rec
.RMagick
, moved these to FrameBuffer
class.How about chunky_png
gem instead of RMagick
gem ?
https://github.com/wvanbergen/chunky_png
I have never used it yet, but it looks easy.
It was very easy that Installing it and try sample because it is pure ruby library.
Nagisa:~ lobin$ gem install chunky_png
Fetching: chunky_png-1.3.10.gem (100%)
Successfully installed chunky_png-1.3.10
Parsing documentation for chunky_png-1.3.10
Installing ri documentation for chunky_png-1.3.10
Done installing documentation for chunky_png after 1 seconds
1 gem installed
Nagisa:~ lobin$ irb
irb(main):001:0> require 'chunky_png'
=> true
irb(main):002:0>
irb(main):003:0> # Creating an image from scratch, save as an interlaced PNG
=> nil
irb(main):004:0> png = ChunkyPNG::Image.new(16, 16, ChunkyPNG::Color::TRANSPARENT)
=> <ChunkyPNG::Image 16x16 [
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
[#00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000 #00000000]
]>
irb(main):005:0> png[1,1] = ChunkyPNG::Color.rgba(10, 20, 30, 128)
=> 169090688
irb(main):006:0> png[2,1] = ChunkyPNG::Color('black @ 0.5')
=> 128
irb(main):007:0> png.save('filename.png', :interlace => true)
=> #<File:filename.png (closed)>
irb(main):008:0>
Nagisa:~ lobin$ ls -l filename.png
-rw-r--r-- 1 lobin staff 117 11 7 23:34 filename.png
Nagisa:~ lobin$ open !$
open filename.png
Nagisa:~ lobin$
Chunky_png looks promising, I wouldn't mind depending on that directly.
Do you think that dependency on vnc-rec should be directly (install-time)?
I think that makes sense too make it a direct dependency too.
changed image library to ChunkyPNG from RMagick.
@lobin-z0x50 I will try to make time to test your branch out next week. Have you had any chance to test out your changes with our docker setup?
You should be able to follow the description here: https://github.com/aquasync/ruby-vnc#running-the-tests and then try to connect to the VNC-server on localhost:5901
(no password) or localhost:5902
(with password: matzisnicesowearenice
).
It could be cool to have a test that did take a screenshot.
@kaspergrubbe Of course. I always checked the take_screenshot
operation with that docker containers.
However, I have not written the test code about the take_screenshot
yet.
I have been toying with the code a bit now.
First I encountered this warning:
/code/ruby-vnc/lib/net/vnc.rb:327: warning: instance variable @fb not initialized
So maybe we should set @fb = nil
in the initialiser?
Vncrec is also spewing a few warnings, but there is not much for us to do with that, since we just depend on them, we can consider fixing it upstream at some point:
/code/vncrec-1.0.6/lib/vncrec/rfb/proxy.rb:48: warning: assigned but unused variable - version
/code/vncrec-1.0.6/lib/vncrec/rfb/proxy.rb:59: warning: assigned but unused variable - pf
/code/vncrec-1.0.6/lib/vncrec/rfb/proxy.rb:167: warning: assigned but unused variable - first_color
I have tested the code and it works well.
However, maybe we should default to the parameter in #take_screenshot
to either nil
or Tempfile.new
. nil
will tell the framebuffer to return a blob, and Tempfile
will tell to return a filedescriptor.
So either:
def take_screenshot(dest = nil)
fb = _load_frame_buffer # on-demand loading
fb.save_pixel_data_as_png dest
end
or:
def take_screenshot(dest = Tempfile.new('ruby-vnc'))
fb = _load_frame_buffer # on-demand loading
fb.save_pixel_data_as_png dest
end
What do you think?
But also, I find it a bit strange that this method returns differently based on its input, maybe we should have two or three different methods instead?
Hi @lobin-z0x50
I have added some specs for testing the #take_screenshot
methods with both IO-object, a path, and a blob: https://github.com/lobin-z0x50/ruby-vnc/pull/3 let me know what you think.
So maybe we should set @fb = nil in the initialiser?
That's right !
However, maybe we should default to the parameter in #take_screenshot to either nil or Tempfile.new.
I think that nil
is better.
def take_screenshot(dest = nil)
I do not like to create implicitly Tempfile and return the filedescriptor. Because I think it should focus on simplify according to "Single Responsibility Principle".
@aquasync I think we are ready to merge if @lobin-z0x50 is otherwise happy with everything š
Thanks for your patience.
@kaspergrubbe Thank you for your kind collaboration !
@aquasync Hey, I know you are busy, but could you give us an indication of when you might have time to look through the changes and give a š or a š ? :)
Yeah looks good to me, nice one! Now might be a good time to cut a new release - it's only been 6 years...
I need a feature to get screenshots. How about such an implementation? RFB frame buffer feature devided from vncrec-ruby. https://github.com/d-theus/vncrec-ruby
example:
TODO: However, I think that it is better to think more about the class structure of RFB and VNC. It may be better to avoid dependency on ImageMagick.
My working environment: Ruby 2.3.0, ImageMagic 6.9.10-12