fix: WSurfaceItemContent buffer lifecycle management#815
Open
Dami-star wants to merge 1 commit intolinuxdeepin:masterfrom
Open
fix: WSurfaceItemContent buffer lifecycle management#815Dami-star wants to merge 1 commit intolinuxdeepin:masterfrom
Dami-star wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Dami-star The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideManages wlroots client buffer lock accounting by introducing adjustClientBufferIgnoreLocks() and applying it consistently when acquiring, releasing, and swapping WSurfaceItemContent buffers so wlroots can correctly reuse client buffers instead of constantly recreating them. Sequence diagram for updated buffer lock and ignore lock managementsequenceDiagram
actor Client
participant WSurfaceItemContent
participant WSurfaceItemContentPrivate as ContentPriv
participant Buffer as qw_buffer
participant ClientBuffer as qw_client_buffer
participant Handle as ClientBufferHandle
participant WlrootsDamage as wlr_client_buffer_apply_damage
Client->>WSurfaceItemContent: commit surface with newBuffer
WSurfaceItemContent->>ContentPriv: updateSurface(newBuffer)
alt live mode and buffer changed
ContentPriv->>Buffer: lock()
ContentPriv->>ClientBuffer: get(Buffer)
ClientBuffer-->>ContentPriv: cb
ContentPriv->>Handle: adjustClientBufferIgnoreLocks(Buffer, +1)
Note over Handle: n_ignore_locks++
ContentPriv->>Handle: adjustClientBufferIgnoreLocks(old buffer, -1)
Note over Handle: n_ignore_locks--
ContentPriv->>ContentPriv: buffer.reset(newBuffer)
else non live mode and pendingBuffer changed
ContentPriv->>Buffer: lock()
ContentPriv->>ClientBuffer: get(Buffer)
ClientBuffer-->>ContentPriv: cb
ContentPriv->>Handle: adjustClientBufferIgnoreLocks(Buffer, +1)
ContentPriv->>Handle: adjustClientBufferIgnoreLocks(old pendingBuffer, -1)
ContentPriv->>ContentPriv: pendingBuffer.reset(newBuffer)
end
Client->>WSurfaceItemContent: later frame display / damage apply
WSurfaceItemContent->>WlrootsDamage: apply_damage(client_buffer)
WlrootsDamage->>Handle: read n_locks, n_ignore_locks
WlrootsDamage-->>WlrootsDamage: effective_locks = n_locks - n_ignore_locks
WlrootsDamage-->>Client: buffer reused when effective_locks == 1
Client->>WSurfaceItemContent: item destroyed or buffer dropped
WSurfaceItemContent->>ContentPriv: destructor / dontCacheLastBuffer / swapBufferIfNeeded
ContentPriv->>Handle: adjustClientBufferIgnoreLocks(buffer, -1)
Note over Handle: n_ignore_locks--
ContentPriv->>ContentPriv: buffer.reset() (unique_ptr deleter calls unlock())
Class diagram for updated WSurfaceItemContent buffer lifecycleclassDiagram
class WSurfaceItemContent {
}
class WSurfaceItemContentPrivate {
+~WSurfaceItemContentPrivate()
+cleanTextureProvider()
+dontCacheLastBuffer : bool
+live : bool
+buffer : qw_buffer_ptr
+pendingBuffer : qw_buffer_ptr
+swapBufferIfNeeded()
+updateSurface(newBuffer : qw_buffer)
+adjustClientBufferIgnoreLocks(buf : qw_buffer, delta : int) static
}
class qw_buffer {
+lock()
+unlock()
}
class qw_client_buffer {
+get(buf : qw_buffer) qw_client_buffer
+handle() ClientBufferHandle
}
class ClientBufferHandle {
+n_locks : int
+n_ignore_locks : int
}
WSurfaceItemContent "1" o-- "1" WSurfaceItemContentPrivate
WSurfaceItemContentPrivate --> qw_buffer : buffer
WSurfaceItemContentPrivate --> qw_buffer : pendingBuffer
qw_client_buffer --> qw_buffer : wraps
qw_client_buffer --> ClientBufferHandle : handle
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Root cause: WSurfaceItemContent held buffer locks but didn't manage n_ignore_locks, causing wlr_client_buffer_apply_damage to see effective_locks > 1 and refuse to reuse buffers. This led to creating new client_buffer on every commit, triggering frequent buffer destruction and crashes in: - assert(buffer->n_locks > 0) in wlr_buffer_unlock - heap corruption in Mesa/OpenGL texture cleanup - assert(!buffer->accessing_data_ptr) in SHM buffers Solution: Added adjustClientBufferIgnoreLocks() helper to manage n_ignore_locks symmetrically with n_locks: - Increment n_ignore_locks when acquiring buffer (alongside lock()) - Decrement n_ignore_locks when releasing buffer (alongside unlock()) - Handle n_ignore_locks in destructor , dontCacheLastBuffer, and swapBufferIfNeeded() This allows wlroots to correctly calculate effective_locks and reuse buffers, eliminating the frequent creation/destruction cycle.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The destructor unconditionally calls
adjustClientBufferIgnoreLocks(..., -1)onbufferandpendingBuffer, but those same pointers may already have been decremented indontCacheLastBuffer()/swapBufferIfNeeded(), which risks drivingn_ignore_locksnegative or otherwise unbalanced; consider centralizing the lifetime management so each lock/unlock pair has exactly one corresponding ignore-lock adjustment. - To guard against subtle misuse of
adjustClientBufferIgnoreLocks, it might be safer to add assertions or defensive checks (e.g. thatn_ignore_locksdoes not underflow when applying a negative delta) so issues are caught early rather than silently corrupting wlroots’ lock accounting. - Since
lock()/unlock()andn_ignore_locksadjustments always need to stay in sync, consider wrapping them in small helper functions or an RAII type instead of manually duplicating thelock()+adjust(..., +1)andadjust(..., -1)patterns in multiple branches.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The destructor unconditionally calls `adjustClientBufferIgnoreLocks(..., -1)` on `buffer` and `pendingBuffer`, but those same pointers may already have been decremented in `dontCacheLastBuffer()` / `swapBufferIfNeeded()`, which risks driving `n_ignore_locks` negative or otherwise unbalanced; consider centralizing the lifetime management so each lock/unlock pair has exactly one corresponding ignore-lock adjustment.
- To guard against subtle misuse of `adjustClientBufferIgnoreLocks`, it might be safer to add assertions or defensive checks (e.g. that `n_ignore_locks` does not underflow when applying a negative delta) so issues are caught early rather than silently corrupting wlroots’ lock accounting.
- Since `lock()`/`unlock()` and `n_ignore_locks` adjustments always need to stay in sync, consider wrapping them in small helper functions or an RAII type instead of manually duplicating the `lock()` + `adjust(..., +1)` and `adjust(..., -1)` patterns in multiple branches.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
codedump: #659 (comment) |
zccrs
requested changes
Apr 2, 2026
| } | ||
|
|
||
| // Adjust n_ignore_locks to allow wlr_client_buffer_apply_damage to reuse client buffers. | ||
| static void adjustClientBufferIgnoreLocks(qw_buffer *buf, int delta) { |
Member
There was a problem hiding this comment.
这样做不好维护，加一个新的struct或class，定位为干净卫生的持有一个wlr buffer，在构造里支持自动lock且更新 n_ignore_locks，析构时自动unlock和更新 n_ignore_locks。此外支持reset可以重新设置一个新的对象持有。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause:
WSurfaceItemContent held buffer locks but didn't manage n_ignore_locks, causing wlr_client_buffer_apply_damage to see effective_locks > 1 and refuse to reuse buffers. This led to creating new client_buffer on every commit, triggering frequent buffer destruction and crashes in:
Solution:
Added adjustClientBufferIgnoreLocks() helper to manage n_ignore_locks symmetrically with n_locks:
This allows wlroots to correctly calculate effective_locks and reuse buffers, eliminating the frequent creation/destruction cycle.
Summary by Sourcery
Fix client buffer lifecycle handling in WSurfaceItemContent to keep wlroots buffer lock counters consistent and allow buffer reuse.
Bug Fixes:
Enhancements: