[Spice-devel] [PATCH 14/22] Create classes encapsulating the X11 display cursor capture
Lukáš Hrázký
lhrazky at redhat.com
Fri Mar 2 13:45:43 UTC 2018
On Fri, 2018-03-02 at 14:28 +0100, Christophe de Dinechin wrote:
> > On 2 Mar 2018, at 11:37, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> >
> > On Thu, 2018-03-01 at 20:01 +0100, Christophe de Dinechin wrote:
> > > > 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.
> >
> > If you pass a functor to std::thread, it will be copied or moved inside it, so that it will also own it, so from this point of view, it's the same, you get RAII too.
>
> I think I am confused by your suggestion. How do we control the ‘join’ in that case? If we did not join, we terminate(). If we call ‘detach’, we no longer own it.
>
> Here is an example to explain what I mean:
>
> #include <thread>
> #include <memory>
> #include <iostream>
>
> struct foo
> {
> foo() { std::cerr << "foo::foo()\n"; }
> foo(const foo &o) { std::cerr << "foo::foo(&)\n"; }
> foo(const foo &&o) { std::cerr << "foo::foo(&&)\n"; }
> ~foo() { std::cerr << "foo::~foo()\n"; }
>
> void operator()(char c) {
> std::cerr << "operator() with '" << c << "'\n";
> for (int i = 0; i < 100; i++)
> {
> fputc(c, stderr);
> std::this_thread::sleep_for(std::chrono::milliseconds(65));
> }
> std::cerr << "operator() completed\n";
> }
> };
>
> int main()
> {
> std::cerr << "Start\n";
> foo f;
> std::thread t(f, 'f');
>
> for (int i = 0; i < 100; i++)
> {
> fputc('m', stderr);
> std::this_thread::sleep_for(std::chrono::milliseconds(65));
> }
t.join();
> }
>
> At end, this program terminates. And in ~foo or in operator(), I cannot join, there’s no thread object there. So I’m not sure what you want to do.
I've added the join to your program, hope it's clear. I am also not
sure how you mean it...
In this case, neither main thread nor 'foo' have an endless loop, they
both end by themselves, so the situation is simple. You only need to
ensure you wait in the main thread for the 't' thread to finish.
That is, the way you use join is in the main thread to wait for the
thread on which you are calling the join, in case that's what's unclear
here.
I may be missing your point with the terminate(), in case it's still
outstanding, can you elaborate?
>
>
> > This also made me realize you should delete the copy constructor of X11CursorUpdater and implement the move constructor.
>
> Done, although not needing it for the moment, I deleted the move ctor too.
>
> >
> > > >
> > > > 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) {
> > >
> > >
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
More information about the Spice-devel
mailing list