[Spice-devel] [RFC PATCH spice 0.8 18/19] client: display channel migration
Yonit Halperin
yhalperi at redhat.com
Tue Sep 20 11:24:38 PDT 2011
On 09/20/2011 07:12 PM, Alon Levy wrote:
> On Mon, Sep 19, 2011 at 12:47:11PM +0300, Yonit Halperin wrote:
>>
>
> Some nitpicks. One question on the added touch_context at the end.
>
>> Signed-off-by: Yonit Halperin<yhalperi at redhat.com>
>> ---
>> client/display_channel.cpp | 158 ++++++++++++++++++++++++++++++++++++++++----
>> client/display_channel.h | 12 +++-
>> client/red_gl_canvas.cpp | 1 +
>> 3 files changed, 155 insertions(+), 16 deletions(-)
>>
>> diff --git a/client/display_channel.cpp b/client/display_channel.cpp
>> index a757183..aa4b52b 100644
>> --- a/client/display_channel.cpp
>> +++ b/client/display_channel.cpp
>> @@ -84,6 +84,28 @@ private:
>> DisplayChannel& _channel;
>> };
>>
>> +class DestroyAllSurfacesEvent: public SyncEvent {
>> +public:
>> + DestroyAllSurfacesEvent(DisplayChannel& channel, bool include_primary = true)
>> + : _channel(channel)
>> + , _include_primary(include_primary)
>> + {
>> + }
>> +
>> + virtual void do_response(AbstractProcessLoop& events_loop)
>> + {
>> + if (_include_primary) {
>> + _channel.do_destroy_all_surfaces();
>> + } else {
>> + _channel.do_destroy_off_screen_surfaces();
>> + }
>> + }
>> +
>> +private:
>> + DisplayChannel& _channel;
>> + bool _include_primary;
>> +};
>> +
>> class CreateSurfaceEvent: public SyncEvent {
>> public:
>> CreateSurfaceEvent(DisplayChannel& channel, int surface_id, int width, int height,
>> @@ -385,7 +407,6 @@ bool VideoStream::is_time_to_display(uint32_t now, uint32_t frame_time)
>> void VideoStream::maintenance()
>> {
>> uint32_t mm_time = _client.get_mm_time();
>> -
>
> Should leave this line.
>
>> remove_dead_frames(mm_time);
>> if (!_update_mark&& !_update_time&& _frames_head != _frames_tail) {
>> VideoFrame* tail =&_frames[frame_slot(_frames_tail)];
>> @@ -412,6 +433,7 @@ void VideoStream::maintenance()
>> }
>> free_frame(_frames_tail++);
>> }
>> +
>
> Should not add this line.
>
>> }
>>
>> uint32_t VideoStream::handle_timer_update(uint32_t now)
>> @@ -544,6 +566,21 @@ void ResetTimer::response(AbstractProcessLoop& events_loop)
>> _client.deactivate_interval_timer(this);
>> }
>>
>> +#define MIGRATION_PRIMARY_SURFACE_TIMEOUT (1000 * 5)
>> +
>> +class MigPrimarySurfaceTimer: public Timer {
>> +public:
>> + virtual void response(AbstractProcessLoop& events_loop)
>> + {
>> + DisplayChannel *channel = static_cast<DisplayChannel*>(events_loop.get_owner());
>> + if (channel->_mig_wait_primary) {
>> + channel->destroy_primary_surface();
>> + channel->_mig_wait_primary = false;
>> + }
>> + channel->get_process_loop().deactivate_interval_timer(this);
>
> Whitespace.
>
>> + }
>> +};
>> +
>> class DisplayHandler: public MessageHandlerImp<DisplayChannel, SPICE_CHANNEL_DISPLAY> {
>> public:
>> DisplayHandler(DisplayChannel& channel)
>> @@ -618,11 +655,11 @@ DisplayChannel::~DisplayChannel()
>> screen()->set_update_interrupt_trigger(NULL);
>> }
>>
>> - //destroy_canvas(); fixme destroy all
>> - destroy_strams();
>> + destroy_streams();
>> + do_destroy_all_surfaces();
>> }
>>
>> -void DisplayChannel::destroy_strams()
>> +void DisplayChannel::destroy_streams()
>> {
>> Lock lock(_streams_lock);
>> for (unsigned int i = 0; i< _streams.size(); i++) {
>> @@ -1021,6 +1058,75 @@ void DisplayChannel::on_disconnect()
>> (*sync_event)->wait();
>> }
>>
>> +void DisplayChannel::do_destroy_all_surfaces()
>> +{
>> + SurfacesCache::iterator s_iter;
>> +
>> + for (s_iter = _surfaces_cache.begin(); s_iter != _surfaces_cache.end(); s_iter++) {
>> + delete (*s_iter).second;
>> + }
>> + _surfaces_cache.clear();
>> +}
>> +
>> +void DisplayChannel::do_destroy_off_screen_surfaces()
>> +{
>> + SurfacesCache::iterator s_iter;
>> + Canvas *primary_canvas = NULL;
>> +
>> + for (s_iter = _surfaces_cache.begin(); s_iter != _surfaces_cache.end(); s_iter++) {
>> + if (s_iter->first == 0) {
>> + primary_canvas = s_iter->second;
>> + } else {
>> + delete s_iter->second;
>> + }
>> + }
>> + _surfaces_cache.clear();
>> + if (primary_canvas) {
>> + _surfaces_cache[0] = primary_canvas;
>> + }
>> +}
>> +
>> +void DisplayChannel::destroy_all_surfaces()
>> +{
>> + AutoRef<DestroyAllSurfacesEvent> destroy_event(new DestroyAllSurfacesEvent(*this));
>> +
>> + get_client().push_event(*destroy_event);
>> + (*destroy_event)->wait();
>> + if (!(*destroy_event)->success()) {
>> + THROW("destroy all surfaces failed");
>> + }
>> +}
>> +
>> +void DisplayChannel::destroy_off_screen_surfaces()
>> +{
>> + AutoRef<DestroyAllSurfacesEvent> destroy_event(new DestroyAllSurfacesEvent(*this, false));
>> +
>> + get_client().push_event(*destroy_event);
>> + (*destroy_event)->wait();
>> + if (!(*destroy_event)->success()) {
>> + THROW("destroy all surfaces failed");
>> + }
>> +}
>> +
>> +void DisplayChannel::on_disconnect_mig_src()
>> +{
>> + _palette_cache.clear();
>> + destroy_streams();
>> + if (screen()) {
>> + screen()->set_update_interrupt_trigger(NULL);
>> + }
>> + _update_mark = 0;
>> + _next_timer_time = 0;
>> + get_client().deactivate_interval_timer(*_streams_timer);
>> + destroy_off_screen_surfaces();
>> + // Not clrearing the primary surface till we receive a new one (or a timeout).
> typo, clearing.
>
>> + if (_surfaces_cache.exist(0)) {
>> + AutoRef<MigPrimarySurfaceTimer> mig_timer(new MigPrimarySurfaceTimer());
>> + get_process_loop().activate_interval_timer(*mig_timer, MIGRATION_PRIMARY_SURFACE_TIMEOUT);
>> + _mig_wait_primary = true;
>> + }
>> +}
>> +
>> bool DisplayChannel::create_sw_canvas(int surface_id, int width, int height, uint32_t format)
>> {
>> try {
>> @@ -1355,26 +1461,50 @@ void DisplayChannel::handle_stream_destroy(RedPeer::InMessage* message)
>>
>> void DisplayChannel::handle_stream_destroy_all(RedPeer::InMessage* message)
>> {
>> - destroy_strams();
>> + destroy_streams();
>> }
>>
>> void DisplayChannel::create_primary_surface(int width, int height, uint32_t format)
>> {
>> + bool do_create_primary = true;
>> #ifdef USE_OGL
>> - Canvas *canvas;
>> + Canvas *canvas;
>> #endif
>> - _mark = false;
>> - attach_to_screen(get_client().get_application(), get_id());
>> - clear_area();
>> + _mark = false;
>> +
>> + /*
>> + * trying to avoid artifacts when the display hasn't changed much
>> + * between the disconnection from the migration src and the
>> + * connection to the target.
>> + */
>> + if (_mig_wait_primary) {
>> + ASSERT(_surfaces_cache.exist(0));
>> + if (_x_res != width || _y_res != height || format != format) {
>> + LOG_INFO("destroy the primary surface of the mig src session");
>> + destroy_primary_surface();
>> + } else {
>> + LOG_INFO("keep the primary surface of the mig src session");
>> + _surfaces_cache[0]->clear();
>> + clear_area();
>> + do_create_primary = false;
>> + }
>> + }
>
> whitespace at the end of the line.
>
>>
>> - AutoRef<CreatePrimarySurfaceEvent> event(new CreatePrimarySurfaceEvent(*this, width, height,
>> + if (do_create_primary) {
>> + LOG_INFO("");
>> + attach_to_screen(get_client().get_application(), get_id());
>> + clear_area();
>> + AutoRef<CreatePrimarySurfaceEvent> event(new CreatePrimarySurfaceEvent(*this, width, height,
>> format));
>> - get_client().push_event(*event);
>> - (*event)->wait();
>> - if (!(*event)->success()) {
>> - THROW("Create primary surface failed");
>> + get_client().push_event(*event);
>> + (*event)->wait();
>> + if (!(*event)->success()) {
>> + THROW("Create primary surface failed");
>> + }
>> }
>>
>> + _mig_wait_primary = false;
>> +
>> _x_res = width;
>> _y_res = height;
>> _format = format;
>> diff --git a/client/display_channel.h b/client/display_channel.h
>> index cdad5ff..755bf25 100644
>> --- a/client/display_channel.h
>> +++ b/client/display_channel.h
>> @@ -114,6 +114,7 @@ public:
>> protected:
>> virtual void on_connect();
>> virtual void on_disconnect();
>> + virtual void on_disconnect_mig_src();
>>
>> private:
>> void set_draw_handlers();
>> @@ -129,13 +130,17 @@ private:
>> void destroy_canvas(int surface_id);
>> void create_canvas(int surface_id, const std::vector<int>& canvas_type, int width, int height,
>> uint32_t format);
>> - void destroy_strams();
>> + void destroy_streams();
>> void update_cursor();
>>
>> void create_primary_surface(int width, int height, uint32_t format);
>> void create_surface(int surface_id, int width, int height, uint32_t format);
>> void destroy_primary_surface();
>> void destroy_surface(int surface_id);
>> + void destroy_all_surfaces();
>> + void do_destroy_all_surfaces();
>> + void destroy_off_screen_surfaces();
>> + void do_destroy_off_screen_surfaces();
>>
>> void handle_mode(RedPeer::InMessage* message);
>> void handle_mark(RedPeer::InMessage* message);
>> @@ -217,11 +222,13 @@ private:
>> #endif
>> InterruptUpdate _interrupt_update;
>>
>> + bool _mig_wait_primary;
>> friend class SetModeEvent;
>> friend class CreatePrimarySurfaceEvent;
>> - friend class DestroyPrimarySurfaceEvent;
>> + friend class DestroyPrimarySurfaceEvent;
> Whitespace at the end of the line.
>
>> friend class CreateSurfaceEvent;
>> friend class DestroySurfaceEvent;
>> + friend class DestroyAllSurfacesEvent;
>> friend class ActivateTimerEvent;
>> friend class VideoStream;
>> friend class StreamsTrigger;
>> @@ -229,6 +236,7 @@ private:
>> friend class StreamsTimer;
>> friend class AttachChannelsEvent;
>> friend class DetachChannelsEvent;
>> + friend class MigPrimarySurfaceTimer;
>> };
>>
>> #endif
>> diff --git a/client/red_gl_canvas.cpp b/client/red_gl_canvas.cpp
>> index e2bff7f..78bf272 100644
>> --- a/client/red_gl_canvas.cpp
>> +++ b/client/red_gl_canvas.cpp
>> @@ -49,6 +49,7 @@ GCanvas::GCanvas(int width, int height, uint32_t format, RedWindow *win,
>>
>> GCanvas::~GCanvas()
>> {
>> + touch_context();
>
> Huh?
>
Wrong patch.
In the patch "client: rewrite surfaces cache" I removed from
DisplayChannel::destroy_canvas:
-#ifdef USE_OGL
- if (canvas->get_pixmap_type() == CANVAS_TYPE_GL) {
- ((GCanvas *)(canvas))->touch_context();
- }
-#endif
and instead I added the touch_context to the GCanavs dtor. However, I'm
rethinking this, since it will not affect only the surfaces' canvases,
but also other GCanvases. Do you know what this call is for? Do we need
to call it also when we destroy all the surfaces (when disconnecting
from the migration src)?
Thanks for the comments,
Yonit
>> gl_canvas_set_textures_lost (_canvas, (int)_textures_lost);
>> _canvas->ops->destroy(_canvas);
>> _canvas = NULL;
>> --
>> 1.7.4.4
>>
More information about the Spice-devel
mailing list