-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 caching + add iOS feature: preCaching #670
Fix iOS caching + add iOS feature: preCaching #670
Conversation
Thanks @themadmrj for preparing this branch again. I'll make this as my top priority to merge this PR. |
self.completionHandler?(true) | ||
} | ||
|
||
/* func playerItem(_ playerItem: CachingPlayerItem, didDownloadBytesSoFar bytesDownloaded: Int, outOf bytesExpected: Int){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it's here? Can you remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useful for debugging or if someone wants the information for some other reason. I would leave it to be uncommented if required.
|
||
var completionHandler: ((_ success:Bool) -> Void)? = nil | ||
|
||
var diskConfig = DiskConfig(name: "BetterPlayerCache", expiry: .date(Date().addingTimeInterval(3600*24*30)), maxSize: 100*1024*1024) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these values should be configured from BP config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maxSize
already configured from BP and passed along. I don't think expiry
is an issue, since if the cache is used a lot, the size limit matters most and will implement LRU. Can always be done in the future if someone needs it.
ios/Classes/CacheManager.swift
Outdated
var completionHandler: ((_ success:Bool) -> Void)? = nil | ||
|
||
var diskConfig = DiskConfig(name: "BetterPlayerCache", expiry: .date(Date().addingTimeInterval(3600*24*30)), maxSize: 100*1024*1024) | ||
let memoryConfig = MemoryConfig(expiry: .never, countLimit: 10, totalCostLimit: 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set the values to the default (which is no limit). In 99.9% of the cases, nobody will touch this setting. NSCache apparently also has an internal mechanism responding to low memory and will evict object if it has to. Therefore, will the default settings, I don't think it is necessary.
// MARK: - Logic | ||
@objc public func preCacheURL(_ url: URL, cacheKey: String?, withHeaders headers: Dictionary<NSObject,AnyObject>, completionHandler: ((_ success:Bool) -> Void)?) { | ||
self.completionHandler = completionHandler | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove line please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? This is where completionHandler
is stored in the class.
@themadmrj Can you please look into my comments? Also it would be great if you will be able to setup some tests for this feature. There's a lot of logic here and I don't know how it works. I know there's no tests at all in BP but we need to start doing this to prevent regression issues which happens a lot. I know it will require additional work from you, so please do it in your spare time. |
I applied the required changes and/or commented. As for the tests, I agree with you 100% but tbh I don't have the time to write them, as I've never done it and would need to invest time to research how to do it properly. |
@jhomlala please keep me posted when you've checked the last commit :) |
Hey @themadmrj I tried this PR but had the following error:
Here's my flutter doctor -v output:
Any pointers would be appreciated. |
Compile issues are fixed now, Ioseph |
@themadmrj I am not Ioseph :) He suggested me this fork. Thanks for the fix. |
@themadmrj I have changed base branch to apply my small fixes. This feature should be in the next release. Again thanks for PR. |
@jhomlala sounds good! |
* Added expandToFill parameter in BetterPlayerConfiguration * Updated changelog * * Added `BetterPlayerControlsConfiguration.theme` factory for `BetterPlayerControlsConfiguration`. * Added null checks in seek commands in BetterPlayerControlsState. * Fixed issue with live stream where player controls were always visible. * Updated tests * Updated tests * Updated tests * Updated tests * Updated tests * Added tests to CI * Update ci.yml * Updated tests * Updated tests * Updated tests * Updated tests * Updated tests, general refactor * Updated cupertino theme * Fixed iOS seek issue * Fix iOS caching + add iOS feature: preCaching (#670) * Fix iOS caching + add iOS feature: preCaching * Added required changes * Fix compile issues * Added stop pre cache iOS implementation * Updated caching implementation * Updated caching implementation * Updated caching implementation * Updated caching implementation * Updated documentation * Fixed video FPS hardcoded to 30 on iOs (#705) Co-authored-by: Jakub <jhomlala@gmail.com> * Updated changelog * set default subtitle from hls (#688) * set default subtitle from hls * remove comments * General refactor * Disabled analysis options. * Flutter 2.5 update * Flutter 2.5 update * Disabled lint * Fixed analyzer * Updated dependencies * Updated version Co-authored-by: themadmrj <themadmrj@users.noreply.github.com> Co-authored-by: Anton Krasov <anton.krasov@gmail.com> Co-authored-by: Siloe Bezerra Bispo <siloebb@gmail.com>
* Added expandToFill parameter in BetterPlayerConfiguration * Updated changelog * * Added `BetterPlayerControlsConfiguration.theme` factory for `BetterPlayerControlsConfiguration`. * Added null checks in seek commands in BetterPlayerControlsState. * Fixed issue with live stream where player controls were always visible. * Updated tests * Updated tests * Updated tests * Updated tests * Updated tests * Added tests to CI * Update ci.yml * Updated tests * Updated tests * Updated tests * Updated tests * Updated tests, general refactor * Updated cupertino theme * Fixed iOS seek issue * Fix iOS caching + add iOS feature: preCaching (jhomlala#670) * Fix iOS caching + add iOS feature: preCaching * Added required changes * Fix compile issues * Added stop pre cache iOS implementation * Updated caching implementation * Updated caching implementation * Updated caching implementation * Updated caching implementation * Updated documentation * Fixed video FPS hardcoded to 30 on iOs (jhomlala#705) Co-authored-by: Jakub <jhomlala@gmail.com> * Updated changelog * set default subtitle from hls (jhomlala#688) * set default subtitle from hls * remove comments * General refactor * Disabled analysis options. * Flutter 2.5 update * Flutter 2.5 update * Disabled lint * Fixed analyzer * Updated dependencies * Updated version Co-authored-by: themadmrj <themadmrj@users.noreply.github.com> Co-authored-by: Anton Krasov <anton.krasov@gmail.com> Co-authored-by: Siloe Bezerra Bispo <siloebb@gmail.com>
On iOS, caching currently does not work as expected, because the cacheKey is ignored. This PR builds a new caching system on iOS which fixes this and also allows pre-caching, which I had previously only added on Android. With this PR, betterplayer will be the only library that fully supports caching and pre-caching videos on both platforms.
Limitations: preCaching on iOS currently does not support pre-caching only a chunk of the file. The whole file will be downloaded, therefore devs should be careful when using the feature with big files.
HLS: the current system is to load everything into memory and save the file to disk once downloaded, so if you plan to read a stream for 2 hours, probably not. But anyway, who uses cache with hls? Since I don't, I recon someone tests it with HLS, I can't tell what the result will be. It should at least play since the base it still an AVPlayerItem.
This is the response to the #548 lost in translation...