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@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?
> > >
> > > 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).
> >
> > > >
> > > > > + : 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.
> (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