[14/17] Create a class encapsulating the X11 display cursor capture

Submitted by Christophe de Dinechin on Feb. 16, 2018, 4:15 p.m.

Details

Message ID 20180216161547.28110-15-christophe@dinechin.org
State New
Headers show
Series "WIP: Refactor the streaming agent towards a more standard C++ style" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 16, 2018, 4:15 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

This class will need to be moved to a separate file in a later patch.
This is done in two steps to make the evolution of the code easier to track,
as well as to facilitate later 'git bisect'

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 src/spice-streaming-agent.cpp | 106 ++++++++++++++++++++++++++----------------
 1 file changed, 66 insertions(+), 40 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index faf850c..f4df6be 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -191,6 +191,70 @@  private:
     std::unique_ptr<uint32_t[]> pixels;
 };
 
+
+class X11CursorThread
+{
+public:
+    X11CursorThread(Stream &stream);
+    ~X11CursorThread();
+
+    static void record_cursor_changes(X11CursorThread *self) { self->cursor_changes(); }
+    void cursor_changes();
+
+private:
+    Stream &stream;
+    Display *display;
+    std::thread thread;
+};
+
+
+X11CursorThread::X11CursorThread(Stream &stream)
+    : stream(stream),
+      display(XOpenDisplay(NULL)),
+      thread(record_cursor_changes, this)
+{
+    if (display == NULL) {
+        throw std::runtime_error("failed to open display\n");
+    }
+    thread.detach();
+}
+
+X11CursorThread::~X11CursorThread()
+{
+    XCloseDisplay(display);
+}
+
+void X11CursorThread::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 thread
+    }
+    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)
+            continue;
+
+        if (cursor->cursor_serial == last_serial)
+            continue;
+
+        last_serial = cursor->cursor_serial;
+        if (!stream.send<X11CursorMessage>(cursor))
+            syslog(LOG_WARNING, "FAILED to send cursor\n");
+    }
+}
+
 }} // namespace spice::streaming_agent
 
 
@@ -338,29 +402,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;
-        if (!stream->send<X11CursorMessage>(cursor))
-            syslog(LOG_WARNING, "FAILED to send cursor\n");
-    }
-}
-
 static void
 do_capture(Stream &stream, const char *streamport, FILE *f_log)
 {
@@ -503,25 +544,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 streamfd(streamport);
-    std::thread cursor_th(cursor_changes, &streamfd, display, event_base);
-    cursor_th.detach();
-
     int ret = EXIT_SUCCESS;
     try {
+        Stream streamfd(streamport);
+        X11CursorThread cursor_thread(streamfd);
         do_capture(streamfd, streamport, f_log);
     }
     catch (std::runtime_error &err) {

Comments

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> This class will need to be moved to a separate file in a later patch.
> This is done in two steps to make the evolution of the code easier to track,
> as well as to facilitate later 'git bisect'
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 106 ++++++++++++++++++++++++++----------------
>  1 file changed, 66 insertions(+), 40 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index faf850c..f4df6be 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -191,6 +191,70 @@ private:
>      std::unique_ptr<uint32_t[]> pixels;
>  };
>  
> +
> +class X11CursorThread
> +{
> +public:
> +    X11CursorThread(Stream &stream);
> +    ~X11CursorThread();
> +
> +    static void record_cursor_changes(X11CursorThread *self) { self->cursor_changes(); }
> +    void cursor_changes();
> +
> +private:
> +    Stream &stream;
> +    Display *display;
> +    std::thread thread;
> +};

I would still very much like this class to be a functor that is passed
to a thread. It IMHO makes for a better encapsulation. You can then
launch it any way you like, for example on the main thread in a test.

FWIW, I came up with the name CursorUpdater :) (since if it's not a
thread we can't call it so)

> +
> +
> +X11CursorThread::X11CursorThread(Stream &stream)
> +    : stream(stream),
> +      display(XOpenDisplay(NULL)),
> +      thread(record_cursor_changes, this)
> +{
> +    if (display == NULL) {
> +        throw std::runtime_error("failed to open display\n");
> +    }
> +    thread.detach();
> +}
> +
> +X11CursorThread::~X11CursorThread()
> +{
> +    XCloseDisplay(display);
> +}
> +
> +void X11CursorThread::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 thread
> +    }
> +    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)
> +            continue;
> +
> +        if (cursor->cursor_serial == last_serial)
> +            continue;
> +
> +        last_serial = cursor->cursor_serial;
> +        if (!stream.send<X11CursorMessage>(cursor))
> +            syslog(LOG_WARNING, "FAILED to send cursor\n");
> +    }
> +}
> +
>  }} // namespace spice::streaming_agent
>  
>  
> @@ -338,29 +402,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;
> -        if (!stream->send<X11CursorMessage>(cursor))
> -            syslog(LOG_WARNING, "FAILED to send cursor\n");
> -    }
> -}
> -
>  static void
>  do_capture(Stream &stream, const char *streamport, FILE *f_log)
>  {
> @@ -503,25 +544,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 streamfd(streamport);
> -    std::thread cursor_th(cursor_changes, &streamfd, display, event_base);
> -    cursor_th.detach();
> -
>      int ret = EXIT_SUCCESS;
>      try {
> +        Stream streamfd(streamport);
> +        X11CursorThread cursor_thread(streamfd);
>          do_capture(streamfd, streamport, f_log);
>      }
>      catch (std::runtime_error &err) {
> On 20 Feb 2018, at 16:42, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> This class will need to be moved to a separate file in a later patch.
>> This is done in two steps to make the evolution of the code easier to track,
>> as well as to facilitate later 'git bisect'
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 106 ++++++++++++++++++++++++++----------------
>> 1 file changed, 66 insertions(+), 40 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index faf850c..f4df6be 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -191,6 +191,70 @@ private:
>>     std::unique_ptr<uint32_t[]> pixels;
>> };
>> 
>> +
>> +class X11CursorThread
>> +{
>> +public:
>> +    X11CursorThread(Stream &stream);
>> +    ~X11CursorThread();
>> +
>> +    static void record_cursor_changes(X11CursorThread *self) { self->cursor_changes(); }
>> +    void cursor_changes();
>> +
>> +private:
>> +    Stream &stream;
>> +    Display *display;
>> +    std::thread thread;
>> +};
> 
> I would still very much like this class to be a functor that is passed
> to a thread. It IMHO makes for a better encapsulation. You can then
> launch it any way you like, for example on the main thread in a test.

Could be done that way too, but I might then have two classes, CursorUpdater and CursorThread. Will do that as a separate patch so that we can decide if it’s better.

(related bug not yet fixed in this series: agent does not die on Control-C)

> 
> FWIW, I came up with the name CursorUpdater :) (since if it's not a
> thread we can't call it so)
> 
>> +
>> +
>> +X11CursorThread::X11CursorThread(Stream &stream)
>> +    : stream(stream),
>> +      display(XOpenDisplay(NULL)),
>> +      thread(record_cursor_changes, this)
>> +{
>> +    if (display == NULL) {
>> +        throw std::runtime_error("failed to open display\n");
>> +    }
>> +    thread.detach();
>> +}
>> +
>> +X11CursorThread::~X11CursorThread()
>> +{
>> +    XCloseDisplay(display);
>> +}
>> +
>> +void X11CursorThread::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 thread
>> +    }
>> +    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)
>> +            continue;
>> +
>> +        if (cursor->cursor_serial == last_serial)
>> +            continue;
>> +
>> +        last_serial = cursor->cursor_serial;
>> +        if (!stream.send<X11CursorMessage>(cursor))
>> +            syslog(LOG_WARNING, "FAILED to send cursor\n");
>> +    }
>> +}
>> +
>> }} // namespace spice::streaming_agent
>> 
>> 
>> @@ -338,29 +402,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;
>> -        if (!stream->send<X11CursorMessage>(cursor))
>> -            syslog(LOG_WARNING, "FAILED to send cursor\n");
>> -    }
>> -}
>> -
>> static void
>> do_capture(Stream &stream, const char *streamport, FILE *f_log)
>> {
>> @@ -503,25 +544,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 streamfd(streamport);
>> -    std::thread cursor_th(cursor_changes, &streamfd, display, event_base);
>> -    cursor_th.detach();
>> -
>>     int ret = EXIT_SUCCESS;
>>     try {
>> +        Stream streamfd(streamport);
>> +        X11CursorThread cursor_thread(streamfd);
>>         do_capture(streamfd, streamport, f_log);
>>     }
>>     catch (std::runtime_error &err) {
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel