> On 22 Jun 2018, at 11:52, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
>
>>
>> What speculation are you talking about? By listing precise use cases and data sources, I was trying to remove that speculation, so I’m confused as to why you still think it’s speculative.
>
> You did list them, but AFAICS you didn't describe the mechanisms of
> each individual feature as it would be implemented and it's interaction
> with the rest of the system. You may have thought it through, there may
> not be any more complexities to them worth mentioning, but I am not
> sure atm., therefore at least I myself would be speculating on that.
I did not expect it would be controversial to get “industry standard monitor configuration data” in the new “monitor config” structure…
>
> Basically, until there is the code implementing the features, there
> always is a chance something is wrong with it.
>
>>> But I'm not against adding some padding which can be used in the future
>>> for such stuff (see below, I think you didn't understand my suggestion
>>> there), which I believe is sufficient to not have backwards
>>> compatibility trouble and that's all we really need to do now…?
>>
>> Don’t you need a way to know which extensions are used? Like flags, or a size, or a version?
>
> I'm sorry, I'm completely lost here, what extensions?
Monitor config extensions. I.e. what you added in the padding in a later version.
>
>> Back on topic,
>
> It's been quite a long time since this discussion was on topic :(
Disagree. The discussion is and has always been about what to put in the new monitor config data structure. I think it’s appropriate for a patch that suggests a new monitor config data structure ;-)
>
>> based on the rest of the discussion, I believe that there is an interesting complexity trade-off between:
>>
>> 1) getting the list of necessary fields right the first time
>> 2) repeated protocol changes for each new data we discover
>> 3) a fully extensible monitor config
>>
>> If we were using Javascript and JSON, I’d got for (3) immediately. But we are using spice.proto, and given that the monitor data comes from either the driver or the monitor, I think that the list I built is relatively exhaustive. So my own complexity analysis is that getting (1) right is easier than getting (3) right. But hey, if you have fun coding (3), go ahead!
>
> I'd just like to point out that IMO _coding_ 3) would be a bad idea no
> matter what. That would just be single-purpose code pile that grows the
> code-base and causes headaches to people dealing with it, while not
> helping much.
>
> I merely mentioned it as a reference to the discussion we had about
> (gradually) replacing the SPICE protocol with an existing protocol
> implementation that's well designed to be extensible. A very big and
> long term goal and a topic for another discussion entirely.
That part is another topic entirely.
But dropping 3 means I would prefer to get 1 right. See last comment on this email for a suggestion.
>
>>> The smileys are there to express
>>> there are no bad feelings and I mean this in a good natured way, but I
>>> do think so. I think this is big enough without adding more stuff on
>>> top.
>>
>> How much bigger would the patch series be if the only difference was to add half a dozen fields to the new structure?
>
> Not much by size, but more work with trying to make sure the fields are
> correct, without having any code that uses them to verify we got it
> right.
Why would you need to check reserved fields?
>
>>>> Do you really think it is so complicated to add half a dozen fields to a structure instead of one?
>>>>
>>>> All I did was look at EDID and select information that might be useful later. It’s a very finite list.
>>>
>>> Yes, but for what purpose.
>>
>> About being “vague”, questioning “for what purpose” is not clear to me… I can’t tell if 1) you thought my examples were vague, 2) you think my list is only vaguely exhaustive, 3) there is only a vague need for it.
>>
>> I suspect (3), so I’ll address that. It’s a fallacy to counter a “I need this” argument with a “I don’t need it” response.
>>
>>
>>> This is either pure speculation
>>
>> See answer to “speculation” above.
>>
>>
>>> or I and reviewers need to go out of their way
>>
>> Does it take more time to search for the use cases, or to understand them? Speaking of which… I was the one going "out of my way" to find as many other use cases I could think of we might have for an extended monitor config…
>
> I just realized that the EDID data is probably only available in the
> guest, which means only for the streaming with the agent case. I think
> we can't get the EDID data in the server for QXL devices with the
> current API, someone correct me if I'm wrong.
Name could be “QXL #n”. Supported resolutions could be “any”. DPI could be standard (72dpi). Do you think there’s more to it?
(and that’s for later anyway)
>
>> Yes, it’s more “complicated” to play chess or design software by looking five moves ahead instead of one. But that makes you a better chess player or software designer in the end.
>>
>>
>>> and think through the intended
>>> use cases, which are more or less vague at this stage.
>>
>> What makes them vague?
>
> See my reply on speculation, it's the same point.
>
>>>>> I'm not sure what some of the info would be useful for at all and the other
>>>>> information is mostly for additional features which are non-trivial and
>>>>> a question whether they would ever get implemented.
>>>>
>>>> If you don’t add the data in the structure, then you probably *decide* that the feature will never be implemented, because the features I listed are too marginal to justify a protocol break by themselves.
>>>
>>> Interesting point. But adding the padding I mentioned would solve this?
>>
>> Yes, as long as you can evaluate its size, which brings you back to the list of required fields. So what do you think you win by going that way?
>
> We can add some small space on top of that to make sure we'll be fine
> with any unexpected changes. It also spares us defining the fields now,
> which would be speculative, the definition would go with the
> implementation that verifies it.
I find it more speculative to leave something undefined than to spend a little time discussing it.
>
>>>>> It's again touching the subject of a more extensible protocol, with
>>>>> which you wouldn't have to try and predict what kind of stuff we'll
>>>>> need for the future. There's a very high chance we'll need something
>>>>> completely different and first thing we would do is to rename the
>>>>> zeroed fields you are suggesting to use them for something else.
>>>>
>>>> How do you compute the “very high chance”? Are you saying there is a high chance that EDID will change by the time we implement that stuff? Or that we will have more urgent need for monitor related data that will force us to evict EDID fields to make room for something else?
>>>>
>>>> In any case, I am not advocating *against* an extensible format. That’s what I planned for smart streaming metrics. I’m advocating *for* some additional fixed fields in the new monitor config, that’s all. If you want to tack some future-proof extensible mechanism on top of this, go ahead. But since an extensible protocol is more complicated, you should justify why you can’t say today what kind of data you might need. So back to how you compute the “very high chance”, which I find personally hard to believe without good evidence.
>>>
>>> Let's call it "gut feeling" :) I don't have a mathematical definition for you.
>>
>> I’m not asking for a mathematical proof. Gut feeling is another name for “intuition”. Your brain processed data, you are not aware of how. Can you dig and figure out why you have this gut feeling, and share that with us? That’s what I asked for. Experience, previous failures in over-designing some data structure, that kind of stuff.
>
> Experience I suppose.
Did not help. I expected “I did this, that happened, therefore I tend to think that…”
On my side, my own experience tells me that looking ahead avoids huge costs later. Like: telling Apple about a binary compatibility issue in IOKit, being ignored, and a friend of mine asking years later “did you write this email? It’s just as bad as you predicted”. Or super-smart HP engineers warning Intel not to repeat for Itanium the same mistake they had made with x86, i.e. gratuitously make it non-virtualizable, being ignored, and then HP and Intel spending amazing efforts just to get decent virtualization performance despite these flaws. Or that other super-smart guy who knew that export templates in C++ were a really bad idea, could not prevent less knowledgeable people from pushing their agenda, ended up after years of work releasing the only compiler to ever support export templates, and only then was able to kill that non-feature (funny to “deprecate” a feature that no compiler except his ever implemented ;-)
All these examples relate to the present discussion in that is that in all cases, someone decided to ignore a problem, responding “it’s only speculation that we even have the problem you are talking about”, and then the ensuing damage was completely out of proportion with what a little bit of looking-ahead could have achieved. Also, they all share the common pattern (along with the present discussion IMO) that someone saw an unnecessary limitation in the original design ahead of others, pointed it out, and was ignored. In other words, in all cases it could have been prevented. I have dozens more horror stories like this if you want them.
Consequently, my own gut feeling, based on that history, is screaming that if we change the monitor config data, we can and should look at future usage of that structure *right now*, not after we have frozen the data structure in stone.
>
>>> I didn't mean EDID would change or anything, but that when actually
>>> implementing the feature, there is the chance I'd need something in the
>>> protocol which we didn't think about when adding the fields.
>>
>> True, but does not *remove* the need for EDID-derived fields for which a need (no matter how vague) has been identified.
>>
>>
>>
>>>>>
>>>>> For that, we can just add some ~20B zeroed padding (or however much we
>>>>> want) to have a space to use in the future…
>>>>
>>>> Straw-man. EDID is 128 bytes, not 20MB, and I only talked about part of it.
>>>
>>> That was 20B, not 20MB.
>>
>> Oh, sorry for misreading… I thought you were exaggerating for emphasis.
>>
>> Then 20 bytes is not enough. The name alone is at least 16 bytes (I think it’s 16 bytes for EDID, but if we want to give a card name when no monitor is connected, then 32 or 64 might be more appropriate for that name alone). EDID is densely packed with flags for resolutions.
>
> I don't think the size is practically a problem here, though this data
> would need comparatively quite a bit more space to what is in there
> already. I'm planning to send an summary of the proposed protocol and
> API changes, I'll mention it there. Though you didn't list the sizes
> needed for the fields, so I'll have to go figuring that out...
>
>>> And since you read it as 20MB, you probably
>>> took this as not being serious, which it was... I seriously suggest to
>>> add an arbitrary zeroed space at the end of the message for future use.
>>> It can be however big you think the extra data you are suggesting would
>>> need.
>>
>> If you want to make it extensible, you may want to look at what the spice.proto marshaller generators can do easily. Arrays with a size before are (relatively) easy. But adding fields of various kinds is not as easy. So if you want to go that route, now I’m the one being concerned that you may be over-complicating things. But since you are the one coding it, feel free to go that route if you are confident with it.
>
> No, I just want to add a zeroed space that would be there for future
> use.
OK. So what about having that zeroed space be something like the following:
// Fields below are reserved and will be zeroed initially
// Future software may indicate that a field contains valid data
// by putting a non-zero value in it. That data can safely
// be ignored by versions of the software that do not need it.
// user-visible display name, e.g. from EDID name or driver
char display_name[32];
// feature flags for the display
uint32 flags;
// Additional information about the current state of the display
uint16 pixels_per_inch;
uint16 gamma; // fixed point, equals gamma x 256
uint16 preferred_aspect_ratio;
uint16 refresh_rate;
// number of available resolutions in the following array
// data is interpreted as a resolution only if flags & 1.
// otherwise, the data is interpreted according to flags.
uint32 available_resolutions_count;
struct available_resolution
{
uint16 flags;
uint16 width;
uint16 height;
uint16 color_depth;
uint16 refresh_rate;
} available_resolutions[0];
This adds 48 bytes of reserved space to your structure, covers all the cases I am presently aware of, is extensible the way you wanted it (simply by repurposing available_resolution[n] to carry some arbitrary data when ~flags & 1, with 64 bits of payload for each array entry), and can I believe be easily described in spice.proto without changing the marshaller generators.
Is that overly complicated?
Thanks
Christophe
>
> Cheers,
> Lukas
>
>> Thanks,
>> Christophe
>>
>>>
>>> Cheers,
>>> Lukas
>>>
>>>> Cheers,
>>>> Christophe
>>>>
>>>>>
>>>>> Cheers,
>>>>> Lukas
>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yeah, as I said, not sure what the reason was, but IMO you could
>>>>>>>>>> have
>>>>>>>>>> had that and still make it much more robust.
>>>>>>>>>
>>>>>>>>> I cannot say a car has a bad design because it does not have the
>>>>>>>>> load
>>>>>>>>> of a big truck, just a different use case. We probably started
>>>>>>>>> using
>>>>>>>>> the car (to continue the metaphor) to load some stuff and we are
>>>>>>>>> now
>>>>>>>>> exceeding the limit. At the beginning we though a car was enough :-
>>>>>>>>> )
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Oh and BTW, not sure how old this is, but I'm sure I'd have much
>>>>>>>>>> worse
>>>>>>>>>> things to say about the code _I_ wrote say like 8 years ago ;)
>>>>>>>>>>
>>>>>>>>>>>>>> Hope this explains it well enough. It's all very complex
>>>>>>>>>>>>>> and goes
>>>>>>>>>>>>>> over
>>>>>>>>>>>>>> multiple interfaces (the network protocols as well as the
>>>>>>>>>>>>>> spice-gtk
>>>>>>>>>>>>>> API), so the more brilliant ideas you'll have, the more
>>>>>>>>>>>>>> welcome they
>>>>>>>>>>>>>> will be :)
>>>>>>>>>>>>>
>>>>>>>>>>>>> If we can avoid modifying all the layers and exposing the
>>>>>>>>>>>>> inner
>>>>>>>>>>>>> details, I would encourage the exploration of other
>>>>>>>>>>>>> solution.
>>>>>>>>>>>>
>>>>>>>>>>>> I don't see how we can do that... And the inner details are
>>>>>>>>>>>> already
>>>>>>>>>>>> quite exposed, unfortunately :(
>>>>>>>>>>>>
>>>>>>>>>>>> As I said, ideas welcome.
>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 1. The streaming-agent sends a new StreamInfo message
>>>>>>>>>>>>>>>> when it
>>>>>>>>>>>>>>>> starts
>>>>>>>>>>>>>>>> streaming. The message contains the output_id of the
>>>>>>>>>>>>>>>> streamed
>>>>>>>>>>>>>>>> monitor.
>>>>>>>>>>>>>>>> The actual value is the index in the list of xrandr
>>>>>>>>>>>>>>>> outputs.
>>>>>>>>>>>>>>>> Basically,
>>>>>>>>>>>>>>>> the number should uniquely identify a monitor in the
>>>>>>>>>>>>>>>> guest
>>>>>>>>>>>>>>>> context under
>>>>>>>>>>>>>>>> X.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 2. The server copies the number into the
>>>>>>>>>>>>>>>> SpiceMsgDisplayMonitorsConfig
>>>>>>>>>>>>>>>> message and sends it to the client as part of the
>>>>>>>>>>>>>>>> monitors_config
>>>>>>>>>>>>>>>> exchange.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 3. The client stores the output_id in it's list of
>>>>>>>>>>>>>>>> monitor_configs. Here
>>>>>>>>>>>>>>>> I had to make significant changes, because the
>>>>>>>>>>>>>>>> monitors_config
>>>>>>>>>>>>>>>> code in
>>>>>>>>>>>>>>>> spice-gtk is very loosely written and also exposes
>>>>>>>>>>>>>>>> quite a few
>>>>>>>>>>>>>>>> unnecessary details to the client app. The client
>>>>>>>>>>>>>>>> sends the
>>>>>>>>>>>>>>>> output_id
>>>>>>>>>>>>>>>> back to the server as part of the monitors_config
>>>>>>>>>>>>>>>> messages
>>>>>>>>>>>>>>>> (VDAgentMonitorsConfigV2) and also uses it for the
>>>>>>>>>>>>>>>> mouse motion
>>>>>>>>>>>>>>>> event,
>>>>>>>>>>>>>>>> which also contains a display ID interpreted by the
>>>>>>>>>>>>>>>> vd_agent. In
>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>> end, the API/ABI towards the client app should remain
>>>>>>>>>>>>>>>> unchanged.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 4. The server passes the output_id in above-mentioned
>>>>>>>>>>>>>>>> messages to
>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>> vd_agent. The output_id is meaningless in the server
>>>>>>>>>>>>>>>> context.
>>>>>>>>>>>>>>>> (Currently, it doesn't pass the monitors_config
>>>>>>>>>>>>>>>> messages if there
>>>>>>>>>>>>>>>> is a
>>>>>>>>>>>>>>>> QXL device that supports it, though. Needs more
>>>>>>>>>>>>>>>> work.)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 5. vd_agent:
>>>>>>>>>>>>>>>> a) For mouse input, the output_id was passed in the
>>>>>>>>>>>>>>>> original
>>>>>>>>>>>>>>>> message,
>>>>>>>>>>>>>>>> so no change needed here, it works.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> b) If the server sends monitors_config to the guest,
>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>> vdagent will
>>>>>>>>>>>>>>>> prefer to use monitors_configs with the output_ids
>>>>>>>>>>>>>>>> set, if such
>>>>>>>>>>>>>>>> are
>>>>>>>>>>>>>>>> present. In that case, it ignores monitors_configs
>>>>>>>>>>>>>>>> with the
>>>>>>>>>>>>>>>> output_id
>>>>>>>>>>>>>>>> unset. If no output_ids are present, it should
>>>>>>>>>>>>>>>> behave as it
>>>>>>>>>>>>>>>> used to.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> A couple of things to note:
>>>>>>>>>>>>>>>> - While I did copy the VDAgentMonitorsConfig to a
>>>>>>>>>>>>>>>> different
>>>>>>>>>>>>>>>> message for
>>>>>>>>>>>>>>>> backwards compatibility, I didn't do the same for
>>>>>>>>>>>>>>>> SpiceMsgDisplayMonitorsConfig, it remains to be done.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> - I didn't introduce any capabilities to handle the
>>>>>>>>>>>>>>>> compatibility, also
>>>>>>>>>>>>>>>> remains to be done. Hopefully it is also clear it
>>>>>>>>>>>>>>>> will be quite a
>>>>>>>>>>>>>>>> non-trivial job to do that :( Ain't gonna make the
>>>>>>>>>>>>>>>> code prettier
>>>>>>>>>>>>>>>> either.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> For your convenience, you can also pull the branches
>>>>>>>>>>>>>>>> below:
>>>>>>>>>>>>>>>> https://gitlab.freedesktop.org/lukash/spice-protocol/
>>>>>>>>>>>>>>>> tree/monitors-config-poc
>>>>>>>>>>>>>>>> https://gitlab.freedesktop.org/lukash/spice-common/tr
>>>>>>>>>>>>>>>> ee/monitors-config-poc
>>>>>>>>>>>>>>>> https://gitlab.com/lhrazky/spice-streaming-agent/tree
>>>>>>>>>>>>>>>> /monitors-config-poc
>>>>>>>>>>>>>>>> https://gitlab.freedesktop.org/lukash/spice/tree/moni
>>>>>>>>>>>>>>>> tors-config-poc
>>>>>>>>>>>>>>>> https://gitlab.freedesktop.org/lukash/spice-gtk/tree/
>>>>>>>>>>>>>>>> monitors-config-poc
>>>>>>>>>>>>>>>> https://gitlab.freedesktop.org/lukash/vd_agent/tree/m
>>>>>>>>>>>>>>>> onitors-config-poc
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> All in all, it's big, complex and invasive. Note I
>>>>>>>>>>>>>>>> did review the
>>>>>>>>>>>>>>>> emergency instructional video [1] and am therefore
>>>>>>>>>>>>>>>> ready for any
>>>>>>>>>>>>>>>> bombardment you'll be sending my way :D (Don't
>>>>>>>>>>>>>>>> hesitate to
>>>>>>>>>>>>>>>> contact me
>>>>>>>>>>>>>>>> with questions either)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Last minute note: I've noticed some of the patches
>>>>>>>>>>>>>>>> are missing
>>>>>>>>>>>>>>>> Signed-off-by line, since they are not for merging,
>>>>>>>>>>>>>>>> should not be
>>>>>>>>>>>>>>>> an
>>>>>>>>>>>>>>>> issue...
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Lukáš Hrázký (16):
>>>>>>>>>>>>>>>> spice-protocol
>>>>>>>>>>>>>>>> Add the StreamInfo message
>>>>>>>>>>>>>>>> Create a version 2 of the VDAgentMonitorsConfig
>>>>>>>>>>>>>>>> message
>>>>>>>>>>>>>>>> spice-common
>>>>>>>>>>>>>>>> add output_id to SpiceMsgDisplayMonitorsConfig
>>>>>>>>>>>>>>>> spice-streaming-agent
>>>>>>>>>>>>>>>> Send a StreamInfo message when starting to stream
>>>>>>>>>>>>>>>> spice-server
>>>>>>>>>>>>>>>> Handle the StreamInfo message from the streaming
>>>>>>>>>>>>>>>> agent
>>>>>>>>>>>>>>>> Use VDAgentMonitorsConfigV2 that contains the
>>>>>>>>>>>>>>>> output_id field
>>>>>>>>>>>>>>>> spice-gtk
>>>>>>>>>>>>>>>> Rework the handling of monitors_config
>>>>>>>>>>>>>>>> Remove the n_display_channels check when sending
>>>>>>>>>>>>>>>> monitors_config
>>>>>>>>>>>>>>>> Use an 'enabled' flag instead of status enum in
>>>>>>>>>>>>>>>> monitors_config
>>>>>>>>>>>>>>>> Use VDAgentMonitorsConfigV2 as the message for
>>>>>>>>>>>>>>>> monitors_config
>>>>>>>>>>>>>>>> Add output_id to the monitors_config
>>>>>>>>>>>>>>>> Use the new output_id as display ID for the mouse
>>>>>>>>>>>>>>>> motion event
>>>>>>>>>>>>>>>> vd_agent
>>>>>>>>>>>>>>>> vdagent: Log the diddly doo X11 error
>>>>>>>>>>>>>>>> Improve/add some logging messages
>>>>>>>>>>>>>>>> Use VDAgentMonitorsConfigV2 instead of
>>>>>>>>>>>>>>>> VDAgentMonitorsConfig
>>>>>>>>>>>>>>>> vdagent: Use output_id from VDAgentMonitorsConfigV2
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> [1] https://www.youtube.com/watch?v=IKqXu-5jw60
>>>>>>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Frediano
>>>>>>>>> _______________________________________________
>>>>>>>>> Spice-devel mailing list
>>>>>>>>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Spice-devel mailing list
>>>>>>>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Spice-devel mailing list
>>>>>>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>>>>
>>>>>> _______________________________________________
>>>>>> Spice-devel mailing list
>>>>>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>>>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>>
>>>>
>>>
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel