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)
> /* 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.
> + /* 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;
>
>
>
> Jonathon
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel