[media] Move StarboardRenderer to MojoRenderer #5113
Conversation
borongc
force-pushed
the
sbplayer_in_gpu
branch
10 times, most recently
from
10d7c58 to
3acdba0
Compare
borongc
force-pushed
the
sbplayer_in_gpu
branch
6 times, most recently
from
aa08231 to
0db7fab
Compare
borongc
force-pushed
the
sbplayer_in_gpu
branch
from
0db7fab to
f3bdc98
Compare
| @@ -37,8 +37,10 @@ std::unique_ptr<AudioEncoder> CreatePlatformAudioEncoder( | |||
| return nullptr; | |||
| } | |||
|
|
|||
| #if !BUILDFLAG(USE_STARBOARD_MEDIA) | |||
There was a problem hiding this comment.
Hmm this is surprising. Does it collide with some similarly named Starboard class?
There was a problem hiding this comment.
Chromium linux build doesn't use MojoCdm, so it defines an empty class here.
For us, we need to check if MojoCdm works on linux build, and I'll put this as a TODO (as this requires Infra team to setup keyboxes to have an internal Chrobalt linux build).
As this PR overwrote CreateCdmFactory in GpuMojoMediaClient for both android and linux, I think MojoCdm would work for us.
There was a problem hiding this comment.
Put this TODO as b/406263478 in media/starboard/starboard_cdm.h.
| @@ -180,6 +185,18 @@ class MEDIA_MOJO_EXPORT GpuMojoMediaClient final : public MojoMediaClient { | |||
| const gfx::ColorSpace& target_color_space, | |||
| mojo::PendingRemote<stable::mojom::StableVideoDecoder> oop_video_decoder) | |||
| final; | |||
| #if BUILDFLAG(USE_STARBOARD_MEDIA) | |||
| std::unique_ptr<Renderer> CreateStarboardRenderer( | |||
There was a problem hiding this comment.
Shouldn't we create a media/mojo/services/gpu_mojo_media_client_starboard.(cc|h) to implement this functionality, like other platforms do? We'd need to go the CreateGpuMediaService() way.
It'd change things quite a bit for this CL though, by extending CreateAudio/VideoDecoder to have starboard-specific implementations, instead of adding a new method all around. Did you consider that route? Ah but I see the current factory methods would not allow for the plane id, durations, extensions etc, right? Maybe leave a note in the CL description about why adding all CreateStarboardRenderer() methods is a must.
There was a problem hiding this comment.
Created media/mojo/services/gpu_mojo_media_client_starboard.cc for Cobalt.
We need CreateStarboardRenderer to create Renderer on Gpu thread. For other chromium platforms, they only use Gpu thread for video/audio decoding/encoding, and cdm tasks, but for us, all of them are inside SbPlayer.
Typically, their Renderer is on utility thread (if MediaFoundation), or other platform-specified thread.
We can migrate to use CreateAudio/VideoDecoder after we break SbPlayer to two parts (media pipeline / hw decoding). But, for now, we will need this specific CreateStarboardRenderer.
| renderer_extension_receiver, | ||
| mojo::PendingRemote<mojom::StarboardRendererClientExtension> | ||
| client_extension_remote) { | ||
| return std::make_unique<StarboardRendererWrapper>( |
There was a problem hiding this comment.
This new code would be covered by media/mojo/services/media_service_unittest.cc tests?
If not then at the very least add a new test case to call this function. Creation-destruction is already enough for downstream tools like ASAN/TSAN etc and/or fuzzer to build upon, so a tiny bit of extra testing goes a long way.
Move StarboardRenderer to Gpu thread, and each of StarboardRenderer is ran on PooledSingleThreadTaskRunner. This allows Starboard to access Chrome_InProcGpuThread for graphical tasks. b/394368542
borongc
force-pushed
the
sbplayer_in_gpu
branch
from
f3bdc98 to
f16500a
Compare
Move StarboardRenderer to Gpu thread, and each of StarboardRenderer is ran on PooledSingleThreadTaskRunner. This allows Starboard to access
Chrome_InProcGpuThreadfor graphical tasks.This PR adds a
CreateStarboardRenderer()to the InterfaceFactory mojom with the purpose of running StarboardRenderer on Gpu thread; this forced an implementation in a number of places, namely MediaInterfaceFactory, MediaInterfaceProxy and FramelessMediaInterfaceProxy.b/394368542
b/378740140
b/405401569