media: Pass H5vcc settings from GlobalFeatures to StarboardRenderer #7836
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new mojo interface to pass settings from GlobalFeatures in the browser process down to SbPlayer. Specifically, it plumbs the use_external_allocator setting. The changes are quite extensive, touching build files, browser and renderer code, and various parts of the media stack. My review has identified a critical issue in the build configuration, a high-severity race condition in how the setting is fetched asynchronously, and a medium-severity issue regarding the robustness of parsing the setting's value.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Mojo interface, CobaltSettings, to allow the renderer process to synchronously query settings from the browser process. This is used to plumb the use_external_allocator setting from GlobalFeatures all the way down to SbPlayer. The changes are mostly plumbing this new boolean flag through various layers. The overall approach is sound, and the implementation is clean. I have a couple of suggestions to improve robustness and maintainability.
kjyoun
changed the title
kjyoun
changed the title
cobalt/browser/metrics/cobalt_metrics_services_manager_client.cc
Outdated
Show resolved
Hide resolved
| int64 int_value; | ||
| }; | ||
|
|
||
| interface CobaltSettings { |
There was a problem hiding this comment.
Should we consider to make this H5vccSettings? As most of setting-related naming, we use h5vcc.
There was a problem hiding this comment.
Renamed to H5vssSettings
kjyoun
force-pushed
the
global-features-connect-mojo-2
branch
from
e20ae66 to
32fdbeb
Compare
cobalt/browser/metrics/cobalt_metrics_services_manager_client.cc
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/coat/CobaltActivity.java
Show resolved
Hide resolved
kjyoun
force-pushed
the
global-features-connect-mojo-2
branch
from
32fdbeb to
bcc50e7
Compare
kjyoun
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively plumbs H5vcc settings from the browser process to the StarboardRenderer in the renderer process. The introduction of the cobalt::mojom::H5vccSettings Mojo interface for synchronously querying settings during initialization is a clean solution. The changes are well-structured and consistently applied throughout the codebase, from the browser to the renderer and down into the media stack. The IPC mechanisms and data structures are correctly implemented. I have one minor suggestion to improve the handling of std::variant for better code safety and readability. Overall, this is a high-quality contribution.
|
I updated PR to pass H5vcc settings map, instead of one boolean value. Since we are planning to add several experiments on h5vcc setting, it would be more efficient to pass the map data once though mojo call. Otherwise, we need to call mojor APIs for each experiment value. |
| renderer_factory_traits->audio_write_duration_remote = | ||
| base::Microseconds(kSbPlayerWriteDurationRemote); | ||
|
|
||
| mojo::Remote<cobalt::mojom::H5vccSettings> cobalt_settings; |
There was a problem hiding this comment.
For L193 to L195, mojo::Remote can be bind when CobaltContentRendererClient::RenderFrameCreated(), so the mojo connection can stay as long as Renderer thread exists.
Then, when Cobalt creates WebMediaPlayer, Cobalt can get the setting every time (L197--L207).
| #include "mojo/public/cpp/bindings/pending_remote.h" | ||
|
|
||
| #if BUILDFLAG(USE_STARBOARD_MEDIA) | ||
| #include "media/base/starboard/starboard_renderer_config.h" |
There was a problem hiding this comment.
Why MojoRendererFactory need to include this?
| #ifndef MEDIA_MOJO_SERVICES_GPU_MOJO_MEDIA_CLIENT_H_ | ||
| #define MEDIA_MOJO_SERVICES_GPU_MOJO_MEDIA_CLIENT_H_ | ||
|
|
||
| #include <map> |
There was a problem hiding this comment.
Including map and string is not gated by USE_STARBOARD_MEDIA. Consider to put them to media/base/starboard/starboard_renderer_config.h so we don't need to add them here.
| mojo::PendingRemote<stable::mojom::StableVideoDecoder> oop_video_decoder, | ||
| base::OnceCallback<void( | ||
| mojo::PendingRemote<stable::mojom::StableVideoDecoder>)> cb) final; | ||
| base::OnceCallback< |
There was a problem hiding this comment.
Is this caused by formating?
3 participants
This PR plumbs H5vcc settings from
GlobalFeaturesin the browser process toStarboardRendererin the renderer process.It introduces a new Mojo interface,
cobalt::mojom::H5vccSettings, to allow the renderer to synchronously query all settings from the browser during initialization.CobaltContentRendererClientnow queries these settings duringRenderThreadStartedand passes them toStarboardRendererviaRendererFactoryTraitsandStarboardRendererConfig.Bug: 455661813