xpra icon
Bug tracker and wiki

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#1431 closed defect (invalid)

pixel data race condition - unnecessary copy?

Reported by: Antoine Martin Owned by: Antoine Martin
Priority: critical Milestone: 2.0
Component: server Version: trunk
Keywords: Cc:

Description

During testing for #1423, it seemed possible for the window pixels data to go away from underneath us whilst in the encode thread.

r14991 tries to fix this by always making copies of the pixel data when we "process_damage_region", which we already did for freezing video frames that get delayed for av-sync.

In 99% of the non-av-sync cases, this is a complete waste of CPU cycles because there will be no concurrent access and destruction.

Assuming this is really needed (not 100% sure yet), we should be able to replace this with a simple lock and either wait for the lock to be freed to free the xshm backing (potentially stalling the UI thread..) or just defer it. ("dealloc" should be enough).

Change History (2)

comment:1 Changed 3 years ago by Antoine Martin

Resolution: invalid
Status: newclosed

Damn, I can no longer reproduce the crash - which must have been caused by something else (a bug in #1423 perhaps)

So r15064 reverts this change and this will need to be backported too.

comment:2 Changed 3 years ago by Antoine Martin

More details on why this is safe as it is:

  • free_image_wrapper is only called in make_data_packet_cb after the encoder has finished using the pixel data - we hold a reference to the image object the whole time
  • the refcounting is done using a cython int (this is well tested in xshm image)

Sub-images (used by the scrolling code) are also safe as the encoder for each sub-image is called from within the encoding function for the parent image (which owns the pixel memory).

Note: See TracTickets for help on using tickets.