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

[Feature Request] Categorize videos in cache based on type #809

Closed
Kiran-B opened this issue Jun 10, 2019 · 12 comments
Closed

[Feature Request] Categorize videos in cache based on type #809

Kiran-B opened this issue Jun 10, 2019 · 12 comments
Milestone

Comments

@Kiran-B
Copy link

Kiran-B commented Jun 10, 2019

Most videos are available in multiple variants. The variants could be 2K SDR, 2K HDR, 4K SDR, 4K HDR, 2K SDR H264. But on any given Mac only one of the variants of the video is being used.

At the moment, all variants of a video are stored in a flat hierarchy with the cache folder.

It would be easier to move videos between Macs, if these are stored within a sub folder named after their respective variant.

Example: [CACHE_DIR]/2K_SDR/[2K_SDR_VIDEOS].mov

This would help even more when we are dealing with non-obvious names like 'b2-4.mov'.

I do realize that most videos have suffix like '_SDR_4K_HEVC' in their name. Hence feel free to ignore this request or set a low priority.

@glouel
Copy link
Collaborator

glouel commented Jun 10, 2019

hi @Kiran-B

I was myself frustrated yesterday, moving the 4K-SDR to my NAS to make space for the HDR videos so I totally get the idea ! I was thinking of moving the JSONs in their own directories too (I kinda expect tvOS 13 json, if it comes this way, as a ressources.tar too) but for the videos that's a great idea too!

The implementation should be easy enough, the upgrade path may be the biggest issue, will have a look at it.

With containairization, I'll also have to consider rethinking the whole cache thing and probably have, as an option, a rotating cache with a user definable size. I'll try to think of all of this when checking this out.

@glouel glouel added this to the 1.5.1 milestone Jun 10, 2019
@Kiran-B
Copy link
Author

Kiran-B commented Jun 12, 2019

I agree with you on containerization challenge. This module will probably need com.apple.security.files.bookmarks.app-scope entitlement for it to have a shared cache. I believe Apple is moving towards XPC extension model for screensaver (which should allow the save to claim this entitlement). The infra appears to be built in Mojave as well (/System/Library/Frameworks/ScreenSaver.framework/PlugIns/iLifeSlideshows.appex).

However they have not yet documented this :(

On a positive note, I could see com.apple.security.temporary-exception.files.absolute-path.read-only entitlement in legacyScreenSaver.appex . So I am wondering if the User provides a path to cache folder, would the screensaver be able to read it. It would definitely not be able to write new videos. So I am thinking of a two part solution,

  1. An App that downloads and manages the videos in a specific folder.
  2. Screensaver that just reads JSON + videos from the path App is using.

@glouel
Copy link
Collaborator

glouel commented Jun 12, 2019

I agree with you on containerization challenge. This module will probably need com.apple.security.files.bookmarks.app-scope entitlement for it to have a shared cache. I believe Apple is moving towards XPC extension model for screensaver (which should allow the save to claim this entitlement). The infra appears to be built in Mojave as well (/System/Library/Frameworks/ScreenSaver.framework/PlugIns/iLifeSlideshows.appex).

Oh this is super interesting, I'm not sure if we'll be allowed this as third parties ? My guess is they want to clamp it as much as possible securitywise for code they don't own.

However they have not yet documented this :(

Indeed !

On a positive note, I could see com.apple.security.temporary-exception.files.absolute-path.read-only entitlement in legacyScreenSaver.appex . So I am wondering if the User provides a path to cache folder, would the screensaver be able to read it.

Oh wow, I didn't see that ! That would be a great news indeed. The way Aerial works right now, the first thing it tries to do is download/write the jsons so that may be why I thought we had no read access either. I need to check this later today, thanks a lot !

It would definitely not be able to write new videos. So I am thinking of a two part solution,

  1. An App that downloads and manages the videos in a specific folder.
  2. Screensaver that just reads JSON + videos from the path App is using.

I hope in the end I can avoid splitting Aerial. There's already an app in the project than runs the control panel (and it fails the same in Catalina) that could be a way to implement this.

But if we have access to the new screensaver API, hmm, that would probably mean having to split into 2 screensavers, which is not much better...

I really want some documentation !! In any case, thanks for the super useful feedback, I'll check the readonly thing asap and let you know. If we do have blanket read only access I'll split the json in a folder (probably a good time to move it to Application Support per that other thread) and maybe change the cache mechanism so we can have "extra" cache folders where Aerial would look for it's videos (on top of the container one). It would only download in the container one but you could manually move it later on if you so wish. I'll check it out !

@glouel
Copy link
Collaborator

glouel commented Jun 12, 2019

Well, I can confirm we have read only access to at least the user folder ! Didn't have time to look deeper into it.

Now I need to think about all this !

@Kiran-B
Copy link
Author

Kiran-B commented Jun 13, 2019

Agree that splitting is not a User friendly move. But if the cache has to be maintained outside of the sandbox, having a separate app seems to be the only choice at the moment.

They would work in a producer/consumer model; where in,

  • the app takes responsibility of downloading/maintaining JSON+videos(basically everything that is written into cache folder)
  • the screensaver module reads the JSON+videos and renders as screensaver. It would also be responsible to allow User set their preference which could be stored within the sandbox.

The app can easily seek com.apple.security.files.bookmarks.app-scope entitlement to allow it to write content to a folder of User's choice.

My usage shares the cache between OS. To explain further I have three volumes.

  1. macOS: Always has the current stable macOS.
  2. futureOS: Has the next macOS. Now Catalina.
  3. shared: Common data to be shared between OSes.

Aerial cache being large, I use the following cache path: /Volumes/shared/Extras/Aerial. Till date this arrangement made possible to share the Aerial cache across OSes.

@gregarios
Copy link

What has become of /Users/Shared/ in Catalina? Maybe that is an option for the video cache...

@Kiran-B
Copy link
Author

Kiran-B commented Jun 13, 2019

@gregarios
AFAIK, the issue is that the screensaver no longer has unrestricted write access to the filesystem. It is now restricted to write data only within its container(sandbox).

What has become of /Users/Shared/ in Catalina?

Catalina also has made interesting changes to file system. During the installation, it splits the OS volume into two. One for OS components which is READ-ONLY for the rest of the world and a data volume which has /Users and /Devices. So definitely the UNIX path /Users/Shared/ would not work.

@glouel
Copy link
Collaborator

glouel commented Jun 13, 2019

My usage shares the cache between OS. To explain further I have three volumes.

  1. macOS: Always has the current stable macOS.
  2. futureOS: Has the next macOS. Now Catalina.
  3. shared: Common data to be shared between OSes.

Aerial cache being large, I use the following cache path: /Volumes/shared/Extras/Aerial. Till date this arrangement made possible to share the Aerial cache across OSes.

Yes I definitely agree that this is a usage scenario that Aerial must support.

I should have a workaround for the cache usage soonish, but I'm not sure it should be considered the main way most people will use the screensaver.

Agree that splitting is not a User friendly move. But if the cache has to be maintained outside of the sandbox, having a separate app seems to be the only choice at the moment.

They would work in a producer/consumer model; where in,

  • the app takes responsibility of downloading/maintaining JSON+videos(basically everything that is written into cache folder)

(Definitely not JSON. This would break first launch and periodically look for new videos, unless you go for something invasive like a resident app which I'd be quite reluctant to go to)

  • the screensaver module reads the JSON+videos and renders as screensaver. It would also be responsible to allow User set their preference which could be stored within the sandbox.

I do get what you are saying and I do like the simplicity of the model (from a programming pov), but for those who don't need to put their videos outside the container, that model from a user perspective is needlessly complex. And it breaks stream and cache as you go, or caching from panel in the container (and first launch/json updates as mentioned above).

If we do have to go the app way, I'm not sure it should be required. I'm not sure how accepting most people would be of that model.

The app can easily seek com.apple.security.files.bookmarks.app-scope entitlement to allow it to write content to a folder of User's choice.

Short term, I'll release the App version form of Aerial to manage your videos, depending on how documentation evolves we'll see where it needs to go.

Since you may know, what about simple things like opening a path in finder, would that work in legacyScreenSaver ? And opening an URL in default browser ? Or opening our new app from control panel ?

If so... what about curl ? 🤔

@glouel
Copy link
Collaborator

glouel commented Jun 24, 2019

@Kiran-B

So I've been progressing a bit about Catalina support (and still WIP for categorizing videos, but it's coming, promise :)).

But since you mentionned some knowledge about XPC, I'm having an issue which you may know about, right now NSOpenPanel (to select a directory) fails with this in console in Catalina:

com.apple.appkit.xpc.openAndSavePanelService	Registered, pid=9208 ASN=0x0,0x34b34b
com.apple.appkit.xpc.openAndSavePanelService	Registered, pid=9208 cgConnectionID=f8cab
com.apple.appkit.xpc.openAndSavePanelService	Current system appearance, (HLTB: 2), (SLS: 1)
com.apple.appkit.xpc.openAndSavePanelService	Post-registration system appearance: (HLTB: 2)
hidd	[HID] [MT] dispatchEvent Dispatching event with 2 children, _eventMask=0x42 _childEventMask=0x42 Cancel=0 Touching=0 inRange=1
cfprefsd	Allowing process impersonation by process com.apple.appkit.xpc.openAndSav (9208) despite not having the com.apple.private.defaults-impersonate entitlement due to it not being sandboxed. Please add com.apple.private.defaults-impersonate instead, this will stop working in the future.
cfprefsd	Allowing process impersonation by process com.apple.appkit.xpc.openAndSav (9208) despite not having the com.apple.private.defaults-impersonate entitlement due to it not being sandboxed. Please add com.apple.private.defaults-impersonate instead, this will stop working in the future.
com.apple.appkit.xpc.openAndSavePanelService	Registering for test daemon availability notify post.
com.apple.appkit.xpc.openAndSavePanelService	notify_get_state check indicated test daemon not ready.
com.apple.appkit.xpc.openAndSavePanelService	SignalReady: pid=9208 asn=0x0-0x34b34b
com.apple.appkit.xpc.openAndSavePanelService	SIGNAL: pid=9208 asn=0x0x-0x3453771

I've been trying to read up on containarization but I'm not sure I understand the impersonate issue here. If you have any idea, that would be super welcome !

Edit: Bah, I didn't read enough of console:

appleeventsd	TCCCreateDesignatedRequirementIdentityFromAuditToken: preparing xpc message
appleeventsd	TCCCreateDesignatedRequirementIdentityFromAuditToken: requesting designated requirement
com.apple.appkit.xpc.openAndSavePanelService	NSApp cache appearance:
-NSRequiresAquaSystemAppearance: 0
-appearance: (null)
-effectiveAppearance: <NSCompositeAppearance: 0x6000027a0e00
 (
    "<NSDarkAquaAppearance: 0x6000027a0b80>",
    "<NSSystemAppearance: 0x6000027a0d00>"
)>
tccd	-[TCCDAccessIdentity staticCode]: static code for: identifier com.apple.ScreenSaver.Engine.legacyScreenSaver, type: 0: 0x7fe58bf46c50 at /System/Library/Frameworks/ScreenSaver.framework/PlugIns/legacyScreenSaver.appex
legacyScreenSaver	ERROR: Unable to display open panel: your app is missing the User Selected File Read app sandbox entitlement. Please ensure that your app's target capabilities include the proper entitlements.

Sooooo, you can get read only access but NOT the ability to pick a file ? That seems... bah. My guess is there's nothing we can do on our end on this one.

One more Edit : I guess that makes sense since the entitlement is marked as temporary and doesn't seem to be listed on Apple's entitlement guide. Whether legacyScreenSaver keeps that entitlement is quite a question.

One last edit : I did implement a workaround for your usage scenario btw, check
#801 (comment)
and beta5 at the bottom

@Kiran-B
Copy link
Author

Kiran-B commented Jun 25, 2019

@glouel Yes. As you noted, your component needs entitlements such as com.apple.security.files.bookmarks.app-scope. Thus was my suggestion to have a companion app that manages cache. The requirement entitlements go into app.

The workaround is to use a text field where User may put the folder path. Your screensaver should be able to read the content in the path entered in text field. This is my interpretation of com.apple.security.temporary-exception.files.absolute-path.read-only

Based on the macOS trend, I wouldn't be surprised if a companion app is mandated by Apple. The App Store downloadable app appears to be the carrier for all documented app extensions. Ex: Xcode extension, Safari extension all seems to need an app.

One last edit : I did implement a workaround for your usage scenario btw, check
#801 (comment)
and beta5 at the bottom

Thanks a ton. Its working 🎉

@glouel
Copy link
Collaborator

glouel commented Jun 25, 2019

Thanks for the info, I think I'll put a temporary field as you suggested, should work fine indeed at least for the beta.

I'm a bit unsure how "temporary" those exceptions will be and if we will have them out of the beta too. If that's the way things go, I'll definitely consider strongly the companion app way. I'm really hoping for better documentation soon and a hint of guidance of where they want screensavers to go 😅

Glad to hear the stop gap is working in the meantime !

@glouel
Copy link
Collaborator

glouel commented Jul 29, 2020

Closing this, with 2.0 the cache is now fully managed.

@glouel glouel closed this as completed Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants