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

feat(DASH): Add support for Content Steering #5710

Merged
merged 17 commits into from Nov 9, 2023

Conversation

avelad
Copy link
Collaborator

@avelad avelad commented Oct 2, 2023

Closes #5704

@avelad avelad marked this pull request as draft October 2, 2023 14:12
@avelad avelad added type: enhancement New feature or request component: HLS The issue involves Apple's HLS manifest format priority: P3 Useful but not urgent component: DASH The issue involves the MPEG DASH manifest format labels Oct 2, 2023
@shaka-bot
Copy link
Collaborator

shaka-bot commented Oct 2, 2023

Incremental code coverage: 94.05%

@avelad avelad force-pushed the content-steering branch 9 times, most recently from 5b27bbf to da0b312 Compare October 3, 2023 14:47
@avelad avelad added this to the v4.6 milestone Oct 5, 2023
@avelad avelad removed the component: HLS The issue involves Apple's HLS manifest format label Oct 31, 2023
@avelad avelad changed the title feat: Add support for Content Steering feat(DASH): Add support for Content Steering Oct 31, 2023
@avelad avelad marked this pull request as ready for review October 31, 2023 14:15
@avelad
Copy link
Collaborator Author

avelad commented Nov 2, 2023

@theodab can you review it? Thanks!

}
}
}
if (!someLocationValid || !this.contentSteeringManager_) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't someLocationValid always be false when this.contentSteeringManager_ is null? I think the second half of this check is unnecessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is necessary because it may be the case that some manifests have incorrect information and the BaseURLs do not have any url with serviceLocation

lib/dash/dash_parser.js Show resolved Hide resolved
lib/net/networking_engine.js Outdated Show resolved Hide resolved
externs/shaka/manifest_parser.js Outdated Show resolved Hide resolved
lib/util/content_steering_manager.js Outdated Show resolved Hide resolved
lib/util/content_steering_manager.js Outdated Show resolved Hide resolved
lib/util/content_steering_manager.js Outdated Show resolved Hide resolved
lib/util/content_steering_manager.js Show resolved Hide resolved
lib/util/content_steering_manager.js Outdated Show resolved Hide resolved
lib/util/content_steering_manager.js Outdated Show resolved Hide resolved
@avelad avelad merged commit 42f491f into shaka-project:main Nov 9, 2023
14 of 15 checks passed
@avelad avelad deleted the content-steering branch November 13, 2023 08:03
Robloche pushed a commit to Robloche/shaka-player that referenced this pull request Nov 30, 2023
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Jan 8, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Jan 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: DASH The issue involves the MPEG DASH manifest format priority: P3 Useful but not urgent status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Content Steering
3 participants