Skip to content

media: Add URL-player renderer client and GetMediaUrl - #10998

Closed
abhijeetk wants to merge 3 commits into
youtube:mainfrom
abhijeetk:url-player-2
Closed

media: Add URL-player renderer client and GetMediaUrl#10998
abhijeetk wants to merge 3 commits into
youtube:mainfrom
abhijeetk:url-player-2

Conversation

@abhijeetk

@abhijeetk abhijeetk commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

Implement UrlPlayerRendererClient for the renderer-process side of
URL-based playback on tvOS. The client extends MojoRendererWrapper,
invokes InitializeWithUrl() on the Mojo renderer pipe, and implements
StarboardRendererClientExtension callbacks for video-hole painting,
rendering mode updates, and SbWindow handle delivery.

Add MediaResource::GetMediaUrl() behind USE_STARBOARD_URL_PLAYER so
URL-based MediaResource implementations can provide the source URL.
The default returns an empty GURL; UrlPlayerDemuxer overrides it in
a later CL.

Build: ninja -C out/tvos-arm64-simulator_qa cobalt nplb media_unittests

Bug: 512045535

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Gemini Suggested Commit Message


media: Add URL-player renderer and GetMediaUrl

Implement UrlPlayerRendererClient to support URL-based playback on
tvOS. This client facilitates playback over Mojo by invoking
InitializeWithUrl on the renderer pipe, specifically for scenarios
where the platform handles media processing (e.g., HLS via AVPlayer).

Add MediaResource::GetMediaUrl() behind the STARBOARD_URL_PLAYER
build flag to allow the renderer to fetch the necessary URL from
the demuxer. This change includes the Mojo plumbing and wrapper
classes required to bridge the renderer and GPU processes.

Bug: 512045535

💡 Pro Tips for a Better Commit Message:

  1. Influence the Result: Want to change the output? You can write custom prompts or instructions directly in the Pull Request description. The model uses that text to generate the message.
  2. Re-run the Generator: Post a comment with: /generate-commit-message

Comment thread media/base/media_resource.h Outdated
Comment thread media/base/media_resource.h Outdated
Comment thread media/base/media_resource.cc Outdated
Add use_starboard_url_player GN capability flag, derived from
use_starboard_media and tvOS platform detection. Expose it as
BUILDFLAG(USE_STARBOARD_URL_PLAYER) through build_config.h and
register matching Mojo enabled_features entry.

Add RendererType::kUrlPlayer = 13 to the C++ enum, Mojom enum,
traits mapping, and histogram. The new enum is gated behind
USE_STARBOARD_URL_PLAYER so non-tvOS builds are unaffected.

This establishes the capability boundary for all subsequent
URL-player CLs.

Bug: 512045535
Introduce InitializeWithUrl(client, source_url) on the Mojo renderer
pipe to carry the media URL from the renderer process to the GPU
process, keeping URL delivery and initialization ordered on the same
pipe.

On the GPU side, add UrlPlayerRenderer for tvOS URL-based playback
via SbPlayerBridge in punch-out mode, and UrlPlayerRendererWrapper
as the Mojo service bridge. The wrapper sets the URL on the renderer
then calls Initialize().

On the renderer-process side, MojoRenderer and MojoRendererWrapper
forward InitializeWithUrl over the Mojo pipe. MojoRendererService
validates the renderer type and URL before forwarding, with runtime
guards for unexpected state and invalid URLs.

Bug: 512045535
@abhijeetk abhijeetk self-assigned this Jun 26, 2026
Implement UrlPlayerRendererClient for the renderer-process side of
URL-based playback on tvOS. The client extends MojoRendererWrapper,
invokes InitializeWithUrl() on the Mojo renderer pipe, and implements
StarboardRendererClientExtension callbacks for video-hole painting,
rendering mode updates, and SbWindow handle delivery.

Add MediaResource::GetMediaUrl() behind USE_STARBOARD_URL_PLAYER so
URL-based MediaResource implementations can provide the source URL.
The default returns an empty GURL; UrlPlayerDemuxer overrides it in
a later CL.

Build: ninja -C out/tvos-arm64-simulator_qa cobalt nplb media_unittests

Bug: 512045535
@abhijeetk
abhijeetk marked this pull request as ready for review June 26, 2026 16:33
@abhijeetk
abhijeetk requested review from a team as code owners June 26, 2026 16:33
@abhijeetk

Copy link
Copy Markdown
Collaborator Author

cc : @rakuco @jkim-julie @Gyuyoung

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new URL-based player renderer (UrlPlayerRenderer) and its associated Mojo wrapper and client components to support platform-mediated playback (such as HLS via AVPlayer on tvOS) using punch-out mode. The review feedback highlights several opportunities to improve robustness and error handling. Key recommendations include adding null checks for client_ and cdm_context to prevent potential null pointer dereferences, gracefully handling a null video_geometry_setter_service_ during initialization, notifying the client of errors when encountering unsupported rendering modes post-initialization, adding a default case for unknown player errors, and correctly handling counter resets for decoded or dropped frames to ensure accurate statistics reporting.

Comment on lines +130 to +134
if (!init_cb_.is_null()) {
std::move(init_cb_).Run(PIPELINE_ERROR_DISCONNECTED);
return;
}
client_->OnError(PIPELINE_ERROR_DISCONNECTED);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If a connection error occurs after initialization has failed or completed, client_ might be null or the object might be in a state where client_ is not set. Adding a null check for client_ prevents potential null pointer dereference crashes.

  if (!init_cb_.is_null()) {
    std::move(init_cb_).Run(PIPELINE_ERROR_DISCONNECTED);
    return;
  }
  if (client_) {
    client_->OnError(PIPELINE_ERROR_DISCONNECTED);
  }

Comment on lines +96 to +107
if (!init_cb_.is_null()) {
std::move(init_cb_).Run(pipeline_status());
}
return;
case StarboardRenderingMode::kInvalid:
LOG(ERROR) << "Invalid rendering mode.";
SetMojoRendererInitialized(
PipelineStatus(PIPELINE_ERROR_INITIALIZATION_FAILED,
"Invalid rendering mode for URL player"));
if (!init_cb_.is_null()) {
std::move(init_cb_).Run(pipeline_status());
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If the rendering mode changes to an unsupported mode (like kDecodeToTexture or kInvalid) after initialization has already completed (meaning init_cb_ is null), the client should be notified of the error via client_->OnError(...) rather than silently returning.

      if (!init_cb_.is_null()) {
        std::move(init_cb_).Run(pipeline_status());
      } else if (client_) {
        client_->OnError(pipeline_status());
      }
      return;
    case StarboardRenderingMode::kInvalid:
      LOG(ERROR) << "Invalid rendering mode.";
      SetMojoRendererInitialized(
          PipelineStatus(PIPELINE_ERROR_INITIALIZATION_FAILED,
                         "Invalid rendering mode for URL player"));
      if (!init_cb_.is_null()) {
        std::move(init_cb_).Run(pipeline_status());
      } else if (client_) {
        client_->OnError(pipeline_status());
      }

Comment on lines +71 to +73
// Subscribe to video geometry changes.
DCHECK(video_geometry_setter_service_);
video_geometry_setter_service_->GetVideoGeometryChangeSubscriber(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

video_geometry_setter_service_ is a raw pointer and could potentially be null. Checking it and failing initialization gracefully with an error callback is safer than relying solely on DCHECK which is disabled in debug builds.

Suggested change
// Subscribe to video geometry changes.
DCHECK(video_geometry_setter_service_);
video_geometry_setter_service_->GetVideoGeometryChangeSubscriber(
if (!video_geometry_setter_service_) {
std::move(init_cb).Run(PIPELINE_ERROR_INITIALIZATION_FAILED);
return;
}
// Subscribe to video geometry changes.
video_geometry_setter_service_->GetVideoGeometryChangeSubscriber(

Comment on lines +80 to +83
DCHECK(task_runner_->RunsTasksInCurrentSequence());
DCHECK(cdm_context);

if (cdm_context_ || SbDrmSystemIsValid(drm_system_)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In release builds, cdm_context could potentially be null. Adding a null check prevents a null pointer dereference when calling cdm_context->GetSbDrmSystem().

  DCHECK(task_runner_->RunsTasksInCurrentSequence());

  if (!cdm_context) {
    std::move(cdm_attached_cb).Run(false);
    return;
  }

  if (cdm_context_ || SbDrmSystemIsValid(drm_system_)) {

Comment on lines +460 to +463
case kSbPlayerErrorMax:
NOTREACHED();
break;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If a new error is added to SbPlayerError in the future, the switch statement will not handle it, causing the player to transition to STATE_ERROR without notifying the pipeline. Adding a default case ensures that any unknown errors are handled and reported gracefully.

    case kSbPlayerErrorMax:
      NOTREACHED();
      break;
    default:
      MEDIA_LOG(ERROR, media_log_) << "Unknown player error: " << error << " message: " << message;
      NotifyError(PIPELINE_ERROR_DECODE);
      break;
  }

Comment on lines +466 to +471
void UrlPlayerRenderer::NotifyError(PipelineStatus status) {
if (init_cb_) {
std::move(init_cb_).Run(status);
} else {
client_->OnError(status);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

All other methods in this class check if client_ is null before invoking callbacks on it. NotifyError should also check if client_ is null to prevent potential null pointer dereference crashes.

Suggested change
void UrlPlayerRenderer::NotifyError(PipelineStatus status) {
if (init_cb_) {
std::move(init_cb_).Run(status);
} else {
client_->OnError(status);
}
void UrlPlayerRenderer::NotifyError(PipelineStatus status) {
if (init_cb_) {
std::move(init_cb_).Run(status);
} else if (client_) {
client_->OnError(status);
}

Comment on lines +212 to +222
if (video_frames_decoded > last_video_frames_decoded_) {
statistics.video_frames_decoded =
video_frames_decoded - last_video_frames_decoded_;
last_video_frames_decoded_ = video_frames_decoded;
}

if (video_frames_dropped > last_video_frames_dropped_) {
statistics.video_frames_dropped =
video_frames_dropped - last_video_frames_dropped_;
last_video_frames_dropped_ = video_frames_dropped;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If the underlying player is recreated or reset (e.g., after a seek or loop), the decoded/dropped frame counters can reset to 0. If video_frames_decoded becomes less than last_video_frames_decoded_, the statistics will not be updated correctly until the new count exceeds the old high value. Handling counter resets ensures accurate statistics reporting.

  if (video_frames_decoded >= last_video_frames_decoded_) {
    statistics.video_frames_decoded =
        video_frames_decoded - last_video_frames_decoded_;
  } else {
    statistics.video_frames_decoded = video_frames_decoded;
  }
  last_video_frames_decoded_ = video_frames_decoded;

  if (video_frames_dropped >= last_video_frames_dropped_) {
    statistics.video_frames_dropped =
        video_frames_dropped - last_video_frames_dropped_;
  } else {
    statistics.video_frames_dropped = video_frames_dropped;
  }
  last_video_frames_dropped_ = video_frames_dropped;

Comment thread build/build_config.h
#define BUILDFLAG_INTERNAL_USE_STARBOARD_MEDIA() (0)
#endif

#if defined(ENABLE_BUILDFLAG_USE_STARBOARD_URL_PLAYER)

@borongc borongc Jun 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #10979 (comment), I think introducing one more flag for a single Starboard platform is not ideal.

@abhijeetk

Copy link
Copy Markdown
Collaborator Author

Thanks for all review comments. We are moving ahead with approach in PR #10698 (UrlPlayer using StarboardRenderer).
Closing this PR.

@abhijeetk abhijeetk closed this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants