Skip to content

Fix caching and ability to play urls with query params. #101

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

Merged
merged 2 commits into from
Sep 7, 2024

Conversation

GraemeHarrison
Copy link
Contributor

@GraemeHarrison GraemeHarrison commented Sep 2, 2024

Fix ability to play URLs with query params:

  • Because the "mp4" extension was always being added to the end of the loader url, it would break any urls with query parameters. Many image services use query params to perform transforms to media like scaling and blurring, etc. Using the appendingPathExtension function fixes the issue.

Fix caching:

  • I noticed that isEnoughToPlay: Bool would always return false from the guard statement. I believe it's because the url extension that was being used in logic such as isEnoughToPlay: Bool was deleting the path extension unnecessarily. Removing this seemed to fix the logic, and isEnoughToPlay now correctly returns true when the downloadedByteCount is sufficient.

@gbuela
Copy link
Contributor

gbuela commented Sep 2, 2024

Hi! good point about URLs with query params.
Have you tested this with a URL without extension like the one mentioned in #77? I'm not sure by looking at the code that it will still work for those cases. I will try it when I have a moment.

@gbuela
Copy link
Contributor

gbuela commented Sep 2, 2024

Hi again, I tried it and found that found that this breaks the scenario described in issue #77 (URLs without extension). We need a solution that works for both issues, with or without extension and with or without queryparams. I could tackle it but I need more time, feel free to do it yourself if you can!

In the example app you can tweak the URL in BasicViewController to test it. The URL that it has now, http://clips.vorwaerts-gmbh.de/big_buck_bunny.mp4 doesn't load for me. This one does: https://download.samplelib.com/mp4/sample-5s.mp4.
You can verify that this one https://media.staticontent.com/media/documents/132420b8-b6b0-4c29-8e81-3cec4522f6f1 works currently in master but it won't load anymore with this PR.

BTW it might be a good idea to make a separate PR for @wxxsw with the caching fix and then either of us can deal with the rest in another PR.

@GraemeHarrison
Copy link
Contributor Author

@gbuela thanks for testing that out and explaining the problem. I'll see if I can come up with something tomorrow, and will consider making a new PR for the caching issue.

@GraemeHarrison
Copy link
Contributor Author

GraemeHarrison commented Sep 6, 2024

@gbuela @wxxsw what is the purpose of prepending AVPlayerItem.loaderPrefix to the url? Is it just to make sure that other logic only proceeds if the url was passed into the GSPlayer library?

Update:
Never mind, I now see that it's because AVAssetResourceLoaderDelegate won't be called if the scheme is HTTP or HTTPS.

…t plays and caches correctly regardless if it has an extension or not.
@GraemeHarrison
Copy link
Contributor Author

@gbuela @wxxsw I have made a new commit that fixes the issues you mentioned. In my testing urls play and cache correctly regardless of whether or not they have query params or path extensions now. Please let me know what you think :)

Copy link
Contributor

@gbuela gbuela left a comment

Choose a reason for hiding this comment

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

looks good to me!

@wxxsw wxxsw merged commit 6000a12 into wxxsw:master Sep 7, 2024
@wxxsw
Copy link
Owner

wxxsw commented Sep 9, 2024

Release version '0.2.28'

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.

3 participants