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

[Lukáš Hrázký]

On Thu, 2019-01-10 at 14:28 -0600, Jonathon Jongsma wrote:
> On Wed, 2019-01-09 at 12:43 +0100, Lukáš Hrázký wrote:
> > 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?
> 
> I think stream_id may be ok.

I'll wait for Frediano to confirm he's fine with stream_id as well...

> > > > +    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?
> 
> Personally I would not try to accomodate multimonitor until we know
> what we need.
> 
> > 
> > > > +    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.
> 
> Well, the address format is defined as 
> 
> "pci/<DOMAIN>/<SLOT>.<FUNCTION>/.../<SLOT>.<FUNCTION>"
> 
> So, aside from the string "pci/", the rest is just hex numbers and
> dots. So we don't need to encode a bunch of special characters, just
> characters within the ASCII range. But I don't suppose it would hurt to
> document it as utf-8...

Yes. I would aslo assume utf-8 is the default and there hardly is any
other consideration anyway. If it's not explicitly stated anywhere, I
would just document strings in general being utf-8 encoded in the
protocol and not document it per string?

Cheers,
Lukas

> > > 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
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
>