> On 23 Feb 2018, at 17:37, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:
>
> On Fri, 2018-02-23 at 12:36 +0100, Christophe de Dinechin wrote:
>>> On 22 Feb 2018, at 19:05, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
>>>
>>> On Thu, 2018-02-22 at 18:51 +0100, Christophe de Dinechin wrote:
>>>>> On 21 Feb 2018, at 10:45, Lukáš Hrázký <lhrazky@xxxxxxxxxx>
>>>>> wrote:
>>>>>
>>>>> On Tue, 2018-02-20 at 16:41 +0100, Christophe de Dinechin
>>>>> wrote:
>>>>>>> On 20 Feb 2018, at 15:29, Lukáš Hrázký <lhrazky@xxxxxxxxxx>
>>>>>>> wrote:
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> Do you have your WIP stuff in a private branch I could look
>>>>>> at? Maybe I can help with the rebasing.
>>>>>
>>>>> I'll push it somewhere so you can have a look, but I can manage
>>>>> the
>>>>> rebase myself :) It would still be an effort to find out what
>>>>> you did
>>>>> during the rebase.
>>>>>
>>>>>> I would probably keep input and output messages in separate
>>>>>> classes. I can’t see much commonality between the two. Using
>>>>>> the same CRTP for input messages, and maybe rename Message as
>>>>>> OutputMessage…
>>>>>
>>>>> Agreed, probably...
>>>>>
>>>>>>>
>>>>>>>> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxx
>>>>>>>> m>
>>>>>>>> ---
>>>>>>>> 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?
>>>>>>
>>>>>> You really love exceptions, don’t you ;-)
>>>>>
>>>>> Well... I don't always get errors, but when I do, I use
>>>>> exceptions to
>>>>> handle them. :D
>>>>>
>>>>> Really, that's what it is. Error = exception.
>>>>
>>>> No. C++CG E2 "Throw an exception to signal that a function can't
>>>> perform its assigned task”.
>>>>
>>>> Examples of errors that are not exceptions: FP NaN and
>>>> infinities, compiler errors (and more generally parsing errors),
>>>> etc. A parser error is not an exception because it’s the job of
>>>> the parser to detect the error…
>>>>
>>>> I could also give examples of exceptions that are not errors,
>>>> though it’s more subtle and OT. Consider a C++ equivalent of an
>>>> interrupted system call. Also, is bad_alloc an “error" or a
>>>> limitation? (one could argue that the error would be e.g. to give
>>>> you some memory belonging to someone else ;-)
>>>
>>> If you put it this way, sure.
>>>
>>>>> That's what they teach
>>>>> you at the uni and what we've always done at my previous job.
>>>>> It's the
>>>>> language's designated mechanism to deal with errors, standard
>>>>> library
>>>>> uses them, ...
>>>>>
>>>>> You brought new light into the topic for me, but I still don't
>>>>> know how
>>>>> to deal with it... Though the fact that you are the author of
>>>>> the
>>>>> exception handling implementation and you are reluctant to use
>>>>> the
>>>>> exception really speaks volumes :)
>>>>>
>>>>>> I usually reserve exceptions for cases which are truly
>>>>>> “catastrophic”, i.e. going the long way and unwindig is the
>>>>>> right way to do it. Unwinding the stack is a very messy
>>>>>> business, takes a lot of time, and if you can just return,
>>>>>> it’s about one and a half gazillion times faster, generates
>>>>>> smaller code, etc etc.
>>>>>>
>>>>>> In this specific case, though, I think that unwinding could
>>>>>> be seen as appropriate, since ATM we have nothing better than
>>>>>> error + abort when it happens. Plus this might avoid a
>>>>>> scenario where you could write twice if the first write
>>>>>> fails. So I’m sold, I think this is the right thing to do.
>>>>>>
>>>>>>> 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?
>>>>>>
>>>>>> I used ‘Info’ here because that’s the information about the
>>>>>> message.
>>>>>
>>>>> I kind of understood that, but still... very vague for me…
>>>>
>>>> Will stay that way for the moment, unless you give me a really
>>>> better name, sorry.
>>>>
>>>>>
>>>>>> I can add the CRTP comment, could not remember the acronym
>>>>>> when I wrote the class ;-)
>>>>>>
>>>>>>>
>>>>>>>> +struct Message
>>>>>>>
>>>>>>> Any reason to not stick with the "class" keyword instead of
>>>>>>> "struct”?
>>>>>>
>>>>>> Not really, more a matter of habit, using struct when it’s
>>>>>> really about data and everything is public. But there,
>>>>>> “Message” evolved far away from POD to warrant using ‘class’.
>>>>>
>>>>> I do like to take the shortcut using struct to spare myself the
>>>>> 'public:' line at the top, but I'd think it would be considered
>>>>> an inconsistency by others... :)
>>>>
>>>> That’s not the reason. I tend to use it to mark a struct as
>>>> “mostly data”, even if it has C++isms in it. Here, I initially
>>>> saw the message is mostly data, i.e. I would not have minded the
>>>> data members being public. But that’s no longer the case. Will
>>>> change it.
>>>>
>>>>>
>>>>>>>
>>>>>>>> +{
>>>>>>>> + template <typename ...PayloadArgs>
>>>>>>>> + Message(PayloadArgs... payload)
>>>>>>>
>>>>>>> "PayloadArgs... payload_args", we have something called
>>>>>>> "Payload" here
>>>>>>> too, getting a bit confusing.
>>>>>>
>>>>>> But that’s the same thing. Payload is initialized with
>>>>>> PayloadArgs, so it makes sense.
>>>>>>
>>>>>> Initially, I had “PayloadConstructorArgs”, but then I started
>>>>>> using it outside of the ctor.
>>>>>
>>>>> I don't think it's the same, here Payload is the message class
>>>>> and
>>>>> PayloadArgs is the content/args. So naming the objects
>>>>> accordingly IMO
>>>>> helps clarity.
>>>>
>>>> The Payload is defined from the PayloadArgs, i.e. its constructor
>>>> is Payload(PayloadArgs…). What is the problem with that?
>>>
>>> The problem is Payload(PayloadArgs... payload).
>>
>> What is the problem with initializing something called a payload with
>> something called payload args?
>>
>> I’m sorry for being dense here, but I really don’t get your
>> objection.
>
>
> I think the problem is that you have a typename Payload and a typename
> PayloadArgs. But you call the concrete variable of type PayloadArgs
> "payload", and you call the variable of type Payload "msg". Here's a
> shortened version of your code:
>
> template <typename Payload, typename Info>
> struct Message
> {
> template <typename ...PayloadArgs>
> Message(PayloadArgs... payload)
> ...
>
> Payload msg;
> };
>
>
> If it was instead changed to something like this, it would be less
> confusing:
>
> template <typename Payload, typename Info>
> struct Message
> {
> template <typename ...PayloadArgs>
> Message(PayloadArgs... payload_args) // or just "args"?
> ...
>
> Payload msg; // or "payload”?
The old code had ‘msg’, but renaming to payload sounds like a good idea based on your explanations.
> };
>
>
> Does that make sense? I found it a bit confusing when reading your code
> as well, though I'm not sure that my confusion is the same as Lukáš’s
Makes perfect sense. And easy to fix.
>
>
>>
>>>
>>>>>
>>>>>>>
>>>>>>>> + : 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().
>>>>>>
>>>>>> I’m not sure “redundant” is the correct word. I could stash
>>>>>> the information in Message, but then the compiler would have
>>>>>> a much harder time inlining the actual expression, which is
>>>>>> always very simple. Unfortunately, we have three different
>>>>>> expressions that take different input, hence the CRTP and the
>>>>>> passing of arguments.
>>>>>>
>>>>>>
>>>>>>> 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).
>>>>>>
>>>>>> It is *entirely* given by the payload, which I assume is a
>>>>>> given, since it’s in the protocol. What else could come into
>>>>>> play?
>>>>>
>>>>> What I mean is that the place you create the data for
>>>>> serialization is
>>>>> only partially given by the payload. Because for example with
>>>>> the
>>>>> FormatMessage, you place the data in the class as a member, but
>>>>> you
>>>>> could have also created them on the stack at the beginning of
>>>>> the
>>>>> write() method. And unless I'm missing something, you could do
>>>>> it this
>>>>> way for all the cases, therefore unify the approach. For the
>>>>> FrameMessage, you are constructing an empty msg member…
>>>>
>>>> I tell you that I don’t want to copy, say, 70K of frame data if I
>>>> can avoid it, and you are suggesting I allocate 70K of stack
>>>> instead? I think you did miss something, yes.
>>>
>>> No, I am not telling you that.
>>
>> Then, I have not understood what you mean. What do you mean with
>> “created them on the stack at the beginning of the write function”?
>> What is “them”? The format message?
>>
>> Is your suggestion that FormatMessage could be split in two writes
>> like the others, and that this would make the code more generic (if a
>> tiny bit less efficient for that specific message)? If that’s what
>> you are saying, then the problem is that the second part is distinct
>> in all three cases anyway, so I’m not getting anything out of this,
>> but I’m making the FormatMessage case less efficient…
>>
>> Sorry, I read and re-read your explanations, I’m afraid I still don’t
>> get it. Maybe with a code outline?
>>
>>
>>>
>>>> (if that clarifies, that frame is given to me by the capture, I
>>>> did not ask the capture to use some buffer I pre-allocated)
>>>>
>>>>>
>>>>>>>
>>>>>>> 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>
>>>>>>
>>>>>> Sure, but I find it more consistent the way I wrote it. Also
>>>>>> easier to read, because you have “type = TYPE” instead of
>>>>>> just <TYPE>, but that’s really a personal preference.
>>>>>>
>>>>>>>
>>>>>>>> + 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.
>>>>>>
>>>>>> The message body has static size, yes. (Header is fixed size
>>>>>> in all three cases).
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +
>>>>>>>> +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.
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +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.
>>>>>>
>>>>>> Ah, good catch. I thought of not writing *this because of the
>>>>>> extra pixels field, but then wrote the wrong class.
>>>>>>
>>>>>>>
>>>>>>>> + }
>>>>>>>> + 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)
>>>>>>
>>>>>> Yes. Good idea.
>>>>>>
>>>>>>>
>>>>>>> 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?
>>>>>>
>>>>>> Yes, but that would require to copy the frames data, which I
>>>>>> considered an expensive-enough operation to try and avoid it.
>>>>>> For cursor pixels, it’s less of an issue a) because we need
>>>>>> to copy anyway, due to format conversion, and they happen
>>>>>> much less frequently.
>>>>>
>>>>> I don't think you would need to copy the data? What I propose
>>>>> actually
>>>>> doesn't mean any change for this method in particular?
>>>>>
>>>>>>>
>>>>>>> That would make the classes a bit simpler?
>>>>>>
>>>>>> Simpler, yes but much less efficient in the important case,
>>>>>> which is sending frames, where it would add one malloc / copy
>>>>>> / free for each frame, which the present code avoids at the
>>>>>> “mere” cost of passing the payload args around ;-)
>>>>>>
>>>>>>
>>>>>> Thanks a lot.
>>>>>>
>>>>>>>
>>>>>>> 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
>>>>>>
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Spice-devel mailing list
>>>>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel