On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>
> - The Stream class now deals with locking and sending messages
> - The Message<> template class deals with the general writing mechanisms
> - Three classes, FormatMessage, FrameMessage and X11CursorMessage represent individual messages
>
> The various classes should be moved to separate headers in a subsequent operation
>
> The design uses templates to avoid any runtime overhead. All the calls are static.
All in all, a nice way to encapsulate the sending of messages. What I
worked on, actually, was the receiving of messages, as that is actually
more important for separating the code (as seen later in the problem
you had with client_codecs and streaming_requested static variables).
I am now wondering how to merge it with your changes and whether the
same Message class hierarchy could be used (without making a mess out
of it). We should discuss it later.
> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> ---
> src/spice-streaming-agent.cpp | 250 ++++++++++++++++++++----------------------
> 1 file changed, 117 insertions(+), 133 deletions(-)
>
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index a989ee7..c174ea4 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -48,24 +48,6 @@ namespace spice
> namespace streaming_agent
> {
>
> -struct FormatMessage
> -{
> - StreamDevHeader hdr;
> - StreamMsgFormat msg;
> -};
> -
> -struct DataMessage
> -{
> - StreamDevHeader hdr;
> - StreamMsgData msg;
> -};
> -
> -struct CursorMessage
> -{
> - StreamDevHeader hdr;
> - StreamMsgCursorSet msg;
> -};
> -
> class Stream
> {
> public:
> @@ -79,24 +61,131 @@ public:
> {
> close(streamfd);
> }
> - operator int() { return streamfd; }
>
> int have_something_to_read(int timeout);
> int read_command_from_device(void);
> int read_command(bool blocking);
>
> +
> + template <typename Message, typename ...PayloadArgs>
> + bool send(PayloadArgs... payload)
> + {
> + Message message(payload...);
> + std::lock_guard<std::mutex> stream_guard(mutex);
> + size_t expected = message.size(payload...);
> + size_t written = message.write(*this, payload...);
> + return written == expected;
> + }
Do you purposefully avoid throwing exceptions here, returning a bool?
The error and exception could actually be checked as low as at the end
of the write_all() method, avoiding all the latter size returning and
checking?
> +
> size_t write_all(const void *buf, const size_t len);
> - int send_format(unsigned w, unsigned h, uint8_t c);
> - int send_frame(const void *buf, const unsigned size);
> - void send_cursor(uint16_t width, uint16_t height,
> - uint16_t hotspot_x, uint16_t hotspot_y,
> - std::function<void(uint32_t *)> fill_cursor);
>
> private:
> int streamfd = -1;
> std::mutex mutex;
> };
>
> +
> +template <typename Payload, typename Info>
"Info" is quite an unimaginative name :) I understand it's the message
itself here and also find it curiously hard to come up with something
better :) "TheMessage" not very good, is it? Still better than "Info"?
Maybe a comment referencing the CRTP to help readers understand?
> +struct Message
Any reason to not stick with the "class" keyword instead of "struct"?
> +{
> + template <typename ...PayloadArgs>
> + Message(PayloadArgs... payload)
"PayloadArgs... payload_args", we have something called "Payload" here
too, getting a bit confusing.
> + : hdr(StreamDevHeader {
> + .protocol_version = STREAM_DEVICE_PROTOCOL,
> + .padding = 0, // Workaround GCC bug "sorry: not implemented"
> + .type = Info::type,
> + .size = (uint32_t) (Info::size(payload...) - sizeof(hdr))
> + }),
> + msg(Info::make(payload...))
> + { }
I find it redundant that you pass the "payload..." (the args :)) to
Info::size() and Info::make() here and then also to Info::write() in
Stream::send(). In the three messages below, you also showcase three
distinct ways of serializing them, caused by this redundancy (I
understand it is partially given by the payload of the messages).
See comments under each class, which all relate to this (hope it's not
too confusing).
> +
> + StreamDevHeader hdr;
> + Payload msg;
> +};
> +
> +
> +struct FormatMessage : Message<StreamMsgFormat, FormatMessage>
> +{
> + FormatMessage(unsigned w, unsigned h, uint8_t c) : Message(w, h, c) {}
> + static const StreamMsgType type = STREAM_TYPE_FORMAT;
Could the type actually be part of the template? As in:
struct FormatMessage : Message<STREAM_TYPE_FORMAT, StreamMsgFormat, FormatMessage>
> + static size_t size(unsigned width, unsigned height, uint8_t codec)
> + {
> + return sizeof(FormatMessage);
> + }
> + static StreamMsgFormat make(unsigned w, unsigned h, uint8_t c)
> + {
> + return StreamMsgFormat{ .width = w, .height = h, .codec = c, .padding1 = {} };
> + }
> + size_t write(Stream &stream, unsigned w, unsigned h, uint8_t c)
> + {
> + return stream.write_all(this, sizeof(*this));
> + }
> +};
This message has static size, so you construct it in make() and then
write this whole object.
> +
> +
> +struct FrameMessage : Message<StreamMsgData, FrameMessage>
> +{
> + FrameMessage(const void *frame, size_t length) : Message(frame, length) {}
> + static const StreamMsgType type = STREAM_TYPE_DATA;
> + static size_t size(const void *frame, size_t length)
> + {
> + return sizeof(FrameMessage) + length;
> + }
> + static StreamMsgData make(const void *frame, size_t length)
> + {
> + return StreamMsgData();
> + }
> + size_t write(Stream &stream, const void *frame, size_t length)
> + {
> + return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(frame, length);
> + }
> +};
Here the message is actually only the frame data and so you construct
an empty message in make(). In write() you just write the stream data
passed to it.
> +
> +struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
> +{
> + X11CursorMessage(XFixesCursorImage *cursor)
> + : Message(cursor),
> + pixels(new uint32_t[pixel_size(cursor)])
> + {
> + fill_pixels(cursor);
> + }
> + static const StreamMsgType type = STREAM_TYPE_CURSOR_SET;
> + static size_t pixel_size(XFixesCursorImage *cursor)
> + {
> + return cursor->width * cursor->height;
> + }
> + static size_t size(XFixesCursorImage *cursor)
> + {
> + return sizeof(X11CursorMessage) + sizeof(uint32_t) * pixel_size(cursor);
> + }
> + static StreamMsgCursorSet make(XFixesCursorImage *cursor)
> + {
> + return StreamMsgCursorSet
> + {
> + .width = cursor->width,
> + .height = cursor->height,
> + .hot_spot_x = cursor->xhot,
> + .hot_spot_y = cursor->yhot,
> + .type = SPICE_CURSOR_TYPE_ALPHA,
> + .padding1 = { },
> + .data = { }
> + };
> + }
> + size_t write(Stream &stream, XFixesCursorImage *cursor)
> + {
> + return stream.write_all(&hdr, sizeof(hdr)) + stream.write_all(pixels.get(), hdr.size);
You are not writing the msg data here, might be the reson for the
missing cursor.
> + }
> + void fill_pixels(XFixesCursorImage *cursor)
> + {
> + uint32_t *pixbuf = pixels.get();
> + unsigned count = cursor->width * cursor->height;
> + for (unsigned i = 0; i < count; ++i)
> + pixbuf[i] = cursor->pixels[i];
> + }
> +private:
> + std::unique_ptr<uint32_t[]> pixels;
> +};
(note in this class some methods could be private and some arguments
const)
Here you add a private member pixels, which you fill in constructor. In
make(), you create the static part of the message. In write(), you
write them.
So, I ask, could you not actually construct all the data to write in
write(), and perhaps even remove the Message::msg member and the make()
method?
That would make the classes a bit simpler?
Cheers,
Lukas
> +
> }} // namespace spice::streaming_agent
>
>
> @@ -201,66 +290,6 @@ size_t Stream::write_all(const void *buf, const size_t len)
> return written;
> }
>
> -int Stream::send_format(unsigned w, unsigned h, uint8_t c)
> -{
> - const size_t msgsize = sizeof(FormatMessage);
> - const size_t hdrsize = sizeof(StreamDevHeader);
> - FormatMessage msg = {
> - .hdr = {
> - .protocol_version = STREAM_DEVICE_PROTOCOL,
> - .padding = 0, // Workaround GCC "not implemented" bug
> - .type = STREAM_TYPE_FORMAT,
> - .size = msgsize - hdrsize
> - },
> - .msg = {
> - .width = w,
> - .height = h,
> - .codec = c,
> - .padding1 = { }
> - }
> - };
> - syslog(LOG_DEBUG, "writing format\n");
> - std::lock_guard<std::mutex> stream_guard(mutex);
> - if (write_all(&msg, msgsize) != msgsize) {
> - return EXIT_FAILURE;
> - }
> - return EXIT_SUCCESS;
> -}
> -
> -int Stream::send_frame(const void *buf, const unsigned size)
> -{
> - ssize_t n;
> - const size_t msgsize = sizeof(FormatMessage);
> - DataMessage msg = {
> - .hdr = {
> - .protocol_version = STREAM_DEVICE_PROTOCOL,
> - .padding = 0, // Workaround GCC "not implemented" bug
> - .type = STREAM_TYPE_DATA,
> - .size = size /* includes only the body? */
> - },
> - .msg = {}
> - };
> -
> - std::lock_guard<std::mutex> stream_guard(mutex);
> - n = write_all(&msg, msgsize);
> - syslog(LOG_DEBUG,
> - "wrote %ld bytes of header of data msg with frame of size %u bytes\n",
> - n, msg.hdr.size);
> - if (n != msgsize) {
> - syslog(LOG_WARNING, "write_all header: wrote %ld expected %lu\n",
> - n, msgsize);
> - return EXIT_FAILURE;
> - }
> - n = write_all(buf, size);
> - syslog(LOG_DEBUG, "wrote data msg body of size %ld\n", n);
> - if (n != size) {
> - syslog(LOG_WARNING, "write_all header: wrote %ld expected %u\n",
> - n, size);
> - return EXIT_FAILURE;
> - }
> - return EXIT_SUCCESS;
> -}
> -
> /* returns current time in micro-seconds */
> static uint64_t get_time(void)
> {
> @@ -304,47 +333,6 @@ static void usage(const char *progname)
> exit(1);
> }
>
> -void
> -Stream::send_cursor(uint16_t width, uint16_t height,
> - uint16_t hotspot_x, uint16_t hotspot_y,
> - std::function<void(uint32_t *)> fill_cursor)
> -{
> - if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> - height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
> - return;
> -
> - const uint32_t cursor_msgsize =
> - sizeof(CursorMessage) + width * height * sizeof(uint32_t);
> - const uint32_t hdrsize = sizeof(StreamDevHeader);
> -
> - std::unique_ptr<uint8_t[]> storage(new uint8_t[cursor_msgsize]);
> -
> - CursorMessage *cursor_msg =
> - new(storage.get()) CursorMessage {
> - .hdr = {
> - .protocol_version = STREAM_DEVICE_PROTOCOL,
> - .padding = 0, // Workaround GCC internal / not implemented compiler error
> - .type = STREAM_TYPE_CURSOR_SET,
> - .size = cursor_msgsize - hdrsize
> - },
> - .msg = {
> - .width = width,
> - .height = height,
> - .hot_spot_x = hotspot_x,
> - .hot_spot_y = hotspot_y,
> - .type = SPICE_CURSOR_TYPE_ALPHA,
> - .padding1 = { },
> - .data = { }
> - }
> - };
> -
> - uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg->msg.data);
> - fill_cursor(pixels);
> -
> - std::lock_guard<std::mutex> stream_guard(mutex);
> - write_all(storage.get(), cursor_msgsize);
> -}
> -
> static void cursor_changes(Stream *stream, Display *display, int event_base)
> {
> unsigned long last_serial = 0;
> @@ -363,12 +351,8 @@ static void cursor_changes(Stream *stream, Display *display, int event_base)
> continue;
>
> 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];
> - };
> - stream->send_cursor(cursor->width, cursor->height,
> - cursor->xhot, cursor->yhot, fill_cursor);
> + if (!stream->send<X11CursorMessage>(cursor))
> + syslog(LOG_WARNING, "FAILED to send cursor\n");
> }
> }
>
> @@ -422,7 +406,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
>
> syslog(LOG_DEBUG, "wXh %uX%u codec=%u\n", width, height, codec);
>
> - if (stream.send_format(width, height, codec) == EXIT_FAILURE)
> + if (!stream.send<FormatMessage>(width, height, codec))
> throw std::runtime_error("FAILED to send format message");
> }
> if (f_log) {
> @@ -434,7 +418,7 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
> hexdump(frame.buffer, frame.buffer_size, f_log);
> }
> }
> - if (stream.send_frame(frame.buffer, frame.buffer_size) == EXIT_FAILURE) {
> + if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size)) {
> syslog(LOG_ERR, "FAILED to send a frame\n");
> break;
> }
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel