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

[Lukáš Hrázký]

On Tue, 2018-07-10 at 06:09 -0400, Frediano Ziglio wrote:
> > 
> > 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.

No, I meant my rant below, not that you pointed out the style issue,
nothing wrong about that...

> > 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;
>     }

Well the style rule always results in this issue for this case, which
makes me really wonder why someone came up with it...

But I don't want to add parentheses just to fix our style issue.

To be clear, the reason I included the rant in my response was to
simply express my dislike for the rule. If more people feel the same
way, perhaps one day we will change the rule.

If nobody expresses their dislike, perhaps to avoid derailing style
discussions on their patches, then nothing will ever change.

For this patch, lets keep it to the rules and cut the noise :)

> > > >          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.

Not sure how you mean it, but any way I imagine it, I don't see it
helping the situation in any way.

Do you mean a generic send_cursor() to send the header and
send_cursor(XFixesCursorImage*) to send the data?

Because otherwise you still have the problem of how to get the data to
the final send_cursor() function.

Anyway, I don't think we really need to worry about this, the code can
be adapted whenever we have a different cursor implementation, no big
issue here.

> > > 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