[Spice-devel] [PATCH spice-streaming-agent 7/8 v3] Send the GraphicsDeviceInfo to the server

[Lukáš Hrázký]

On Thu, 2019-01-17 at 14:29 -0600, Jonathon Jongsma wrote:
> On Wed, 2019-01-16 at 13:52 +0100, Lukáš Hrázký wrote:
> > Adds serialization of the GraphicsDeviceInfo message and sends it to
> > the
> > server when it starts to stream.
> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > ---
> >  src/spice-streaming-agent.cpp | 54 +++++++++++++++++++++++++++++--
> > ----
> >  1 file changed, 45 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
> > agent.cpp
> > index 3024d98..891e4cb 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -31,6 +31,7 @@
> >  #include <poll.h>
> >  #include <syslog.h>
> >  #include <signal.h>
> > +#include <algorithm>
> >  #include <exception>
> >  #include <stdexcept>
> >  #include <memory>
> > @@ -93,6 +94,33 @@ public:
> >      }
> >  };
> >  
> > +class DeviceDisplayInfoMessage : public
> > OutboundMessage<StreamMsgDeviceDisplayInfo, DeviceDisplayInfoMessage,
> > STREAM_TYPE_DEVICE_DISPLAY_INFO>
> > +{
> > +public:
> > +    DeviceDisplayInfoMessage(const DeviceDisplayInfo &info) :
> > OutboundMessage(info) {}
> > +
> > +    static size_t size(const DeviceDisplayInfo &info)
> > +    {
> > +        return sizeof(PayloadType) +
> > std::min(info.device_address.length(), 255lu) + 1;
> 
> Where does this 255 come from? Can we at least use a constant or
> something?

Oh, yes. I should have put that into a constant.

The same limit is also in the server and the vd_agent, Frediano
suggested to put it into spice-protocol. I'm not entirely sure, as it
looks a bit ad-hoc and I didn't find a good place in spice-protocol to
put it in, so I didn't do it. Practically it will also probably never
have any impact...

> > +    }
> > +
> > +    void write_message_body(StreamPort &stream_port, const
> > DeviceDisplayInfo &info)
> > +    {
> > +        std::string device_address = info.device_address;
> > +        if (device_address.length() > 255) {
> > +            syslog(LOG_WARNING,
> > +                   "device address of stream id %u is longer than
> > 255 bytes, trimming.", info.stream_id);
> > +            device_address = device_address.substr(0, 255);
> > +        }
> > +        StreamMsgDeviceDisplayInfo strm_msg_info{};
> > +        strm_msg_info.stream_id = info.stream_id;
> > +        strm_msg_info.device_display_id = info.device_display_id;
> > +        strm_msg_info.device_address_len = device_address.length() +
> > 1;
> > +        stream_port.write(&strm_msg_info, sizeof(strm_msg_info));
> > +        stream_port.write(device_address.c_str(),
> > device_address.length() + 1);
> > +    }
> > +};
> > +
> >  static bool streaming_requested = false;
> >  static bool quit_requested = false;
> >  static std::set<SpiceVideoCodecType> client_codecs;
> > @@ -217,17 +245,25 @@ do_capture(StreamPort &stream_port, FrameLog
> > &frame_log)
> >              throw std::runtime_error("cannot find a suitable capture
> > system");
> >          }
> >  
> > +        std::vector<DeviceDisplayInfo> display_info;
> >          try {
> > -            std::vector<DeviceDisplayInfo> display_info = capture-
> > > get_device_display_info();
> > 
> > -            syslog(LOG_DEBUG, "Got device info of %lu devices from
> > the plugin", display_info.size());
> > -            for (const auto &info : display_info) {
> > -                syslog(LOG_DEBUG, "   id %u: device address %s,
> > device display id: %u",
> > -                       info.id,
> > -                       info.device_address.c_str(),
> > -                       info.device_display_id);
> > -            }
> > +            display_info = capture->get_device_display_info();
> >          } catch (const Error &e) {
> > -            syslog(LOG_ERR, "Error while getting device info: %s",
> > e.what());
> > +            syslog(LOG_ERR, "Error while getting device display
> > info: %s", e.what());
> > +        }
> > +
> > +        syslog(LOG_DEBUG, "Got device info of %zu devices from the
> > plugin", display_info.size());
> > +        for (const auto &info : display_info) {
> > +            syslog(LOG_DEBUG, "   stream id %u: device address: %s,
> > device display id: %u",
> > +                   info.stream_id,
> > +                   info.device_address.c_str(),
> > +                   info.device_display_id);
> > +        }
> > +
> > +        if (display_info.size() > 0) {
> > +            stream_port.send<DeviceDisplayInfoMessage>(display_info[
> > 0]);
> 
> Shouldn't we have some handling here in case size() is greater than 1?
> at least print a warning or something?

You mean if there are more than one display/monitor in the info vector
to print a warning we have more than one display and only stream the
first one? I'm not sure why, but I can add it... 

> > +        } else {
> > +            syslog(LOG_ERR, "Empty device display info from the
> > plugin");
> >          }
> >  
> >          while (!quit_requested && streaming_requested) {
> 
> 
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> 
>