Skip to content
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.

Sign up for GitHub

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

Disable progressive playback support by default #4547

Merged
merged 7 commits into from
Dec 19, 2024
Merged

Disable progressive playback support by default #4547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e6d8eef
Add BASE_FEATURE flag for progressive playback
osagie98 Dec 9, 2024
dc96748
Add media element test
osagie98 Dec 10, 2024
90e71ea
Change comment
osagie98 Dec 10, 2024
51d4d66
Remove test
osagie98 Dec 11, 2024
dc3f57d
Re add unit test
osagie98 Dec 12, 2024
dc4f6de
Merge branch 'main' into progressive-flag
osagie98 Dec 18, 2024
573c11c
Disable progressive support by default
osagie98 Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions media/base/media_switches.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,13 @@ const base::FeatureParam<int> kDecreaseProcessingAudioFifoSizeValue{

#endif

#if BUILDFLAG(USE_STARBOARD_MEDIA)
// When disabled, Cobalt rejects progressive video formats.
BASE_FEATURE(kCobaltProgressivePlayback,
"CobaltProgressivePlayback",
base::FEATURE_DISABLED_BY_DEFAULT);
#endif // BUILDFLAG(USE_STARBOARD_MEDIA)

#if BUILDFLAG(IS_CHROMEOS)
// To control running audio communication effect on Chrome OS Audio Server.
BASE_FEATURE(kCrOSSystemAEC,
Expand Down
3 changes: 3 additions & 0 deletions media/base/media_switches.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,9 @@ MEDIA_EXPORT BASE_DECLARE_FEATURE(kDecreaseProcessingAudioFifoSize);
MEDIA_EXPORT extern const base::FeatureParam<int>
kDecreaseProcessingAudioFifoSizeValue;
#endif
#if BUILDFLAG(USE_STARBOARD_MEDIA)
MEDIA_EXPORT BASE_DECLARE_FEATURE(kCobaltProgressivePlayback);
#endif // BUILDFLAG(USE_STARBOARD_MEDIA)
#if BUILDFLAG(IS_CHROMEOS)
MEDIA_EXPORT BASE_DECLARE_FEATURE(kCrOSSystemAEC);
MEDIA_EXPORT BASE_DECLARE_FEATURE(kCrOSSystemAECDeactivatedGroups);
Expand Down
12 changes: 10 additions & 2 deletions media/filters/demuxer_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@
#endif // BUILDFLAG(ENABLE_HLS_DEMUXER)

#if BUILDFLAG(ENABLE_FFMPEG)
// ProgressiveDemuxer is enabled when use_starboard_media=true and media_use_ffmpeg=false.
// The ProgressiveDemuxer is enabled when use_starboard_media=true and
osagie98 marked this conversation as resolved.
Show resolved Hide resolved
// media_use_ffmpeg=false. Cobalt enables the ProgressiveDemuxer by
// default. This can disabled by the BASE_FEATURE
// CobaltProgressivePlayback at runtime.
#elif BUILDFLAG(USE_STARBOARD_MEDIA)
#include "media/starboard/progressive/demuxer_extension_wrapper.h" // nogncheck
#include "media/starboard/progressive/progressive_demuxer.h" // nogncheck
Expand Down Expand Up @@ -417,7 +420,12 @@ PipelineStatus DemuxerManager::CreateDemuxer(
#if BUILDFLAG(ENABLE_FFMPEG)
SetDemuxer(CreateFFmpegDemuxer());
#elif BUILDFLAG(USE_STARBOARD_MEDIA)
SetDemuxer(CreateProgressiveDemuxer());
if (base::FeatureList::IsEnabled(media::kCobaltProgressivePlayback)) {
SetDemuxer(CreateProgressiveDemuxer());
} else {
LOG(INFO) << "Cobalt progressive playback is disabled via base features.";
osagie98 marked this conversation as resolved.
Show resolved Hide resolved
return DEMUXER_ERROR_COULD_NOT_OPEN;
}
#else
return DEMUXER_ERROR_COULD_NOT_OPEN;
#endif
Expand Down
51 changes: 39 additions & 12 deletions third_party/blink/renderer/core/html/media/html_media_element.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,26 +405,53 @@ std::ostream& operator<<(std::ostream& stream,
return stream << static_cast<void const*>(&media_element);
}

#if BUILDFLAG(USE_STARBOARD_MEDIA)
// Checks for progressive formats served by the YouTube H5 player.
// These formats have a mime type of "video/mp4", and lists both audio and
// video codecs under the "codecs" parameter.
bool IsProgressiveFormat(const ContentType& content_type) {
const String type = content_type.GetType();
const String codecs = content_type.Parameter("codecs");

if (type.empty() && codecs.empty()) {
return false;
}

Vector<String> split_codecs;
const String separator(", ");
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to double check if this can handle queries without space after ',', e.g. 'video/mp4; codecs="avc1.42001E,mp4a.40.2"', which are also valid.

Since we are not checking the exact value, splitting on ',' can handle cases with or without extra space. In either case, we should comment here mentioning that this is not comprehensive.

Also, is this function called by the H5 player with a single codec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a new PR #4624 that strips whitespace.

Now that progressive is disabled, this function will be called on single codec formats as well. There may be a performance hit that I haven't analyzed.

codecs.Split(separator, split_codecs);
return type.Utf8() == "video/mp4" && split_codecs.size() == 2;
}
#endif // BUILDFLAG(USE_STARBOARD_MEDIA)

} // anonymous namespace

// static
MIMETypeRegistry::SupportsType HTMLMediaElement::GetSupportsType(
const ContentType& content_type) {
#if BUILDFLAG(USE_STARBOARD_MEDIA)
// Interupt Chromium's IsTypeSupported() from here for better performance.
SbMediaSupportType support_type =
SbMediaCanPlayMimeAndKeySystem(content_type.Raw().Ascii().c_str(), "");
MIMETypeRegistry::SupportsType result;
switch (support_type) {
case kSbMediaSupportTypeNotSupported:
result = MIMETypeRegistry::kNotSupported;
break;
case kSbMediaSupportTypeMaybe:
result = MIMETypeRegistry::kMaybeSupported;
break;
case kSbMediaSupportTypeProbably:
result = MIMETypeRegistry::kSupported;
break;
if (!base::FeatureList::IsEnabled(media::kCobaltProgressivePlayback) &&
IsProgressiveFormat(content_type)) {
LOG(INFO) << "Content type \'" << content_type.Raw()
osagie98 marked this conversation as resolved.
Show resolved Hide resolved
<< "\' is unsupported as Cobalt progressive playback is disabled "
"via base features.";
result = MIMETypeRegistry::kNotSupported;
} else {
const SbMediaSupportType support_type =
SbMediaCanPlayMimeAndKeySystem(content_type.Raw().Ascii().c_str(), "");
switch (support_type) {
case kSbMediaSupportTypeNotSupported:
result = MIMETypeRegistry::kNotSupported;
break;
case kSbMediaSupportTypeMaybe:
result = MIMETypeRegistry::kMaybeSupported;
break;
case kSbMediaSupportTypeProbably:
result = MIMETypeRegistry::kSupported;
break;
}
}
LOG(INFO) << __func__ << "(" << content_type.Raw() << ") -> " << result;
return result;
Expand Down
44 changes: 44 additions & 0 deletions third_party/blink/renderer/core/html/media/html_media_element_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@
#include "third_party/blink/renderer/platform/wtf/vector.h"
#include "ui/gfx/geometry/size.h"

#if BUILDFLAG(USE_STARBOARD_MEDIA)
#include "base/test/scoped_feature_list.h"
#include "third_party/blink/renderer/platform/network/mime/content_type.h"
#include "third_party/blink/renderer/platform/network/mime/mime_type_registry.h"
#endif // BUILDFLAG(USE_STARBOARD_MEDIA)

using ::testing::_;
using ::testing::AnyNumber;
using ::testing::NanSensitiveDoubleEq;
Expand Down Expand Up @@ -1568,4 +1574,42 @@ TEST_P(HTMLMediaElementTest, CanFreezeWithMediaPlayerAttached) {
EXPECT_FALSE(MediaIsPlaying());
}

#if BUILDFLAG(USE_STARBOARD_MEDIA)
TEST(HTMLMediaElementTest, CanHandleCobaltProgressiveSupportQueries) {
osagie98 marked this conversation as resolved.
Show resolved Hide resolved
const ContentType progressive_type(
"video/mp4; codecs=\"avc1.42001E, mp4a.40.2\"");
const ContentType adaptive_video_type(
"video/mp4; codecs=\"avc1.4d4015\"; framerate=30");
const ContentType adaptive_audio_type(
"audio/mp4; codecs=\"mp4a.40.2\"; channels=2");

{
base::test::ScopedFeatureList scoped_list;
scoped_list.InitAndDisableFeature(media::kCobaltProgressivePlayback);
// Reject progressive content types when CobaltProgressivePlayback is
// disabled.
EXPECT_EQ(HTMLMediaElement::GetSupportsType(progressive_type),
MIMETypeRegistry::kNotSupported);
// Continue to support adaptive content types.
EXPECT_NE(HTMLMediaElement::GetSupportsType(adaptive_video_type),
MIMETypeRegistry::kNotSupported);
EXPECT_NE(HTMLMediaElement::GetSupportsType(adaptive_audio_type),
MIMETypeRegistry::kNotSupported);
}
{
base::test::ScopedFeatureList scoped_list(
media::kCobaltProgressivePlayback);
// Accept progressive content types when CobaltProgressivePlayback is
// enabled.
EXPECT_NE(HTMLMediaElement::GetSupportsType(progressive_type),
MIMETypeRegistry::kNotSupported);
// Continue to support adaptive content types.
EXPECT_NE(HTMLMediaElement::GetSupportsType(adaptive_video_type),
MIMETypeRegistry::kNotSupported);
EXPECT_NE(HTMLMediaElement::GetSupportsType(adaptive_audio_type),
MIMETypeRegistry::kNotSupported);
}
}
#endif // BUILDFLAG(USE_STARBOARD_MEDIA)

} // namespace blink
Loading