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 caching + add iOS feature: preCaching #670

Merged

Conversation

jonasN5
Copy link
Contributor

@jonasN5 jonasN5 commented Aug 26, 2021

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...

@jhomlala
Copy link
Owner

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){
Copy link
Owner

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?

Copy link
Contributor Author

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.

ios/Classes/CacheManager.swift Outdated Show resolved Hide resolved
ios/Classes/CacheManager.swift Outdated Show resolved Hide resolved
ios/Classes/CacheManager.swift Outdated Show resolved Hide resolved
ios/Classes/CacheManager.swift Outdated Show resolved Hide resolved
ios/Classes/BetterPlayerPlugin.m Outdated Show resolved Hide resolved

var completionHandler: ((_ success:Bool) -> Void)? = nil

var diskConfig = DiskConfig(name: "BetterPlayerCache", expiry: .date(Date().addingTimeInterval(3600*24*30)), maxSize: 100*1024*1024)
Copy link
Owner

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.

Copy link
Contributor Author

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.

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)
Copy link
Owner

Choose a reason for hiding this comment

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

These too.

Copy link
Contributor Author

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

Copy link
Owner

Choose a reason for hiding this comment

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

Remove line please.

Copy link
Contributor Author

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.

ios/Classes/CachingPlayerItem.swift Outdated Show resolved Hide resolved
@jhomlala
Copy link
Owner

@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.

@jonasN5
Copy link
Contributor Author

jonasN5 commented Aug 26, 2021

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.

@jonasN5 jonasN5 requested a review from jhomlala August 26, 2021 17:42
@jonasN5
Copy link
Contributor Author

jonasN5 commented Aug 29, 2021

@jhomlala please keep me posted when you've checked the last commit :)

@berlinLovesArt
Copy link

Hey @themadmrj I tried this PR but had the following error:

Xcode's output:
↳
    3 warnings generated.
    /Users/berlin/Presence/ios/Pods/Swime/Sources/Swime.swift:41:21: warning: 'init(bytes:)' is deprecated: use `init(_:)` instead
        self.init(data: Data(bytes: bytes))
                        ^
    /Users/berlin/Presence/ios/Pods/Swime/Sources/Swime.swift:41:21: warning: 'init(bytes:)' is deprecated: use `init(_:)` instead
        self.init(data: Data(bytes: bytes))
                        ^
    /Users/berlin/Presence/ios/Pods/Swime/Sources/Swime.swift:41:21: warning: 'init(bytes:)' is deprecated: use `init(_:)` instead
        self.init(data: Data(bytes: bytes))
                        ^
    <module-includes>:1:9: note: in file included from <module-includes>:1:
    #import "Headers/better_player-umbrella.h"
            ^
    /Users/berlin/Presence/ios/Pods/Target Support Files/better_player/better_player-umbrella.h:13:9: note: in file included from /Users/berlin/Presence/ios/Pods/Target Support
    Files/better_player/better_player-umbrella.h:13:
    #import "BetterPlayer.h"
            ^
    /Users/berlin/flutter/.pub-cache/git/betterplayer-06c301a4a5e6035c17f25d9f3551351da7a9582c/ios/Classes/BetterPlayer.h:54:34: warning: 'width' used as the name of the previous
    parameter rather than as part of the selector
    - (void)setTrackParameters:(int) width: (int) height: (int)bitrate;
                                     ^
    /Users/berlin/flutter/.pub-cache/git/betterplayer-06c301a4a5e6035c17f25d9f3551351da7a9582c/ios/Classes/BetterPlayer.h:54:34: note: introduce a parameter name to make 'width' part of
    the selector
    - (void)setTrackParameters:(int) width: (int) height: (int)bitrate;
                                     ^
    /Users/berlin/flutter/.pub-cache/git/betterplayer-06c301a4a5e6035c17f25d9f3551351da7a9582c/ios/Classes/BetterPlayer.h:54:39: note: or insert whitespace before ':' to use 'width' as
    parameter name and have an empty entry in the selector
    - (void)setTrackParameters:(int) width: (int) height: (int)bitrate;
                                          ^
    <module-includes>:1:9: note: in file included from <module-includes>:1:
    #import "Headers/better_player-umbrella.h"
            ^
    /Users/berlin/Presence/ios/Pods/Target Support Files/better_player/better_player-umbrella.h:13:9: note: in file included from /Users/berlin/Presence/ios/Pods/Target Support
    Files/better_player/better_player-umbrella.h:13:
    #import "BetterPlayer.h"
            ^
    /Users/berlin/flutter/.pub-cache/git/betterplayer-06c301a4a5e6035c17f25d9f3551351da7a9582c/ios/Classes/BetterPlayer.h:54:47: warning: 'height' used as the name of the previous
    parameter rather than as part of the selector
    - (void)setTrackParameters:(int) width: (int) height: (int)bitrate;
                                                  ^
    /Users/berlin/flutter/.pub-cache/git/betterplayer-06c301a4a5e6035c17f25d9f3551351da7a9582c/ios/Classes/BetterPlayer.h:54:47: note: introduce a parameter name to make 'height' part of
    the selector
    - (void)setTrackParameters:(int) width: (int) height: (int)bitrate;
                                                  ^
    /Users/berlin/flutter/.pub-cache/git/betterplayer-06c301a4a5e6035c17f25d9f3551351da7a9582c/ios/Classes/BetterPlayer.h:54:53: note: or insert whitespace before ':' to use 'height' as
    parameter name and have an empty entry in the selector
    - (void)setTrackParameters:(int) width: (int) height: (int)bitrate;
                                                        ^
    /Users/berlin/flutter/.pub-cache/git/betterplayer-06c301a4a5e6035c17f25d9f3551351da7a9582c/ios/Classes/CacheManager.swift:23:40: error: generic parameter 'Key' could not be inferred
        lazy var storage: Cache.Storage? = {
                                           ^
    <module-includes>:1:9: note: in file included from <module-includes>:1:
    #import "Headers/better_player-umbrella.h"
            ^
    /Users/berlin/Presence/ios/Pods/Target Support Files/better_player/better_player-umbrella.h:13:9: note: in file included from /Users/berlin/Presence/ios/Pods/Target Support
    Files/better_player/better_player-umbrella.h:13:
    #import "BetterPlayer.h"
            ^
    /Users/berlin/flutter/.pub-cache/git/betterplayer-06c301a4a5e6035c17f25d9f3551351da7a9582c/ios/Classes/CachingPlayerItem.swift:79:46: warning: cast from 'Dictionary<NSObject,
    AnyObject>.Element' (aka '(key: NSObject, value: AnyObject)') to unrelated type 'String' always fails
                        request.setValue((_value as! String), forHTTPHeaderField: (_key as! String))
                                          ~~~~~~ ^   ~~~~~~
    /Users/berlin/flutter/.pub-cache/git/betterplayer-06c301a4a5e6035c17f25d9f3551351da7a9582c/ios/Classes/CachingPlayerItem.swift:79:85: warning: cast from 'Int' to unrelated type
    'String' always fails
                        request.setValue((_value as! String), forHTTPHeaderField: (_key as! String))
                                                                                   ~~~~ ^   ~~~~~~
    note: Using new build system
    note: Building targets in parallel
    note: Planning build
    note: Analyzing workspace
    note: Constructing build description
    note: Build preparation complete
    warning: Capabilities for Signing & Capabilities may not function correctly because its entitlements use a placeholder team ID. To resolve this, select a development team in the Runner
    editor. (in target 'Runner' from project 'Runner')

Could not build the application for the simulator.
Error launching application on iPhone 12 Pro.

Here's my flutter doctor -v output:

[✓] Flutter (Channel stable, 2.2.3, on macOS 11.5 20G71 darwin-x64, locale en-IN)
    • Flutter version 2.2.3 at /Users/berlin/flutter
    • Framework revision f4abaa0735 (9 weeks ago), 2021-07-01 12:46:11 -0700
    • Engine revision 241c87ad80
    • Dart version 2.13.4

[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.3)
    • Android SDK at /Users/berlin/Library/Android/sdk
    • Platform android-30, build-tools 30.0.3
    • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.10+0-b96-7281165)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Xcode 12.5, Build version 12E262
    • CocoaPods version 1.10.1

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2020.3)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.10+0-b96-7281165)

[✓] VS Code (version 1.59.1)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.25.0

[✓] Connected device (2 available)
    • iPhone 12 Pro (mobile) • CB8A1B9B-05FD-4AC8-B603-FBEA9A46E589 • ios            • com.apple.CoreSimulator.SimRuntime.iOS-14-5 (simulator)
    • Chrome (web)           • chrome                               • web-javascript • Google Chrome 92.0.4515.159

• No issues found!

Any pointers would be appreciated.

@jonasN5
Copy link
Contributor Author

jonasN5 commented Aug 31, 2021

Compile issues are fixed now, Ioseph

@berlinLovesArt
Copy link

berlinLovesArt commented Sep 1, 2021

@themadmrj I am not Ioseph :) He suggested me this fork. Thanks for the fix.

@jhomlala jhomlala changed the base branch from master to feature/august_changes_2_ios_pre_caching September 7, 2021 19:02
@jhomlala
Copy link
Owner

jhomlala commented Sep 7, 2021

@themadmrj I have changed base branch to apply my small fixes. This feature should be in the next release. Again thanks for PR.

@jhomlala jhomlala merged commit 57a04e0 into jhomlala:feature/august_changes_2_ios_pre_caching Sep 7, 2021
@jonasN5
Copy link
Contributor Author

jonasN5 commented Sep 7, 2021

@jhomlala sounds good!

@jonasN5 jonasN5 deleted the ios_cache branch September 9, 2021 07:10
jhomlala added a commit that referenced this pull request Sep 20, 2021
* 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>
geriby23 pushed a commit to threadable/betterplayer that referenced this pull request Jul 16, 2024
* 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>
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