gpu: In-browser-process mode for zero-copy video capture [chromium/src : main]
Sean Li (Gerrit)
Sean Li added 2 comments![]( "Open in Gerrit")
class InProcessVideoCaptureGpuContextProvider finalIIUC, `InProcessVideoCaptureGpuContextProvider` should handle `gpu_workarounds`.
std::unique_ptr<InProcessVideoCaptureGpuContextProvider>>The establishment of GPU channel in browser is async. If the establishment of GPU channel is after [HW buffer submit](https://source.chromium.org/chromium/chromium/src/+/main:media/capture/video/android/video_capture_device_android.cc;l=472-479;drc=c1b41d1ecd522fa07dd2fb895abefed65084da6d) or GPU channel is lost during VCD streaming, it will break VCD.
We should have a mechanism to recover instead of returning error in VCD.
Related details
- Eugene Zemtsov
- Vasiliy Telezhnikov
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. |
Sean Li (Gerrit)
Sean Li added 1 comment![]( "Open in Gerrit")
class InProcessVideoCaptureGpuContextProvider finalIIUC, `InProcessVideoCaptureGpuContextProvider` should handle `gpu_workarounds`.
And `gpu_workarounds` should be always ready before VCD is created and starts.
Eugene Zemtsov (Gerrit)
Eugene Zemtsov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Eugene Zemtsov
| Commit-Queue | +1 |
2 comments
Sean LiIIUC, `InProcessVideoCaptureGpuContextProvider` should handle `gpu_workarounds`.
And `gpu_workarounds` should be always ready before VCD is created and starts.
Done
The establishment of GPU channel in browser is async. If the establishment of GPU channel is after [HW buffer submit](https://source.chromium.org/chromium/chromium/src/+/main:media/capture/video/android/video_capture_device_android.cc;l=472-479;drc=c1b41d1ecd522fa07dd2fb895abefed65084da6d) or GPU channel is lost during VCD streaming, it will break VCD.
We should have a mechanism to recover instead of returning error in VCD.
- Sean Li
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov added 1 comment![]( "Open in Gerrit")
std::atomic<bool>& GetIsFromAngleGlobal() {Instead of doing this and adding silent fallback from VulkanFromAngle to native, could we make a way (e.g by passing arg to ctor) to force it always use native ignoring the feature flag?
Silent fallback has quite a bit side-effects in terms of what configuration we run on which devices and it's already too complicated.
Related details
- Eugene Zemtsov
- Sean Li
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. |
Eugene Zemtsov (Gerrit)
Eugene Zemtsov added 1 comment![]( "Open in Gerrit")
Instead of doing this and adding silent fallback from VulkanFromAngle to native, could we make a way (e.g by passing arg to ctor) to force it always use native ignoring the feature flag?
Silent fallback has quite a bit side-effects in terms of what configuration we run on which devices and it's already too complicated.
- Sean Li
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vasiliy Telezhnikov
| Code-Review | +1 |
1 comment
Related details
- Eugene Zemtsov
- Sean Li
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
- Eugene Zemtsov
- Markus Handell
- Sean Li
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Thanks! hbos@, could you review //services/video_capture as closer OWNER?
Related details
- Eugene Zemtsov
- Henrik Boström
- Markus Handell
- Sean Li
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sean Li (Gerrit)
Sean Li added 2 comments![]( "Open in Gerrit")
hardware_buffer_available_cb_ = base::BindPostTask(Why do we save callback here, and validate it in `OnHardwareBufferAvailable`?
class Core final : public base::RefCountedDeleteOnSequence<Core>,`BrowserGpuChannelHostProvider` is deleted on the UI thread. Why do we need `RefCountedDeleteOnSequence` here?
Related details
- Eugene Zemtsov
- Henrik Boström
- Markus Handell
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. |
Sean Li (Gerrit)
Sean Li added 1 comment![]( "Open in Gerrit")
hardware_buffer_available_cb_ = base::BindPostTask(Why do we save callback here, and validate it in `OnHardwareBufferAvailable`?
I just saw the comment. However, if there is already a `OnHardwareBufferAvailableOnMainThread` queued before `hardware_buffer_available_cb_` is set to `null`, it still have a change to execute.
Eugene Zemtsov (Gerrit)
Eugene Zemtsov added 2 comments![]( "Open in Gerrit")
Sean LiWhy do we save callback here, and validate it in `OnHardwareBufferAvailable`?
I just saw the comment. However, if there is already a `OnHardwareBufferAvailableOnMainThread` queued before `hardware_buffer_available_cb_` is set to `null`, it still have a change to execute.
Good point. The added `InvalidateWeakPtrs()` call resolves this.
class Core final : public base::RefCountedDeleteOnSequence<Core>,`BrowserGpuChannelHostProvider` is deleted on the UI thread. Why do we need `RefCountedDeleteOnSequence` here?
While BrowserGpuChannelHostProvider is destroyed on the UI thread, Core implements `gpu::GpuChannelLostObserver::OnGpuChannelLost()`.
`OnGpuChannelLost()` is called on the IPC/GPU thread, where `base::BindOnce(&Core::OnGpuChannelLostOnUIThread, this)` increments Core's reference count off the UI thread.
If the last `scoped_refptr<Core>` reference is released on the IPC thread (or during sequence shutdown), `Core` would be destructed on a non-UI thread without `base::RefCountedDeleteOnSequence<Core>`.
`RefCountedDeleteOnSequence` ensures `~Core()` is guaranteed to execute on the UI task runner.
Related details
- Henrik Boström
- Markus Handell
- Sean Li
Eugene Zemtsov (Gerrit)
Eugene Zemtsov added 2 comments![]( "Open in Gerrit")
+dcheng because other reviewers seem to be unavailable.
class Core final : public base::RefCountedDeleteOnSequence<Core>,Eugene Zemtsov`BrowserGpuChannelHostProvider` is deleted on the UI thread. Why do we need `RefCountedDeleteOnSequence` here?
While BrowserGpuChannelHostProvider is destroyed on the UI thread, Core implements `gpu::GpuChannelLostObserver::OnGpuChannelLost()`.
`OnGpuChannelLost()` is called on the IPC/GPU thread, where `base::BindOnce(&Core::OnGpuChannelLostOnUIThread, this)` increments Core's reference count off the UI thread.
If the last `scoped_refptr<Core>` reference is released on the IPC thread (or during sequence shutdown), `Core` would be destructed on a non-UI thread without `base::RefCountedDeleteOnSequence<Core>`.
`RefCountedDeleteOnSequence` ensures `~Core()` is guaranteed to execute on the UI task runner.
- Daniel Cheng
- Henrik Boström
- Markus Handell
- Sean Li
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
I am in top-level owners for //services, but I'm not really qualified to review capture code.
Would it make sense for there to be more OWNERS? e.g. would vasilyt@ be a good addition? :)
class Core final : public base::RefCountedDeleteOnSequence<Core>,Probably worth adding some comments here about the threading requirements.
Related details
- Eugene Zemtsov
- Henrik Boström
- Markus Handell
- Sean Li
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. |
Sean Li (Gerrit)
Sean Li added 1 comment![]( "Open in Gerrit")
class Core final : public base::RefCountedDeleteOnSequence<Core>,Eugene Zemtsov`BrowserGpuChannelHostProvider` is deleted on the UI thread. Why do we need `RefCountedDeleteOnSequence` here?
Eugene ZemtsovWhile BrowserGpuChannelHostProvider is destroyed on the UI thread, Core implements `gpu::GpuChannelLostObserver::OnGpuChannelLost()`.
`OnGpuChannelLost()` is called on the IPC/GPU thread, where `base::BindOnce(&Core::OnGpuChannelLostOnUIThread, this)` increments Core's reference count off the UI thread.
If the last `scoped_refptr<Core>` reference is released on the IPC thread (or during sequence shutdown), `Core` would be destructed on a non-UI thread without `base::RefCountedDeleteOnSequence<Core>`.
`RefCountedDeleteOnSequence` ensures `~Core()` is guaranteed to execute on the UI task runner.
Done
GpuChannelHost use a [lock](https://source.chromium.org/chromium/chromium/src/+/main:gpu/ipc/client/gpu_channel_host.cc;l=498-511) to keep thread-safety. IIUC, we don't have to use `RefCountedDeleteOnSequence`. We can just let `Core` released with `BrowserGpuChannelHostProvider` on the UI thread, and use `getWeakPtr()` in OnGpuChannelLost().
```
void OnGpuChannelLost() override {
ui_task_runner_->PostTask(
FROM_HERE, base::BindOnce(&Core::OnGpuChannelLostOnUIThread, getWeakPtr()));
}
```
Even more, we don't need to have `Core(delegate)` pattern for this. Or, we can use `DeleteSoon` to release `Core` instead of releasing `BrowserGpuChannelHostProvider`.
Related details
- Eugene Zemtsov
- Henrik Boström
- Markus Handell
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
@vas...@chromium.org, could you do the technical review and LGTM of //services/video_capture as well? I'm happy to stamp for OWNERS once you've LGTM'd it. Thanks!
Eugene Zemtsov (Gerrit)
Eugene Zemtsov added 2 comments![]( "Open in Gerrit")
class Core final : public base::RefCountedDeleteOnSequence<Core>,Eugene Zemtsov`BrowserGpuChannelHostProvider` is deleted on the UI thread. Why do we need `RefCountedDeleteOnSequence` here?
Eugene ZemtsovWhile BrowserGpuChannelHostProvider is destroyed on the UI thread, Core implements `gpu::GpuChannelLostObserver::OnGpuChannelLost()`.
`OnGpuChannelLost()` is called on the IPC/GPU thread, where `base::BindOnce(&Core::OnGpuChannelLostOnUIThread, this)` increments Core's reference count off the UI thread.
If the last `scoped_refptr<Core>` reference is released on the IPC thread (or during sequence shutdown), `Core` would be destructed on a non-UI thread without `base::RefCountedDeleteOnSequence<Core>`.
`RefCountedDeleteOnSequence` ensures `~Core()` is guaranteed to execute on the UI task runner.
Sean LiDone
GpuChannelHost use a [lock](https://source.chromium.org/chromium/chromium/src/+/main:gpu/ipc/client/gpu_channel_host.cc;l=498-511) to keep thread-safety. IIUC, we don't have to use `RefCountedDeleteOnSequence`. We can just let `Core` released with `BrowserGpuChannelHostProvider` on the UI thread, and use `getWeakPtr()` in OnGpuChannelLost().
```
void OnGpuChannelLost() override {
ui_task_runner_->PostTask(
FROM_HERE, base::BindOnce(&Core::OnGpuChannelLostOnUIThread, getWeakPtr()));
}
```Even more, we don't need to have `Core(delegate)` pattern for this. Or, we can use `DeleteSoon` to release `Core` instead of releasing `BrowserGpuChannelHostProvider`.
Unfortunately, neither WeakPtr nor DeleteSoon without refcounting works here due to cross-thread sequence safety:
1. WeakPtr sequence safety: weak_ptr_factory_ is bound to the UI sequence (`ui_task_runner_`), but `GpuChannelHost` invokes `OnGpuChannelLost()` on the GPU/IO thread. Calling `GetWeakPtr()` on the IO thread
is unsafe and triggers sequence checker DCHECKs / data races. GpuChannelHost's internal lock only protects its own observer list iteration, not Core's members.
2. DeleteSoon & Lifetime: Without refcounting, posting `OnGpuChannelLostOnUIThread` from the IO thread using `base::Unretained(this)` would race with `DeleteSoon` on the UI thread. If `DeleteSoon` executes
first on the UI thread, the callback task executes on a deleted pointer.
class Core final : public base::RefCountedDeleteOnSequence<Core>,Probably worth adding some comments here about the threading requirements.
Done. Added a comment documenting the threading model and why RefCountedDeleteOnSequence is required.
Related details
- Henrik Boström
- Markus Handell
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eugene Zemtsov (Gerrit)
Eugene Zemtsov removed Henrik Boström from this change![]( "Open in Gerrit")
- Markus Handell
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sean Li (Gerrit)
Sean Li added 1 comment![]( "Open in Gerrit")
class Core final : public base::RefCountedDeleteOnSequence<Core>,Eugene Zemtsov`BrowserGpuChannelHostProvider` is deleted on the UI thread. Why do we need `RefCountedDeleteOnSequence` here?
Eugene ZemtsovWhile BrowserGpuChannelHostProvider is destroyed on the UI thread, Core implements `gpu::GpuChannelLostObserver::OnGpuChannelLost()`.
`OnGpuChannelLost()` is called on the IPC/GPU thread, where `base::BindOnce(&Core::OnGpuChannelLostOnUIThread, this)` increments Core's reference count off the UI thread.
If the last `scoped_refptr<Core>` reference is released on the IPC thread (or during sequence shutdown), `Core` would be destructed on a non-UI thread without `base::RefCountedDeleteOnSequence<Core>`.
`RefCountedDeleteOnSequence` ensures `~Core()` is guaranteed to execute on the UI task runner.
Sean LiDone
Eugene ZemtsovGpuChannelHost use a [lock](https://source.chromium.org/chromium/chromium/src/+/main:gpu/ipc/client/gpu_channel_host.cc;l=498-511) to keep thread-safety. IIUC, we don't have to use `RefCountedDeleteOnSequence`. We can just let `Core` released with `BrowserGpuChannelHostProvider` on the UI thread, and use `getWeakPtr()` in OnGpuChannelLost().
```
void OnGpuChannelLost() override {
ui_task_runner_->PostTask(
FROM_HERE, base::BindOnce(&Core::OnGpuChannelLostOnUIThread, getWeakPtr()));
}
```Even more, we don't need to have `Core(delegate)` pattern for this. Or, we can use `DeleteSoon` to release `Core` instead of releasing `BrowserGpuChannelHostProvider`.
Unfortunately, neither WeakPtr nor DeleteSoon without refcounting works here due to cross-thread sequence safety:
1. WeakPtr sequence safety: weak_ptr_factory_ is bound to the UI sequence (`ui_task_runner_`), but `GpuChannelHost` invokes `OnGpuChannelLost()` on the GPU/IO thread. Calling `GetWeakPtr()` on the IO thread
is unsafe and triggers sequence checker DCHECKs / data races. GpuChannelHost's internal lock only protects its own observer list iteration, not Core's members.
2. DeleteSoon & Lifetime: Without refcounting, posting `OnGpuChannelLostOnUIThread` from the IO thread using `base::Unretained(this)` would race with `DeleteSoon` on the UI thread. If `DeleteSoon` executes
first on the UI thread, the callback task executes on a deleted pointer.
From [base/memory/weak_ptr.h](https://source.chromium.org/chromium/chromium/src/+/main:base/memory/weak_ptr.h;l=50-69;drc=853aabf2c9c58f9e0389c13ed41d843806dca3b9),
`weak_ptr` must always be dereferenced and invalidated on the same SequencedTaskRunner otherwise checking the pointer would be racey.
IIUC, the only limitation is: `weak_ptr` has to dereferenced on one sequence(`ui_task_runner_`) and `weak_ptr_factory_` has to be destroyed on the some sequence since it invalidated all of the `weak_ptr`s in the destructor.
`GetWeakPtr()` is ok to be called on any sequence.
Take the code in Ash VCD as example,
[CameraHalDelegate::SetCameraModuleOnIpcThread](https://source.chromium.org/chromium/chromium/src/+/main:media/capture/video/chromeos/camera_hal_delegate.cc;l=452-469;drc=e90bc5c44ab2617de657fe534770d5a7ca51274e) ->[CameraHalDelegate::NotifyVideoCaptureDevicesChanged()](https://source.chromium.org/chromium/chromium/src/+/main:media/capture/video/chromeos/camera_hal_delegate.cc;l=1088-1090;drc=853aabf2c9c58f9e0389c13ed41d843806dca3b9) -> [SystemEventMonitorProxy::NotifyVideoCaptureDevicesChanged](https://source.chromium.org/chromium/chromium/src/+/main:media/capture/video/chromeos/camera_hal_delegate.cc;l=162-168;drc=853aabf2c9c58f9e0389c13ed41d843806dca3b9) run on the `IPC thread`, and it call `GetWeakPtr` on a `WeakPtrFactory` which is bound on the UI thread.
Related details
- Eugene Zemtsov
- Markus Handell
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. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Vasiliy Telezhnikov
| Code-Review | +0 |
2 comments
Sorry for the delay, this slipped from my attention set.
There is another non-obvious requirement that `GetWeakPtr` is called on the same thread as `InvalidateWeakPtrs` (and so it makes everything single threaded, as this needs to happen on the same thread as dereference happen), but as we don't call `InvalidateWeakPtrs` on this weakptr factory, this is fine and we can call GetWeakPtr on io thread.
We actually do this right in this file just above in `VideoCaptureServiceImpl::VizGpuContextProvider`. And the related questions, why do we need different BrowserGpuChannelHostProvider instead of using the one above?
Sean Li (Gerrit)
Sean Li added 1 comment![]( "Open in Gerrit")
Why do we need different BrowserGpuChannelHostProvider instead of using the one above?
VizGpuContextProvider establishes the GPU channel synchronously but establishment of the GPU channel in browser process has to be asynchronous. Therefore, VizGpuContextProvider is for service in utility process, and BrowserGpuChannelHostProvider is for service in browser process.
Context: https://chromium-review.git.corp.google.com/c/chromium/src/+/7836765
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov added 4 comments![]( "Open in Gerrit")
base::WeakPtrFactory<VideoCaptureDeviceAndroid> weak_ptr_factory_{this};This now can be used only on main thread, which is not obvious. Can you add `GUARDED_BY_CONTEXT` and sequence checker?
Ideally we should avoid InvalidateWeakPtrs, but that would require some other ways to discard old callbacks (e.g a counter that we increment in Start and compare in callback or inner object that is alive only when we started), but I don't have strong feelings about this.
In the capture machinery is fine with async, could we just have one class that is always Async? There is quite a bit of code churn here.
media::VideoCaptureGpuChannelHost::GetInstance().OnContextLost();Question: Why do we call OnContextLost when we just restored the context? Is it just telling observers that "something changed"?
retry_delay_ = std::min(retry_delay_ * 2, base::Seconds(30));I don't think there is point of doing delay logic, you can retry immediately. The only way why establishing channel is supposed to fail if newly started gpu process crashes during start up.
Adding delay will not help with that.
Eugene Zemtsov (Gerrit)
Eugene Zemtsov added 4 comments![]( "Open in Gerrit")
base::WeakPtrFactory<VideoCaptureDeviceAndroid> weak_ptr_factory_{this};This now can be used only on main thread, which is not obvious. Can you add `GUARDED_BY_CONTEXT` and sequence checker?
Ideally we should avoid InvalidateWeakPtrs, but that would require some other ways to discard old callbacks (e.g a counter that we increment in Start and compare in callback or inner object that is alive only when we started), but I don't have strong feelings about this.
Done. Added SEQUENCE_CHECKER(main_sequence_checker_); and annotated weak_ptr_factory_ (and main-thread methods) with GUARDED_BY_CONTEXT(main_sequence_checker_).
class Core final : public base::RefCountedDeleteOnSequence<Core>,Done. Combined `VizGpuContextProvider` and `BrowserGpuChannelHostProvider` into `GpuChannelHostProvider`
media::VideoCaptureGpuChannelHost::GetInstance().OnContextLost();Question: Why do we call OnContextLost when we just restored the context? Is it just telling observers that "something changed"?
Yes. `OnContextLost()` signals `VideoCaptureDeviceAndroid` to invalidate cached YCbCr info and stale shared image handles so they re-query parameters against the new GPU channel.
retry_delay_ = std::min(retry_delay_ * 2, base::Seconds(30));I don't think there is point of doing delay logic, you can retry immediately. The only way why establishing channel is supposed to fail if newly started gpu process crashes during start up.
Adding delay will not help with that.
I removed the delay timer and capped retries to a maximum of 3 attempts. The reconnect task is delayed by 20ms though.
Related details
- Markus Handell
- Sean Li
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vasiliy Telezhnikov
- Eugene Zemtsov
- Markus Handell
- Sean Li
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov added 1 comment![]( "Open in Gerrit")
- services/video_capture: Generalized BrowserGpuChannelHostProvider and refactored it with a thread-safe Core helper to safely handle GPU loss. Added exponential backoff retries for GPU channel establishment.nit: commit message is a bit outdated now.
Related details
- Eugene Zemtsov
- Markus Handell
- Sean Li
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. |
Markus Handell (Gerrit)
Markus Handell added 1 comment![]( "Open in Gerrit")
- Eugene Zemtsov
- Ilya Nikolaevskiy
- Sean Li
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. |