Metal implementation (using CIFilter) + Simplified Chinese translation #20

importRyan · 2021-08-01T02:44:14Z

As discussed via email, I'll continue to look at CPU-side timing to boost performance for larger window capture areas from this starting point. Monitoring the preview window's screen membership to switch preferred Metal devices might also be better.

Finally, sorry re: using one commit for the Filter menu additions and Mandarin translations; hopefully it's still ok to sort through.

- Bumped macOS target to 10.11 for MTKView - Bumped version to 2.1, build to 125 - macOS only resizing delegate

- Bump min deployment target to 10.13 to support Metal CIColorKernel init - Add fcikernel flags to Build Settings

…oats - Per correspondence with the authors, linearizing sRGB is not required despite the derivation. The publication also rendered its figures without. - Did not move HCIRN to use half-precision floats because I'm not familiar with the algo.

A FilterStore - Loads CIFilter from a FilterVendor. The app will have as many instances of a light-weight FilterVendor as there are filters known to initialize. - Holds a reference and instructs its application to a CIImage reference - Changes between vision simulations upon instruction

Swaps between HCIRN and Machado methodologies. Setup as a singelton defaulting to HCIRN. Forwards any user intents to the current Store.

- Move some objc constants into Swift temporarily into a consolidated Model folder

…mplementation of CGWindowListCreateImage - Still limited in performance by CGWindowListCreateImage at higher res. I should focus more on this than the sideshow effort below: - User setting to reduce performance simply skips some calls, as before - Uses CVDisplayLinkCreateWithActiveCGDisplays and its callback as a hardware refresh-synced timer, albiet generalized to all displays not just the current one. More precision would come from tracking the display showing the MTKView, possibly observing preferredFramesPerSecond as you pointed out.

- Manages Metal context for CIFilters and drives an MTKView - Relocated ScreenCapture into Metal folder as it's a potentially shared protocol with device-specific implementations

0.5 opacity would cause a problem (once not using OpenGL)

… Temporarily bump to 10.15 - Swapped for Filter View in Storyboard - Removed FilteredView - MTKView.preferredMetalDevice available w/ 10.15

…nslation - Handle intents in WindowController alongside VisionType - Indented several existing menu items and window menu items for consistency

- WindowController file diff shows up with false positive changes maybe from control I

michelf

So that's a big PR, making it a bit difficult to review. But this new code structure makes several improvements so I think it's worth merging, and the new filters address issue #18.

This is a first round of comments. I should have more later.

General notes:

It's a bit unfortunate that tabs were changed to spaces in many places; it creates "false" changes in the diff. Opened Project-level indentation settings #22.
Also, for iOS the way forward will be to translate the remaining Objective-C code to Swift. Not in scope for this PR though.

michelf · 2021-08-01T21:42:36Z

Sim Daltonism.xcodeproj/project.pbxproj

 					};
 					1EF253061C27498600DB3B18 = {
 						CreatedOnToolsVersion = 7.2;
-						DevelopmentTeam = R5LCPCXJPZ;
+						DevelopmentTeam = FVC2SFVFYG;


Changing development team settings is going to be a problem if I keep getting pull requests from you or others. Not something to address in this pull request, but I created issue #21.

Agreed. Forgot to remove that before submitting. Apologies.

michelf · 2021-08-01T21:43:44Z

Mac App/ViewController.swift

+
+        // Grab frame on main thread
+        let initialFrame = view.frame
+        filteredView.frame = initialFrame


In the current version, the filtered view's frame is inset a bit from the window edges (except on the top). This has two effects:

the border is larger and more visible, which helps make the window visible given that discounting the shadow effect it is otherwise transparent

the inset means there is no region near the edges where the window resize arrow cursor is showing up but you can't grab the window because it is ignoring mouse events

Insetting by 3 points on the sides and bottom edges should keep things working as they currently are.

michelf · 2021-08-01T21:44:30Z

Metal/ScreenCapture/CGWindowListScreenCapturer.swift

+
+    func removeMonitorsFor(window: NSWindow) {
+        let center = NotificationCenter.default
+        center.removeObserver(self, name: NSWindow.didChangeOcclusionStateNotification, object: window)


Is this an oversight? You are removing the observer only for one notification here while setupMonitorForInteraction above sets four observers.

michelf · 2021-08-01T23:50:50Z

Metal/Stores/FilterStoreManager.swift

+        return Self.shared
+    }
+
+    private init(vision: NSInteger, simulation: NSInteger) {


I'm a bit surprised by the usage of NSInteger here and elsewhere.

It's not needed in Swift: it's always equivalent to Int. In fact, all Apple APIs expose NSInteger as Int in Swift.

Shouldn't these be cases in an enum so we have a nice list of constants to work with? Or is there a reason why not to use an enum that I am overlooking?

Agreed and good to know. I had delayed making a proper Swift enum in thinking I'd get the objc sorted so the model wouldn't be duplicated. But that's unnecessary. Will fix up.

michelf · 2021-08-02T00:00:41Z

Metal/Stores/HCIRNFilterStore.swift

+        self.visionSimulation = vision
+        DispatchQueue.global().async { [weak self] in
+            self?.visionFilter = self?.loadFilter(for: vision)
+        }


Is this thread-safe? You are assigning to visionFilter from a global queue here while the same visionFilter variable is accessed by applyFilter that gets called from the queue in CGWindowListScreenCapturer.

Correct. CIFilters are not thread safe. Yet, there are no user-adjusted dynamic values, so I figured it shouldn't rear its head? But, yeah, it's absolutely incorrect. I'll change how these filters are handled to make it clean and address another concern re: letting windows have different active filters.

michelf · 2021-08-02T00:03:51Z

Metal/MetalRenderer.swift

+
+    /// Prepare the frame received by applying available filter(s). Call MTKView.draw(in:) to execute.
+    func render(_ image: CIImage) {
+        self.image = FilterStoreManager.shared.current.applyFilter(to: image) ?? image


Is this thread-safe? The current property here is accessed from the from the queue in CGWindowListScreenCapturer, but the main thread can also change the current filter. Looks like a race condition to me.

Calling applyFilter here appears to have the same problem. (See other comment.)

michelf · 2021-08-02T13:30:53Z

More thoughts about the FilterStore system of classes...

As explained above it appears to have a thread safety issue. I think it'd be better if all the stateful parts where stored within the same class that would encapsulate all the read and write operations on that state. That would make thread safety issues easier to deal with.

Another issue: the chosen filter is now stored as a global state. If the user creates multiple filter windows it is no longer possible to select a different filter for each one.

importRyan · 2021-08-03T04:24:50Z

Awesome lessons here for my first PR. 👍🏻 Thanks. I'll change up that system. Never thought of windows w/ separate filters. Neat use case. Thanks for the careful review.

Fix MetalRenderer typo

Called by hosting ViewController in viewDidDisappear().

Write a DTO wrapper for persistence (window restoration or UD)

importRyan · 2021-08-10T07:05:24Z

@michelf Michel, I've come back round to this. The latest commits should address the to-dos identified earlier.

Re: window bounds, the mouse monitor inset rect had a math error. I've fixed that and given it a few pixels more than prior.

michelf · 2021-08-12T02:28:16Z

Thank you for those changes. I should be able to review this next weekend.

michelf

I see there's now a FilterStoreManager protocol with a single QueueOwningFilterStoreManager implementation. I'm not sure what's the point of that. I'd encourage you to not build more structure than what's actually needed. This is particularly true here since we're not going to need many kinds of FilterStoreManager in the app. In general, more structure means more code to maintain and more components to update when making changes later, so it's better to stay lean when you can.

I think the above comment also applies to the two FilterStores too. I think it'd be simpler to follow what the code is doing if they were both merged within the FilterStoreManager with a single function having two-level switch in the style:

 switch sim {
    case .wicklineHCIRN:
      switch vision {
        case .normal: return nil
        case .deutan: return CIFilter(name: vendor.DeutanFilterName)
        ...
      }
   case .machadoEtAl:
      switch vision {
        case .normal: return nil
        ...
      }
 }

To summarize, that structure you built for choosing the CIFilter is not pulling its weight in my opinion. Two protocols (FilterStoreManager and FilterStore) and three classes (FilterStoreManager, HCIRNFilterStore, MachadoFilterStore) could easily be merged into a single class. A single class with the responsibility of transforming a (VisionType, Simulation) pair into a CIFilter. (Note: keeping the *Vendor classes separate is a good idea though.)

I'll admit I'm also guilty sometime of writing too much structure to encapsulate simple tasks. I guess it's a natural thing to do when trying to split a problem domain into its components: you sometime end up with too many components. What happens later if you keep them is it makes future changes to the code more complicated because some responsibility is scattered through a sea of components.

Most of your PR and the features it adds is quite good, that's why I'm putting extra efforts into reviewing it. Please keep going. I'll probably have more comments later about smaller things (it's a big PR after all). Feel free to ask if you have questions or want some clarification.

michelf · 2021-08-12T02:02:26Z

Mac App/Model/UserDefaults+WindowRestoration.swift

+    }
+}
+
+fileprivate enum VisionTypeDTO: Int {


I don't understand the point of this enum. It has all the same types as VisionType, why use a different enum?

In this case, it is a perhaps unnecessary habit of isolating persisted models from app models (i.e., the rawValue Int). It can simply use VisionType / Simulation and a persistenceValue so it is at least explicit what a change could disrupt.

michelf · 2021-08-12T02:03:40Z

Mac App/Model/UserDefaults+WindowRestoration.swift

+    static let SimulationKey = "SimulationKey"
+}
+
+fileprivate enum SimulationDTO: Int {


Same with this one. Why not use Simulation?

michelf · 2021-08-12T02:16:18Z

Metal/ScreenCapture/CGWindowListScreenCapturer.swift

+        guard unhideOnNextDisplay else { return }
+        DispatchQueue.main.async { [weak self] in
+            // recheck, because the state could have changed since the dispatch
+            guard self?.unhideOnNextDisplay == true else { return }
+            self?.unhideOnNextDisplay = false


I think there's a thread safety issue here, although not of your own making since the same logic is present in the older code: unhideOnNextDisplay is used from both the main queue and the capturing queue. Added #23 to track this.

Good catch. This commit moves the main queue write to the filter queue.

michelf · 2021-08-17T02:22:56Z

Mac App/Model/Errors.swift

+    alertHelpButton: "Contact the developer about this error."
+) as Error
+
+let AVScreenCaptureSessionSetupFailure = NSError(


Am I mistaken or is this error constant not used anywhere?

Thanks for catching that. It was a stub for adding a screen recording permissions prompt for 10.15+. This commit:

presents an error from CGWindowListCapturer with an adaptive image

adds translations for the Metal errors

One class now controls vision type changes and swapping filter vendors. A CIFilterVendor uses a stringly-typed method to create CIFilters. To simplify how the new FilterStore would call for filters, the VisionType enum has been extended with standardized CIFilter names.

… single queue

importRyan · 2021-08-24T02:33:38Z

Most of your PR and the features it adds is quite good, that's why I'm putting extra efforts into reviewing it.

As a self-taught developer, I welcome all the feedback you've given. Thank you for all your comments. It's fun to contribute.

when trying to split a problem domain into its components: you sometime end up with too many components.

Agreed. Condensing them removed a lot of boilerplate. The only differences really were in the Vendors.

michelf · 2021-10-27T19:49:26Z

I've left this hanging for too long, sorry.

I think I'll accept this as is and perhaps make a few changes myself later if needed. Can you just remove or revert 7bf5c79?, I'm not sure this fix is correct and I'd rather address it separately. (It's not like you introduced that bug, it was there before too.)

…lay to a single queue" This reverts commit 7bf5c79.

importRyan · 2021-10-28T18:37:24Z

Reverted. I've also meant to come back to this. Thanks for the prompt.

michelf · 2021-10-29T11:53:19Z

Merged! Again, thank you. I look forward to other pull requests.

I made some changes (rationale in each commit message):

b68ae39: Merged ThreadSafeFilterStore and FitlerStore in one class
628937d: Renamed ScreenCapturer protocol to ImageCapturer
b7e4c9e: Moving Simulation and VisionType enums out of the Mac folder

I also believe there's too much code in UserDefaults+WindowRestoration.swift for what it does, but I'm not sure what to do yet with this. I suppose migrating this to iOS might help figure out how to streamline this.

importRyan added 18 commits July 31, 2021 17:05

Add .DS_Store to git ignore

56d8b3a

Added paused MTKView subclass, bump to 10.11

49a9281

- Bumped macOS target to 10.11 for MTKView - Bumped version to 2.1, build to 125 - macOS only resizing delegate

Remove entitlements from Copy Bundle Resournces to silence warning

b593d00

Update project settings to silence warning

bb634d7

Port OpenGL HCIRN filter to CIColorKernel, bump to 10.13

80c2818

- Bump min deployment target to 10.13 to support Metal CIColorKernel init - Add fcikernel flags to Build Settings

Add FilterStoreManager

377666c

Swaps between HCIRN and Machado methodologies. Setup as a singelton defaulting to HCIRN. Forwards any user intents to the current Store.

Register Metal filters on app launch

898aeac

- Move some objc constants into Swift temporarily into a consolidated Model folder

Added MetalRenderer

beafd36

- Manages Metal context for CIFilters and drives an MTKView - Relocated ScreenCapture into Metal folder as it's a potentially shared protocol with device-specific implementations

Update vision simulation intents to use FilterStore/FilterStoreManager

2657d17

Make ColorView opaque to cover MTKView flicker during window resize

1e94d9f

0.5 opacity would cause a problem (once not using OpenGL)

Implemented ViewController to link MTKView, Renderer, ScreenCapturer.…

04b0787

… Temporarily bump to 10.15 - Swapped for Filter View in Storyboard - Removed FilteredView - MTKView.preferredMetalDevice available w/ 10.15

Update storyboard menu bar for HCIRN/Machado + Simplified Chinese tra…

4f0b68c

…nslation - Handle intents in WindowController alongside VisionType - Indented several existing menu items and window menu items for consistency

Remove OpenGL imports from bridging header file

ddc73e8

Update copyright

4f4a975

- WindowController file diff shows up with false positive changes maybe from control I

Drop deployment target to 10.14 by manually finding preferred MTLDevice

5d76823

importRyan mentioned this pull request Aug 1, 2021

Correcting color blindness simulation matrices aalto-ui/aim#13

Closed

michelf requested changes Aug 2, 2021

View reviewed changes

importRyan added 8 commits August 9, 2021 23:05

XPROJ — Revert DevelopmentTeam

4943859

Remove singleton FilterStoreManager

908a1bf

Define a thread-safe FilterStoreManager protocol

69f98bd

Concrete implementation of a thread-safe FilterStoreManager

882dc3e

FilterStores — adapt to being managed on an unowned queue

5a0cb33

MetalRenderer — use the new FilterStoreManager

bc4b374

Fix MetalRenderer typo

ViewController — Ask its parent window for a FilterStoreManager

8780ea8

DispatchQueue — Vend uniquely named queues

e1e1974

importRyan added 6 commits August 10, 2021 00:00

WindowController — Manage filters per instance

8fb1b34

CGWindowListScreenCapturer — Use external queue

fc4b0fa

CGWindowListScreenCapturer — Remove observers in stop() method

6052f26

Called by hosting ViewController in viewDidDisappear().

UserDefaults — Isolate read/writes for vision/simulation

fb15ac6

Vision + Simulation enum for window restoration, runtime, UserDefaults

114f9a4

Write a DTO wrapper for persistence (window restoration or UD)

Window Resizing — Expand inset for ignoresMouseEvents

0da090c

importRyan force-pushed the metal-cifilter branch from a2e2529 to 0da090c Compare August 10, 2021 07:00

michelf requested changes Aug 17, 2021

View reviewed changes

importRyan added 2 commits August 23, 2021 15:04

Correct CIFilter display name typo

f086d0a

importRyan force-pushed the metal-cifilter branch from 052f681 to f086d0a Compare August 23, 2021 22:04

importRyan added 2 commits August 23, 2021 15:16

CGWindowListScreenCapturer — michelf#23 Move unhideOnNextDisplay to a…

7bf5c79

… single queue

ScreenCapture — Add permissions alert with image for 10.15+

b925098

Revert "CGWindowListScreenCapturer — michelf#23 Move unhideOnNextDisp…

02eb29c

…lay to a single queue" This reverts commit 7bf5c79.

michelf merged commit 02eb29c into michelf:master Oct 29, 2021

michelf mentioned this pull request Jul 13, 2024

window shows only desktop #27

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metal implementation (using CIFilter) + Simplified Chinese translation #20

Metal implementation (using CIFilter) + Simplified Chinese translation #20

importRyan commented Aug 1, 2021

michelf left a comment

michelf Aug 1, 2021

importRyan Aug 3, 2021

michelf Aug 1, 2021

michelf Aug 1, 2021

michelf Aug 1, 2021

importRyan Aug 3, 2021

michelf Aug 2, 2021

importRyan Aug 3, 2021 •

edited

Loading

michelf Aug 2, 2021

michelf commented Aug 2, 2021

importRyan commented Aug 3, 2021

importRyan commented Aug 10, 2021

michelf commented Aug 12, 2021

michelf left a comment

michelf Aug 12, 2021

importRyan Aug 24, 2021

michelf Aug 12, 2021

michelf Aug 12, 2021

importRyan Aug 24, 2021

michelf Aug 17, 2021

importRyan Aug 24, 2021 •

edited

Loading

importRyan commented Aug 24, 2021

michelf commented Oct 27, 2021

importRyan commented Oct 28, 2021

michelf commented Oct 29, 2021

Metal implementation (using CIFilter) + Simplified Chinese translation #20

Metal implementation (using CIFilter) + Simplified Chinese translation #20

Conversation

importRyan commented Aug 1, 2021

michelf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

importRyan Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michelf commented Aug 2, 2021

importRyan commented Aug 3, 2021

importRyan commented Aug 10, 2021

michelf commented Aug 12, 2021

michelf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

importRyan Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

importRyan commented Aug 24, 2021

michelf commented Oct 27, 2021

importRyan commented Oct 28, 2021

michelf commented Oct 29, 2021

importRyan Aug 3, 2021 •

edited

Loading

importRyan Aug 24, 2021 •

edited

Loading