> On 21 Feb 2018, at 10:45, Lukáš Hrázký <lhra...@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ý <lhra...@redhat.com> wrote:
>>>
>>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>>>> From: Christophe de Dinechin <dinec...@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 <dinec...@redhat.com>
>>>> ---
>>>> 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 ;-)
> 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?
>
>>>
>>>> + : 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.
(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@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
