DamageSubtract is misdesigned, making DeltaRectangles mode unusable
Submitted by Nathaniel J. Smith @njs
Assigned to Keith Packard @keithp
Description
damageproto.txt, when describing the DamageSubtract request, states:
DamageSubtract
[...]
Synchronously modifies the regions in the following manner:
(A) If repair is None:
1) parts = damage
2) damage = <empty>
(B) Otherwise:
1) parts = damage INTERSECT repair
2) damage = damage - parts
3) Generate DamageNotify for remaining damage areas
(Letters added for ease of reference.) My issue is with point B.3.
In my application[1], I request damage mode DamageReportDeltaRectangles. I accumulate these damage notifications in a GdkRegion in my client, and from time to time a separate socket becomes writeable and I pick a rectangle from my Region, subtract it from the server damage region (to indicate that I would like to re-enable damage notification for that rectangle), and send the image of that rectangle down my socket. This all works nicely, except for B.3: every time I re-enable damage notifications on one rectangle with DamageSubtract, I get a giant flurry of pointless notifications for damage rectangles I have already recorded. Then I handle the next rectangle, and get another flurry... ultimately this makes a simple redraw operation O(n^2), and in real-world cases can cause my program to thrash for several minutes to deliver a single screen update.
The only way DeltaRectangles mode makes any sense to use at all is if one has this basic strategy of accumulating damage locally and issuing DamageSubtract when a piece of it is handled. Given such a strategy, re-issuing notifications is utterly pointless.
A simple but inefficient workaround is to use mode RawRectangles; for this mode the current X server violates the spec and makes DamageSubtract a complete no-op.
I am not entirely certain what the right fix here is: Option 1: Stop re-issuing notifications, as per B.3, for damage level DeltaRectangles. Option 2: Just kill B.3 entirely -- never re-issue damage notifications.
Either of these will solve my problem, obviously; the question is which is cleaner, and in particular whether there is any use case where the re-issued damage notifications are valuable. They seem to be in the spec entirely for the benefit of mode NonEmpty, but I can't figure out how one could use them without race conditions. If one is using notification mode NonEmpty, one should be setting repair=None anyway, and avoiding this whole problem... Maybe Keith or Eric, whose names are on the spec, can explain the rationale?
Also, looking through all the different uses of DamageSubtract in google codesearch, it appears that they fall into two classes: -- traditional cm's, which use NonEmpty and set repair=None, so they take branch A and B.3 is irrelevant. -- apps that want to keep track of what's on the screen (like xpra, and byzanz, and vino), which use DeltaRectangles and are bitten by this bug.
So just removing damage re-notifications entirely seems like a viable course of action.
Anyway, what I'd request: -- damage re-issuing be fixed for, at least, DeltaRectangles mode for 7.4 -- the Damage protocol version get bumped to 1.2 so that apps can tell that DeltaRectangles is usable Let me know if there's anything I can do to help this occur.
[1] http://partiwm.org/wiki/xpra
Version: 7.3 (2007.09)