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

Investigate Data compression #15

Open
ChrishonWyllie opened this issue Jan 11, 2020 · 16 comments
Open

Investigate Data compression #15

ChrishonWyllie opened this issue Jan 11, 2020 · 16 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@ChrishonWyllie
Copy link
Owner

Compressing Data requires iOS 13.0, which may be too restricting.
Additionally, it seems that compressing Data results in a larger file so it seems unnecessary given the original file size

@ChrishonWyllie ChrishonWyllie added the enhancement New feature or request label Jan 11, 2020
@ChrishonWyllie ChrishonWyllie added this to the Celestial-1.0 milestone Jan 11, 2020
@ChrishonWyllie ChrishonWyllie self-assigned this Jan 11, 2020
@johndpope
Copy link

johndpope commented Apr 16, 2021

when this video exports / https://d21m91m763fb64.cloudfront.net/videos/8acab1c0-7cb0-11eb-821a-9b4798e5df00-8secondz-video.mp4 the video gets incorrectly resized.

It would be nice to be able to bypass the compression to disk.

UPDATE - this fixed things for me for now.
playerView = URLVideoPlayerView(delegate: self, cacheLocation: .none)

@ChrishonWyllie
Copy link
Owner Author

Hi @johndpope
I'm having a bit of trouble reproducing this.
Would you mind sharing a screenshot of the video when it is incorrectly resized using the .fileSystem cacheLocation?

@johndpope
Copy link

was happening on the iphone12pro - maybe reproducible on simulator - it was like the video stretched to a 1/4 of the size. top - left quarter. I like this repo - and it was my approach to do prefetching here - TikTok seem to get 6 videos eagerly upon app start - I'm still unsure best way to do this.

@ChrishonWyllie
Copy link
Owner Author

Hmm, here's what I tried to reproduce it:

I'm not aware of the dimensions of the original video file, is this the incorrect dimension?

Screen Shot 2021-04-20 at 8 53 40 PM

Also regarding TikTok and a few other social media apps, I'm still trying to see how they do that.
I agree, it seems as if there's some content that is already available and doesn't need to be fetched (or rather it doesn't appear to the user)

One approach could be to kick off the prefetching of the videos during the AppDelegate willFinish or didFinish phase.
I haven't tried that as of yet, however. Not sure if those few milliseconds before the view is displayed would give the impression that the videos were already downloaded.

@johndpope
Copy link

I think the results would be comparable to
Something as crude as
Uicollectionview / prefetch cells at index path
+1 - get video
+2 - get video
....
+6 - get video

@johndpope
Copy link

johndpope commented Apr 21, 2021

If you hit the URL you’ll see it’s a 9:16 format

uPDATE

this crude approach works

I want to use your code to simply pick up / merge from existing download. It seems you are the only one on GitHub who has achieved this. Will take another look at it when I have more bandwidth

https://gist.github.com/johndpope/8d4ecbdeb1ecb3188b55afc5282d544e

@ChrishonWyllie
Copy link
Owner Author

Ah yes, I see its in the 9:16 format. I'll spend some time to correct this.
Thank you for pointing this out.

@johndpope
Copy link

Hi Chrishon -
a tiny wishlist (if I may) - I'd like to be able to bypass the export / resizing - and just save original (bloated) video.
(BTW - I believe that export is running on background (as opposed to utility) thread - which may not run when battery is low. )

@ChrishonWyllie
Copy link
Owner Author

Hi John

Sure, I can do that. An option for bypassing the resizing will be provided.
And you are correct about the background vs. utility threads. I will address that as well.

@ChrishonWyllie
Copy link
Owner Author

Hi @johndpope

I have released a new version 0.8.94
The major changes are:

  • Removed the default compression for videos. Instead the original downloaded video will be used. But there are options in the initializer for exporting to a lower quality.
  • Replaced .background Qos with .utility
  • Added resolution and aspectRatio properties to the URLVideoPlayerView to help with determining the proper frame requirements

As for the video you provided that had an aspect ratio of 9:16, I implemented some changes to the example to address this concern.
The cells will now resize based on the ratio of the video and the other elements within the cell.
I used your video to test this, but didn't include it in the final version since I wasn't sure if this was a public video.

Please take a look at the CHANGELOG for a little more detail, and let me know if you have any issues.

@johndpope
Copy link

great - I will test this out. thanks!

@johndpope
Copy link

johndpope commented Apr 30, 2021

@ChrishonWyllie
Copy link
Owner Author

Hmm, there's have quite a few observers going on.
In the stopObserving() function, they clear the kvoObservers array, but I figured they would have to individually stop observing each one within the array, rather than setting to empty.

@johndpope
Copy link

johndpope commented May 1, 2021 via email

@ChrishonWyllie
Copy link
Owner Author

Yes. I had to adjust my kvo approach to use this. I have some proprietary code base but I want to send you a snippet. I have introduced a state engine from gamekit to shift through the fetching / playable states. I also have prototyped using digger library but it is not as good as your approach as it kind of blocks user from viewing video until it has fully downloaded. Spent a few weeks on it but still not there. The good thing with digger is it provides some granular control over stopping and starting the workers / downloads. JP

On Saturday, May 1, 2021, Chrishon Wyllie @.***> wrote: Hmm, there's have quite a few observers going on. In the stopObserving() function, they clear the kvoObservers array, but I figured they would have to individually stop observing each one within the array, rather than setting to empty. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACGZSTZTMUG3GYCFYECHELTLM52PANCNFSM4KFQMAAA .

Sure, if you want me to take a look at it, you can email me privately if you prefer.

@ChrishonWyllie
Copy link
Owner Author

ChrishonWyllie commented May 6, 2021

So for the granular control that you mentioned, Celestial provide the ability to pause and resume download tasks:

    func pauseDownload(for sourceURL: URL)
    // Usage: Celestial.shared.pauseDownloaf(for: someURL)
    
    func resumeDownload(for sourceURL: URL)
    // Usage: Celestial.shared.resumeDownloaf(for: someURL)

Those are essentially what the two prefetching APIs are doing under the hood, but this lets you pause/resume individual tasks.
These functions are independent of the URLVideoPlayerView or URLImageView
I need to update the docs to include these, if it wasn't mentioned.
However, there's a bug I noticed with this and working on it now (Update, resolved in version 0.8.118)


As for your code snippets, I'm not seeing any standout reasons why the video needs to wait for a complete download to begin playing.
I see the status observer for the .readyToPlay case, but I can't see whether the play() function is called when it hits that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants