[crd host][linux] Fix issues with DesktopCapturerProxy [chromium/src : main]
0 views
Skip to first unread message
Yuwei Huang (Gerrit)
Feb 19, 2026, 3:38:16 AM (3 days ago) Feb 19
to Joe Downing, chromium...@chromium.org, chromotin...@chromium.org
Attention needed from Joe Downing
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Joe Downing
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Feb 19, 2026, 10:12:04 AM (3 days ago) Feb 19
to Yuwei Huang, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org
Attention needed from Yuwei Huang
Joe Downing voted and added 3 comments![]( "Open in Gerrit")
Votes added by Joe Downing
| Code-Review | +1 |
3 comments
File remoting/host/desktop_capturer_proxy.cc
Line 69, Patchset 5 (Latest):
void RunCapturerTask(R (webrtc::DesktopCapturer::*method)(Args...),Joe Downing . unresolved
Can you add a comment on this method to indicate why it is useful, essentially that it allows callers to set capture params before the underlying capturer has been created.
Line 75, Patchset 5 (Latest):
bool capturer_set_ = false;Joe Downing . unresolved
Can you continue to check whether `capturer_` is null rather than introduce the `capturer_set_` member? I don't think it's valid to to set capturer_ to null via the setter and it looks like capturer_ is not released before desctruction.
Line 188, Patchset 5 (Latest):
(capturer_.get()->*method)(std::forward<Args>(args)...);Joe Downing . unresolved
Is `capturer_->*method` valid here? Or is .get() required due to templating?
Related details
Attention is currently required from:
- Yuwei Huang
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Feb 19, 2026, 4:48:05 PM (2 days ago) Feb 19
to Joe Downing, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org
Yuwei Huang voted and added 4 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +2 |
4 comments
Patchset-level comments
File remoting/host/desktop_capturer_proxy.cc
Line 69, Patchset 5:
void RunCapturerTask(R (webrtc::DesktopCapturer::*method)(Args...),Joe Downing . resolved
Can you add a comment on this method to indicate why it is useful, essentially that it allows callers to set capture params before the underlying capturer has been created.
Yuwei Huang
Done
Can you continue to check whether `capturer_` is null rather than introduce the `capturer_set_` member? I don't think it's valid to to set capturer_ to null via the setter and it looks like capturer_ is not released before desctruction.
Yuwei Huang
Done. The code in [LegacyInteractionStrategy](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/legacy_interaction_strategy.cc;l=121;drc=cd4d28a2c2add65c76bb3252dc28370982577e33) suggests that the capturer could potentially be null, but there is no way it can work with a null capturer, so I just changed it to log at DFATAL level.
Line 188, Patchset 5:
(capturer_.get()->*method)(std::forward<Args>(args)...);Joe Downing . resolved
Is `capturer_->*method` valid here? Or is .get() required due to templating?
Yuwei Huang
`->*` is actually a single operator, and the current C++ spec doesn't support overloading it.
FWIW this template isn't going to work once I replace `webrtc::DesktopCapturer` with `remoting::DesktopCapturer` in this file, so I'm replacing it with `std::invocable` and `std::invoke` in this CL. The latter does not require getting the raw pointer.
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Feb 19, 2026, 6:44:46 PM (2 days ago) Feb 19
to Yuwei Huang, Joe Downing, chromium...@chromium.org, chromotin...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: remoting/host/desktop_capturer_proxy.cc
Insertions: 21, Deletions: 19.
@@ -65,14 +65,17 @@
void OnCaptureResult(webrtc::DesktopCapturer::Result result,
std::unique_ptr<webrtc::DesktopFrame> frame) override;
- template <typename R, typename... Args>
- void RunCapturerTask(R (webrtc::DesktopCapturer::*method)(Args...),
- typename std::decay<Args>::type&&... args);
+ // Either runs the capturer task right away if it is set, or queue it so that
+ // it can be run after the underlying capturer is created. This is to allow
+ // callers to set capture params before the underlying capturer has been
+ // created.
+ template <typename F, typename... Args>
+ requires std::invocable<F, webrtc::DesktopCapturer*, Args...>
+ void RunCapturerTask(F&& method, Args&&... args);
base::WeakPtr<DesktopCapturerProxy> proxy_;
scoped_refptr<base::SequencedTaskRunner> caller_task_runner_;
std::unique_ptr<webrtc::DesktopCapturer> capturer_;
- bool capturer_set_ = false;
// Tasks to be run after `capturer_` is set.
base::queue<base::OnceCallback<void(webrtc::DesktopCapturer*)>>
@@ -95,11 +98,13 @@
void DesktopCapturerProxy::Core::SetCapturer(
std::unique_ptr<webrtc::DesktopCapturer> capturer) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
- DCHECK(!capturer_set_);
DCHECK(!capturer_);
capturer_ = std::move(capturer);
- capturer_set_ = true;
+ if (!capturer_) {
+ LOG(DFATAL) << "Trying to set the underlying capturer to null.";
+ return;
+ }
while (!pending_tasks_.empty()) {
if (capturer_) {
std::move(pending_tasks_.front()).Run(capturer_.get());
@@ -177,25 +182,22 @@
result, std::move(frame)));
}
-template <typename R, typename... Args>
-void DesktopCapturerProxy::Core::RunCapturerTask(
- R (webrtc::DesktopCapturer::*method)(Args...),
- typename std::decay<Args>::type&&... args) {
+template <typename F, typename... Args>
+ requires std::invocable<F, webrtc::DesktopCapturer*, Args...>
+void DesktopCapturerProxy::Core::RunCapturerTask(F&& method, Args&&... args) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
- if (capturer_set_) {
- if (capturer_) {
- (capturer_.get()->*method)(std::forward<Args>(args)...);
- }
+ if (capturer_) {
+ std::invoke(std::forward<F>(method), capturer_,
+ std::forward<Args>(args)...);
return;
}
pending_tasks_.push(base::BindOnce(
- [](R (webrtc::DesktopCapturer::*method)(Args...),
- typename std::decay<Args>::type&&... args,
- webrtc::DesktopCapturer* capturer) {
- (capturer->*method)(std::forward<Args>(args)...);
+ [](F&& method, Args&&... args, webrtc::DesktopCapturer* capturer) {
+ std::invoke(std::forward<F>(method), capturer,
+ std::forward<Args>(args)...);
},
- method, std::forward<Args>(args)...));
+ std::forward<F>(method), std::forward<Args>(args)...));
}
DesktopCapturerProxy::DesktopCapturerProxy(
```
Change information
Commit message:
[crd host][linux] Fix issues with DesktopCapturerProxy
GnomeInteractionStrategy uses DesktopCapturerProxy to allow a
DesktopCapturer to be created and returned to the caller before the
underlying PipeWire capturer is create, but the logic appears to be
problematic. `set_capturer` is not allowed after `Start` is called, and
the DesktopCapturer will be transferred to WebRtcVideoStream, which runs
tasks in a dedicated thread, so it is not safe to access its WeakPtr
from the thread of GnomeInteractionStrategy, which is the network
thread. The current logic doesn't seem to have issues with the
single-process Wayland host, but it fails a DCHECK with the
multi-process Wayland host.
This CL fixes this by making DesktopCaptureProxy::Core store pending
tasks and execute them after the underlying capturer is set, and
introducing a GetSetCapturerCallback() method which binds to the WeakPtr
of the Core and allows setting the capturer from any threads.
Bug: 475611769
Change-Id: I7efc98f0ea8e12d958adc27afd93224a0ca09f33
Reviewed-by: Joe Downing <joe...@chromium.org>
Commit-Queue: Yuwei Huang <yuw...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1587476}
Files:
- M remoting/host/desktop_capturer_proxy.cc
- M remoting/host/desktop_capturer_proxy.h
- M remoting/host/linux/gnome_interaction_strategy.cc
- M remoting/host/linux/gnome_interaction_strategy.h
Change size: M
Delta: 4 files changed, 107 insertions(+), 56 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Joe Downing
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages