Media Foundation for Clear: MF->default renderer fallback on error [chromium/src : main]
Brian Beecher (Gerrit)
Attention is currently required from: Dale Curtis, Xiaohan Wang, William Carr, Frank Li.
Brian Beecher would like Dale Curtis, Xiaohan Wang, William Carr and Frank Li to review this change.
Media Foundation for Clear: MF->default renderer fallback on error
This change implements a renderer fallback path for the MF for Clear
feature. With this change we'll fall back to the default Chromium
renderer if/when the Media Foundation renderer fails for clear playback.
Bug: 1295957
Change-Id: Ifb9194e108ec5388e003d811b1dfeaeca674f33e
---
M content/renderer/media/media_factory.cc
M media/base/pipeline_impl.cc
M media/base/pipeline_impl.h
M media/base/pipeline_impl_unittest.cc
M media/remoting/integration_test.cc
M media/renderers/win/media_engine_notify_impl.cc
M media/renderers/win/media_foundation_renderer_integration_test.cc
M media/test/pipeline_integration_test_base.cc
M media/test/pipeline_integration_test_base.h
M third_party/blink/renderer/core/exported/web_media_player_impl_unittest.cc
M third_party/blink/renderer/platform/media/web_media_player_impl.cc
M third_party/blink/renderer/platform/media/web_media_player_impl.h
12 files changed, 332 insertions(+), 91 deletions(-)
To view, visit change 3470178. To unsubscribe, or for help writing mail filters, visit settings.
Brian Beecher (Gerrit)
Attention is currently required from: Dale Curtis, Xiaohan Wang, William Carr, Frank Li.
1 comment:
Patchset:
This change is ready for review.
To view, visit change 3470178. To unsubscribe, or for help writing mail filters, visit settings.
Xiaohan Wang (Gerrit)
Attention is currently required from: Dale Curtis, Brian Beecher, William Carr, Frank Li.
6 comments:
Patchset:
Thanks! I didn't review all the details (tests). But I have some high level comments for discussion.
File media/base/pipeline_impl.h:
Patch Set #1, Line 35:
absl::optional<RendererType>)>;Please comment on what the new parameters are and why we need them.
File media/base/pipeline_impl.cc:
renderer_type_ && *renderer_type_ == RendererType::kMediaFoundation &&
default_renderer_type &&
*default_renderer_type == RendererType::kDefaultThis makes me a bit sad. If we really need the RendererType (see below), it seems to me we should add a media::Renderer::GetType() function instead of passing it through multiple layers. (If we decide to do it, it should be in a separate CL.)
Patch Set #1, Line 462:
// or if playback fails the error will be propagated.hmm, I wonder whether we should check this earlier in all cases (and on all platforms). I don't see a case where we want to resume in an error state.... If so, then we don't need to check all the types...
+dalecurtis for his insights on this as well
Patch Set #1, Line 618:
!media::SupportMediaFoundationClearPlayback()) {Sorry I don't understand this logic. I though if the CDM requires MFR we should always create MFR? Or are you saying if media::SupportMediaFoundationClearPlayback() is true we MFR should already be the base renderer so we don't need to reset the renderer_type? If so, it'd be great to have some comments and even better a DCHECK...
File third_party/blink/renderer/platform/media/web_media_player_impl.cc:
Patch Set #1, Line 1951:
if (status == media::PIPELINE_ERROR_DECODE &&MFR could return other error statuses, e.g. PIPELINE_ERROR_COULD_NOT_RENDER
https://source.chromium.org/chromium/chromium/src/+/main:media/renderers/win/media_foundation_renderer.cc;l=443Any reason we don't want to fallback on all errors?
FWIW, PipelineStatus is now a TypedStatus. You can attach some "cause" if needed if that helps with fallback logic.
To view, visit change 3470178. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Brian Beecher, Xiaohan Wang, William Carr, Frank Li.
2 comments:
File media/base/pipeline_impl.cc:
renderer_type_ && *renderer_type_ == RendererType::kMediaFoundation &&
default_renderer_type &&
*default_renderer_type == RendererType::kDefault
This makes me a bit sad. […]
Yes, I agree we should growing any more code under BUILDFLAG here. Even the section below I think should be removed in favor of adding necessary functions on all platforms.
Patch Set #1, Line 462:
// or if playback fails the error will be propagated.hmm, I wonder whether we should check this earlier in all cases (and on all platforms). […]
I don't think we want to resume from error state on any platform. I don't think this should be controlled at this layer, but instead the renderer factory selector.
To view, visit change 3470178. To unsubscribe, or for help writing mail filters, visit settings.
Xiaohan Wang (Gerrit)
Attention is currently required from: Dale Curtis, Brian Beecher, William Carr, Frank Li.
1 comment:
File media/base/pipeline_impl.cc:
renderer_type_ && *renderer_type_ == RendererType::kMediaFoundation &&
default_renderer_type &&
*default_renderer_type == RendererType::kDefault
Yes, I agree we should growing any more code under BUILDFLAG here. […]
Thinking more on this, you could also just add something like media::Renderer::ShouldFallbackToDefaultRendererOnError(), which will by default return false so most Render impls don't need to override it. Then MediaFoundationRendererClient can override based on whether `cdm_context_` is set or not. WDYT?
To view, visit change 3470178. To unsubscribe, or for help writing mail filters, visit settings.