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.
Created attachment 55343 [details] [review] gio-coroutine: hide g_condition_wait_source, use GLib style convention
Created attachment 55344 [details] [review] Create a GCoroutine, get rid of wait_queue
Created attachment 55345 [details] [review] Remove the non-interruptible version g_io_wait() Use the common g_coroutine_socket_wait()
Created attachment 55346 [details] [review] Make g_coroutine_condition_wait() cancellable
Created attachment 55347 [details] [review] Log if condition wait got cancelled
Created attachment 55348 [details] [review] wait for cached images that haven't been added to the cache yet
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 on attachment 55343 [details] [review] gio-coroutine: hide g_condition_wait_source, use GLib style convention Review of attachment 55343 [details] [review]: ----------------------------------------------------------------- Ack
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 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 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 on attachment 55347 [details] [review] Log if condition wait got cancelled Review of attachment 55347 [details] [review]: ----------------------------------------------------------------- Ignore the previous comment. Ack
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 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?
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.
Created attachment 55513 [details] [review] Hide g_condition_wait_source, use GLib style convention
Created attachment 55514 [details] [review] Create a GCoroutine, get rid of wait_queue
Created attachment 55515 [details] [review] Remove the non-interruptible version g_io_wait() Use the common g_coroutine_socket_wait()
Created attachment 55516 [details] [review] Make g_coroutine_condition_wait() cancellable
Created attachment 55517 [details] [review] Log if condition wait got cancelled
Created attachment 55518 [details] [review] wait for cached images that haven't been added to the cache yet
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 on attachment 55513 [details] [review] Hide g_condition_wait_source, use GLib style convention Review of attachment 55513 [details] [review]: ----------------------------------------------------------------- Ack.
Comment on attachment 55514 [details] [review] Create a GCoroutine, get rid of wait_queue Review of attachment 55514 [details] [review]: ----------------------------------------------------------------- Ack.
Comment on attachment 55515 [details] [review] Remove the non-interruptible version g_io_wait() Review of attachment 55515 [details] [review]: ----------------------------------------------------------------- Ack.
Comment on attachment 55516 [details] [review] Make g_coroutine_condition_wait() cancellable Review of attachment 55516 [details] [review]: ----------------------------------------------------------------- Ack.
Comment on attachment 55517 [details] [review] Log if condition wait got cancelled Review of attachment 55517 [details] [review]: ----------------------------------------------------------------- Ack.
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.
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.