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

fix(ios): fix external text tracks crashes with m3u8 files #3330

Merged

Conversation

KrzysztofMoch
Copy link
Collaborator

Summary

iOS does not support sideload subtitles for HLS playlist like m3u8. Added a safeguard so that such cases do not crash the application.

Test plan

  • example app does not crash when using textTracks with m3u8 as source

@freeboub
Copy link
Collaborator

freeboub commented Nov 4, 2023

Thank you for the crash fix, but It would be better to make the functionality working :)
I tried on my side without success, can you please do a complementary test ? can you test If this feature working on 5.2 version ?
I know I used it on android, but I am not sure I tested it on ios.
If not working on 5.2 ios, then let's go with it !

@freeboub freeboub changed the title fix(ios): fix m3u8 crashes fix(ios): fix external text tracks crashes with m3u8 files Nov 4, 2023
ios/Video/Features/RCTVideoUtils.swift Outdated Show resolved Hide resolved
ios/Video/Features/RCTVideoUtils.swift Outdated Show resolved Hide resolved
ios/Video/Features/RCTVideoUtils.swift Outdated Show resolved Hide resolved
ios/Video/Features/RCTVideoUtils.swift Outdated Show resolved Hide resolved
ios/Video/Features/RCTVideoUtils.swift Outdated Show resolved Hide resolved
ios/Video/Features/RCTVideoUtils.swift Outdated Show resolved Hide resolved
ios/Video/Features/RCTVideoUtils.swift Outdated Show resolved Hide resolved
ios/Video/Features/RCTVideoUtils.swift Outdated Show resolved Hide resolved
ios/Video/Features/RCTVideoUtils.swift Outdated Show resolved Hide resolved
ios/Video/Features/RCTVideoUtils.swift Outdated Show resolved Hide resolved
@KrzysztofMoch
Copy link
Collaborator Author

Thank you for the crash fix, but It would be better to make the functionality working :)
I tried on my side without success, can you please do a complementary test ? can you test If this feature working on 5.2 version ?
I know I used it on android, but I am not sure I tested it on ios.
If not working on 5.2 ios, then let's go with it !

@freeboub I have checked this and this is not working on 5.2 too (but is not crashing)

@freeboub
Copy link
Collaborator

freeboub commented Nov 6, 2023

Thank you for the crash fix, but It would be better to make the functionality working :)
I tried on my side without success, can you please do a complementary test ? can you test If this feature working on 5.2 version ?
I know I used it on android, but I am not sure I tested it on ios.
If not working on 5.2 ios, then let's go with it !

@freeboub I have checked this and this is not working on 5.2 too (but is not crashing)

Thank you for the test, so let's go with that

ios/Video/RCTVideo.swift Outdated Show resolved Hide resolved
if let description = _source?.description {
mapping[.commonIdentifierDescription] = description
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

There again spacing change.
But I accept these one 😆

@freeboub
Copy link
Collaborator

@KrzysztofMoch I merged the PR for caching, this Pr now conflct with master... Sorry

@KrzysztofMoch KrzysztofMoch merged commit 782e7e0 into TheWidlarzGroup:master Nov 17, 2023
1 check passed
@KrzysztofMoch KrzysztofMoch deleted the fix/m3u8-crashes branch November 17, 2023 07:19
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.

None yet

2 participants