Re: [PATCH spice-streaming-agent v4 1/2] Move out the cursor-updating code into it's own class

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2018-07-09 at 05:18 -0400, Frediano Ziglio wrote:
> > 
> > Wraps the code in a functor which can then be used in std::thread. Keeps
> > the call to std::thread::detach() for now, as the thread actually cannot
> > be properly joined. It spends most of the time blocking on XNextEvent()
> > and there is no way of interrupting it.
> > 
> > It seems XNextEvent() is acutally using a socket, so a proper solution
> > might be to integrate this into the loop of the main process.
> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
> 
> Acked-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> 
> This part of the code is getting maybe too "importance".
> Not all settings potentially will use this part of the code.
> This part is useful to support client mouse if the plugin is not
> able to capture the cursor shape but on other cases (like for instance
> if the capture came with the cursor) is useless and should be
> disabled.

I'm not so sure about this, from my limited experience client-side
cursor is much smoother and more responsive than server-side (or rather
guest-side in this case).

> Not sure if this patch helps or create more issues but
> I don't think introduce much regression on this.

Any issues on your mind? In my opinion this patch makes it easier, if
anything, to enable this conditionally.

Cheers,
Lukas

> Frediano
> 
> > ---
> >  src/Makefile.am               |   2 +
> >  src/cursor-updater.cpp        | 106 ++++++++++++++++++++++++++++++++++
> >  src/cursor-updater.hpp        |  34 +++++++++++
> >  src/spice-streaming-agent.cpp |  84 +--------------------------
> >  4 files changed, 145 insertions(+), 81 deletions(-)
> >  create mode 100644 src/cursor-updater.cpp
> >  create mode 100644 src/cursor-updater.hpp
> > 
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index fead680..40544ba 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -52,6 +52,8 @@ spice_streaming_agent_SOURCES = \
> >  	spice-streaming-agent.cpp \
> >  	concrete-agent.cpp \
> >  	concrete-agent.hpp \
> > +	cursor-updater.cpp \
> > +	cursor-updater.hpp \
> >  	error.cpp \
> >  	error.hpp \
> >  	frame-log.cpp \
> > diff --git a/src/cursor-updater.cpp b/src/cursor-updater.cpp
> > new file mode 100644
> > index 0000000..8f65e83
> > --- /dev/null
> > +++ b/src/cursor-updater.cpp
> > @@ -0,0 +1,106 @@
> > +/* A class that monitors X11 cursor changes and sends the cursor image over
> > the
> > + * streaming virtio port.
> > + *
> > + * \copyright
> > + * Copyright 2016-2018 Red Hat Inc. All rights reserved.
> > + */
> > +
> > +#include "cursor-updater.hpp"
> > +
> > +#include "error.hpp"
> > +
> > +#include <spice/stream-device.h>
> > +#include <spice/enums.h>
> > +
> > +#include <cstring>
> > +#include <functional>
> > +#include <memory>
> > +#include <X11/extensions/Xfixes.h>
> > +
> > +
> > +namespace spice {
> > +namespace streaming_agent {
> > +
> > +namespace {
> > +
> > +void send_cursor(StreamPort &stream_port, unsigned width, unsigned height,
> > int hotspot_x, int hotspot_y,
> > +                 std::function<void(uint32_t *)> fill_cursor)
> > +{
> > +    if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH || height >=
> > STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
> > +        return;
> > +    }
> > +
> > +    size_t cursor_size =
> > +        sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
> > +        width * height * sizeof(uint32_t);
> > +    std::unique_ptr<uint8_t[]> msg(new uint8_t[cursor_size]);
> > +
> > +    StreamDevHeader
> > &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
> > +    memset(&dev_hdr, 0, sizeof(dev_hdr));
> > +    dev_hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > +    dev_hdr.type = STREAM_TYPE_CURSOR_SET;
> > +    dev_hdr.size = cursor_size - sizeof(StreamDevHeader);
> > +
> > +    StreamMsgCursorSet &cursor_msg(*reinterpret_cast<StreamMsgCursorSet
> > *>(msg.get() + sizeof(StreamDevHeader)));
> > +    memset(&cursor_msg, 0, sizeof(cursor_msg));
> > +
> > +    cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
> > +    cursor_msg.width = width;
> > +    cursor_msg.height = height;
> > +    cursor_msg.hot_spot_x = hotspot_x;
> > +    cursor_msg.hot_spot_y = hotspot_y;
> > +
> > +    uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg.data);
> > +    fill_cursor(pixels);
> > +
> > +    std::lock_guard<std::mutex> guard(stream_port.mutex);
> > +    stream_port.write(msg.get(), cursor_size);
> > +}
> > +
> > +} // namespace
> > +
> > +CursorUpdater::CursorUpdater(StreamPort *stream_port) :
> > stream_port(stream_port)
> > +{
> > +    display = XOpenDisplay(nullptr);
> > +    if (display == nullptr) {
> > +        throw Error("Failed to open X display");
> > +    }
> > +
> > +    int error_base;
> > +    if (!XFixesQueryExtension(display, &xfixes_event_base, &error_base)) {
> > +        throw Error("XFixesQueryExtension failed");
> > +    }
> > +
> > +    XFixesSelectCursorInput(display, DefaultRootWindow(display),
> > XFixesDisplayCursorNotifyMask);
> > +}
> > +
> > +[[noreturn]] void CursorUpdater::operator()()
> > +{
> > +    unsigned long last_serial = 0;
> > +
> > +    while (1) {
> > +        XEvent event;
> > +        XNextEvent(display, &event);
> > +        if (event.type != xfixes_event_base + 1) {
> > +            continue;
> > +        }
> > +
> > +        XFixesCursorImage *cursor = XFixesGetCursorImage(display);
> > +        if (!cursor) {
> > +            continue;
> > +        }
> > +
> > +        if (cursor->cursor_serial == last_serial) {
> > +            continue;
> > +        }
> > +
> > +        last_serial = cursor->cursor_serial;
> > +        auto fill_cursor = [cursor](uint32_t *pixels) {
> > +            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
> > +                pixels[i] = cursor->pixels[i];
> > +        };
> > +        send_cursor(*stream_port, cursor->width, cursor->height,
> > cursor->xhot, cursor->yhot, fill_cursor);
> > +    }
> > +}
> > +
> > +}} // namespace spice::streaming_agent
> > diff --git a/src/cursor-updater.hpp b/src/cursor-updater.hpp
> > new file mode 100644
> > index 0000000..d5f00af
> > --- /dev/null
> > +++ b/src/cursor-updater.hpp
> > @@ -0,0 +1,34 @@
> > +/* A class that monitors X11 cursor changes and sends the cursor image over
> > the
> > + * streaming virtio port.
> > + *
> > + * \copyright
> > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > + */
> > +
> > +#ifndef SPICE_STREAMING_AGENT_CURSOR_UPDATER_HPP
> > +#define SPICE_STREAMING_AGENT_CURSOR_UPDATER_HPP
> > +
> > +#include "stream-port.hpp"
> > +
> > +#include <X11/Xlib.h>
> > +
> > +
> > +namespace spice {
> > +namespace streaming_agent {
> > +
> > +class CursorUpdater
> > +{
> > +public:
> > +    CursorUpdater(StreamPort *stream_port);
> > +
> > +    void operator()();
> > +
> > +private:
> > +    StreamPort *stream_port;
> > +    Display *display;  // the X11 display
> > +    int xfixes_event_base;  // event number for the XFixes events
> > +};
> > +
> > +}} // namespace spice::streaming_agent
> > +
> > +#endif // SPICE_STREAMING_AGENT_CURSOR_UPDATER_HPP
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index 2e93049..4103720 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -6,6 +6,7 @@
> >  
> >  #include "concrete-agent.hpp"
> >  #include "mjpeg-fallback.hpp"
> > +#include "cursor-updater.hpp"
> >  #include "frame-log.hpp"
> >  #include "stream-port.hpp"
> >  #include "error.hpp"
> > @@ -35,9 +36,6 @@
> >  #include <thread>
> >  #include <vector>
> >  #include <string>
> > -#include <functional>
> > -#include <X11/Xlib.h>
> > -#include <X11/extensions/Xfixes.h>
> >  
> >  using namespace spice::streaming_agent;
> >  
> > @@ -253,70 +251,6 @@ static void usage(const char *progname)
> >      exit(1);
> >  }
> >  
> > -static void
> > -send_cursor(StreamPort &stream_port, unsigned width, unsigned height, int
> > hotspot_x, int hotspot_y,
> > -            std::function<void(uint32_t *)> fill_cursor)
> > -{
> > -    if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH || height >=
> > STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
> > -        return;
> > -    }
> > -
> > -    size_t cursor_size =
> > -        sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
> > -        width * height * sizeof(uint32_t);
> > -    std::unique_ptr<uint8_t[]> msg(new uint8_t[cursor_size]);
> > -
> > -    StreamDevHeader
> > &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
> > -    memset(&dev_hdr, 0, sizeof(dev_hdr));
> > -    dev_hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > -    dev_hdr.type = STREAM_TYPE_CURSOR_SET;
> > -    dev_hdr.size = cursor_size - sizeof(StreamDevHeader);
> > -
> > -    StreamMsgCursorSet &cursor_msg(*reinterpret_cast<StreamMsgCursorSet
> > *>(msg.get() + sizeof(StreamDevHeader)));
> > -    memset(&cursor_msg, 0, sizeof(cursor_msg));
> > -
> > -    cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
> > -    cursor_msg.width = width;
> > -    cursor_msg.height = height;
> > -    cursor_msg.hot_spot_x = hotspot_x;
> > -    cursor_msg.hot_spot_y = hotspot_y;
> > -
> > -    uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg.data);
> > -    fill_cursor(pixels);
> > -
> > -    std::lock_guard<std::mutex> guard(stream_port.mutex);
> > -    stream_port.write(msg.get(), cursor_size);
> > -}
> > -
> > -static void cursor_changes(StreamPort *stream_port, Display *display, int
> > event_base)
> > -{
> > -    unsigned long last_serial = 0;
> > -
> > -    while (1) {
> > -        XEvent event;
> > -        XNextEvent(display, &event);
> > -        if (event.type != event_base + 1) {
> > -            continue;
> > -        }
> > -
> > -        XFixesCursorImage *cursor = XFixesGetCursorImage(display);
> > -        if (!cursor) {
> > -            continue;
> > -        }
> > -
> > -        if (cursor->cursor_serial == last_serial) {
> > -            continue;
> > -        }
> > -
> > -        last_serial = cursor->cursor_serial;
> > -        auto fill_cursor = [cursor](uint32_t *pixels) {
> > -            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
> > -                pixels[i] = cursor->pixels[i];
> > -        };
> > -        send_cursor(*stream_port, cursor->width, cursor->height,
> > cursor->xhot, cursor->yhot, fill_cursor);
> > -    }
> > -}
> > -
> >  static void
> >  do_capture(StreamPort &stream_port, FrameLog &frame_log)
> >  {
> > @@ -474,22 +408,10 @@ int main(int argc, char* argv[])
> >          }
> >          old_args.clear();
> >  
> > -        Display *display = XOpenDisplay(NULL);
> > -        if (display == NULL) {
> > -            throw Error("Failed to open X display");
> > -        }
> > -
> > -        int event_base, error_base;
> > -        if (!XFixesQueryExtension(display, &event_base, &error_base)) {
> > -            throw Error("XFixesQueryExtension failed");
> > -        }
> > -        Window rootwindow = DefaultRootWindow(display);
> > -        XFixesSelectCursorInput(display, rootwindow,
> > XFixesDisplayCursorNotifyMask);
> > -
> >          StreamPort stream_port(stream_port_name);
> >  
> > -        std::thread cursor_th(cursor_changes, &stream_port, display,
> > event_base);
> > -        cursor_th.detach();
> > +        std::thread cursor_updater{CursorUpdater(&stream_port)};
> > +        cursor_updater.detach();
> >  
> >          do_capture(stream_port, frame_log);
> >      }
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]