[Spice-devel] [PATCH 14/22] Create classes encapsulating the X11 display cursor capture

[Christophe de Dinechin]

> On 1 Mar 2018, at 14:56, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> 
> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin at redhat.com>
>> 
>> There are two classes:
>> - The X11CursorUpdater class sends the messages. It can be used when
>>  no thread is required.
>> - The X11CursorThread spawns an X11CursorUpdater in a new thread.
>> 
>> These classes will be moved to a separate file in a later patch, but
>> are kept in the same file at this stage to make it easier to track
>> changes and bisect.
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 118 +++++++++++++++++++++++++++---------------
>> 1 file changed, 76 insertions(+), 42 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 8bbd457..d1996fd 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -206,6 +206,80 @@ private:
>>     }
>> };
>> 
>> +class X11CursorUpdater
>> +{
>> +public:
>> +    X11CursorUpdater(Stream &stream);
>> +    ~X11CursorUpdater();
>> +    void send_cursor_changes();
>> +
>> +private:
>> +    Stream &stream;
>> +    Display *display;
>> +};
>> +
>> +class X11CursorThread
>> +{
>> +public:
>> +    X11CursorThread(Stream &stream);
>> +    static void record_cursor_changes(X11CursorThread *self) { self->updater.send_cursor_changes(); }
>> +
>> +private:
>> +    X11CursorUpdater updater;
>> +    std::thread thread;
>> +};
> 
> Ok, I was wondering what you had in mind when you said you'll probably
> make two classes. What is the X11CursorThread useful for, though? It is
> basically a glue, which isn't necessary, if you turn X11CursorUpdater
> into a functor? (as per my original "AgentRunner" patch, let me know if
> I need to clarify here)

It’s to have RAII on the updater. The thread owns the updater.

> 
> The only other practical thing I see it doing is wrapping these two
> lines into a single call:
> 
> std::thread cursor_th(X11CursorUpdater(...));
> cursor_th.detach();
> 
> I think these two calls would be fine in main() below and we could
> leave out the glue class...
> 
> (I also have my doubts about the detach, perhaps there should be a
> proper join, haven't looked into it yet. Might be causing the Ctrl-C
> not working.)

Yes.

> 
>> +
>> +X11CursorUpdater::X11CursorUpdater(Stream &stream)
>> +    : stream(stream),
>> +      display(XOpenDisplay(NULL))
>> +{
>> +    if (display == NULL) {
>> +        throw Error("failed to open display").syslog();
>> +    }
>> +}
>> +
>> +X11CursorUpdater::~X11CursorUpdater()
>> +{
>> +    XCloseDisplay(display);
>> +}
>> +
>> +void X11CursorUpdater::send_cursor_changes()
>> +{
>> +    unsigned long last_serial = 0;
>> +
>> +    int event_base, error_base;
>> +    if (!XFixesQueryExtension(display, &event_base, &error_base)) {
>> +        syslog(LOG_WARNING, "XFixesQueryExtension failed, not sending cursor changes\n");
>> +        return; // also terminates the X11CursorThread if that's how we were launched
>> +    }
>> +    Window rootwindow = DefaultRootWindow(display);
>> +    XFixesSelectCursorInput(display, rootwindow, XFixesDisplayCursorNotifyMask);
>> +
>> +    while (true) {
>> +        XEvent event;
>> +        XNextEvent(display, &event);
>> +        if (event.type != event_base + 1) {
>> +            continue;
>> +        }
>> +
>> +        XFixesCursorImage *cursor = XFixesGetCursorImage(display);
>> +        if (!cursor || cursor->cursor_serial == last_serial) {
>> +            continue;
>> +        }
>> +
>> +        last_serial = cursor->cursor_serial;
>> +        stream.send<X11CursorMessage>(cursor);
>> +    }
>> +}
>> +
>> +X11CursorThread::X11CursorThread(Stream &stream)
>> +    : updater(stream),
>> +      thread(record_cursor_changes, this)
>> +{
>> +    thread.detach();
>> +}
>> +
>> +
>> }} // namespace spice::streaming_agent
>> 
>> static bool quit_requested = false;
>> @@ -377,31 +451,6 @@ static void usage(const char *progname)
>>     exit(1);
>> }
>> 
>> -static void cursor_changes(Stream *stream, 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;
>> -        stream->send<X11CursorMessage>(cursor);
>> -    }
>> -}
>> -
>> static void
>> do_capture(Stream &stream, const char *streamport, FILE *f_log)
>> {
>> @@ -554,25 +603,10 @@ int main(int argc, char* argv[])
>>         }
>>     }
>> 
>> -    Display *display = XOpenDisplay(NULL);
>> -    if (display == NULL) {
>> -        syslog(LOG_ERR, "failed to open display\n");
>> -        return EXIT_FAILURE;
>> -    }
>> -    int event_base, error_base;
>> -    if (!XFixesQueryExtension(display, &event_base, &error_base)) {
>> -        syslog(LOG_ERR, "XFixesQueryExtension failed\n");
>> -        return EXIT_FAILURE;
>> -    }
>> -    Window rootwindow = DefaultRootWindow(display);
>> -    XFixesSelectCursorInput(display, rootwindow, XFixesDisplayCursorNotifyMask);
>> -
>> -    Stream stream(streamport);
>> -    std::thread cursor_th(cursor_changes, &stream, display, event_base);
>> -    cursor_th.detach();
>> -
>>     int ret = EXIT_SUCCESS;
>>     try {
>> +        Stream stream(streamport);
>> +        X11CursorThread cursor_thread(stream);
>>         do_capture(stream, streamport, f_log);
>>     }
>>     catch (Error &err) {