[Spice-devel] [PATCH 14/22] Create classes encapsulating the X11 display cursor capture
Christophe de Dinechin
cdupontd at redhat.com
Fri Mar 2 21:10:28 UTC 2018
> On 2 Mar 2018, at 14:45, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>
> 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();
I put that join in the destructor. If you put it in line like this, it’s not RAII anymore, since an exception in the main thread will not call 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…
Well, precisely, the whole reason for adding a wrapper class was that this join was in the destructor, so called in case of exception.
I’m not sure if we are talking past one another once more…
>
> 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
>>
>>
> _______________________________________________
> 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