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 :)
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