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

Jump to bottom

fix(Demo): Allow manifest type for DAI custom assets #4977

Merged
merged 1 commit into from
Feb 9, 2023
Merged

fix(Demo): Allow manifest type for DAI custom assets #4977

merged 1 commit into from
Feb 9, 2023

Conversation

avelad
Copy link
Member

@avelad avelad commented Feb 8, 2023

Fixes #4887

@avelad avelad added type: bug Something isn't working correctly component: demo page The issue is in the demo page; does not affect production applications component: ads The issue involves the Shaka Player ads API or the use of other ad SDKs priority: P2 Smaller impact or easy workaround labels Feb 8, 2023
@avelad avelad added this to the v4.4 milestone Feb 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Incremental code coverage: No instrumented code was changed.

theodab
theodab previously approved these changes Feb 8, 2023
View reviewed changes
demo/main.js Outdated
case 'dash':
case 'MPD':
case 'mpd':
request.format = 'dash';
Copy link
Contributor

Choose a reason for hiding this comment

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

Man, I had to actually look through the code to verify that the string values of these enums are in lowercase. The public documentation only says that the enums are StreamRequest.StreamFormat.DASH and StreamRequest.StreamFormat.HLS.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am going to change it, but the cases of the case are for the value that the user enters in the popup, and here it is transformed to the value that DAI expects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I know.
I was just commenting, since I was trying to cross-reference with the IMA SDK docs to make sure the values you put in were right. I was disappointed that the docs were so vague about that enum.

@avelad avelad requested a review from theodab February 9, 2023 07:24
@avelad
Copy link
Member Author

avelad commented Feb 9, 2023

I'll wait for the tests to pass and then I merge it. Thanks @theodab !

@avelad avelad merged commit 1e50630 into shaka-project:main Feb 9, 2023
@avelad avelad deleted the dai-dash-demo branch February 9, 2023 07:48
@github-actions github-actions bot mentioned this pull request Feb 9, 2023
Merged
joeyparrish pushed a commit that referenced this pull request Feb 9, 2023
@avelad @joeyparrish
fix(Demo): Allow manifest type for DAI custom assets (#4977)
2a66112 
Fixes #4887
joeyparrish pushed a commit that referenced this pull request Feb 9, 2023
@avelad @joeyparrish
fix(Demo): Allow manifest type for DAI custom assets (#4977)
d341f93 
Fixes #4887
joeyparrish pushed a commit that referenced this pull request Feb 9, 2023
@avelad @joeyparrish
fix(Demo): Allow manifest type for DAI custom assets (#4977)
ea1ab66 
Fixes #4887
joeyparrish pushed a commit that referenced this pull request Feb 9, 2023
@avelad @joeyparrish
fix(Demo): Allow manifest type for DAI custom assets (#4977)
d67ca2b 
Fixes #4887
This was referenced Feb 9, 2023
Merged
Merged
Merged
Merged
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@theodab theodab theodab approved these changes

@joeyparrish joeyparrish Awaiting requested review from joeyparrish

@littlespex littlespex Awaiting requested review from littlespex

Assignees
No one assigned
Labels
component: ads The issue involves the Shaka Player ads API or the use of other ad SDKs component: demo page The issue is in the demo page; does not affect production applications priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.

DASH playback not supported in DAI demos of shakaplayer
2 participants
@avelad @theodab