Bug 44570

Summary: Not waiting for cached images that haven't been added to the cache yet
Product: Spice Reporter: Yonit Halperin <yhalperi>
Component: spice-gtkAssignee: Spice Bug List <spice-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: gio-coroutine: hide g_condition_wait_source, use GLib style convention
Create a GCoroutine, get rid of wait_queue
Remove the non-interruptible version g_io_wait()
Make g_coroutine_condition_wait() cancellable
Log if condition wait got cancelled
wait for cached images that haven't been added to the cache yet
Lower connection error from warning to debug, it's normal to fail
Hide g_condition_wait_source, use GLib style convention
Create a GCoroutine, get rid of wait_queue
Remove the non-interruptible version g_io_wait()
Make g_coroutine_condition_wait() cancellable
Log if condition wait got cancelled
wait for cached images that haven't been added to the cache yet

Description Yonit Halperin 2012-01-08 03:45:28 UTC
The images cache is shared among all the display channels (one channel per monitor). When there is more than one display channel, it is possible that display channel X will get a reference to a cached image M, that should be added to the cache by display channel Y. In the client side, display channel X might receive this reference before Y added M. Thus, if M is not found in the cache, X should wait for it.

In addition, the cache doesn't store the property of lossy/lossless for the images and does not assure the image is lossless when receiving SPICE_IMAGE_TYPE_FROM_CACHE_LOSSLESS. see channel-display.image_put_lossy, channel-display.image_replace_lossy and channel-display.image_get_lossless.
If an image in the cache is lossy, channel-display.image_get_lossless should wait till it is replaced with a lossless image.

Fixing this bug, together with the fix for bug 44179 should solve error msgs like: 
(../common/canvas_base.c:1157) canvas_get_image_internal: condition surface != NULL failed
(../common/canvas_base.c:2208) canvas_draw_copy: condition src_image != NULL failed

which occur when working with more than one monitor.
Comment 1 Marc-Andre Lureau 2012-01-09 09:41:45 UTC
Created attachment 55343 [details] [review]
gio-coroutine: hide g_condition_wait_source, use GLib style convention
Comment 2 Marc-Andre Lureau 2012-01-09 09:41:54 UTC
Created attachment 55344 [details] [review]
Create a GCoroutine, get rid of wait_queue
Comment 3 Marc-Andre Lureau 2012-01-09 09:42:02 UTC
Created attachment 55345 [details] [review]
Remove the non-interruptible version g_io_wait()

Use the common g_coroutine_socket_wait()
Comment 4 Marc-Andre Lureau 2012-01-09 09:42:12 UTC
Created attachment 55346 [details] [review]
Make g_coroutine_condition_wait() cancellable
Comment 5 Marc-Andre Lureau 2012-01-09 09:42:25 UTC
Created attachment 55347 [details] [review]
Log if condition wait got cancelled
Comment 6 Marc-Andre Lureau 2012-01-09 09:42:34 UTC
Created attachment 55348 [details] [review]
wait for cached images that haven't been added to the cache yet
Comment 7 Marc-Andre Lureau 2012-01-09 09:47:11 UTC
Comment on attachment 55348 [details] [review]
wait for cached images that haven't been added to the cache yet

Review of attachment 55348 [details] [review]:
-----------------------------------------------------------------

::: gtk/channel-display.c
@@ +438,5 @@
> +        .id = id,
> +        .image = NULL
> +    };
> +    if (!g_coroutine_condition_wait(g_coroutine_self(), wait_image, &wait))
> +        SPICE_DEBUG("wait lossless got cancelled");

I need to update that log
Comment 8 Yonit Halperin 2012-01-12 00:21:29 UTC
Comment on attachment 55343 [details] [review]
gio-coroutine: hide g_condition_wait_source, use GLib style convention

Review of attachment 55343 [details] [review]:
-----------------------------------------------------------------

Ack
Comment 9 Yonit Halperin 2012-01-12 00:32:05 UTC
Comment on attachment 55344 [details] [review]
Create a GCoroutine, get rid of wait_queue

Review of attachment 55344 [details] [review]:
-----------------------------------------------------------------

Ack + small comments

::: gtk/gio-coroutine.c
@@ +64,2 @@
>      g_return_val_if_fail(sock != NULL, 0);
>  

add g_return_val_if_fail(&self->coroutine == coroutine_self) ?

@@ +80,4 @@
>          return *ret;
>  }
>  
> +void g_io_wakeup(GCoroutine *self)

"self" is a bit confusing here
Comment 10 Yonit Halperin 2012-01-12 00:47:02 UTC
Comment on attachment 55345 [details] [review]
Remove the non-interruptible version g_io_wait()

Review of attachment 55345 [details] [review]:
-----------------------------------------------------------------

Hi,
I have a question that is related to this patch and the next patch as well:
Do we always want to cancel waiting when wakeup is called? We surely want to cancel waiting when we destroy the session,
but there is also a wakup call on spice-channel.spice_msg_out_send, which I'm not sure if we want it to cancel waiting.
Comment 11 Yonit Halperin 2012-01-12 00:50:30 UTC
Comment on attachment 55347 [details] [review]
Log if condition wait got cancelled

Review of attachment 55347 [details] [review]:
-----------------------------------------------------------------

::: gtk/spice-channel.c
@@ +1867,5 @@
>      GIOCondition ret;
>  
>      do {
> +        if (!g_coroutine_condition_wait(&c->coroutine, wait_migration, channel))
> +            SPICE_DEBUG("migration wait cancelled");

was "if (c->state == SPICE_CHANNEL_STATE_MIGRATING)" removed intentionally?
Comment 12 Yonit Halperin 2012-01-12 00:52:42 UTC
Comment on attachment 55347 [details] [review]
Log if condition wait got cancelled

Review of attachment 55347 [details] [review]:
-----------------------------------------------------------------

Ignore the previous comment. Ack
Comment 13 Marc-Andre Lureau 2012-01-12 03:57:47 UTC
Comment on attachment 55344 [details] [review]
Create a GCoroutine, get rid of wait_queue

Review of attachment 55344 [details] [review]:
-----------------------------------------------------------------

::: gtk/gio-coroutine.c
@@ +64,2 @@
>      g_return_val_if_fail(sock != NULL, 0);
>  

I think it's a sane assumption. I just thought that technically there is no limitation in managing/scheduling another coroutine, if you know what you are doing...

@@ +80,4 @@
>          return *ret;
>  }
>  
> +void g_io_wakeup(GCoroutine *self)

In the header, we prefer using the name of the object (coroutine here), as "self" is more an implementation detail for current object. "self" doesn't really exist outside of the method.

that's more by convention..
Comment 14 Marc-Andre Lureau 2012-01-12 04:04:08 UTC
Comment on attachment 55345 [details] [review]
Remove the non-interruptible version g_io_wait()

Review of attachment 55345 [details] [review]:
-----------------------------------------------------------------

Yeah, I think we want to be a bit more careful here.

So I propose adding g_coroutine_condition_cancel() to cancel the condition, complementary to g_coroutine_wakeup() which will switch to specified coroutine. ok with that?
Comment 15 Marc-Andre Lureau 2012-01-12 10:43:39 UTC
Created attachment 55512 [details] [review]
Lower connection error from warning to debug, it's normal to fail

We try several connections, so it's normal to fail for some.
No need to warn.
Comment 16 Marc-Andre Lureau 2012-01-12 10:43:44 UTC
Created attachment 55513 [details] [review]
Hide g_condition_wait_source, use GLib style convention
Comment 17 Marc-Andre Lureau 2012-01-12 10:43:48 UTC
Created attachment 55514 [details] [review]
Create a GCoroutine, get rid of wait_queue
Comment 18 Marc-Andre Lureau 2012-01-12 10:43:53 UTC
Created attachment 55515 [details] [review]
Remove the non-interruptible version g_io_wait()

Use the common g_coroutine_socket_wait()
Comment 19 Marc-Andre Lureau 2012-01-12 10:43:57 UTC
Created attachment 55516 [details] [review]
Make g_coroutine_condition_wait() cancellable
Comment 20 Marc-Andre Lureau 2012-01-12 10:44:04 UTC
Created attachment 55517 [details] [review]
Log if condition wait got cancelled
Comment 21 Marc-Andre Lureau 2012-01-12 10:44:09 UTC
Created attachment 55518 [details] [review]
wait for cached images that haven't been added to the cache yet
Comment 22 Hans de Goede 2012-01-13 05:40:49 UTC
Comment on attachment 55512 [details] [review]
Lower connection error from warning to debug, it's normal to fail

Review of attachment 55512 [details] [review]:
-----------------------------------------------------------------

Ack.
Comment 23 Hans de Goede 2012-01-13 05:42:08 UTC
Comment on attachment 55513 [details] [review]
Hide g_condition_wait_source, use GLib style convention

Review of attachment 55513 [details] [review]:
-----------------------------------------------------------------

Ack.
Comment 24 Hans de Goede 2012-01-13 05:45:33 UTC
Comment on attachment 55514 [details] [review]
Create a GCoroutine, get rid of wait_queue

Review of attachment 55514 [details] [review]:
-----------------------------------------------------------------

Ack.
Comment 25 Hans de Goede 2012-01-13 05:53:48 UTC
Comment on attachment 55515 [details] [review]
Remove the non-interruptible version g_io_wait()

Review of attachment 55515 [details] [review]:
-----------------------------------------------------------------

Ack.
Comment 26 Hans de Goede 2012-01-13 06:04:13 UTC
Comment on attachment 55516 [details] [review]
Make g_coroutine_condition_wait() cancellable

Review of attachment 55516 [details] [review]:
-----------------------------------------------------------------

Ack.
Comment 27 Hans de Goede 2012-01-13 06:05:09 UTC
Comment on attachment 55517 [details] [review]
Log if condition wait got cancelled

Review of attachment 55517 [details] [review]:
-----------------------------------------------------------------

Ack.
Comment 28 Hans de Goede 2012-01-13 06:07:06 UTC
Comment on attachment 55518 [details] [review]
wait for cached images that haven't been added to the cache yet

Review of attachment 55518 [details] [review]:
-----------------------------------------------------------------

Ack.
Comment 29 Marc-Andre Lureau 2012-01-16 02:26:22 UTC
all pushed in v0.8

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.