[Spice-devel] [PATCH 12/17] Convert message writing from C style to C++ style
Lukáš Hrázký
lhrazky at redhat.com
Mon Feb 26 10:01:04 UTC 2018
On Fri, 2018-02-23 at 19:07 +0100, Christophe de Dinechin wrote:
> > 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.
That's what I meant as well. Apparently I need to work on my explaining
skills :)
> >
> >
> > >
> > > >
> > > > > >
> > > > > > > >
> > > > > > > > > + : 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?
Sending a patch in reply to this (coudln't think of a way to clearly
outline it in an email). It's hard to explain and I am not doing the
best job at that.
> > >
> > > >
> > > > > (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