[media] Report errors via MediaError to WebApp #4453
Conversation
borongc
force-pushed
the
android_error
branch
from
cc9b66d
to
24b8edd
Compare
|
Note that part of implementation of |
| @@ -68501,6 +68501,8 @@ Called by update_use_counter_css.py.--> | |||
| <int value="9" label="kCastStreaming"/> | |||
| <int value="10" label="kContentEmbedderDefined"/> | |||
| <int value="11" label="kTest"/> | |||
| <!-- Cobalt: Renderer for Starboard (SbPlayer) --> | |||
| <int value="12" label="kStarboard"/> | |||
media/starboard/BUILD.gn
Outdated
There was a problem hiding this comment.
Unrelated but I'd change l.15
assert(is_cobalt, "This file builds for cobalt builds, not Chromium builds")
with
assert(is_starboard_media)
There was a problem hiding this comment.
My understanding with a previous chat with Borong is this file may compile when use_starboard_media = false in the future : we want to compare cobalt with starboard media, cobalt without starboard media on android. I'll leave it to Borong to add the assert if needed.
There was a problem hiding this comment.
The BUILD.gn file is built when is_cobalt=true. StarboardRenderer is a subcomponent for //media (
cobalt/media/media_options.gni
Line 354 in 8125775
use_starboard_media, it is right here.
| client_->OnError(PIPELINE_ERROR_DECODE); | ||
| break; | ||
| case kSbPlayerErrorMax: | ||
| NOTREACHED(); |
There was a problem hiding this comment.
Would this kSbPlayerErrorMax ever appear?
| LOG(ERROR) << "StarboardRenderer::OnPlayerError() called with code " << error | ||
| << " and message \"" << message << "\""; | ||
| NOTIMPLEMENTED(); | ||
| switch (error) { |
There was a problem hiding this comment.
Should we have a fallback for other cases not specified now?
There was a problem hiding this comment.
For SbPlayer, we only have kSbPlayerErrorDecode and kSbPlayerErrorCapabilityChanged for errors. It should be fine here for not specifying a default case.
| @@ -2838,6 +2823,14 @@ std::unique_ptr<media::Renderer> WebMediaPlayerImpl::CreateRenderer( | |||
| media_metrics_provider_->SetRendererType(renderer_type_); | |||
| media_log_->SetProperty<MediaLogProperty::kRendererName>(renderer_type_); | |||
|
|
|||
| #if BUILDFLAG(USE_STARBOARD_MEDIA) | |||
| LOG(INFO) << "Renderer Type is " << GetRendererName(renderer_type_) << "."; | |||
There was a problem hiding this comment.
This is already logged in Media DevTools in l.2824, is this extra LOG() needed?
There was a problem hiding this comment.
We may not always use DevTools for media. This can ensure we can have this in adb bugreport.
|
@niranjanyardi @andrewsavage1 for the questions on GN files. |
This PR adds `StarboardRendererFactory`, which contains `media_log_` to `StarboardRenderer`. Chromium uses both `RendererClient::OnError` and `media_log_` to report error code and message to MediaError. As a result, Cobalt adapts it from C25 to report error messages to WebApp. b/375271948 b/375278384
borongc
force-pushed
the
android_error
branch
from
24b8edd
to
153dc3c
Compare
This PR adds
StarboardRendererFactory, which containsmedia_log_toStarboardRenderer, and is necessary for error reporting.Chromium uses both
RendererClient::OnErrorandmedia_log_to report error code and message toMediaError. As a result, Cobalt adapts it from C25 to report error messages to WebApp.b/375271948
b/375278384