diff mbox

[PATCHv4,6/6] ui/vnc: disable adaptive update calculations if not needed

Message ID 1389172118-25402-7-git-send-email-pl@kamp.de
State New
Headers show

Commit Message

Peter Lieven Jan. 8, 2014, 9:08 a.m. UTC 
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Wayne Xia Jan. 9, 2014, 8:29 a.m. UTC | #1 
于 2014/1/8 17:08, Peter Lieven 写道:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   ui/vnc.c |    9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index da552fe..a742d32 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>               acl = 1;
>   #endif
>           } else if (strncmp(options, "lossy", 5) == 0) {
> +#ifdef CONFIG_VNC_JPEG
>               vs->lossy = true;
> +#endif
>           } else if (strncmp(options, "non-adaptive", 12) == 0) {
>               vs->non_adaptive = true;
>           } else if (strncmp(options, "share=", 6) == 0) {
> @@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>           }
>       }
> 
> +    /* adaptive updates are only used with tight encoding and
> +     * if lossy updates are enabled so we can disable all the
> +     * calculations otherwise */
> +    if (!vs->lossy) {
> +        vs->non_adaptive = true;
> +    }
> +

  The code seems: if vs->loosy == false, then vs->non_adaptive = true,
translate as: if loosy update is not used, then don't do adaptive
update., which doesn't conform with the comments. I am not sure if this
is on expectation.


>   #ifdef CONFIG_VNC_TLS
>       if (acl && x509 && vs->tls.x509verify) {
>           if (!(vs->tls.acl = qemu_acl_init("vnc.x509dname"))) {
>
Peter Lieven Jan. 9, 2014, 4:25 p.m. UTC | #2 
Am 09.01.2014 09:29, schrieb Wenchao Xia:
> 于 2014/1/8 17:08, Peter Lieven 写道:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   ui/vnc.c |    9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index da552fe..a742d32 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>               acl = 1;
>>   #endif
>>           } else if (strncmp(options, "lossy", 5) == 0) {
>> +#ifdef CONFIG_VNC_JPEG
>>               vs->lossy = true;
>> +#endif
>>           } else if (strncmp(options, "non-adaptive", 12) == 0) {
>>               vs->non_adaptive = true;
>>           } else if (strncmp(options, "share=", 6) == 0) {
>> @@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>           }
>>       }
>>
>> +    /* adaptive updates are only used with tight encoding and
>> +     * if lossy updates are enabled so we can disable all the
>> +     * calculations otherwise */
>> +    if (!vs->lossy) {
>> +        vs->non_adaptive = true;
>> +    }
>> +
>   The code seems: if vs->loosy == false, then vs->non_adaptive = true,
> translate as: if loosy update is not used, then don't do adaptive
> update., which doesn't conform with the comments. I am not sure if this
> is on expectation.
It don't see the logic break. The option means non_adaptive, not adaptive.

I write "adaptive updates are only used ... with lossy updates...". Which
is the same as "without lossy updates we don't need adaptive updates".

Peter

>
>
>>   #ifdef CONFIG_VNC_TLS
>>       if (acl && x509 && vs->tls.x509verify) {
>>           if (!(vs->tls.acl = qemu_acl_init("vnc.x509dname"))) {
>>
Wayne Xia Jan. 10, 2014, 3:09 a.m. UTC | #3 
于 2014/1/10 0:25, Peter Lieven 写道:
> Am 09.01.2014 09:29, schrieb Wenchao Xia:
>> 于 2014/1/8 17:08, Peter Lieven 写道:
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> ---
>>>    ui/vnc.c |    9 +++++++++
>>>    1 file changed, 9 insertions(+)
>>>
>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>> index da552fe..a742d32 100644
>>> --- a/ui/vnc.c
>>> +++ b/ui/vnc.c
>>> @@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>                acl = 1;
>>>    #endif
>>>            } else if (strncmp(options, "lossy", 5) == 0) {
>>> +#ifdef CONFIG_VNC_JPEG
>>>                vs->lossy = true;
>>> +#endif
>>>            } else if (strncmp(options, "non-adaptive", 12) == 0) {
>>>                vs->non_adaptive = true;
>>>            } else if (strncmp(options, "share=", 6) == 0) {
>>> @@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>            }
>>>        }
>>>
>>> +    /* adaptive updates are only used with tight encoding and
>>> +     * if lossy updates are enabled so we can disable all the
>>> +     * calculations otherwise */
>>> +    if (!vs->lossy) {
>>> +        vs->non_adaptive = true;
>>> +    }
>>> +
>>    The code seems: if vs->loosy == false, then vs->non_adaptive = true,
>> translate as: if loosy update is not used, then don't do adaptive
>> update., which doesn't conform with the comments. I am not sure if this
>> is on expectation.
> It don't see the logic break. The option means non_adaptive, not adaptive.
> 
> I write "adaptive updates are only used ... with lossy updates...". Which

  So tight encoding means loosy updates?

> is the same as "without lossy updates we don't need adaptive updates".
> 
> Peter
> 
>>
>>
>>>    #ifdef CONFIG_VNC_TLS
>>>        if (acl && x509 && vs->tls.x509verify) {
>>>            if (!(vs->tls.acl = qemu_acl_init("vnc.x509dname"))) {
>>>
> 
>
Peter Lieven Jan. 10, 2014, 10:28 p.m. UTC | #4 
Am 10.01.2014 04:09, schrieb Wenchao Xia:
> 于 2014/1/10 0:25, Peter Lieven 写道:
>> Am 09.01.2014 09:29, schrieb Wenchao Xia:
>>> 于 2014/1/8 17:08, Peter Lieven 写道:
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>    ui/vnc.c |    9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>>> index da552fe..a742d32 100644
>>>> --- a/ui/vnc.c
>>>> +++ b/ui/vnc.c
>>>> @@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>>                acl = 1;
>>>>    #endif
>>>>            } else if (strncmp(options, "lossy", 5) == 0) {
>>>> +#ifdef CONFIG_VNC_JPEG
>>>>                vs->lossy = true;
>>>> +#endif
>>>>            } else if (strncmp(options, "non-adaptive", 12) == 0) {
>>>>                vs->non_adaptive = true;
>>>>            } else if (strncmp(options, "share=", 6) == 0) {
>>>> @@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>>            }
>>>>        }
>>>>
>>>> +    /* adaptive updates are only used with tight encoding and
>>>> +     * if lossy updates are enabled so we can disable all the
>>>> +     * calculations otherwise */
>>>> +    if (!vs->lossy) {
>>>> +        vs->non_adaptive = true;
>>>> +    }
>>>> +
>>>    The code seems: if vs->loosy == false, then vs->non_adaptive = true,
>>> translate as: if loosy update is not used, then don't do adaptive
>>> update., which doesn't conform with the comments. I am not sure if this
>>> is on expectation.
>> It don't see the logic break. The option means non_adaptive, not adaptive.
>>
>> I write "adaptive updates are only used ... with lossy updates...". Which
>   So tight encoding means loosy updates?
It means you can only enable lossy updates if you have tight encoding. So if you are
missing tight encoding or lossy is false then you can set non_adaptive to true.

Peter
Wayne Xia Jan. 13, 2014, 2:42 a.m. UTC | #5 
于 2014/1/11 6:28, Peter Lieven 写道:
> Am 10.01.2014 04:09, schrieb Wenchao Xia:
>> 于 2014/1/10 0:25, Peter Lieven 写道:
>>> Am 09.01.2014 09:29, schrieb Wenchao Xia:
>>>> 于 2014/1/8 17:08, Peter Lieven 写道:
>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>> ---
>>>>>     ui/vnc.c |    9 +++++++++
>>>>>     1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>>>> index da552fe..a742d32 100644
>>>>> --- a/ui/vnc.c
>>>>> +++ b/ui/vnc.c
>>>>> @@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>>>                 acl = 1;
>>>>>     #endif
>>>>>             } else if (strncmp(options, "lossy", 5) == 0) {
>>>>> +#ifdef CONFIG_VNC_JPEG
>>>>>                 vs->lossy = true;
>>>>> +#endif
>>>>>             } else if (strncmp(options, "non-adaptive", 12) == 0) {
>>>>>                 vs->non_adaptive = true;
>>>>>             } else if (strncmp(options, "share=", 6) == 0) {
>>>>> @@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>>>             }
>>>>>         }
>>>>>
>>>>> +    /* adaptive updates are only used with tight encoding and
>>>>> +     * if lossy updates are enabled so we can disable all the
>>>>> +     * calculations otherwise */
>>>>> +    if (!vs->lossy) {
>>>>> +        vs->non_adaptive = true;
>>>>> +    }
>>>>> +
>>>>     The code seems: if vs->loosy == false, then vs->non_adaptive = true,
>>>> translate as: if loosy update is not used, then don't do adaptive
>>>> update., which doesn't conform with the comments. I am not sure if this
>>>> is on expectation.
>>> It don't see the logic break. The option means non_adaptive, not adaptive.
>>>
>>> I write "adaptive updates are only used ... with lossy updates...". Which
>>    So tight encoding means loosy updates?
> It means you can only enable lossy updates if you have tight encoding. So if you are
> missing tight encoding or lossy is false then you can set non_adaptive to true.
> 
> Peter
> 
  I see the logic, guess I punctuated the comments in a wrong way, the
real meaning may be:

    /* Adaptive updates are only used with tight encoding and
     * if lossy updates are enabled, so we can disable all the
     * calculations otherwise */
Peter Lieven Jan. 13, 2014, 8:27 a.m. UTC | #6 
On 13.01.2014 03:42, Wenchao Xia wrote:
> 于 2014/1/11 6:28, Peter Lieven 写道:
>> Am 10.01.2014 04:09, schrieb Wenchao Xia:
>>> 于 2014/1/10 0:25, Peter Lieven 写道:
>>>> Am 09.01.2014 09:29, schrieb Wenchao Xia:
>>>>> 于 2014/1/8 17:08, Peter Lieven 写道:
>>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>>> ---
>>>>>>      ui/vnc.c |    9 +++++++++
>>>>>>      1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>>>>> index da552fe..a742d32 100644
>>>>>> --- a/ui/vnc.c
>>>>>> +++ b/ui/vnc.c
>>>>>> @@ -3170,7 +3170,9 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>>>>                  acl = 1;
>>>>>>      #endif
>>>>>>              } else if (strncmp(options, "lossy", 5) == 0) {
>>>>>> +#ifdef CONFIG_VNC_JPEG
>>>>>>                  vs->lossy = true;
>>>>>> +#endif
>>>>>>              } else if (strncmp(options, "non-adaptive", 12) == 0) {
>>>>>>                  vs->non_adaptive = true;
>>>>>>              } else if (strncmp(options, "share=", 6) == 0) {
>>>>>> @@ -3187,6 +3189,13 @@ void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
>>>>>>              }
>>>>>>          }
>>>>>>
>>>>>> +    /* adaptive updates are only used with tight encoding and
>>>>>> +     * if lossy updates are enabled so we can disable all the
>>>>>> +     * calculations otherwise */
>>>>>> +    if (!vs->lossy) {
>>>>>> +        vs->non_adaptive = true;
>>>>>> +    }
>>>>>> +
>>>>>      The code seems: if vs->loosy == false, then vs->non_adaptive = true,
>>>>> translate as: if loosy update is not used, then don't do adaptive
>>>>> update., which doesn't conform with the comments. I am not sure if this
>>>>> is on expectation.
>>>> It don't see the logic break. The option means non_adaptive, not adaptive.
>>>>
>>>> I write "adaptive updates are only used ... with lossy updates...". Which
>>>     So tight encoding means loosy updates?
>> It means you can only enable lossy updates if you have tight encoding. So if you are
>> missing tight encoding or lossy is false then you can set non_adaptive to true.
>>
>> Peter
>>
>    I see the logic, guess I punctuated the comments in a wrong way, the
> real meaning may be:
>
>      /* Adaptive updates are only used with tight encoding and
>       * if lossy updates are enabled, so we can disable all the
>       * calculations otherwise */
>

Agreed. The comma there makes it clearer.

Peter
diff mbox

Patch

diff --git a/ui/vnc.c b/ui/vnc.c
index da552fe..a742d32 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3170,7 +3170,9 @@  void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
             acl = 1;
 #endif
         } else if (strncmp(options, "lossy", 5) == 0) {
+#ifdef CONFIG_VNC_JPEG
             vs->lossy = true;
+#endif
         } else if (strncmp(options, "non-adaptive", 12) == 0) {
             vs->non_adaptive = true;
         } else if (strncmp(options, "share=", 6) == 0) {
@@ -3187,6 +3189,13 @@  void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
         }
     }
 
+    /* adaptive updates are only used with tight encoding and
+     * if lossy updates are enabled so we can disable all the
+     * calculations otherwise */
+    if (!vs->lossy) {
+        vs->non_adaptive = true;
+    }
+
 #ifdef CONFIG_VNC_TLS
     if (acl && x509 && vs->tls.x509verify) {
         if (!(vs->tls.acl = qemu_acl_init("vnc.x509dname"))) {