[Spice-devel] [PATCH 12/17] Convert message writing from C style to C++ style

Christophe de Dinechin cdupontd at redhat.com
Fri Feb 23 18:07:27 UTC 2018



> On 23 Feb 2018, at 17:37, Jonathon Jongsma <jjongsma at redhat.com> 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 at redhat.com> 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 at redhat.com>
>>>>> 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 at redhat.com>
>>>>>>> wrote:
>>>>>>> 
>>>>>>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin
>>>>>>> wrote:
>>>>>>>> From: Christophe de Dinechin <dinechin at redhat.com>
>>>>>>>> 
>>>>>>>> - 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 at redhat.co
>>>>>>>> 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 at lists.freedesktop.org
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>>>> 
>>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> Spice-devel mailing list
>>>>> Spice-devel at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>> 
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel



More information about the Spice-devel mailing list