[Spice-devel] [PATCH spice-streaming-agent 2/5] Add file with utilities for Xorg

[Lukáš Hrázký]

On Mon, 2018-02-05 at 02:58 -0500, Frediano Ziglio wrote:
> > 
> > On Thu, 2018-02-01 at 16:40 +0000, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  src/Makefile.am               |  2 ++
> > >  src/spice-streaming-agent.cpp |  1 +
> > >  src/xorg-utils.cpp            | 33 +++++++++++++++++++++++++++++++++
> > >  src/xorg-utils.hpp            |  8 ++++++++
> > >  4 files changed, 44 insertions(+)
> > >  create mode 100644 src/xorg-utils.cpp
> > >  create mode 100644 src/xorg-utils.hpp
> > > 
> > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > index 8d5c5bd..bbfaa35 100644
> > > --- a/src/Makefile.am
> > > +++ b/src/Makefile.am
> > > @@ -53,6 +53,8 @@ spice_streaming_agent_SOURCES = \
> > >  	static-plugin.hpp \
> > >  	concrete-agent.cpp \
> > >  	concrete-agent.hpp \
> > > +	xorg-utils.cpp \
> > > +	xorg-utils.hpp \
> > >  	mjpeg-fallback.cpp \
> > >  	jpeg.cpp \
> > >  	jpeg.hpp \
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
> > > agent.cpp
> > > index 94d9d25..a688d1f 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -33,6 +33,7 @@
> > >  #include <spice-streaming-agent/plugin.hpp>
> > >  
> > >  #include "hexdump.h"
> > > +#include "xorg-utils.hpp"
> > >  #include "concrete-agent.hpp"
> > >  
> > >  using namespace std;
> > > diff --git a/src/xorg-utils.cpp b/src/xorg-utils.cpp
> > > new file mode 100644
> > > index 0000000..184ca63
> > > --- /dev/null
> > > +++ b/src/xorg-utils.cpp
> > > @@ -0,0 +1,33 @@
> > > +#include <exception>
> > > +#include <stdexcept>
> > > +#include <X11/Xatom.h>
> > > +
> > > +#include "xorg-utils.hpp"
> > > +
> > > +int
> > > +get_win_prop_int(Display *display, Window win, Atom atom)
> > > +{
> > > +    int status;
> > > +    unsigned char *prop;
> > > +    unsigned long bytes_after, nitems;
> > > +    int actual_format;
> > > +    Atom actual_type;
> > > +
> > > +    status = XGetWindowProperty(display, win, atom, 0, 64,
> > > +                                False, XA_INTEGER, &actual_type,
> > > +                                &actual_format, &nitems,
> > > &bytes_after,
> > > +                                &prop);
> > > +    if (status == Success && nitems > 0) {
> > > +        switch (actual_format) {
> > > +        case 8:
> > > +            return *(const signed char *)prop;
> > > +        case 16:
> > > +            return *(const short *)prop;
> > > +        case 32:
> > > +            // although format is 32 is represented always as a long
> > > which
> > > +            // could be 64 bit
> > > +            return *(const long *)prop;
> > > +        }
> > > +    }
> > > +    throw std::runtime_error("error getting property");
> > > +}
> > > diff --git a/src/xorg-utils.hpp b/src/xorg-utils.hpp
> > > new file mode 100644
> > > index 0000000..b5916de
> > > --- /dev/null
> > > +++ b/src/xorg-utils.hpp
> > > @@ -0,0 +1,8 @@
> > > +#ifndef STREAMING_AGENT_XORG_UTILS_HPP
> > > +#define STREAMING_AGENT_XORG_UTILS_HPP
> > > +
> > > +#include <X11/Xlib.h>
> > 
> > Not having hands on experience, but from what I've heard and read XCB
> > is better and simpler than Xlib, any reason why we are using Xlib
> > instead of XCB here?
> > 
> > > +int get_win_prop_int(Display *display, Window win, Atom atom);
> > > +
> > > +#endif // STREAMING_AGENT_XORG_UTILS_HPP
> 
> There are different reason. The main difference between XLib and XCB is
> that XLib is synchronous, for every function called you have to wait
> for the reply from the X server which can be remote. XCB allows to send
> a batch of requests even if they return a value.
> XCB is more recent.
> We already started using XLib (I think for the Xfixes code to handle the
> mouse). You can get a XCB connection from the Xlib one. In this case would
> be more a complication than a simplification as you have only a single
> function call so you would have to add a step to retrieve XCB connection,
> change the property request, wait for the reply (in Xlib is automatic).
> Would be more sensible to remove all Xlib so removing the need to get a
> XCB connection from a Display* but this is more a follow up.

Sure. I just used the opportunity to ask and I'm not even sure it would
better for us myself. If you didn't evaluate this before, though, it
would be good for us to evaluate it now and potentially switch while
the codebase is still small.

Lukas