[Spice-devel] [PATCH spice-streaming-agent] Remove X11 dependency from send_cursor

[Frediano Ziglio]

> 
> I know that this was acked by Christophe, but I wanted to comment,
> because that makes me unhappy.
> 
> 
> Frediano Ziglio writes:
> 
> > Allows a better encapsulation in the future
> >
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  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)
> 
> I find it really strange and error prone to pass the buffer one way
> (as a lambda argument) and its size (implicitly) another way (as a
> computation based on function arguments, i.e. width * height).
> I would have at the very least added a 'pixels' name and a buffer size
> in the lambda interface.
> 
> That suggests something is wrong about the "encapsulation". And indeed,
> if you have to copy pixels to convert from 'long' to 'uint32_t' and use
> that as a justification, it makes me *very* nervous. That alone would
> have been enough for me to nack this way of doing this.
> 
> But what annoys me really is that you are aware that I started a patch
> series where I began working on encapsulation, and trying to do it the
> C++ way, not by happily paper-patching advanced C++ over low-level
> C-style byte-level memory copies.
> 
> One of the comments that came out of the discussion of my patch series
> was precisely X11 encapsulation. So it would be reasonable to at least
> envision that addressing that comment could be part of my effort for the
> next iteration of the series, wouldn't it?
> 

Yes, the idea was to prepare for a better encapsulation having less
dependency.
Yes, I did not mention this in the comment.

> Based on the structure of my patch series, there is already a call
> that looks like
> 
>      stream->send_cursor(*cursor);
> 
> From that, you could correctly infer that this function could be
> an X11-specific feature, in an X11-specific stream class. That provides
> all the encapsulation you need, in a proper C++ way, using inheritance
> to deal with commonalities rather than using std::function and lambdas
> simply to pass a glorified memcpy.
> 

This make the class have dependency on the device itself and the
graphic system while is just called SpiceStream. Or would you prefer
to have part of the class implementation system dependent?

> But now that this patch is acked, I guess that I will have to rebase
> my RAII branch on what I frankly see as not even half of a quarter of some
> brain-damaged idea of encapsulation. For what benefit? What urgent
> problem did this patch solve that was not better addressed by working on
> real encapsulation?
> 

I already rebased your branch on https://gitlab.cee.redhat.com/fziglio/spice-streaming-agent-new/tree/c3d
if you want to avoid to rebase. There are other patches on the top, was trying
to change the encapsulation. The branch is surely not in production state.
Yes, the repository is RH internal, I'll have to move externally.
You stated in a previous comment that a different encapsulation was not on
top of your priority so I decided to propose some changes.

> >  {
> > -    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];
> > +        };
> > +        send_cursor(cursor->width, cursor->height, cursor->xhot,
> > cursor->yhot, fill_cursor);
> >      }
> >  }
> 

Frediano