Async, retain cycles, tests, buffering and more

[iwasrobbed]

Hey @andreipitis 👋

I've decided to use this video player in a production application and found some issues while digging through and testing it out. I did not change the external API very much (view the updated README for the minimal changes).

If you'd rather not have these changes, I'm fine with forking off as well; just let me know but figured I'd try and give back.

Todo:

• [ ] There are some pre-existing scrubber bugs where it jumps around a lot when scrubbing and letting go. I'll look into those sometime soon
• [ ] Add more test coverage

Changes (based on commit msgs):

• ✅ Allow a custom font to be set for the time labels
• ✅ Only show hours in the time if there are any hours to show
• ✅ Expose the underlying videoPlayerView within videoPlayer so we can access the closure callbacks
• ✅ Expose a videoURL setter
• ✅ Ensure startPlayingWhenReady hides controls when used on the UI player w/ controls
• ✅ Move previous/next logic down into the videoPlayerView and create a queue for handling that
• ✅ Add buffering callbacks and some error handling, such as AVPlayerItemFailedToPlayToEndTime notification
• ✅ Assets are now loaded asynchronously (mandatory on iOS)
• ✅ Fix tests for async changes and removed ! bang operators everywhere except tests since it was causing some difficult-to-find EXC_BAD_ACCESS issues in the example app; it's safer to use an optional var with a guard pattern here
• ✅ Make scrubber also change value when tapped
• ✅ Fixed all retain cycles so the player gets released from memory (there were quite a few areas strongly held and memory kept growing each time)
• ✅ Ensure the progress loader stops animating correctly
• ✅ Only show the progress loader after ~0.5s have passed so it doesn't show a blip on faster network connections
• ✅ Allow multiple subscribers for closures (similar to delegates, overwriting an existing closure value meant you lost any existing implementation/callback so it was causing strange behavior)
• ✅ Add a controlsInitiallyHidden property w/ tests (default = false)

Caution:

• Going forward, tests will have to be written with an asynchronous approach since assets can load either async or sync depending on if they've already been loaded (default AVAsset behavior).
  • This means some tests could pass locally and then fail on travis or another machine, so it's something to keep in mind.
• The way all these setters are calling each other while also allowing multiple subscribers for closures means it's hard to debug at times. Might be best to rethink in a deterministic manner

[codecov-io]

Codecov Report

Merging #9 into master will decrease coverage by 7.88%. The diff coverage is 72.36%.

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
- Coverage   83.52%   75.63%   -7.89%     
==========================================
  Files          13       13              
  Lines         959     1252     +293     
==========================================
+ Hits          801      947     +146     
- Misses        158      305     +147

Impacted Files	Coverage Δ
Example/ASPVideoPlayer/AppDelegate.swift	100% <ø> (ø)	:arrow_up:
ASPVideoPlayer/Classes/Math.swift	100% <100%> (ø)	:arrow_up:
...ExampleUITests/ASPVideoPlayer_ExampleUITests.swift	100% <100%> (ø)	:arrow_up:
Example/ASPVideoPlayer/PlayerViewController.swift	100% <100%> (ø)	:arrow_up:
ASPVideoPlayer/Classes/AnimationForwarder.swift	100% <100%> (ø)	:arrow_up:
Example/ASPVideoPlayer/ViewController.swift	28.33% <29.31%> (-50%)	:arrow_down:
ASPVideoPlayer/Classes/Loader.swift	43.18% <43.18%> (-48.82%)	:arrow_down:
ASPVideoPlayer/Classes/Scrubber.swift	53.84% <53.84%> (-3.43%)	:arrow_down:
ASPVideoPlayer/Classes/ASPVideoPlayerView.swift	77.43% <77.43%> (-15.9%)	:arrow_down:
ASPVideoPlayer/Classes/ControlButtons.swift	79.45% <79.45%> (-1.37%)	:arrow_down:
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 845262a...d329d44. Read the comment docs.

[andreipitis]

Hi @iwasrobbed 👋

I apologise for the late reply.

Wow! This is amazing work 🎉 . I might make some minor changes before I publish the next release but overall this looks awesome.

I have one question regarding the multiple observers on closures (dd1ee0965f9a2da2251b76d2c5c6e1307456a7fc). Do you have a specific use case where you need multiple observers or is this change only because you want to have an observer on the ASPVideoPlayer instance? Adding the closure functionality to ASPVideoPlayer sounds like a good idea but I'm not sure if it should support multiple observers.

Thank you very much for contributing to this project.

[iwasrobbed]

hey @andreipitis! regarding multiple observers, there are a few considerations:

• since the video controls already set certain videoPlayerView observers, it prevents you from also listening in on those events if you're using the controls
  • you could split these out into their own observers specifically for the controls, but then the videoPlayerView would know too much about the controls instead of being nicely abstracted so you'd still need a way to do one-to-many observation on the videoPlayerView
• a common scenario:
  • if you wanted to animate the video (e.g. fade in) during start / end while also using the controls; that would be impossible to do before because you'd overwrite the controls observer and cause some really strange behavior (it happened to me at first)
• future scenarios:
  • listening in on playing to know when to show subtitles
  • listening in on playing to know when to pre-buffer next videos
• the only other patterns would be delegation (one-to-one) or notifications (one-to-many); for closure-based observers in my other projects, we tend to use a "subscriber" pattern where you subscribe w/ a model object that takes a closure (so you can do equality comparisons between two models and it allows you to "unsubscribe"), but it felt a little heavy for this project so I just went with this simpler approach

[andreipitis]

Hey @iwasrobbed!

When I created this component, it only had the ASPVideoPlayerView. Then I thought I should include a simple player for someone who just wanted to play videos, but was not interested in getting callbacks, or just didn't want to create a custom UI. That’s why I didn’t include the closure functionality for the ASPVideoPlayer component, but I guess I should have expected someone would want both the UI and the closures 😄 .

I planned to include a delegate, or to forward the closures to the ASPVideoPlayer but I haven’t gotten around to it yet.

This is a good approach however it does have it’s set of issues:

• you can’t remove an observer
• you implemented something simple to clear all observers if a nil value is provided, however this produces the exact same behaviour of overriding the controls closures.

I will probably change this once I merge the PR.

I also found a few issues related to some of your other changes:

• The scrubber issue that you already mentioned. I have a quick fix for this:
  • replace #selector(ASPVideoPlayerControls.progressSliderChanged(slider:)), for: [.valueChanged]) with the initial code #selector(ASPVideoPlayerControls.progressSliderChanged(slider:)), for: [.touchUpInside]) in the ASPVideoPlayerControls class.
  • replace sendActions(for: .valueChanged) with sendActions(for: .touchUpInside) in the Scrubber class in didTapScrubber)
• In the example project with the 4 videos, sometimes only 3 of them would be playable. I think this might be because of the async load. The items load in a different order from the videoURL array order, which is used in the ASPVideoPlayer logic to show or hide the next/previous button.
• The AVQueuePlayer removes items when playback if finished. If shouldLoop is set to false, when the last item is done playing, it gets removed and the logic for getting the currentVideoIndex in ASPVideoPlayerView will break, which means that you can’t go back to the previous video either. This is one of the main reasons why I avoided using AVQueuePlayer
• If you drag the scrubber to the end of the video, instead of playing the next one, it restarts the current video (I can only reproduce this with the 30 minute stream video).

I will try to find a fix for the remaining issues and if I manage I will merge your PR.

[iwasrobbed]

I agree, the closures design has its limitations. It was just a stop-gap until something else could be implemented since I needed to move quickly while trying to limit bugs or API changes.

Re: AVQueuePlayer: I'm not really using it as a queue, but the only advantage I saw over a regular player is that it avoids the white/black blip in between looping videos. My app displays only one video at a time and loops it, so this was an important consideration for me.

If you need any help on other bugs, feel free to assign to me. Otherwise, I'd be happy to test your changes and obviously feel free to change anything you'd like or cherry-pick anything from this PR if you'd rather not merge it all.