Re: [PATCH 14/22] Create classes encapsulating the X11 display cursor capture

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

 




> On 2 Mar 2018, at 14:45, Lukáš Hrázký <lhrazky@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
>>>>> 
>>>>> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>>>>>> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>>>>>> 
>>>>>> 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@xxxxxxxxxx>
>>>>>> ---
>>>>>> 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@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>> 
>> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




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