On Mon, 2018-08-20 at 13:08 +0200, Lukáš Hrázký wrote:
> On Fri, 2018-08-17 at 11:25 -0500, Jonathon Jongsma wrote:
> > On Fri, 2018-08-17 at 11:41 +0200, Lukáš Hrázký wrote:
> > > On Thu, 2018-08-16 at 16:08 -0500, Jonathon Jongsma wrote:
> > > > On Thu, 2018-08-16 at 18:26 +0200, Lukáš Hrázký wrote:
> > > > > The message is meant to contain information related to the
> > > > > stream,
> > > > > e.g.
> > > > > now the guest (xrandr) output ID of the streamed monitor.
> > > > >
> > > > > Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
> > > > > ---
> > > > > spice/stream-device.h | 11 +++++++++++
> > > > > 1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/spice/stream-device.h b/spice/stream-device.h
> > > > > index 6add42b..be22e06 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 stream information message */
> > > > > + STREAM_TYPE_INFO,
> > > > > } StreamMsgType;
> > > > >
> > > > > typedef enum StreamCapabilities {
> > > > > @@ -140,6 +142,15 @@ typedef struct StreamMsgData {
> > > > > uint8_t data[0];
> > > > > } StreamMsgData;
> > > > >
> > > > > +/* Message containing information about the stream, sent
> > > > > when a
> > > > > new
> > > > > stream is created.
> > > > > + */
> > > > > +typedef struct StreamMsgInfo {
> > > > > + /* The xrandr output ID (index of the output in the
> > > > > list)
> > > > > that
> > > > > is being
> > > > > + * streamed, 0-based.
> > > > > + */
> > > > > + uint32_t guest_output_id;
> > > > > +} StreamMsgInfo;
> > > > > +
> > > > > /* 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.
> > > >
> > > >
> > > > So, here's where our work starts to overlap a bit (and why it's
> > > > a
> > > > bit
> > > > unfortunate that I haven't gotten around to sending out a
> > > > proposal
> > > > for
> > > > multi-monitor support so far, perhaps).
> > >
> > > It's not much of a problem, I just did it the simple way not
> > > knowing
> > > what the plans were for multi-monitor, but it's not hard to
> > > change.
> > >
> > > > The change above assumes that there will be a single output for
> > > > a
> > > > single stream device. This is currently true because we only
> > > > support a
> > > > single output. For a variety of reasons, I've chosen to
> > > > implement
> > > > multimonitor support by multiplexing multiple outputs over a
> > > > single
> > > > stream device. I'll share my current working patch for the
> > > > stream
> > > > device protocol below.
> > > >
> > > > But first I'll try to explain why I chose to multiplex displays
> > > > over a
> > > > single stream device. In general, I think it makes things
> > > > easier.
> > > > Some
> > > > examples:
> > > >
> > > > * if you use one stream device per output, you need to decide
> > > > before
> > > > you launch the vm how many outputs it will support and
> > > > allocate
> > > > those devices by passing the correct options to qemu. To add
> > > > an
> > > > additional output, you need to re-start qemu with different
> > > > parameters.
> > >
> > > This alone is reason enough to do it the way you did it...
> > >
> > > > * We currently use a hard-coded spice port name (org.spice-
> > > > space.stream.0). If we have a separate stream device for
> > > > each
> > > > display, we will have multiple port names. How do those port
> > > > names
> > > > get communicated from the server to the guest?
> > > > * If you have multiple stream devices, do we launch a separate
> > > > spice-
> > > > streaming-agent process for each one? Or a single streaming
> > > > agent
> > > > that handles all of them?
> > > > * You also have to decide how to handle odd issues such as the
> > > > fact
> > > > that the driver may be perfectly capable of enabling
> > > > additional
> > > > monitors, but we don't have sufficient stream devices for
> > > > them.
> > > > So
> > > > what do you do when the vm was configured with only a single
> > > > spice
> > > > device, but the user enables an additional display from
> > > > within
> > > > the
> > > > guest? Now perhaps the guest has two displays enabled, but
> > > > we
> > > > have
> > > > no way of sending that second display to the client.
> > > >
> > > > So there were a lot of little things like this that led me to
> > > > decide
> > > > that using a single stream device was simpler and more
> > > > flexible.
> > > > There
> > > > may have been other reasons that I've forgotten already as
> > > > well.
> > > >
> > > > The only real downside I could think of was that the stream
> > > > device
> > > > protocol would have to change in a non-backward-compatible way
> > > > (see
> > > > below). But since the streaming agent is still in early stages,
> > > > that
> > > > doesn't seem like a huge deal.
> > >
> > > I'm not sure, we made a release and no compatibility disclaimer
> > > was
> > > made about the protocol compatibility (only about API/ABI
> > > instability),
> > > but at this stage I'd say we shouldn't worry about compatibility
> > > yet,
> > > probability of changes is still high.
> > >
> > > > Of course, it's possible that I missed
> > > > other disadvantages, so let me know any problems you see with
> > > > this
> > > > approach.
> > >
> > > I can only think of possible performace issues of a single port
> > > with
> > > multiplexed 4 4k displays for example? Not sure of the behaviour
> > > and
> > > if
> > > something would be different if there were 4 ports? Maybe a test
> > > would
> > > be in place before we commit to a solution?
> > >
> > > > Anyway, here's the current proof-of-concept changes to the
> > > > stream
> > > > device protocol that I'm working with:
> > >
> > > Looks fine to me in general, hopefully Frediano takes a look as
> > > well
> > > :)
> > >
> > > > diff --git a/spice/stream-device.h b/spice/stream-device.h
> > > > index 6add42b..57c768f 100644
> > > > --- a/spice/stream-device.h
> > > > +++ b/spice/stream-device.h
> > > > @@ -58,7 +58,7 @@
> > > > */
> > > >
> > > > /* version of the protocol */
> > > > -#define STREAM_DEVICE_PROTOCOL 1
> > > > +#define STREAM_DEVICE_PROTOCOL 2
> > > >
> > > > typedef struct StreamDevHeader {
> > > > /* should be STREAM_DEVICE_PROTOCOL */
> > > > @@ -78,8 +78,8 @@ typedef enum StreamMsgType {
> > > > STREAM_TYPE_INVALID = 0,
> > > > /* allows to send version information */
> > > > STREAM_TYPE_CAPABILITIES,
> > > > - /* send screen resolution */
> > > > - STREAM_TYPE_FORMAT,
> > > > + /* send guest output configuration */
> > > > + STREAM_TYPE_OUTPUT_CONFIG,
> > >
> > > So the naming is "output_config" here. I don't mind, if we want
> > > to
> > > call
> > > each "monitor" by the name "output" here? At this point I don't
> > > have
> > > much of an opinion, except... (see below)
> >
> > I'm not quite sure what you mean by 'calling each "monitor" by the
> > name
> > "output"'...
>
> By that I mean that you've picked a new name, "output", for the thing
> you are identifying with it, which I called "monitor". "monitor" and
> its ID "monitor_id" is the name that is used for it in rest of SPICE
> I
> think.
>
> You may have picked "output_id" because I used that in my patches...
> (continued below)
>
> > >
> > > > /* stream data */
> > > > STREAM_TYPE_DATA,
> > > > /* server ask to start a new stream */
> > > > @@ -115,6 +115,16 @@ typedef struct StreamMsgCapabilities {
> > > >
> > > > #define STREAM_MSG_CAPABILITIES_MAX_BYTES 1024
> > > >
> > > > +typedef struct StreamMsgOutput {
> > > > + uint8_t output_id;
> > >
> > > ... "output_id" is very similar to the "guest_output_id" I used
> > > throughout the monitor ID series. (though you've got uint8_t here
> > > and
> > > I
> > > use uint32_t)
> > >
> > > Most important role of the "output_id" here is being the unique
> > > identifier of the "Outputs" as you define them here. A major
> > > question
> > > we should answer is whether we want to use this "output_id" also
> > > as
> > > the
> > > guest_output_id. At this moment, "guest_output_id" is a sequence
> > > starting from 0, so "output_id" can easily be equal to that, but
> > > they
> > > are not in essence not the same numbers.
> > >
> > > On a side note, a question was already raised what will the
> > > "guest_output_id"s be on Wayland. The answer is I have no idea.
> > > On
> > > Wayland it seems each compositor is doing the multi-monitor setup
> > > itself, which seems like a nightmare for us.
> >
> > In my current patches, the value of output_id is the value I
> > receive
> > from xrandr. So it is equal to the guest output id. But you're
> > right
> > that it's not stated explicitly here and it probably should be. I
> > can't
> > remember now why I used uint8_t. I thought I based it some some
> > existing monitors config struct, but now I'm not sure...
>
> ... or because you also used the "output" ID from xrandr.
>
> But do you understand my point? Xrandr "outputs" and their "stream",
> i.e. the streamed content of each of the output (I'll call it
> "monitor
> stream" here) are two different things and there may not always be a
> 1:1 relation between them (e.g. we may one day decide we need more
> "monitor streams" for a single "output" or vice versa, though it
> sounds
> very contrieved). Or they may not always be so nice as 0, 1, 2, ...
>
> So practically I suggest we assing our own "monitor_id" to always be
> a
> sequence 0, 1, 2, ... i.e. a unique ID we have in our control. And
> then
> add the "guest_output_id" to the StreamMsgOutput that would contain
> whatever ID is used in the guest to identify the output as an
> information associated with the "monitor_id".
>
> Do I make sense this time? :) By doing this we prevent any future
> problems with the IDs if a change needs to be made. Given how hard to
> change our protocols are, I think this is very prudent :)
Hmm, I'm afraid to say that I'm a little bit confused about this. Can
you give a concrete example (even if it's a little contrived) to
describe what you're talking about?
>
> Cheers,
> Lukas
>
> > Good point about Wayland. We should probably ensure that we design
> > the
> > interface with Wayland in mind so that we can support that in the
> > future. At the moment, I don't know what identifier we would use
> > for
> > Wayland, however. Needs more investigation.
> >
> > >
> > > > + /* screen resolution/stream size */
> > > > + uint32_t width;
> > > > + uint32_t height;
> > > > + /* coordinates of the top-left corner of the screen */
> > > > + int32_t x;
> > > > + int32_t y;
> > > > +} StreamMsgOutput;
> > > > +
> > > > /* Define the format of the stream, start a new stream.
> > > > * This message is sent by the guest to the host to
> > > > * tell the host the new stream format.
> > > > @@ -122,21 +132,21 @@ typedef struct StreamMsgCapabilities {
> > > > * States allowed: Ready
> > > > * state will change to Streaming.
> > > > */
> > > > -typedef struct StreamMsgFormat {
> > > > - /* screen resolution/stream size */
> > > > - uint32_t width;
> > > > - uint32_t height;
> > > > +typedef struct StreamMsgOutputConfig {
> > > > /* as defined in SpiceVideoCodecType enumeration */
> > > > uint8_t codec;
> > > > - uint8_t padding1[3];
> > > > -} StreamMsgFormat;
> > > > + uint8_t n_outputs;
> > > > + StreamMsgOutput outputs[0];
> > > > +} StreamMsgOutputConfig;
> > > > +
> > >
> > > So the "codec" is common to all the outputs and each output has
> > > it's
> > > own dimensions. Makes sense...
> > >
> > > Cheers,
> > > Lukas
> > >
> > > > -/* This message contains just raw data stream.
> > > > +/* This message contains just raw data stream for the
> > > > specified
> > > > output.
> > > > * This message is sent by the guest to the host.
> > > > *
> > > > * States allowed: Streaming
> > > > */
> > > > typedef struct StreamMsgData {
> > > > + uint8_t output_id;
> > > > uint8_t data[0];
> > > > } StreamMsgData;
> > > >
> > > >
> >
> > Thanks for the comments
> >
> > Jonathon
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel