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.

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

Conversation

osagie98
Copy link
Contributor

@osagie98 osagie98 commented Dec 9, 2024

Implements a BASE_FEATURE for Cobalt progressive playback support and disables it by default. Progressive playback can be enabled at runtime by passing the argument "--enable-features=CobaltProgressivePlayback". This change only affects Cobalt Starboard Media builds.

b/382791540

Progressive playback support can be disabled at runtime by passing the
argument "--disable-features=CobaltProgressivePlayback". Progressive
playback support remains enabled by default. This change only affects
Cobalt Starboard Media builds.

b/382791540
@osagie98 osagie98 requested a review from a team as a code owner December 9, 2024 23:22
Copy link
Contributor

@yell0wd0g yell0wd0g left a comment

Choose a reason for hiding this comment

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

This should add a unit test (to third_party/blink/renderer/core/html/media/html_media_element_test.cc), it shouldn't be too hard.

The function in base/test/scoped_feature_list.h can help for enabling base::Features safely for testing.

@osagie98
Copy link
Contributor Author

This should add a unit test (to third_party/blink/renderer/core/html/media/html_media_element_test.cc), it shouldn't be too hard.

The function in base/test/scoped_feature_list.h can help for enabling base::Features safely for testing.

@yell0wd0g I commented elsewhere in this PR:

I've removed the test to unblock this (it crashes on Android). The test is saved, and once we validate blink tests on Android I'll re-add it.

@yell0wd0g
Copy link
Contributor

This should add a unit test (to third_party/blink/renderer/core/html/media/html_media_element_test.cc), it shouldn't be too hard.
The function in base/test/scoped_feature_list.h can help for enabling base::Features safely for testing.

@yell0wd0g I commented elsewhere in this PR:

I've removed the test to unblock this (it crashes on Android). The test is saved, and once we validate blink tests on Android I'll re-add it.

If you can compile and run it on Linux, I'd say add it, since anyway blink_unittest APK is not ran anywhere, so it wouldn't crash : )
It's best to land the unit test with the code itself, for history purposes for example (cherry-picking etc).

Copy link
Contributor

@yell0wd0g yell0wd0g left a comment

Choose a reason for hiding this comment

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

Looking good but I'd add the unit test anyhow and also try and make const any variable that can be const.

@osagie98 osagie98 requested a review from yell0wd0g December 12, 2024 19:42
Copy link
Contributor

@yell0wd0g yell0wd0g left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for bringing the unit tests!

@osagie98
Copy link
Contributor Author

Merge pending discussion if we want to start with C25 only: #4564

@osagie98 osagie98 changed the title Add BASE_FEATURE flag for progressive playback Disable progressive playback support by default Dec 18, 2024
@osagie98 osagie98 enabled auto-merge (squash) December 18, 2024 23:13
@osagie98 osagie98 merged commit 9158f9a into youtube:main Dec 19, 2024
72 of 74 checks passed
@osagie98 osagie98 deleted the progressive-flag branch December 19, 2024 21:48
}

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.

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.

4 participants