>
> On Tue, Nov 21, 2017 at 03:35:55AM -0500, Frediano Ziglio wrote:
> > >
> > > On Tue, Nov 21, 2017 at 03:25:35AM -0500, Frediano Ziglio wrote:
> > > > >
> > > > > On Mon, Nov 20, 2017 at 03:18:14PM +0000, Frediano Ziglio wrote:
> > > > > > Allows a better encapsulation in the future
> > > > > >
> > > > > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > > > > ---
> > > > > > src/spice-streaming-agent.cpp | 28 +++++++++++++++++-----------
> > > > > > 1 file changed, 17 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/src/spice-streaming-agent.cpp
> > > > > > b/src/spice-streaming-agent.cpp
> > > > > > index 23b9768..53ffbf0 100644
> > > > > > --- a/src/spice-streaming-agent.cpp
> > > > > > +++ b/src/spice-streaming-agent.cpp
> > > > > > @@ -22,6 +22,7 @@
> > > > > > #include <mutex>
> > > > > > #include <thread>
> > > > > > #include <vector>
> > > > > > +#include <functional>
> > > > > > #include <X11/Xlib.h>
> > > > > > #include <X11/extensions/Xfixes.h>
> > > > > >
> > > > > > @@ -284,15 +285,17 @@ static void usage(const char *progname)
> > > > > > exit(1);
> > > > > > }
> > > > > >
> > > > > > -static void send_cursor(const XFixesCursorImage &image)
> > > > > > +static void
> > > > > > +send_cursor(unsigned width, unsigned height, int hotspot_x, int
> > > > > > hotspot_y,
> > > > > > + std::function<void(uint32_t *)> fill_cursor)
> > > > > > {
> > > > > > - if (image.width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> > > > > > - image.height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
> > > > > > + if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> > > > > > + height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
> > > > > > return;
> > > > > >
> > > > > > size_t cursor_size =
> > > > > > sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
> > > > > > - image.width * image.height * sizeof(uint32_t);
> > > > > > + width * height * sizeof(uint32_t);
> > > > > > std::unique_ptr<uint8_t[]> msg(new uint8_t[cursor_size]);
> > > > > >
> > > > > > StreamDevHeader
> > > > > > &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
> > > > > > @@ -305,14 +308,13 @@ static void send_cursor(const
> > > > > > XFixesCursorImage
> > > > > > &image)
> > > > > > memset(&cursor_msg, 0, sizeof(cursor_msg));
> > > > > >
> > > > > > cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
> > > > > > - cursor_msg.width = image.width;
> > > > > > - cursor_msg.height = image.height;
> > > > > > - cursor_msg.hot_spot_x = image.xhot;
> > > > > > - cursor_msg.hot_spot_y = image.yhot;
> > > > > > + cursor_msg.width = width;
> > > > > > + cursor_msg.height = height;
> > > > > > + cursor_msg.hot_spot_x = hotspot_x;
> > > > > > + cursor_msg.hot_spot_y = hotspot_y;
> > > > > >
> > > > > > uint32_t *pixels = reinterpret_cast<uint32_t
> > > > > > *>(cursor_msg.data);
> > > > > > - for (unsigned i = 0; i < image.width * image.height; ++i)
> > > > > > - pixels[i] = image.pixels[i];
> > > > > > + fill_cursor(pixels);
> > > > > >
> > > > > > std::lock_guard<std::mutex> stream_guard(stream_mtx);
> > > > > > write_all(streamfd, msg.get(), cursor_size);
> > > > > > @@ -336,7 +338,11 @@ static void cursor_changes(Display *display,
> > > > > > int
> > > > > > event_base)
> > > > > > continue;
> > > > > >
> > > > > > last_serial = cursor->cursor_serial;
> > > > > > - send_cursor(*cursor);
> > > > > > + auto fill_cursor = [&](uint32_t *pixels) {
> > > > > > + for (unsigned i = 0; i < cursor->width *
> > > > > > cursor->height;
> > > > > > ++i)
> > > > > > + pixels[i] = cursor->pixels[i];
> > > > > > + };
> > > > >
> > > > > Can't you just pass cursor->pixels to send_cursor() rather than
> > > > > building
> > > > > a lambda and passing that?
> > > > >
> > > > > Christophe
> > > > >
> > > >
> > > > Because the format is X11 format so it won't remove the dependency.
> > >
> > > I only see uint32_t being copied around, not sure what you mean by "X11
> > > format"
> > >
> > > Christophe
> > >
> >
> > Actually believe or not cursor->pixels is long* while destination is a
> > uint32_t*,
> > so on 64 bit are different.
>
> Hmm, to be honest, this feels to me more like a quick shortcut than a proper
> separation. Have you considered introducing a proper CursorImage class
> with a uint8_t *marshall() vfunc, and a CursorImageX11 child class with
> a CursorImageX11(const XFixesCursorImage&) constructor?
> Not that easy to guess what you have in mind without seeing "the future"
> that you alude to in the log ;)
>
> Christophe
>
My idea is that the class that represent the device (there are already some
proposal from me and Christophe de Dinechin) should be more platform independent
so X11 is surely not a dependency I want (should support both Wayland and
Windows). Passing a function to fill the array also avoids having an extra copy
as would fill the final buffer, marshalling to a different buffer could require
an extra copy. It's true I could have a void marshall(uint32_t *output_buffer)
instead.
On the other way it seems it's the third level stop over we are adding to
a bug fix that was not even considered.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel