-
Notifications
You must be signed in to change notification settings - Fork 207
media: Add RendererType::kUrlPlayer and buildflag #10979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just
use_starboard_url_player = use_starboard_media && is_iosis fine. A comment describing the variable would be good too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
use_starboard_url_player = use_starboard_media && (is_ios || is_tvos), why not just useis_iosoris_tvosbehind the code gated byuse_starboard_media?As both
use_starboard_url_playeranduse_starboard_mediaare for Starboard Player, I think it's not ideal to introduce one more global build flag for a single Starboard platform.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. Let me walk through the reasoning.
use_starboard_url_playeris not a platform-specific flag, it is a capability flag. Today this capability is limited to tvOS, but conceptually it represents "this build can delegate playback to a platform URL player."At the Starboard API layer, we already have
SB_HAS(PLAYER_WITH_URL), the Starboard layer already treats URL player as a distinct capability with its own flag. However, that flag is defined in platform headers (starboard/tvos/shared/configuration_public.h) and is not available in GN, Mojom IDL, or upstream Chromium C++ files that do not include Starboard headers. So above Starboard, we need another way to express the same capability.There is no
is_tvosflag in the build system:Unlike
is_ios,is_mac, oris_linux, there is no general GNis_tvosvariable.Why a single GN variable is required:
Mojom
[EnableIf=...]accepts only a single feature name from GN. It cannot express compound conditions likeuse_starboard_media && is_ios && target_platform == "tvos". So we need a dedicated GN variable for the Mojom gate. Leaving the Mojom methods ungated ("always present") is not viable either as it forces every platform to implement URL player methods.We need these Mojom methods because in the URL player architecture the complete player lives in the GPU process, and we need IPC to share player status (duration, buffering info, etc.) back to the renderer/JS side.
Consistency across layers(like GN/Starboard/mojom/non-starboard):
Since we need a GN variable for the Mojom gate anyway, reusing the same derived flag in C++ keeps the URL player condition tied to one named capability. This is not trying to replace every tvOS platform guard in the tree. The platform-specific code can still use the appropriate platform guard for its layer. The point is narrower: URL player is a specific media capability, and the code that depends on that capability should not have to reconstruct it differently in each layer.
Using
[EnableIf=is_ios]would work mechanically for the current tvOS build, but it would make the Mojom method availability depend on the iOS platform bucket rather than on URL-player support. That is the wrong boundary for this IPC. The URL-player methods are needed because this build uses the URL-player architecture, where playback state has to cross process boundaries, not simply because the build is in the iOS family.With a named capability flag, the same concept is available consistently above the Starboard layer:
SB_HAS(PLAYER_WITH_URL)[EnableIf=use_starboard_url_player]BUILDFLAG(USE_STARBOARD_URL_PLAYER)The same reasoning applies on the C++ side. We could continue using the compound check (
BUILDFLAG(USE_STARBOARD_MEDIA) && BUILDFLAG(IS_IOS_TVOS)), but that condition is broader than URL player because it is also true for the regular Starboard media path. As URL player diverges further from the HTML5 player path, keeping that distinction explicit becomes more important.The flag is derived, not independent:
This is not a new user-facing configuration. It is derived in one place:
It is a named alias for the compound condition.
Naming:
We agree the current name is confusing. The
use_starboard_prefix makes this look like a sibling ofuse_starboard_media, while the intent is to describe URL player capability. We are happy to rename it to something clearer, for exampleenable_url_player,enable_platform_url_player, orhas_player_with_url.If we settle on a clearer name, for example
enable_url_player, the Starboard-side define(SB_HAS_PLAYER_WITH_URL) could also be aligned in a follow-up. That is not required for these CLs stack, but it would make the capability naming more consistent across layers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the place that tvOS need to delegate playbacks to SbPlayer using URL player? I haven't checked tvOS code yet, but I think there should be a way for tvOS to build it without additional GLOBAL build flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tvOS-specific delegation happens when HLS playback is initialized with a URL instead of demuxed media buffers.
Regular Starboard playback uses Chromium's media pipeline: Chromium fetches/demuxes the media and sends encoded buffers to Starboard through
SbPlayerWriteSamples(). In that path Chromium owns most of the pipeline state.The tvOS URL-player path is different. For HLS, tvOS needs to delegate playback to Apple's
AVPlayer. This native player accepts only URL of media file to play. In our Starboard layer that is exposed through the URL-player APIs in
starboard/tvos/shared/media/url_player.h:SbUrlPlayerCreate,SbUrlPlayerSetDrmSystem, andSbUrlPlayerGetExtraInfo. Our main responsibility is to send player url from chromium world to starboard world where this player lives but using chromium media pipeline.In the Chromium/Cobalt side,
UrlPlayerRendererWrapper::InitializeWithUrl()carries the URLinto the GPU-side renderer. That reaches
SbPlayerBridge::CreateUrlPlayer(), which callssbplayer_interface_->CreateUrlPlayer(...); the default implementation delegates toSbUrlPlayerCreate().Because AVPlayer owns manifest fetching, demuxing, buffering, decoding, and rendering in this
path, Chromium's normal demuxer/buffer pipeline is bypassed. But JS still needs media state such
as duration, buffered ranges, current time, ended/error, and encrypted init data. That is why
the URL-player path adds IPC/Mojom surface such as
InitializeWithUrl()to send URL from renderer to GPU and player status updates( like duration, buffered ranges) from GPU to renderer for status plumbing.I do not intend this to be an independent global knob. The flag is derived from the same tvOS/Starboard condition you mentioned. The reason it exists as a named build feature is that the affected code is above the Starboard layer: shared media/Mojo/Blink files do not include Starboard platform headers, so they cannot
use
SB_HAS(PLAYER_WITH_URL). Mojom also cannot directly reference compound condition.; it needsGN to expose a named feature for
[EnableIf=...].Once that named feature exists for Mojom, using the same flag consistently in GN and C++ means the URL-player capability is identified the same way at every layer above Starboard, rather than each layer reconstructing it from a different combination of platform and media flags.So this is effectively the "URL player capability" equivalent of
SB_HAS(PLAYER_WITH_URL)for the layers above Starboard.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments are splits to many PRs, but refer to https://github.com/youtube/cobalt/pull/11066/changes#r3501515960.