[Spice-devel] [PATCH spice-protocol 2/8] Add the StreamMsgGraphicsDeviceInfo message

[Lukáš Hrázký]

On Wed, 2019-01-09 at 02:38 -0500, Frediano Ziglio wrote:
> > 
> > The message contains information about the graphics device and monitor
> > belonging to a particular video stream (which maps to a channel) from
> > the streaming agent.
> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > ---
> >  spice/stream-device.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/spice/stream-device.h b/spice/stream-device.h
> > index 6add42b..77d76af 100644
> > --- a/spice/stream-device.h
> > +++ b/spice/stream-device.h
> > @@ -90,6 +90,8 @@ typedef enum StreamMsgType {
> >      STREAM_TYPE_CURSOR_SET,
> >      /* guest cursor position */
> >      STREAM_TYPE_CURSOR_MOVE,
> > +    /* the graphics device display information message (device address and
> > display id) */
> > +    STREAM_TYPE_DEVICE_DISPLAY_INFO,
> >  } StreamMsgType;
> >  
> >  typedef enum StreamCapabilities {
> > @@ -140,6 +142,13 @@ typedef struct StreamMsgData {
> >      uint8_t data[0];
> >  } StreamMsgData;
> >  
> > +typedef struct StreamMsgDeviceDisplayInfo {
> 
> No much documentation, when it is supposed to be sent?
> In which state?
> From the agent, from the server or both?

I'll add some documentation.

> No capabilities for a new message?

No, the streaming agent protocol is not stable yet. Better to not add a
capability and make the code more complex if it's not necessary. We've
discussed this before...

> > +    uint32_t id;
> 
> I suppose is not the same "device_display_id", not clear what is it.

Right, this is basically the equivalent of monitor_id, here it is kind
of the ID of the stream to which the display info belongs. It's
basically just a sequence of 0, 1, 2, ... and since only a single
stream/monitor is supported at the moment, only a single message of
this type is sent and it has id = 0.

Suggestions how to name it? monitor_id? stream_id?

> > +    uint32_t device_display_id;
> > +    uint32_t device_address_len;
> 
> No limit? Is 4gb fine? I suppose you want a length to be able to
> extend the message in the future, otherwise you can use the size
> of the message.

How do you want me to set a limit here? In a comment?

I initially had these wrapped in a message that contained an array of
these, hence the length. It is still unclear how this will exactly work
with multi-monitor support. This message is likely going to change with
that... Should I change it now?

> > +    uint8_t device_address[0];  // a zero-terminated string
> 
> Encoding? Utf-8?

The contents are the address of the device, I don't think utf really
matters here.

> I suppose by "zero-terminated" you mean if not terminated is a
> protocol violation.

Yes. Do you prefer something else?

Cheers,
Lukas

> > +} StreamMsgDeviceDisplayInfo;
> > +
> >  /* Tell to stop current stream and possibly start a new one.
> >   * This message is sent by the host to the guest.
> >   * Allows to communicate the codecs supported by the clients.
> 
> Frediano