Run the UI in process and replace (some) sh with Python

[DemiMarie]

Running the UI in-process guarantees that a UI crash will stop the flow
of video information. A UI crash that did not stop streaming would be a
security risk. Python code is both more flexible and easier to secure
than shell scripts.

[DemiMarie]

Please note that this PR is full of duplicated code and should not be merged yet.

[ElliotKillick]

Great, Demi! I really like how the Python port is coming!!

Yes, please refactor to reduce the duplicated code! The original UI was as all nicely sorted into different files in object oriented form and looked amazing.

I saw you removed the notification that pops up when a video stream is started. In the case of the of using a webcam, when we integrate with the Qubes Devices system tray it will create a notification anyway so that's no problem there. (Granted, it will be without the webcam/screen icon but we could easily add the functionality to have an icon for the Qubes Devices tray notification if we wanted to.) However, in the case of screen sharing there will simply be no notification as it won't integrate with the Qubes Devices system tray. Do you think that screen sharing should have a notification at the start or is a tray icon all the way through sufficient?

So yes, all good but I would like to keep Qubes Video Companion very modular and and object oriented (all components of which we would import into the single Python process).

[ElliotKillick]

Also, on your git commits it says they are "Unverified" in a yellow tag because you haven't uploaded your public PGP key to GitHub. We aren't relying on the GitHub infrastructure for any security I'm aware of that but perhaps you should upload it just so GitHub is happy.

[ElliotKillick]

By the way, why did you implement a get_resolutions() using v4l2-ctl? I already implemented this in Python and I told you I was making a C port for it using ioctls as that would be a much more proper solution. You said you weren't going to do this. I'm confused.

Also, in my UI component I added this fallback for Qubes R4.0 dom0 because it does not have the newest AyatanaAppIndicator3 fork but it still works the exact same way which you seemed to leave out:
https://github.com/elliotkillick/qubes-video-companion/blob/25a8b397952565bd42079e9519f74b793c0304de/ui/tray_icon.py#L18-L26

I must ask, why are you rewriting some of the code I've already written except all in one file (not exactly a great practice for maintainable code as I'm sure you know)? I know you said there is a lot of deduplication work to be done but in the case of the tray icon UI you implemented I've already done it in a nice class for you (if you want to drop the notifications UI then you could refactor it to not to have a base class). We want it all in one process, not all in one file. Please do refactor this code, thank you. (I'm very picky about code quality haha, Hope this doesn't sound too harsh or anything. It seems like you intended to do a full refactoring on the code anyway)

The video receiver and GStreamer Python ports I like (but the receiver needs a full Python implementation of course). Nice job converting that xrandr command to get the screen dimensions into Python with Gdk too! I also like that by looking at the code there now seems to be a button on the Qubes Video Companion tray icon to stop the stream, I held off on doing that because I figured that would be done for the webcam at least in the Qubes Devices system tray integration but cool nonetheless.

[ElliotKillick]

I was never going to commit this because I decided to just make the ioctl C program (I wrote this a long while ago) but I think my Python implementation of parsing v4l2-ctl --list-formats-ext (supported webcam formats) is more robust if that's how we want to do it at least for now. Basically it gets all the pixel formats, dimensions, and FPS all in one hierarchical dict() (dictionary) which is self contained in a class then any function such as get_best_format() can just do with that data as it likes:
b1b4eca

I've been quite busy honestly so I haven't made much progress on the C program but I may make a push to get it done soon.

[DemiMarie]

By the way, why did you implement a get_resolutions() using v4l2-ctl? I already implemented this in Python and I told you I was making a C port for it using ioctls as that would be a much more proper solution. You said you weren't going to do this. I'm confused.

I didn’t initially plan to do so, but I needed something so that I could stop hard-coding the resolution and frame rate. I had also forgotten that you had written the Python version.

Also, in my UI component I added this fallback for Qubes R4.0 dom0 because it does not have the newest AyatanaAppIndicator3 fork but it still works the exact same way which you seemed to leave out:
https://github.com/elliotkillick/qubes-video-companion/blob/25a8b397952565bd42079e9519f74b793c0304de/ui/tray_icon.py#L18-L26

That certainly needs to be fixed!

I must ask, why are you rewriting some of the code I've already written except all in one file (not exactly a great practice for maintainable code as I'm sure you know)?

This started out as “how can I get this to work at all?”. I built it by editing /etc/qubes-rpc/qvc.Webcam in a TemplateVM. My sys-usb is a DispVM (based on a qube with an empty private volume), so /usr/local/etc/qubes-rpc/qvc.Webcam was not an option. Once I got it working, I just copied it to the qube I use for this project and made the PR. My excuse is that I wanted to use this in “production” (real-world videoconferences) as soon as possible.

I know you said there is a lot of deduplication work to be done but in the case of the tray icon UI you implemented I've already done it in a nice class for you (if you want to drop the notifications UI then you could refactor it to not to have a base class). We want it all in one process, not all in one file. Please do refactor this code, thank you.

Indeed this code is bad ― see above for how it got that way. I should have marked this as a draft because it is nowhere near ready to be merged.

(I'm very picky about code quality haha, Hope this doesn't sound too harsh or anything. It seems like you intended to do a full refactoring on the code anyway)

Being picky about code quality is a good thing. My main concern is proper installation. Python code usually lives under /usr/lib/python<version>/site-packages instead of /usr/share.

The video receiver and GStreamer Python ports I like (but the receiver needs a full Python implementation of course). Nice job converting that xrandr command to get the screen dimensions into Python with Gdk too! I also like that by looking at the code there now seems to be a button on the Qubes Video Companion tray icon to stop the stream, I held off on doing that because I figured that would be done for the webcam at least in the Qubes Devices system tray integration but cool nonetheless.

[ElliotKillick]

My main concern is proper installation. Python code usually lives under /usr/lib/python/site-packages instead of /usr/share.

Yes, that's certainly a good idea! It's just that we haven't had much Python code up until now so I figured how it was at the time was alright for now.

@DemiMarie I want to add you to the list of maintainers in the README. I just need to know what PGP key fingerprint you want to be put there. It should be the same key you use for code signing in this repo, however, the one you used is "unverified" and upon searching you up on keys.gnupg.net there are many keys in your possession.

[DemiMarie]

Update: the code is now in much better shape. A common Service base class contains all of the common logic, while the Webcam and ScreenShare subclasses contain the code specific to their specific video sources. I removed the UserInterface and Notification classes as I considered them to be too small to justify classes of their own, but I kept the TrayIcon class. The ScreenShare code has been tested and works. The Webcam code has been tested on my development qube, but it does not have access to an actual webcam, so it fails when trying to read from a nonexistent video device. Nevertheless, all of the non-trivial code in Webcam is essentially the same as the code I run in sys-usb, which has been used successfully in real-world videoconferencing. In particular, the GStreamer pipeline is identical.

Right now, the only piece of code that I would not consider production-quality is the Video4Linux2 format selection code in webcam.py. I have a slight preference for my code over yours for three reasons: mine does not impose any hard limits on resolution or frame rate, mine only supports the MJPG format supported by the GStreamer pipeline, and mine does not make any persistent changes to the webcam state itself. My code is also significantly shorter. Nevertheless, I would be fine with either. In any case, this code will be rewritten in C, so I consider it the least important.

receiver.py currently execs gst-launch-1.0. The main reasons for doing this are to reduce memory usage and to improve message reporting. A Python interpreter has nonzero memory overhead, and the message handler of gst-launch-1.0 is probably aware of all of GStreamer’s message types. My custom message printer is only aware of three of them.

[DemiMarie]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

I, Demi Marie Obenour <demi@invisiblethingslab.com>, use a key with the
following fingerprint to sign commits to this repository:

8F3BD112BD78566DEABA584FCFA776934CEF6E3F

This message is signed by the key I use to sign commits to official Qubes OS
repositories.
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEgBRHLtDc35Ihl/uViZDnlwzlN+gFAmCZ9IYACgkQiZDnlwzl
N+if6w//ZSRsSlR1NwSvaVnnCIwgwBbUieaQbOzTxt3OFfEoOC9u5/V8ELpLgrhj
KJH3SuAV39gNv+tvvyPZnBPfwAh6i9hNoX9bPHH+Ni6s6dcIMvXUHFpEqOZTMo5W
3d5kxqcuT0WTlmn/75GQyyiTRdn1Gkxc+CjtXy2b0UadNS9+LuUt/Q0bkWFv2C/u
xbDyHEWRkhyEv9qVQE0SBpImk7JTxUGqbx1rj+fys8331TsCzIiiWVq1vUjHkYFC
5XrEz62Kiu/7DjumTVGH/nk6NnrVclGYmpExCIBz4p3p34ADCifta9UJuw+FRIiV
nBUtEcJL+7v6SPWcI73DSFTDI3eZEcdCuj+b3wgiHVjsom2efr+FCldNS9r3N2Kd
kA/GoDINCWfanKGceTTbjBeOALe4+qrFFkskyTNHiHujSNSpFoHyVvNofWoWMvHv
PJkKHtxIgY4pRhHyBOvnekHsHeJNfffNEcs3mM0XG+Yz/KMYT9pi3i6mgCALnrXT
XlLOm/WLrj88SyfwG4mcQBFH1mSRX2dm2TUGjr5r2Hg5KGx13Ys195huGruz/yD4
cYKSPemKR6r0k/qXneeDpiKw+6tDPK0D97F9KzZFwMno7VTcFNl6Lt2g4ZescHTR
GSrlzEs14M71i3pJygX1effnpALhN6RBJbaCQZ1a+9b6SNsBWzQ=
=5lIT
-----END PGP SIGNATURE-----

[ElliotKillick]

Hi Demi, thanks for waiting for on me to getting around to work on this! Amazing job on this code, it is bordering on immaculate! I just did some more cleanups, organizing some functionality into functions tending to some pylint messages, making things more uniform and whatnot.

I also changed the way in which the webcam.py and screenshare.py services start and interact with the main service.py to create an instance of the Webcam and ScreenShare classes in order to create the service. I'm not sure how what you had was working for you but it wasn't for me because it seemed like you were trying to run functions from a class without first creating and instance of it which of course isn't possible.

Next, I changed a few things in the video sender component to make it compatible with the software in Qubes R4.0 dom0 (Fedora 25). That as well as just one tiny change for the video receiving component for Qubes R4.0 that was necessary or it would cease to work. I did unfortunately have to remove a bit of the optimization and function annotations you added to make this happen but we can add them straight back once Qubes R4.1 is released and R4.0 is deprecated. We could also create a branch for R4.0 compatibility if you want. It's important to note that the vast majority of Qubes users still run R4.0 (including myself) as it is the stable build so it's important that we continue to provide support for it. Personally, I've been thinking about switching to R4.1 but I'm unsure as to how stable it is as I do use Qubes OS daily for everything I do on the computer so it needs to be completely stable.

Then, I updated (and improved) the Makefile and the packages so they build successfully with all of our changes.

And lastly, I restructured the whole project to put the video sending components into the sender folder and the video receiving components into the receiver folder so the folder hierarchy makes a lot more sense now.

[ElliotKillick]

I removed the UserInterface and Notification classes as I considered them to be too small to justify classes of their own

I 100% agree. My plan was to (eventually) make this shell script here also use that Notification Python class instead of using notify-send thus making the class justifiable but I guess it's not a big deal.

Right now, the only piece of code that I would not consider production-quality is the Video4Linux2 format selection code in webcam.py. I have a slight preference for my code over yours for three reasons: mine does not impose any hard limits on resolution or frame rate, mine only supports the MJPG format supported by the GStreamer pipeline, and mine does not make any persistent changes to the webcam state itself. My code is also significantly shorter. Nevertheless, I would be fine with either. In any case, this code will be rewritten in C, so I consider it the least important.

Sounds good, you went for a different regex approach for your parser whereas I want with a line-by-line parser. I considered using regex but it seemed easier (at least for me) to do a line-by-line approach then trying to put together a big regex. I also knew that a line-by-line parser would be much more extensible (should that be necessary) and orders of magnitude faster because regex is generally very slow. However, it's still technically a micro-optimization so it's not worth worrying about. Let's just leave it as is because as you say, this will be rewritten in C with ioctlss so it's the least of our concerns.

[ElliotKillick]

I noticed in the Makefile you also also installed the /etc/qubes/rpc-config/qvc.* files into dom0 making them the only ones in there. This is opposed to the VMs which have other rpc-config files in that directory. Can you confirm this is correct?

[DemiMarie]

I noticed in the Makefile you also also installed the /etc/qubes/rpc-config/qvc.* files into dom0 making them the only ones in there. This is opposed to the VMs which have other rpc-config files in that directory. Can you confirm this is correct?

It is not.

[ElliotKillick]

Ready to merge?

[DemiMarie]

I also changed the way in which the webcam.py and screenshare.py services start and interact with the main service.py to create an instance of the Webcam and ScreenShare classes in order to create the service. I'm not sure how what you had was working for you but it wasn't for me because it seemed like you were trying to run functions from a class without first creating and instance of it which of course isn't possible.

I did test the code on my system, but it might very well have not worked on older versions of Python.

[ElliotKillick]

I did test the code on my system, but it might very well have not worked on older versions of Python.

I momentarily thought this also affected newer versions of Python but after further inspection it was just an oddity with how I had the kali AppVM I was using for testing set up that caused me to believe that.

However, those fixes are still definitely necessary the older version of Python in dom0.

[DemiMarie]

I did test the code on my system, but it might very well have not worked on older versions of Python.

I momentarily thought this also affected newer versions of Python but after further inspection it was just an oddity with how I had the kali AppVM I was using for testing set up that caused me to believe that.

However, those fixes are still definitely necessary the older version of Python in dom0.

That definitely makes sense! Time to go ahead and merge I guess.

[DemiMarie]

(not sure if you should do it for code-signing reasons)

[ElliotKillick]

For review, would the standard merge commit method be best or a rebase merge?

[DemiMarie]

For review, would the standard merge commit method be best or a rebase merge?

I suspect that merge commit is better here, as it preserves commit signatures.