Media Foundation Frame Server Mode: Part 2: Feature Flag [chromium/src : main]
William Carr (Gerrit)
Attention is currently required from: Dale Curtis, Xiaohan Wang, Frank Li, David Springgay.
William Carr would like Dale Curtis, Xiaohan Wang, Frank Li and David Springgay to review this change.
Media Foundation Frame Server Mode: Part 2: Feature Flag
Add feature flag for Media Foundation for Clear & allow Media Factory to register Media Foundation Renderer as the default web media player when the flag is enabled on supported systems.
Bug: 1258887
Change-Id: I08d95f868bc6cb308ae27ffafbf97f5f162dacbd
---
M chrome/browser/flag_descriptions.cc
M media/base/media_switches.cc
M chrome/browser/about_flags.cc
A media/renderers/win/media_foundation_helpers.h
M chrome/browser/flag_descriptions.h
M chrome/browser/flag-metadata.json
M tools/metrics/histograms/enums.xml
M content/renderer/media/media_factory.cc
A media/renderers/win/media_foundation_helpers.cc
M media/renderers/BUILD.gn
M content/browser/media/media_interface_proxy.cc
M media/base/media_switches.h
12 files changed, 111 insertions(+), 3 deletions(-)
To view, visit change 3241732. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Dale Curtis, Xiaohan Wang, Frank Li, David Springgay.
Frank Li (Gerrit)
Attention is currently required from: Dale Curtis, Xiaohan Wang, William Carr, David Springgay.
2 comments:
File content/browser/media/media_interface_proxy.cc:
Patch Set #1, Line 60:
media/renderers/win/media_foundation_helpers.hI wonder we should move the code of it into media\base\win\mf_helpers.{h, cc}
File content/renderer/media/media_factory.cc:
Patch Set #1, Line 770:
RendererType::kMediaPlayerAt the moment, this is enum value is used in Android platform only. It seems we should add another enum such as RendererType::kMediaFoundationClear to make it clear for the intention.
BTW, web_media_player_impl.cc!UsesAudioService() needs an update with the enum value.
To view, visit change 3241732. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Xiaohan Wang, William Carr, Frank Li, David Springgay.
3 comments:
Patchset:
lg2m % renderer type.
File content/renderer/media/media_factory.cc:
Patch Set #1, Line 770:
RendererType::kMediaPlayer
At the moment, this is enum value is used in Android platform only. […]
Yeah this is the wrong type, can we not just use the same kMediaFoundation type above? Why do we need a second media foundataion entry?
File media/renderers/win/media_foundation_helpers.cc:
Patch Set #1, Line 13:
bool MediaFoundationHelpers::SupportClearContent() {media/base/win/mf_helpers seems like a better place.
To view, visit change 3241732. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Dale Curtis, Xiaohan Wang, Frank Li, David Springgay.
4 comments:
Patchset:
I realized that with MF for Clear enabled Widevine content would be broken so I've added in a measure to address this in addition to the patchset 1 feedback changes.
File content/browser/media/media_interface_proxy.cc:
Patch Set #1, Line 60:
media/renderers/win/media_foundation_helpers.h
I wonder we should move the code of it into media\base\win\mf_helpers. […]
I can do this. Note I'll need to refactor a little to remove the feature check due to a dep cycle, so callers have been updated to perform the feature check themselves:
c:\chromium\src\out\release_x64>autoninja chrome
call C:\chromium\depot_tools\ninja.bat chrome -j 560
[0/1] Regenerating ninja files
ERROR Dependency cycle:
//ui/gl:gl ->
//media/base/win:media_foundation_util ->
//media/base:base ->
//gpu/ipc/common:common ->
//gpu/ipc/common:ipc_common_sources ->
//ui/gl:gl
File content/renderer/media/media_factory.cc:
Patch Set #1, Line 770:
RendererType::kMediaPlayer
Yeah this is the wrong type, can we not just use the same kMediaFoundation type above? Why do we nee […]
Yeah no problem - this was a hold-over from when we first started experimenting with this a few years back so I don't recall the reasoning for using the MediaPlayer type.
File media/renderers/win/media_foundation_helpers.cc:
Patch Set #1, Line 13:
bool MediaFoundationHelpers::SupportClearContent() {media/base/win/mf_helpers seems like a better place.
Ack
To view, visit change 3241732. To unsubscribe, or for help writing mail filters, visit settings.
Frank Li (Gerrit)
Attention is currently required from: Xiaohan Wang, William Carr, David Springgay.
Patch set 2:Code-Review +1
2 comments:
Patchset:
lgtm % nit
File media/base/pipeline_impl.cc:
Patch Set #2, Line 598:
enablednit.
enabled,
To view, visit change 3241732. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Xiaohan Wang, Olga Sharonova, mark a. foltz, David Springgay.
William Carr would like Olga Sharonova and mark a. foltz to review this change.
Media Foundation Frame Server Mode: Part 2: Feature Flag
Add feature flag for Media Foundation for Clear & allow Media Factory to register Media Foundation Renderer as the default web media player when the flag is enabled on supported systems.
Bug: 1258887
Change-Id: I08d95f868bc6cb308ae27ffafbf97f5f162dacbd
---
M chrome/browser/flag_descriptions.cc
M media/base/media_switches.cc
M chrome/browser/about_flags.cc
M chrome/browser/flag_descriptions.h
M chrome/browser/flag-metadata.json
M media/base/win/mf_helpers.h
M tools/metrics/histograms/enums.xml
M content/renderer/media/media_factory.cc
M media/base/pipeline_impl.cc
M media/base/win/mf_helpers.cc
M content/browser/media/media_interface_proxy.cc
M media/base/media_switches.h
12 files changed, 97 insertions(+), 7 deletions(-)
William Carr (Gerrit)
Attention is currently required from: Xiaohan Wang, Olga Sharonova, mark a. foltz, David Springgay.
2 comments:
Patchset:
Olga & Mark - could you suggest an appropriate reviewer for the content/browser/media files modified?
Thanks,
Bill
File media/base/pipeline_impl.cc:
Patch Set #2, Line 598:
enabled
nit. […]
Ack
To view, visit change 3241732. To unsubscribe, or for help writing mail filters, visit settings.
mark a. foltz (Gerrit)
Attention is currently required from: Xiaohan Wang, William Carr, Olga Sharonova, David Springgay.
1 comment:
Patchset:
xhwang@ is an OWNER here.
To view, visit change 3241732. To unsubscribe, or for help writing mail filters, visit settings.
Xiaohan Wang (Gerrit)
Attention is currently required from: William Carr, Olga Sharonova, David Springgay.
6 comments:
Patchset:
Thanks for the change and sorry about the delay. I just have a few comments for discussion.
File chrome/browser/flag-metadata.json:
Patch Set #5, Line 2205:
"wicarr"I am not sure about this one, but typically the default is @chromium.org. So you might want to use the full email wic...@microsoft.com here.
File content/browser/media/media_interface_proxy.cc:
base::FeatureList::IsEnabled(media::kMediaFoundationClear) &&
media::MediaFoundationHelpers::SupportClearContent())This combination shows up in so many places that I feel we should have a help function for it. Or maybe check the feature in MediaFoundationHelpers::SupportClearContent()?
Patch Set #5, Line 461:
DLOG(ERROR) << "Hardware secure decryption disabled!";This comment needs to be updated now
File media/base/media_switches.cc:
Patch Set #5, Line 771:
kMediaFoundationClearnit: Typically the feature name matches the string, otherwise people can get confused, e.g. needs to use the string in commandline but uses a different feature name in the code. I'd suggest we use kMediaFoundationClearPlayback, which is more readable to me. That name is longer, but it's fine since we probably want to add a helper function, see above.
File media/base/win/mf_helpers.h:
Patch Set #5, Line 98:
static bool SupportClearContent();nit: Do you plan to add more functions here in the future? If not just use a function without a class?
To view, visit change 3241732. To unsubscribe, or for help writing mail filters, visit settings.
Olga Sharonova (Gerrit)
Attention is currently required from: William Carr, David Springgay.
1 comment:
Patchset:
Dale already reviewed media parts.
To view, visit change 3241732. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: Xiaohan Wang, David Springgay.
5 comments:
File chrome/browser/flag-metadata.json:
Patch Set #5, Line 2205:
"wicarr"
I am not sure about this one, but typically the default is @chromium.org. […]
Done
File content/browser/media/media_interface_proxy.cc:
base::FeatureList::IsEnabled(media::kMediaFoundationClear) &&
media::MediaFoundationHelpers::SupportClearContent())
This combination shows up in so many places that I feel we should have a help function for it. […]
That was actually how I originally implemented but it was pointed out that there was an existing file for media foundation helpers.
The existing mf_helpers function cannot include media/base deps as that introduces a circular dependency.
What I'll do is make a new file that lives in the same path as mf_helpers but that is built into media/base so that it can do the feature check.
Patch Set #5, Line 461:
DLOG(ERROR) << "Hardware secure decryption disabled!";This comment needs to be updated now
Done
File media/base/media_switches.cc:
Patch Set #5, Line 771:
kMediaFoundationClear
nit: Typically the feature name matches the string, otherwise people can get confused, e.g. […]
Done
File media/base/win/mf_helpers.h:
Patch Set #5, Line 98:
static bool SupportClearContent();
nit: Do you plan to add more functions here in the future? If not just use a function without a clas […]
No planned functions at the moment - will move to a function.
To view, visit change 3241732. To unsubscribe, or for help writing mail filters, visit settings.
Xiaohan Wang (Gerrit)
Attention is currently required from: William Carr, David Springgay.
Patch set 6:Code-Review +1
2 comments:
Patchset:
lgtm % nit. Thanks!
File content/browser/media/media_interface_proxy.cc:
Patch Set #6, Line 181:
Contentnit: "Playback" to be consistent?
To view, visit change 3241732. To unsubscribe, or for help writing mail filters, visit settings.
William Carr (Gerrit)
Attention is currently required from: William Carr, David Springgay.
Patch set 7:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: media/base/win/mf_feature_checks.h
Insertions: 1, Deletions: 1.
@@ -8,7 +8,7 @@
#include "media/base/media_export.h"
namespace media {
-MEDIA_EXPORT bool SupportMediaFoundationClearContent();
+MEDIA_EXPORT bool SupportMediaFoundationClearPlayback();
} // namespace media
#endif // MEDIA_BASE_WIN_MF_FEATURE_CHECKS_H_
```
```
The name of the file: content/renderer/media/media_factory.cc
Insertions: 1, Deletions: 1.
@@ -737,7 +737,7 @@
#endif
#if BUILDFLAG(IS_WIN)
- bool use_mf_for_clear = media::SupportMediaFoundationClearContent();
+ bool use_mf_for_clear = media::SupportMediaFoundationClearPlayback();
// Only use MediaFoundationRenderer when MediaFoundationCdm is available or
// MediaFoundation for Clear is supported.
if (media::MediaFoundationCdm::IsAvailable() || use_mf_for_clear) {
```
```
The name of the file: media/base/pipeline_impl.cc
Insertions: 1, Deletions: 1.
@@ -593,7 +593,7 @@
if (cdm_context_) {
if (cdm_context_->RequiresMediaFoundationRenderer()) {
renderer_type = RendererType::kMediaFoundation;
- } else if (media::SupportMediaFoundationClearContent()) {
+ } else if (media::SupportMediaFoundationClearPlayback()) {
// When MediaFoundation for Clear is enabled, the base renderer
// type is set to MediaFoundation. In order to ensure DRM systems
// built on non-Media Foundation pipelines continue to work we
```
```
The name of the file: media/base/win/mf_feature_checks.cc
Insertions: 1, Deletions: 1.
@@ -9,7 +9,7 @@
namespace media {
-bool SupportMediaFoundationClearContent() {
+bool SupportMediaFoundationClearPlayback() {
return base::win::GetVersion() >= base::win::Version::WIN10_RS3 &&
base::FeatureList::IsEnabled(media::kMediaFoundationClearPlayback);
}
```
```
The name of the file: content/browser/media/media_interface_proxy.cc
Insertions: 2, Deletions: 2.
@@ -178,7 +178,7 @@
void CreateDCOMPSurfaceRegistry(
mojo::PendingReceiver<media::mojom::DCOMPSurfaceRegistry> receiver)
override {
- if (media::SupportMediaFoundationClearContent() ||
+ if (media::SupportMediaFoundationClearPlayback() ||
(base::FeatureList::IsEnabled(media::kHardwareSecureDecryption) &&
media::MediaFoundationCdm::IsAvailable())) {
// TODO(crbug.com/1233379): Pass IO task runner and remove the PostTask()
@@ -454,7 +454,7 @@
DCHECK(thread_checker_.CalledOnValidThread());
// TODO(xhwang): Also check protected media identifier content setting.
- if (!media::SupportMediaFoundationClearContent() &&
+ if (!media::SupportMediaFoundationClearPlayback() &&
!base::FeatureList::IsEnabled(media::kHardwareSecureDecryption)) {
DLOG(ERROR) << "Both hardware secure decryption & media foundation for "
"clear are disabled!";
```
Media Foundation Frame Server Mode: Part 2: Feature Flag
Add feature flag for Media Foundation for Clear & allow Media Factory to register Media Foundation Renderer as the default web media player when the flag is enabled on supported systems.
Bug: 1258887
Change-Id: I08d95f868bc6cb308ae27ffafbf97f5f162dacbd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3241732
Commit-Queue: William Carr <wic...@microsoft.com>
Reviewed-by: Xiaohan Wang <xhw...@chromium.org>
Reviewed-by: Frank Li <fra...@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#939188}
---
M chrome/browser/flag_descriptions.cc
M media/base/media_switches.cc
M chrome/browser/about_flags.cc
M chrome/browser/flag_descriptions.h
A media/base/win/mf_feature_checks.h
M chrome/browser/flag-metadata.json
M media/base/win/BUILD.gn
M tools/metrics/histograms/enums.xml
M content/renderer/media/media_factory.cc
M media/base/pipeline_impl.cc
A media/base/win/mf_feature_checks.cc
M media/base/win/mf_helpers.cc
M content/browser/media/media_interface_proxy.cc
M media/base/media_switches.h
M media/base/BUILD.gn
15 files changed, 123 insertions(+), 11 deletions(-)