[Spice-devel] [PATCH spice-gtk 2/2] channel-main: Add support for VD_AGENT_CAP_SPARSE_MONITORS_CONFIG (rhbz#881072)

[Marc-André Lureau]

Hi

Why not be consistant with QXLMonitorsConfig and add an id field to
VDAgentMonitorConfig? That's why spice-gtk didn't care much about
QXLMonitorsConfig id, since VDAgentMonitorConfig didn't had one (since
it was meant to reflect real hw I suppose), it couldn't set it back
(unless the agent learned to deal with "sparse" monitors in some
ways). I agree it could have been done in the first place. Patch looks
good otherwise.

On Fri, Jan 11, 2013 at 10:55 PM, Hans de Goede <hdegoede at redhat.com> wrote:
> Hi,
>
>
> On 01/11/2013 09:02 PM, Marc-André Lureau wrote:
>>
>> Hi
>>
>> My understanding of this patch is that you send all monitors config,
>> regardless if there are disabled. This is a bit wasteful, but not a
>> real problem.
>>
>> Why is that needed if the implementation handle "missing" monitors id
>> as disabled? (which is what the current implementation should do, no?)
>
>
> This code builds a VDAgentMonitorsConfig which contains
> a "struct VDAgentMonConfig monitors[]" array and the
> VDAgentMonConfig struct does not contain ids, instead the expectation is
> that the info for the display with id x is stored at monitors[x]
>
> Note that this being a problem is already acknowledged in the current
> code with a FIXME comment, which this patch actually fixes.
>
> Regards,
>
> Hans
>
>
>
>
>
>>
>> On Thu, Jan 10, 2013 at 11:52 PM, Hans de Goede <hdegoede at redhat.com>
>> wrote:
>>>
>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>> ---
>>>   gtk/channel-main.c | 20 ++++++++++++++------
>>>   1 file changed, 14 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/gtk/channel-main.c b/gtk/channel-main.c
>>> index 653b989..720fcfa 100644
>>> --- a/gtk/channel-main.c
>>> +++ b/gtk/channel-main.c
>>> @@ -939,11 +939,15 @@ gboolean
>>> spice_main_send_monitor_config(SpiceMainChannel *channel)
>>>       c = channel->priv;
>>>       g_return_val_if_fail(c->agent_connected, FALSE);
>>>
>>> -    monitors = 0;
>>> -    /* FIXME: fix MonitorConfig to be per display */
>>> -    for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
>>> -        if (c->display[i].enabled)
>>> -            monitors += 1;
>>> +    if (spice_main_agent_test_capability(channel,
>>> +
>>> VD_AGENT_CAP_SPARSE_MONITORS_CONFIG)) {
>>> +        monitors = SPICE_N_ELEMENTS(c->display);
>>> +    } else {
>>> +        monitors = 0;
>>> +        for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
>>> +            if (c->display[i].enabled)
>>> +                monitors += 1;
>>> +        }
>>>       }
>>>
>>>       size = sizeof(VDAgentMonitorsConfig) + sizeof(VDAgentMonConfig) *
>>> monitors;
>>> @@ -956,8 +960,12 @@ gboolean
>>> spice_main_send_monitor_config(SpiceMainChannel *channel)
>>>
>>>       j = 0;
>>>       for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
>>> -        if (!c->display[i].enabled)
>>> +        if (!c->display[i].enabled) {
>>> +            if (spice_main_agent_test_capability(channel,
>>> +
>>> VD_AGENT_CAP_SPARSE_MONITORS_CONFIG))
>>> +                j++;
>>>               continue;
>>> +        }
>>>           mon->monitors[j].depth  = c->display_color_depth ?
>>> c->display_color_depth : 32;
>>>           mon->monitors[j].width  = c->display[j].width;
>>>           mon->monitors[j].height = c->display[j].height;
>>> --
>>> 1.8.0.2
>>>
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>>
>>
>>
>



-- 
Marc-André Lureau