[Spice-devel] [PATCH spice-streaming-agent v4 2/2] CursorUpdater: pass the cursor pointer directly to send_cursor

[Frediano Ziglio]

> 
> On Mon, 2018-07-09 at 05:21 -0400, Frediano Ziglio wrote:
> > > 
> > > Pass the pointer to X cursor struct directly instead of using a lambda
> > > to fill in the pixels.
> > > 
> > > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > > ---
> > >  src/cursor-updater.cpp | 28 ++++++++++++++--------------
> > >  1 file changed, 14 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/src/cursor-updater.cpp b/src/cursor-updater.cpp
> > > index 8f65e83..5901172 100644
> > > --- a/src/cursor-updater.cpp
> > > +++ b/src/cursor-updater.cpp
> > > @@ -23,16 +23,17 @@ namespace streaming_agent {
> > >  
> > >  namespace {
> > >  
> > > -void send_cursor(StreamPort &stream_port, unsigned width, unsigned
> > > height,
> > > int hotspot_x, int hotspot_y,
> > > -                 std::function<void(uint32_t *)> fill_cursor)
> > > +void send_cursor(StreamPort &stream_port, XFixesCursorImage *cursor)
> > >  {
> > > -    if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH || height >=
> > > STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
> > > +    if (cursor->width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> > > +        cursor->height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
> > > +    {
> > 
> > style
> 
> Ok, so you left me guessing what the issue is, I suppose you want me to
> do this?
> 
>     if (cursor->width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
>         cursor->height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
>         return;
>     }
> 
> Sure... but I'll not spare you the rant :)
> 

Sorry, was not meant to be a rant, just though you just missed it.

> This is much worse on the eyes and makes little sense to me, especially
> along with the brace rules on functions and classes.
> 
> (I'm done :))
> 

Yes, the combination of our rules in this case does not make clear when
the if condition ends. Maybe parenthesis can help in this case, like

    if ((cursor->width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH) ||
        (cursor->height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)) {
        return;
    }

> > >          return;
> > >      }
> > >  
> > >      size_t cursor_size =
> > >          sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
> > > -        width * height * sizeof(uint32_t);
> > > +        cursor->width * cursor->height * sizeof(uint32_t);
> > >      std::unique_ptr<uint8_t[]> msg(new uint8_t[cursor_size]);
> > >  
> > >      StreamDevHeader
> > >      &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
> > > @@ -45,13 +46,15 @@ void send_cursor(StreamPort &stream_port, unsigned
> > > width,
> > > unsigned height, int h
> > >      memset(&cursor_msg, 0, sizeof(cursor_msg));
> > >  
> > >      cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
> > > -    cursor_msg.width = width;
> > > -    cursor_msg.height = height;
> > > -    cursor_msg.hot_spot_x = hotspot_x;
> > > -    cursor_msg.hot_spot_y = hotspot_y;
> > > +    cursor_msg.width = cursor->width;
> > > +    cursor_msg.height = cursor->height;
> > > +    cursor_msg.hot_spot_x = cursor->xhot;
> > > +    cursor_msg.hot_spot_y = cursor->yhot;
> > >  
> > >      uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg.data);
> > > -    fill_cursor(pixels);
> > > +    for (unsigned i = 0; i < cursor->width * cursor->height; ++i) {
> > > +        pixels[i] = cursor->pixels[i];
> > > +    }
> > >  
> > >      std::lock_guard<std::mutex> guard(stream_port.mutex);
> > >      stream_port.write(msg.get(), cursor_size);
> > > @@ -95,11 +98,8 @@ CursorUpdater::CursorUpdater(StreamPort *stream_port)
> > > :
> > > stream_port(stream_port)
> > >          }
> > >  
> > >          last_serial = cursor->cursor_serial;
> > > -        auto fill_cursor = [cursor](uint32_t *pixels) {
> > > -            for (unsigned i = 0; i < cursor->width * cursor->height;
> > > ++i)
> > > -                pixels[i] = cursor->pixels[i];
> > > -        };
> > > -        send_cursor(*stream_port, cursor->width, cursor->height,
> > > cursor->xhot, cursor->yhot, fill_cursor);
> > > +
> > > +        send_cursor(*stream_port, cursor);
> > >      }
> > >  }
> > > 
> > 
> > Why bounding send_cursor to a specific (X11) implementation?
> 
> We have no other implementation and the function is local, so it can be
> made generic when and if needed, no real reason to make the code more
> complex now.
> 
> If there was an elegant way to make the send_cursor method independent,
> I'd use it, but I don't see one. I don't like the lambda much and I
> remember c3d had an issue with it too, though I forgot what it was
> exactly. This is a preparation patch for encapsulating the sending of
> the messages which is mostly based on his work.
> 
> > Would not be better to add another overloaded send_cursor instead?
> 
> Not sure what you mean by overloaded send_cursor?
> 

Another send_cursor function that accepts a XFixesCursorImage and
call the other send_cursor.

> > I don't thing cursor can be nullptr, in this case I would use a reference
> > instead, better constant, I don't see why we should change it.
> 
> Sure, I just used a pointer because that's what I had, and yeah, it
> should be const.
> 

Frediano