Enable GPU channel for in-process Video Capture Service. [chromium/src : main]
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
viz::Gpu::Create(std::move(gpu_remote), GetIOThreadTaskRunner({}));browser already has a channel to the gpu process, and it's bound on the UI thread I believe:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.h;drc=9381f87f8ca50194061da262b82dae609988d974;l=51
can you use that instead?
Related details
- 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")
viz::Gpu::Create(std::move(gpu_remote), GetIOThreadTaskRunner({}));browser already has a channel to the gpu process, and it's bound on the UI thread I believe:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.h;drc=9381f87f8ca50194061da262b82dae609988d974;l=51can you use that instead?
Thanks for the suggestion! However, IIUC, the browser's main GPU channel is primarily dedicated to critical components like the UI Compositor and WebGL. I tried to use it for the in-process Video Capture Service but got race conditions and crashes (e.g., SharedImage mailbox not found and waiting on non-existent sync token). This happens because sharing the same GpuChannelHost and underlying SequenceId between the background Video Capture thread and the UI/Compositor thread causes interleaved deferred messages, which breaks the strict ordering of SyncToken release counts required for cross-channel synchronization with the Renderer.
To maintain proper isolation (consistent with the out-of-process path), the in-process service requires its own dedicated GPU channel, which I will establish purely within content/browser via a dedicated viz::GpuClient.
Related details
- Bo Liu
- vikas soni
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")
I am still working on the test failure. Some of the content or browser tests init VCS. However, we don't have GPU process in unit tests, so the test crashes and fails.
This doesn't happen in utility process path. The reason is that we don't really create a real utility process in unit tests, so the test doesn't try to create a channel to GPU process.
Sean Li (Gerrit)
Sean Li added 1 comment![]( "Open in Gerrit")
One other issue:
During the Android browser test (components_browsertests and content_browsertests), tests timed out and unexpectedly deadlocked during teardown due to `ParallelExecutionFence` hanging indefinitely. The deadlock occurs because the process-lifetime or sequence-lifetime singletons (i.e., the base::NoDestructor VCS singleton and the static viz::GpuClient SequenceLocalStorageSlot) eagerly instantiate persistent, active Mojo SimpleWatchers running on background ThreadPool/IO task runners to monitor Mojo pipe health asynchronously. Since the singletons linger beyond the test body execution, the ParallelExecutionFence blocks and waits forever for these background watchers to drain.
Related details
- Bo Liu
- vikas soni
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. |
Bo Liu (Gerrit)
Bo Liu added 5 comments![]( "Open in Gerrit")
is this still supposed to be WIP?
viz::Gpu::Create(std::move(gpu_remote), GetIOThreadTaskRunner({}));Sean Librowser already has a channel to the gpu process, and it's bound on the UI thread I believe:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.h;drc=9381f87f8ca50194061da262b82dae609988d974;l=51can you use that instead?
Thanks for the suggestion! However, IIUC, the browser's main GPU channel is primarily dedicated to critical components like the UI Compositor and WebGL. I tried to use it for the in-process Video Capture Service but got race conditions and crashes (e.g., SharedImage mailbox not found and waiting on non-existent sync token). This happens because sharing the same GpuChannelHost and underlying SequenceId between the background Video Capture thread and the UI/Compositor thread causes interleaved deferred messages, which breaks the strict ordering of SyncToken release counts required for cross-channel synchronization with the Renderer.
To maintain proper isolation (consistent with the out-of-process path), the in-process service requires its own dedicated GPU channel, which I will establish purely within content/browser via a dedicated viz::GpuClient.
Thanks for the suggestion! However, IIUC, the browser's main GPU channel is primarily dedicated to critical components like the UI Compositor and WebGL. I tried to use it for the in-process Video Capture Service but got race conditions and crashes (e.g., SharedImage mailbox not found and waiting on non-existent sync token). This happens because sharing the same GpuChannelHost and underlying SequenceId between the background Video Capture thread and the UI/Compositor thread causes interleaved deferred messages, which breaks the strict ordering of SyncToken release counts required for cross-channel synchronization with the Renderer.
Do you have a concrete example of how things break? I don't really have a theory of how things would break.. but then I don't really understand what video capture service does with the gpu channel either. Perhaps there's circular wait? Which I believe gpu/viz side would detect at runtime and then generate errors, rather than actually deadlocking.
To maintain proper isolation (consistent with the out-of-process path), the in-process service requires its own dedicated GPU channel, which I will establish purely within content/browser via a dedicated viz::GpuClient.
GetGpuClientSlot() {there are two SequenceLocalStorageSlot in this file now, should be refactored so they are owned by the same global var
also I highly doubt the SequenceLocal of this.. I assume all of this is accessed from the same thread, and certainly don't want to create multiple gpu channels just based on the sequence, in which case just a regular NoDestructor should be fine
or can have the service be shutdown when all mojo clients go away
CONTENT_EXPORT void ResetVideoCaptureServiceForTesting();don't need to be public API unless it's called from outside of content
// UI thread while SLS is fully alive.what's that?
Related details
- Sean Li
- vikas soni
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")
is this still supposed to be WIP?
- Bo Liu
- vikas soni
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 has uploaded the change for review![]( "Open in Gerrit")
Commit message
Enable GPU channel for in-process Video Capture Service.
This CL enables the GPU channel for the Video Capture Service when it
runs in the browser process. This allows the service to access GPU
resources (like `SharedImageInterface`) which are required for features
like Zero-Copy video capture on Android.
Change diff
Change information
- M content/browser/video_capture_service_impl.cc
- M content/public/common/content_features.cc
- M services/video_capture/DEPS
- M services/video_capture/video_capture_service_impl.cc
- M services/video_capture/video_capture_service_impl.h
Related details
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")
don't need to be public API unless it's called from outside of content
Removed the API.
// UI thread while SLS is fully alive.Sean Liwhat's that?
Removed it.
Related details
- Bo Liu
- vikas soni
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")
viz::Gpu::Create(std::move(gpu_remote), GetIOThreadTaskRunner({}));Sean Librowser already has a channel to the gpu process, and it's bound on the UI thread I believe:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.h;drc=9381f87f8ca50194061da262b82dae609988d974;l=51can you use that instead?
Bo LiuThanks for the suggestion! However, IIUC, the browser's main GPU channel is primarily dedicated to critical components like the UI Compositor and WebGL. I tried to use it for the in-process Video Capture Service but got race conditions and crashes (e.g., SharedImage mailbox not found and waiting on non-existent sync token). This happens because sharing the same GpuChannelHost and underlying SequenceId between the background Video Capture thread and the UI/Compositor thread causes interleaved deferred messages, which breaks the strict ordering of SyncToken release counts required for cross-channel synchronization with the Renderer.
To maintain proper isolation (consistent with the out-of-process path), the in-process service requires its own dedicated GPU channel, which I will establish purely within content/browser via a dedicated viz::GpuClient.
Thanks for the suggestion! However, IIUC, the browser's main GPU channel is primarily dedicated to critical components like the UI Compositor and WebGL. I tried to use it for the in-process Video Capture Service but got race conditions and crashes (e.g., SharedImage mailbox not found and waiting on non-existent sync token). This happens because sharing the same GpuChannelHost and underlying SequenceId between the background Video Capture thread and the UI/Compositor thread causes interleaved deferred messages, which breaks the strict ordering of SyncToken release counts required for cross-channel synchronization with the Renderer.
Do you have a concrete example of how things break? I don't really have a theory of how things would break.. but then I don't really understand what video capture service does with the gpu channel either. Perhaps there's circular wait? Which I believe gpu/viz side would detect at runtime and then generate errors, rather than actually deadlocking.
To maintain proper isolation (consistent with the out-of-process path), the in-process service requires its own dedicated GPU channel, which I will establish purely within content/browser via a dedicated viz::GpuClient.
Hi Bo,
Thanks for the suggestion again. My previous implementation is wrong, and [7836765](https://chromium-review.git.corp.google.com/c/chromium/src/+/7836765) works.
I think why we can't use `viz::Gpu` for in-process path is because viz::Gpu doesn't destroy the channel to gpu in destructor. Even it has cleanup in destructor, its owner(VideoCaptureServiceImpl) is a NoDestructor, there is no a good timing to destory the channel and make the test failing.
And for [7836765](https://chromium-review.git.corp.google.com/c/chromium/src/+/7836765), the TODO is to check the recoverable issue when the gpu channel is broken unexpectedly(GPU process crashes). Besides, the lifetime of `VideoCaptureServiceImpl` is longer than the GPU channel in `BrowserGpuChannelHostFactory`. It means that we have to handle the case when the channel is not available.
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
- Sean Li
- vikas soni
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 4 comments![]( "Open in Gerrit")
I am still working on the test failure. Some of the content or browser tests init VCS. However, we don't have GPU process in unit tests, so the test crashes and fails.
This doesn't happen in utility process path. The reason is that we don't really create a real utility process in unit tests, so the test doesn't try to create a channel to GPU process.
Done
One other issue:
During the Android browser test (components_browsertests and content_browsertests), tests timed out and unexpectedly deadlocked during teardown due to `ParallelExecutionFence` hanging indefinitely. The deadlock occurs because the process-lifetime or sequence-lifetime singletons (i.e., the base::NoDestructor VCS singleton and the static viz::GpuClient SequenceLocalStorageSlot) eagerly instantiate persistent, active Mojo SimpleWatchers running on background ThreadPool/IO task runners to monitor Mojo pipe health asynchronously. Since the singletons linger beyond the test body execution, the ParallelExecutionFence blocks and waits forever for these background watchers to drain.
Disable the feature in test.
viz::Gpu::Create(std::move(gpu_remote), GetIOThreadTaskRunner({}));Sean Librowser already has a channel to the gpu process, and it's bound on the UI thread I believe:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.h;drc=9381f87f8ca50194061da262b82dae609988d974;l=51can you use that instead?
Bo LiuThanks for the suggestion! However, IIUC, the browser's main GPU channel is primarily dedicated to critical components like the UI Compositor and WebGL. I tried to use it for the in-process Video Capture Service but got race conditions and crashes (e.g., SharedImage mailbox not found and waiting on non-existent sync token). This happens because sharing the same GpuChannelHost and underlying SequenceId between the background Video Capture thread and the UI/Compositor thread causes interleaved deferred messages, which breaks the strict ordering of SyncToken release counts required for cross-channel synchronization with the Renderer.
To maintain proper isolation (consistent with the out-of-process path), the in-process service requires its own dedicated GPU channel, which I will establish purely within content/browser via a dedicated viz::GpuClient.
Sean LiThanks for the suggestion! However, IIUC, the browser's main GPU channel is primarily dedicated to critical components like the UI Compositor and WebGL. I tried to use it for the in-process Video Capture Service but got race conditions and crashes (e.g., SharedImage mailbox not found and waiting on non-existent sync token). This happens because sharing the same GpuChannelHost and underlying SequenceId between the background Video Capture thread and the UI/Compositor thread causes interleaved deferred messages, which breaks the strict ordering of SyncToken release counts required for cross-channel synchronization with the Renderer.
Do you have a concrete example of how things break? I don't really have a theory of how things would break.. but then I don't really understand what video capture service does with the gpu channel either. Perhaps there's circular wait? Which I believe gpu/viz side would detect at runtime and then generate errors, rather than actually deadlocking.
To maintain proper isolation (consistent with the out-of-process path), the in-process service requires its own dedicated GPU channel, which I will establish purely within content/browser via a dedicated viz::GpuClient.
Hi Bo,
Thanks for the suggestion again. My previous implementation is wrong, and [7836765](https://chromium-review.git.corp.google.com/c/chromium/src/+/7836765) works.
I think why we can't use `viz::Gpu` for in-process path is because viz::Gpu doesn't destroy the channel to gpu in destructor. Even it has cleanup in destructor, its owner(VideoCaptureServiceImpl) is a NoDestructor, there is no a good timing to destory the channel and make the test failing.
And for [7836765](https://chromium-review.git.corp.google.com/c/chromium/src/+/7836765), the TODO is to check the recoverable issue when the gpu channel is broken unexpectedly(GPU process crashes). Besides, the lifetime of `VideoCaptureServiceImpl` is longer than the GPU channel in `BrowserGpuChannelHostFactory`. It means that we have to handle the case when the channel is not available.
The life time of existing client(compositor) is shorter than VideoCaptureServiceImpl. I need more time to check whether it can cause a conflict.
In the same time, [GPU channel establishment is not sync in android](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.cc;l=325-329;drc=221b0bad9f1ea388055800ebb48f8eff365a89ab). In this case, if gpu channel broken during camera is streaming, the service can be broken. Therefore, I prefer the current implementation and add a TODO here. Thanks.
there are two SequenceLocalStorageSlot in this file now, should be refactored so they are owned by the same global var
also I highly doubt the SequenceLocal of this.. I assume all of this is accessed from the same thread, and certainly don't want to create multiple gpu channels just based on the sequence, in which case just a regular NoDestructor should be fine
or can have the service be shutdown when all mojo clients go away
- Bo Liu
- vikas soni
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 added 1 comment![]( "Open in Gerrit")
scoped_feature_list_.InitAndDisableFeature(This feature is disabled by default already, see https://source.chromium.org/chromium/chromium/src/+/main:media/base/media_switches.cc;l=1128;drc=470d96df224c921a642f6d0d4f623ced20754b2e;bpv=1;bpt=1.
I don't really care for adding scoped feature lists at the base class here. If we are running into a han on shutdown we should use the TearDown method of this class to clean up anything that we think would hang further test execution.
Related details
- Bo Liu
- Sean Li
- vikas soni
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. |
Bo Liu (Gerrit)
Bo Liu added 4 comments![]( "Open in Gerrit")
+handellm to help answer some the questions wrt to VideoCaptureService in open comments
looking around the actual service code, looks like it calls synchronous establish channel:
https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;drc=eda1bdbd0694acebc51f1e1a7321c72c6e9421ed;l=184
I think that's just going to deadlock (unless a channel is already created) regardless of which way to go. So sounds like this needs some VideoCaptureService changes required anyway.
viz::Gpu::Create(std::move(gpu_remote), GetIOThreadTaskRunner({}));why bind this to IO thread? seems like the service itself is bound to UI thread?
viz::Gpu::Create(std::move(gpu_remote), GetIOThreadTaskRunner({}));Sean Librowser already has a channel to the gpu process, and it's bound on the UI thread I believe:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.h;drc=9381f87f8ca50194061da262b82dae609988d974;l=51can you use that instead?
Bo LiuThanks for the suggestion! However, IIUC, the browser's main GPU channel is primarily dedicated to critical components like the UI Compositor and WebGL. I tried to use it for the in-process Video Capture Service but got race conditions and crashes (e.g., SharedImage mailbox not found and waiting on non-existent sync token). This happens because sharing the same GpuChannelHost and underlying SequenceId between the background Video Capture thread and the UI/Compositor thread causes interleaved deferred messages, which breaks the strict ordering of SyncToken release counts required for cross-channel synchronization with the Renderer.
To maintain proper isolation (consistent with the out-of-process path), the in-process service requires its own dedicated GPU channel, which I will establish purely within content/browser via a dedicated viz::GpuClient.
Sean LiThanks for the suggestion! However, IIUC, the browser's main GPU channel is primarily dedicated to critical components like the UI Compositor and WebGL. I tried to use it for the in-process Video Capture Service but got race conditions and crashes (e.g., SharedImage mailbox not found and waiting on non-existent sync token). This happens because sharing the same GpuChannelHost and underlying SequenceId between the background Video Capture thread and the UI/Compositor thread causes interleaved deferred messages, which breaks the strict ordering of SyncToken release counts required for cross-channel synchronization with the Renderer.
Do you have a concrete example of how things break? I don't really have a theory of how things would break.. but then I don't really understand what video capture service does with the gpu channel either. Perhaps there's circular wait? Which I believe gpu/viz side would detect at runtime and then generate errors, rather than actually deadlocking.
To maintain proper isolation (consistent with the out-of-process path), the in-process service requires its own dedicated GPU channel, which I will establish purely within content/browser via a dedicated viz::GpuClient.
Sean LiHi Bo,
Thanks for the suggestion again. My previous implementation is wrong, and [7836765](https://chromium-review.git.corp.google.com/c/chromium/src/+/7836765) works.
I think why we can't use `viz::Gpu` for in-process path is because viz::Gpu doesn't destroy the channel to gpu in destructor. Even it has cleanup in destructor, its owner(VideoCaptureServiceImpl) is a NoDestructor, there is no a good timing to destory the channel and make the test failing.
And for [7836765](https://chromium-review.git.corp.google.com/c/chromium/src/+/7836765), the TODO is to check the recoverable issue when the gpu channel is broken unexpectedly(GPU process crashes). Besides, the lifetime of `VideoCaptureServiceImpl` is longer than the GPU channel in `BrowserGpuChannelHostFactory`. It means that we have to handle the case when the channel is not available.
The life time of existing client(compositor) is shorter than VideoCaptureServiceImpl. I need more time to check whether it can cause a conflict.
In the same time, [GPU channel establishment is not sync in android](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.cc;l=325-329;drc=221b0bad9f1ea388055800ebb48f8eff365a89ab). In this case, if gpu channel broken during camera is streaming, the service can be broken. Therefore, I prefer the current implementation and add a TODO here. Thanks.
its owner(VideoCaptureServiceImpl) is a NoDestructor, there is no a good timing to destory the channel and make the test failing.
Trying to accommodate tests is not a good reason to decide one way or another. One way is don't leak the service, and actually destroy it when no longer needed. Or have the test do a clean up.
And for 7836765, the TODO is to check the recoverable issue when the gpu channel is broken unexpectedly(GPU process crashes). Besides, the lifetime of VideoCaptureServiceImpl is longer than the GPU channel in BrowserGpuChannelHostFactory. It means that we have to handle the case when the channel is not available.
Any client that uses the GPU has to be ready to handle GPU process going away due to a crash or whatever. That doesn't change whether creating a new channel or reusing the existing one.
The life time of existing client(compositor) is shorter than VideoCaptureServiceImpl. I need more time to check whether it can cause a conflict.
You get a scoped_refptr<GpuChannel>, so just don't drop the ref and it's fine.
In the same time, GPU channel establishment is not sync in android. In this case, if gpu channel broken during camera is streaming, the service can be broken.
What does that have to do with sync vs async? And again, you'll have to deal with GPU crash either way.
------
I'm not saying you have to reuse the existing channel. But I'd at least want some valid reason to reuse or not reuse.
&BindInProcessInstance, std::move(receiver), std::move(gpu_remote));if viz_gpu_bound is already true, then this is just an empty remote?
Related details
- Markus Handell
- Sean Li
- vikas soni
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 5 comments![]( "Open in Gerrit")
+handellm to help answer some the questions wrt to VideoCaptureService in open comments
looking around the actual service code, looks like it calls synchronous establish channel:
https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;drc=eda1bdbd0694acebc51f1e1a7321c72c6e9421ed;l=184I think that's just going to deadlock (unless a channel is already created) regardless of which way to go. So sounds like this needs some VideoCaptureService changes required anyway.
I tried it offline on ChromeOS and Android. I kill GPU process and can see GPU channel can be re-establish directly.
looking around the actual service code, looks like it calls synchronous establish channel
Yes, currently Video capture service use sync call. If we want to use async call, we have to redefine the state and handling when the gpu channel is broken and reestablish.
viz::Gpu::Create(std::move(gpu_remote), GetIOThreadTaskRunner({}));why bind this to IO thread? seems like the service itself is bound to UI thread?
IIUC, the internal service side in `viz::Gpu` need an IO thread.
Trying to accommodate tests is not a good reason to decide one way or another. One way is don't leak the service, and actually destroy it when no longer needed. Or have the test do a clean up.
I use a cleanup function for testing instead.
Any client that uses the GPU has to be ready to handle GPU process going away due to a crash or whatever. That doesn't change whether creating a new channel or reusing the existing one.
What does that have to do with sync vs async? And again, you'll have to deal with GPU crash either way.
I think the main issue is, for existing gpu channel for compositor, it's not allowed to be created as sync. Not sure about the context, maybe it's now allowed to block the thread when browser is bringing up in a early stage.
However, the gpu channel establishment designed for VCS, it's a sync call. And VCS relies on it to do the implementation. If we want to reuse the existing gpu channel for compositor, maybe we have to redefine the behavior when GPU channel is broken.
&BindInProcessInstance, std::move(receiver), std::move(gpu_remote));if viz_gpu_bound is already true, then this is just an empty remote?
Yes. I use this to avoid multiple call on `SetVizGpu()`.
This feature is disabled by default already, see https://source.chromium.org/chromium/chromium/src/+/main:media/base/media_switches.cc;l=1128;drc=470d96df224c921a642f6d0d4f623ced20754b2e;bpv=1;bpt=1.
I don't really care for adding scoped feature lists at the base class here. If we are running into a han on shutdown we should use the TearDown method of this class to clean up anything that we think would hang further test execution.
It is enabled in [test config](https://source.chromium.org/chromium/chromium/src/+/main:testing/variations/fieldtrial_testing_config.json;l=1725;drc=9eb4f1086a9fd771cbaf18249370793842e5b007).
I add a cleanup function for it.
Related details
- Bo Liu
- Dave Tapuska
- Markus Handell
- vikas soni
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 abandoned this change.
Sean Li abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy added 3 comments![]( "Open in Gerrit")
Why is this done differently from other platforms?
Can you make use of media/capture/video/video_capture_gpu_channel_host.cc?
The comment states that it's already enabled: https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;l=301;drc=3a644a3bda5c62358d9af9bc0705e049d3017375
#if BUILDFLAG(IS_ANDROID) && BUILDFLAG(ENABLE_GPU_CHANNEL_MEDIA_CAPTURE)Do we need it as a build flag? I suggest removing it. The code should be behind some flag, e.g. `kAndroidZeroCopyVideoCapture`.
if (!is_gpu_channel_set) {This is essentially `if (false)`. The bool is assigned in the previous line. Or am I misreading the code?
Related details
- Bo Liu
- Henrik Boström
- 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. |
Bo Liu (Gerrit)
Bo Liu added 2 comments![]( "Open in Gerrit")
Why is this done differently from other platforms?
Can you make use of media/capture/video/video_capture_gpu_channel_host.cc?The comment states that it's already enabled: https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;l=301;drc=3a644a3bda5c62358d9af9bc0705e049d3017375
one technical difficulty is synchronous establish channel here I assume from the browser UI thread:
https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;l=186
is definitely going to deadlock on android *if* it involves starting the GPU process in the call. (It's fine if GPU process is already running, which is probably most cases). That's why it's NOTREACHED for android here:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.cc;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;l=328
ok, I didn't expect to abandon https://chromium-review.googlesource.com/c/chromium/src/+/7800745 and to have this written
I expect a discussion of the questions (not necessarily concerns) raised
the initial question is can and should this service share gpu channel with the compositor or create its own when it's in the browser; I guess this CL answers yes it can, but should it?
and then need to address the synchronous establish channel for android, regardless of which path to take
Related details
- Henrik Boström
- 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 3 comments![]( "Open in Gerrit")
Bo LiuWhy is this done differently from other platforms?
Can you make use of media/capture/video/video_capture_gpu_channel_host.cc?The comment states that it's already enabled: https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;l=301;drc=3a644a3bda5c62358d9af9bc0705e049d3017375
one technical difficulty is synchronous establish channel here I assume from the browser UI thread:
https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;l=186is definitely going to deadlock on android *if* it involves starting the GPU process in the call. (It's fine if GPU process is already running, which is probably most cases). That's why it's NOTREACHED for android here:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.cc;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;l=328
Hi Team,
`VizGpuContextProvider` and `viz::Gpu` was designed for the VCS utility process. I tried to init it in browser process in [7800745](https://chromium-review.git.corp.google.com/c/chromium/src/+/7800745) and it did work in Clank, but failed in tests.
It failed due to 2 following reasons.
1. It build the GPU channel synchronously and cause deadlock in tests. [Example run](https://ci.chromium.org/ui/p/chromium/builders/try/android-x86-rel/b8681274751681750625/test-results?q=ExactID%3A%3A%2F%2Fcontent%2Ftest%5C%3Acontent_browsertests%21gtest%3A%3ANetworkServiceBrowserSimpleCacheTest%23HttpCacheWrittenToDiskOnApplicationStateChange+VHash%3A32316f6899274cc2&clean=).
2. It doesn't release GPU channel and the test watcher regard it as a violation. Even we add them to the destructor, it will not be triggered since the VCS in browser process is a NonDestructor. We have to have some cleanup functions in every tests which will init the VCS.
Therefore, I think this CL may be a more suitable way to have a GPU channel for VCS in browser process.
ok, I didn't expect to abandon https://chromium-review.googlesource.com/c/chromium/src/+/7800745 and to have this written
I expect a discussion of the questions (not necessarily concerns) raised
the initial question is can and should this service share gpu channel with the compositor or create its own when it's in the browser; I guess this CL answers yes it can, but should it?
and then need to address the synchronous establish channel for android, regardless of which path to take
Hi Bo,
Sorry for not providing enough context. I think both of the CLs do work but I can't resolve the testing issue in [7800745](https://chromium-review.googlesource.com/c/chromium/src/+/7800745). Therefore, I choose to implement it by reusing the channel in browser process and make Android Video Capture Device adapt the async channel build. I put my conclusion in the other thread.
This is essentially `if (false)`. The bool is assigned in the previous line. Or am I misreading the code?
- Bo Liu
- Henrik Boström
- Ilya Nikolaevskiy
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. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
Bo LiuWhy is this done differently from other platforms?
Can you make use of media/capture/video/video_capture_gpu_channel_host.cc?The comment states that it's already enabled: https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;l=301;drc=3a644a3bda5c62358d9af9bc0705e049d3017375
Sean Lione technical difficulty is synchronous establish channel here I assume from the browser UI thread:
https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;l=186is definitely going to deadlock on android *if* it involves starting the GPU process in the call. (It's fine if GPU process is already running, which is probably most cases). That's why it's NOTREACHED for android here:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.cc;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;l=328
Hi Team,
`VizGpuContextProvider` and `viz::Gpu` was designed for the VCS utility process. I tried to init it in browser process in [7800745](https://chromium-review.git.corp.google.com/c/chromium/src/+/7800745) and it did work in Clank, but failed in tests.
It failed due to 2 following reasons.
1. It build the GPU channel synchronously and cause deadlock in tests. [Example run](https://ci.chromium.org/ui/p/chromium/builders/try/android-x86-rel/b8681274751681750625/test-results?q=ExactID%3A%3A%2F%2Fcontent%2Ftest%5C%3Acontent_browsertests%21gtest%3A%3ANetworkServiceBrowserSimpleCacheTest%23HttpCacheWrittenToDiskOnApplicationStateChange+VHash%3A32316f6899274cc2&clean=).
2. It doesn't release GPU channel and the test watcher regard it as a violation. Even we add them to the destructor, it will not be triggered since the VCS in browser process is a NonDestructor. We have to have some cleanup functions in every tests which will init the VCS.Therefore, I think this CL may be a more suitable way to have a GPU channel for VCS in browser process.
- Henrik Boström
- 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. |
Sean Li (Gerrit)
Sean Li added 1 comment![]( "Open in Gerrit")
Bo LiuWhy is this done differently from other platforms?
Can you make use of media/capture/video/video_capture_gpu_channel_host.cc?The comment states that it's already enabled: https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;l=301;drc=3a644a3bda5c62358d9af9bc0705e049d3017375
Sean Lione technical difficulty is synchronous establish channel here I assume from the browser UI thread:
https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;l=186is definitely going to deadlock on android *if* it involves starting the GPU process in the call. (It's fine if GPU process is already running, which is probably most cases). That's why it's NOTREACHED for android here:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.cc;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;l=328
Bo LiuHi Team,
`VizGpuContextProvider` and `viz::Gpu` was designed for the VCS utility process. I tried to init it in browser process in [7800745](https://chromium-review.git.corp.google.com/c/chromium/src/+/7800745) and it did work in Clank, but failed in tests.
It failed due to 2 following reasons.
1. It build the GPU channel synchronously and cause deadlock in tests. [Example run](https://ci.chromium.org/ui/p/chromium/builders/try/android-x86-rel/b8681274751681750625/test-results?q=ExactID%3A%3A%2F%2Fcontent%2Ftest%5C%3Acontent_browsertests%21gtest%3A%3ANetworkServiceBrowserSimpleCacheTest%23HttpCacheWrittenToDiskOnApplicationStateChange+VHash%3A32316f6899274cc2&clean=).
2. It doesn't release GPU channel and the test watcher regard it as a violation. Even we add them to the destructor, it will not be triggered since the VCS in browser process is a NonDestructor. We have to have some cleanup functions in every tests which will init the VCS.Therefore, I think this CL may be a more suitable way to have a GPU channel for VCS in browser process.
neither should affect the decision to share gpu channel or not
Hi @vika...@chromium.org,
Do you know if there is any side effect to share a gpu channel. Thanks.
Related details
- Bo Liu
- Henrik Boström
- Ilya Nikolaevskiy
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. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy added 2 comments![]( "Open in Gerrit")
Bo LiuWhy is this done differently from other platforms?
Can you make use of media/capture/video/video_capture_gpu_channel_host.cc?The comment states that it's already enabled: https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;l=301;drc=3a644a3bda5c62358d9af9bc0705e049d3017375
Sean Lione technical difficulty is synchronous establish channel here I assume from the browser UI thread:
https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;l=186is definitely going to deadlock on android *if* it involves starting the GPU process in the call. (It's fine if GPU process is already running, which is probably most cases). That's why it's NOTREACHED for android here:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/gpu/browser_gpu_channel_host_factory.cc;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;l=328
Bo LiuHi Team,
`VizGpuContextProvider` and `viz::Gpu` was designed for the VCS utility process. I tried to init it in browser process in [7800745](https://chromium-review.git.corp.google.com/c/chromium/src/+/7800745) and it did work in Clank, but failed in tests.
It failed due to 2 following reasons.
1. It build the GPU channel synchronously and cause deadlock in tests. [Example run](https://ci.chromium.org/ui/p/chromium/builders/try/android-x86-rel/b8681274751681750625/test-results?q=ExactID%3A%3A%2F%2Fcontent%2Ftest%5C%3Acontent_browsertests%21gtest%3A%3ANetworkServiceBrowserSimpleCacheTest%23HttpCacheWrittenToDiskOnApplicationStateChange+VHash%3A32316f6899274cc2&clean=).
2. It doesn't release GPU channel and the test watcher regard it as a violation. Even we add them to the destructor, it will not be triggered since the VCS in browser process is a NonDestructor. We have to have some cleanup functions in every tests which will init the VCS.Therefore, I think this CL may be a more suitable way to have a GPU channel for VCS in browser process.
Sean Lineither should affect the decision to share gpu channel or not
Hi @vika...@chromium.org,
Do you know if there is any side effect to share a gpu channel. Thanks.
Ok, it makes sence, but please document that in a comment. Note that because the capture service is run from the browser process instead of an utility, like on other platforms, a different way of establishing the GPU channel is needed.
I don't have a strong opinion on which GP
Related details
- Bo Liu
- Henrik Boström
- 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. |
vikas soni (Gerrit)
vikas soni added 1 comment![]( "Open in Gerrit")
sorry for the late reply.
I think sharing a gpu channel seems fine to me. with sharing there will be probably less overhead of creating 2 gpu channels. also mojo should be efficient enough to handle and multiplex tasks from various clients.
@bo...@chromium.org -> Do you have any particular concerns in sharing the gpu channel. Are there any specific side effects or hidden edge cases we aren't aware of.
Also establishing the GPU channel asynchronously seems to be the only feasible design for Android to prevent UI thread blocking and eventual deadlocks.
Overall this CL approach seems fine to me % Bo's concrens.
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
static bool is_gpu_channel_set = false;this reads wrong but that's mostly because of the existing code.. it makes no sense that if this is called a second time, that we simply drop the receiver. but looking at the code, it won't happen because the other side is in a SequenceLocalStorageSlot (so still broken if it's called from multiple threads).. but if there are two global-ish things, then they should really just be merged into one global-ish thing to avoid lifetime issues like this
for this change, might make sense for service to take gpu::GpuChannelEstablishFactory, which an interface that works in both browser and renderer processes, then have service handle establish channel internally
Related details
- Henrik Boström
- 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)
Message from Sean Li![]( "Open in Gerrit")
- Bo Liu
- Henrik Boström
- Ilya Nikolaevskiy
- Sean Li
Sean Li (Gerrit)
Sean Li added 1 comment![]( "Open in Gerrit")
static bool is_gpu_channel_set = false;this reads wrong but that's mostly because of the existing code.. it makes no sense that if this is called a second time, that we simply drop the receiver. but looking at the code, it won't happen because the other side is in a SequenceLocalStorageSlot (so still broken if it's called from multiple threads).. but if there are two global-ish things, then they should really just be merged into one global-ish thing to avoid lifetime issues like this
for this change, might make sense for service to take gpu::GpuChannelEstablishFactory, which an interface that works in both browser and renderer processes, then have service handle establish channel internally
Yes, I did consider this.
However, [gpu_workarounds](https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;l=324-333?q=video_capture%2Fvideo_capture_service_impl.cc&ss=chromium%2Fchromium%2Fsrc) have to be ready before we create the VCD factory. If we let service take gpu::GpuChannelEstablishFactory and get `gpu_channel_host` async in the service, there can be a race condition.
Therefore, the implementation here is to make sure `gpu_channel_host` always ready when VideoCaptureService is created.
Related details
- Bo Liu
- Henrik Boström
- Ilya Nikolaevskiy
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. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
static bool is_gpu_channel_set = false;Sean Lithis reads wrong but that's mostly because of the existing code.. it makes no sense that if this is called a second time, that we simply drop the receiver. but looking at the code, it won't happen because the other side is in a SequenceLocalStorageSlot (so still broken if it's called from multiple threads).. but if there are two global-ish things, then they should really just be merged into one global-ish thing to avoid lifetime issues like this
for this change, might make sense for service to take gpu::GpuChannelEstablishFactory, which an interface that works in both browser and renderer processes, then have service handle establish channel internally
Yes, I did consider this.
However, [gpu_workarounds](https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;l=324-333?q=video_capture%2Fvideo_capture_service_impl.cc&ss=chromium%2Fchromium%2Fsrc) have to be ready before we create the VCD factory. If we let service take gpu::GpuChannelEstablishFactory and get `gpu_channel_host` async in the service, there can be a race condition.
Therefore, the implementation here is to make sure `gpu_channel_host` always ready when VideoCaptureService is created.
gpu info can change though, and it is handled (but on win only? odd..)
https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;drc=d635c024a266e5abb2e1b8a68fe6a4fa7f7c5755;l=393
but maybe having no GpuChannelHost and invalid gpu info/workaround is a new state for service, but it sounds easier to manage than having creation of the initial channel here, but then have service handle creation on gpu crash?
maybe video pcature owners want to chime in here?
Related details
- Henrik Boström
- 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. |
Sean Li (Gerrit)
Sean Li added 1 comment![]( "Open in Gerrit")
static bool is_gpu_channel_set = false;Sean Lithis reads wrong but that's mostly because of the existing code.. it makes no sense that if this is called a second time, that we simply drop the receiver. but looking at the code, it won't happen because the other side is in a SequenceLocalStorageSlot (so still broken if it's called from multiple threads).. but if there are two global-ish things, then they should really just be merged into one global-ish thing to avoid lifetime issues like this
for this change, might make sense for service to take gpu::GpuChannelEstablishFactory, which an interface that works in both browser and renderer processes, then have service handle establish channel internally
Bo LiuYes, I did consider this.
However, [gpu_workarounds](https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;l=324-333?q=video_capture%2Fvideo_capture_service_impl.cc&ss=chromium%2Fchromium%2Fsrc) have to be ready before we create the VCD factory. If we let service take gpu::GpuChannelEstablishFactory and get `gpu_channel_host` async in the service, there can be a race condition.
Therefore, the implementation here is to make sure `gpu_channel_host` always ready when VideoCaptureService is created.
gpu info can change though, and it is handled (but on win only? odd..)
https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;drc=d635c024a266e5abb2e1b8a68fe6a4fa7f7c5755;l=393but maybe having no GpuChannelHost and invalid gpu info/workaround is a new state for service, but it sounds easier to manage than having creation of the initial channel here, but then have service handle creation on gpu crash?
maybe video pcature owners want to chime in here?
Hi Ilya,
Do you have any concern on the behavior of this CL? I think maybe we can land this CL with a TODO and add a new state for invalid gpu info later?
Thanks!
Related details
- Bo Liu
- Henrik Boström
- Ilya Nikolaevskiy
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. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy added 1 comment![]( "Open in Gerrit")
static bool is_gpu_channel_set = false;Sean Lithis reads wrong but that's mostly because of the existing code.. it makes no sense that if this is called a second time, that we simply drop the receiver. but looking at the code, it won't happen because the other side is in a SequenceLocalStorageSlot (so still broken if it's called from multiple threads).. but if there are two global-ish things, then they should really just be merged into one global-ish thing to avoid lifetime issues like this
for this change, might make sense for service to take gpu::GpuChannelEstablishFactory, which an interface that works in both browser and renderer processes, then have service handle establish channel internally
Bo LiuYes, I did consider this.
However, [gpu_workarounds](https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;l=324-333?q=video_capture%2Fvideo_capture_service_impl.cc&ss=chromium%2Fchromium%2Fsrc) have to be ready before we create the VCD factory. If we let service take gpu::GpuChannelEstablishFactory and get `gpu_channel_host` async in the service, there can be a race condition.
Therefore, the implementation here is to make sure `gpu_channel_host` always ready when VideoCaptureService is created.
Sean Ligpu info can change though, and it is handled (but on win only? odd..)
https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;drc=d635c024a266e5abb2e1b8a68fe6a4fa7f7c5755;l=393but maybe having no GpuChannelHost and invalid gpu info/workaround is a new state for service, but it sounds easier to manage than having creation of the initial channel here, but then have service handle creation on gpu crash?
maybe video pcature owners want to chime in here?
Hi Ilya,
Do you have any concern on the behavior of this CL? I think maybe we can land this CL with a TODO and add a new state for invalid gpu info later?
Thanks!
The gpuinfo change is handled on windows because there's no GPU connection there. Yet the capturer produces textures, so it has to be informed about possible GPU Adapter change.
If we have the connection to he GPU service in the VideoCaptureService, there's no need to inform it about the change externally. It can monitor the context loss internally for GPU infor change. However, if it uses SharedImageInterface instead of managing textures itself, there's no need to even monitor the GPUInfo change. The GPU service owns SIs, so capture doesn't need to do anything: it will just call SI methods and images will be created on the apropriate adapter inside GPU service. Some SIs would become invalid and some error handling will be needed, but that's a small issue.
Since the end goal of this project is to use SharedImageInterface, I think all we have to do is ensure that the connection is reestablished on the context loss.
Related details
- Bo Liu
- Henrik Boström
- 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")
static bool is_gpu_channel_set = false;Sean Lithis reads wrong but that's mostly because of the existing code.. it makes no sense that if this is called a second time, that we simply drop the receiver. but looking at the code, it won't happen because the other side is in a SequenceLocalStorageSlot (so still broken if it's called from multiple threads).. but if there are two global-ish things, then they should really just be merged into one global-ish thing to avoid lifetime issues like this
for this change, might make sense for service to take gpu::GpuChannelEstablishFactory, which an interface that works in both browser and renderer processes, then have service handle establish channel internally
Bo LiuYes, I did consider this.
However, [gpu_workarounds](https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;l=324-333?q=video_capture%2Fvideo_capture_service_impl.cc&ss=chromium%2Fchromium%2Fsrc) have to be ready before we create the VCD factory. If we let service take gpu::GpuChannelEstablishFactory and get `gpu_channel_host` async in the service, there can be a race condition.
Therefore, the implementation here is to make sure `gpu_channel_host` always ready when VideoCaptureService is created.
Sean Ligpu info can change though, and it is handled (but on win only? odd..)
https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;drc=d635c024a266e5abb2e1b8a68fe6a4fa7f7c5755;l=393but maybe having no GpuChannelHost and invalid gpu info/workaround is a new state for service, but it sounds easier to manage than having creation of the initial channel here, but then have service handle creation on gpu crash?
maybe video pcature owners want to chime in here?
Ilya NikolaevskiyHi Ilya,
Do you have any concern on the behavior of this CL? I think maybe we can land this CL with a TODO and add a new state for invalid gpu info later?
Thanks!
The gpuinfo change is handled on windows because there's no GPU connection there. Yet the capturer produces textures, so it has to be informed about possible GPU Adapter change.
If we have the connection to he GPU service in the VideoCaptureService, there's no need to inform it about the change externally. It can monitor the context loss internally for GPU infor change. However, if it uses SharedImageInterface instead of managing textures itself, there's no need to even monitor the GPUInfo change. The GPU service owns SIs, so capture doesn't need to do anything: it will just call SI methods and images will be created on the apropriate adapter inside GPU service. Some SIs would become invalid and some error handling will be needed, but that's a small issue.
Since the end goal of this project is to use SharedImageInterface, I think all we have to do is ensure that the connection is reestablished on the context loss.
Thanks for the context. Currently Android VCD doesn't use GPUInfo, so we only need to reset SharedImageInterface when gpu channel lost.
Related details
- Bo Liu
- Henrik Boström
- Ilya Nikolaevskiy
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 3 comments![]( "Open in Gerrit")
Done
#if BUILDFLAG(IS_ANDROID) && BUILDFLAG(ENABLE_GPU_CHANNEL_MEDIA_CAPTURE)Do we need it as a build flag? I suggest removing it. The code should be behind some flag, e.g. `kAndroidZeroCopyVideoCapture`.
Hi Ilya,
Are you refer to `ENABLE_GPU_CHANNEL_MEDIA_CAPTURE`? If so, I think we still need it, because IIUC, some OS(e.g. cast OS) doesn't support gpu channel and it will cause a compile error.
Sean Lithis reads wrong but that's mostly because of the existing code.. it makes no sense that if this is called a second time, that we simply drop the receiver. but looking at the code, it won't happen because the other side is in a SequenceLocalStorageSlot (so still broken if it's called from multiple threads).. but if there are two global-ish things, then they should really just be merged into one global-ish thing to avoid lifetime issues like this
for this change, might make sense for service to take gpu::GpuChannelEstablishFactory, which an interface that works in both browser and renderer processes, then have service handle establish channel internally
Bo LiuYes, I did consider this.
However, [gpu_workarounds](https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;l=324-333?q=video_capture%2Fvideo_capture_service_impl.cc&ss=chromium%2Fchromium%2Fsrc) have to be ready before we create the VCD factory. If we let service take gpu::GpuChannelEstablishFactory and get `gpu_channel_host` async in the service, there can be a race condition.
Therefore, the implementation here is to make sure `gpu_channel_host` always ready when VideoCaptureService is created.
Sean Ligpu info can change though, and it is handled (but on win only? odd..)
https://source.chromium.org/chromium/chromium/src/+/main:services/video_capture/video_capture_service_impl.cc;drc=d635c024a266e5abb2e1b8a68fe6a4fa7f7c5755;l=393but maybe having no GpuChannelHost and invalid gpu info/workaround is a new state for service, but it sounds easier to manage than having creation of the initial channel here, but then have service handle creation on gpu crash?
maybe video pcature owners want to chime in here?
Ilya NikolaevskiyHi Ilya,
Do you have any concern on the behavior of this CL? I think maybe we can land this CL with a TODO and add a new state for invalid gpu info later?
Thanks!
Sean LiThe gpuinfo change is handled on windows because there's no GPU connection there. Yet the capturer produces textures, so it has to be informed about possible GPU Adapter change.
If we have the connection to he GPU service in the VideoCaptureService, there's no need to inform it about the change externally. It can monitor the context loss internally for GPU infor change. However, if it uses SharedImageInterface instead of managing textures itself, there's no need to even monitor the GPUInfo change. The GPU service owns SIs, so capture doesn't need to do anything: it will just call SI methods and images will be created on the apropriate adapter inside GPU service. Some SIs would become invalid and some error handling will be needed, but that's a small issue.
Since the end goal of this project is to use SharedImageInterface, I think all we have to do is ensure that the connection is reestablished on the context loss.
Thanks for the context. Currently Android VCD doesn't use GPUInfo, so we only need to reset SharedImageInterface when gpu channel lost.
Added 2 TODOs.
1. The lifetime issue of the in-process VCS.
2. Make Android VCD adapt to asynchronously fetching `gpu_workarounds` so that we can handle all GPU channel initialization internally within the service.
Related details
- Bo Liu
- Henrik Boström
- Ilya Nikolaevskiy
- vikas soni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy voted Code-Review+1![]( "Open in Gerrit")
- Bo Liu
- Henrik Boström
- Sean Li
- vikas soni
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")
Hi Daniel and Avi,
Could you help to take a look since Henrik is on a parental leave? Thanks!
Related details
- Avi Drissman
- Bo Liu
- Daniel Cheng
- vikas soni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Avi Drissman
| Code-Review | +1 |
1 comment
- Bo Liu
- Daniel Cheng
- Sean Li
- vikas soni
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
2 comments
Mostly a stamp but with one request for comments.
// internally within the service.Nit: it looks like this class lives on multiple threads; it would be nice if there was a class that described the lifetime/threading requirements for this class
Related details
- Bo Liu
- Sean Li
- vikas soni
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. |