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

BufferMode: disk - implementation #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Eil88
Copy link

@Eil88 Eil88 commented Jun 11, 2019

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

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:
  • download/buffer a remote file to a disk and, upon completion, provide a temp url of the downloaded file to a user. I think this is the most common scenario of how users would use CachingPlayerItem – they just need the content to be played while it's downloading, and means of saving the content after it's loaded.
  • support playback of local files provided by a url. (but this can be achieved by using a regular AVPlayerItem I suppose)
    5**) Consider not exposing DiskResourceLoaderDelegate at all and provide only the implementation described in 5* to simplify things.

What do you think?

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.

None yet

2 participants