Re: [PATCH 12/17] Convert message writing from C style to C++ style

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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.
> 
> 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;
> +    }
> +
>      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>
> +struct Message
> +{
> +    template <typename ...PayloadArgs>
> +    Message(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...))
> +    { }
> +
> +    StreamDevHeader hdr;
> +    Payload         msg;

style, no column indentation.

> +};
> +
> +
> +struct FormatMessage : Message<StreamMsgFormat, FormatMessage>
> +{
> +    FormatMessage(unsigned w, unsigned h, uint8_t c) : Message(w, h, c) {}
> +    static const StreamMsgType type = STREAM_TYPE_FORMAT;
> +    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));
> +    }
> +};
> +
> +
> +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);
> +    }
> +};
> +
> +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);

I think this is wrong as contains also the size of "pixels"
smart pointer.
Maybe you mean sizeof(Message<StreamMsgCursorSet, X11CursorMessage>) ?

> +    }
> +    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);
> +    }
> +    void fill_pixels(XFixesCursorImage *cursor)
> +    {
> +        uint32_t *pixbuf = pixels.get();
> +        unsigned count = cursor->width * cursor->height;

you can reuse pixel_size here.

> +        for (unsigned i = 0; i < count; ++i)
> +            pixbuf[i] = cursor->pixels[i];

I know this is copied, would be best to fix and add brackets.

> +    }
> +private:
> +    std::unique_ptr<uint32_t[]> pixels;
> +};
> +
>  }} // 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");

brackets.

>      }
>  }
>  
> @@ -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;
>              }

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]