neekeetab / CachingPlayerItem

Play and cache media content simultaneously on your iOS device
MIT License
520 stars 89 forks source link

BufferMode: disk - implementation #22

Open Eil88 opened 5 years ago

Eil88 commented 5 years ago

Hi there! Thanks for the very useful code that you provided. I took the liberty to work on a custom implementation, to avoid loading the full downloading resource into memory. I am curious as to what you think about it!

Cheers, Alessandro

neekeetab commented 5 years ago

Hi Alessandro,

Thank you for putting so much effort into this! I appreciate it a lot!

I like what you tried to accomplish with the PR. At the same time, I think the PR greatly increases the complexity of CachingPlayerItem, and it feels like the API now needs to be redesigned. In particular, I think there's a need for a separate protocol for handling disk interactions. Also, callbacks in the new delegate methods feel weird and should be avoided IMO. I think one uses either delegate or callbacks, not both.

What I propose: 1) Create an entity called ResourceLoader that would have all the common logic and/or definitions among implementations of different loaders. 2) Rename ResourceLoaderDelegate to MemoryResourceLoader and conform it to/subclass from the ResourceLoader mentioned above. Make it public. 3) Same thing with DiskResourceLoader. Also make it public. 4) DiskResourceLoader has its own delegate DiskResourceLoaderDelegate and communicates with the outside world directly via this delegate. This means that a user has to provide concrete ResourceLoader (either MemoryResourceLoader or DiskResourceLoader) instance upon initialization of CachingPlayerItem. 5*) (optional) It would be really nice to have an implementation of DiskResourceLoader that would:

What do you think?