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@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)
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?
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.
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?
> {
> - 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);
> }
> }
--
Cheers,
Christophe de Dinechin (IRC c3d)
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel