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(HLS): Correctly identify webvtt subtitles with cmft extension #7791

Merged
merged 2 commits into from
Dec 23, 2024
Merged

fix(HLS): Correctly identify webvtt subtitles with cmft extension #7791

merged 2 commits into from
Dec 23, 2024

Conversation

avelad
Copy link
Member

@avelad avelad commented Dec 20, 2024

Fixes #7766

@avelad avelad added type: bug Something isn't working correctly component: HLS The issue involves Apple's HLS manifest format component: captions/subtitles The issue involves captions or subtitles priority: P2 Smaller impact or easy workaround labels Dec 20, 2024
@avelad avelad added this to the v4.13 milestone Dec 20, 2024
@shaka-bot
Copy link
Collaborator

Incremental code coverage: 100.00%

@avelad avelad force-pushed the hls-subs branch 3 times, most recently from 5119920 to fa2af56 Compare December 20, 2024 15:40
@joeyparrish
Copy link
Member

with cmft extension

I don't see that extension referenced anywhere in the PR. Is something missing?

joeyparrish
joeyparrish requested changes Dec 20, 2024
View reviewed changes
lib/hls/hls_parser.js
@@ -2935,6 +2935,12 @@ shaka.hls.HlsParser = class {

if (!mimeType) {
mimeType = await this.guessMimeType_(type, codecs, segments);

if (type == shaka.util.ManifestParserUtils.ContentType.TEXT &&
Copy link
Member

Choose a reason for hiding this comment

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

I can't tell what this logic is doing. Can you please add an explanatory comment?

joeyparrish
joeyparrish approved these changes Dec 20, 2024
View reviewed changes
Copy link
Member

@joeyparrish joeyparrish left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approved after a side conversation about the extension. But I would still like to see a comment added explaining the new logic, specifically answering the question "why?"

@avelad avelad merged commit 7ef5e62 into shaka-project:main Dec 23, 2024
14 of 17 checks passed
@shaka-bot shaka-bot mentioned this pull request Dec 23, 2024
Open
@avelad avelad deleted the hls-subs branch December 23, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joeyparrish joeyparrish joeyparrish approved these changes

@theodab theodab Awaiting requested review from theodab

@tykus160 tykus160 Awaiting requested review from tykus160

Assignees
No one assigned
Labels
component: captions/subtitles The issue involves captions or subtitles component: HLS The issue involves Apple's HLS manifest format priority: P2 Smaller impact or easy workaround type: bug Something isn't working correctly
Projects
None yet
Milestone
v4.13
Development

Successfully merging this pull request may close these issues.

HLS CMAF stream with subtitles doesn't play
3 participants
@avelad @shaka-bot @joeyparrish