[macOS] FlutterSurfaceManager should not return surfaces that are in use

shell/platform/darwin/macos/framework/Source/FlutterSurface.h

@@ -32,7 +32,13 @@
 @property(readonly, nonatomic, nonnull) IOSurfaceRef ioSurface;
 @property(readonly, nonatomic) CGSize size;
 @property(readonly, nonatomic) int64_t textureId;
+// Whether the surface is currently in use by the compositor.
+@property(readonly, nonatomic) BOOL isInUse;
 
 @end
 
+@interface FlutterSurface (Testing)
+@property(readwrite, nonatomic) BOOL isInUseOverride;
+@end
+
 #endif  // FLUTTER_SHELL_PLATFORM_DARWIN_MACOS_FRAMEWORK_SOURCE_FLUTTERSURFACE_H_

shell/platform/darwin/macos/framework/Source/FlutterSurface.mm

@@ -10,6 +10,8 @@ @interface FlutterSurface () {
   CGSize _size;
   IOSurfaceRef _ioSurface;
   id<MTLTexture> _texture;
+  // Used for testing.
+  BOOL _isInUseOverride;
 }
 @end
 
@@ -27,6 +29,18 @@ - (int64_t)textureId {
   return reinterpret_cast<int64_t>(_texture);
 }
 
+- (BOOL)isInUse {
+  return _isInUseOverride || IOSurfaceIsInUse(_ioSurface);
+}
+
+- (BOOL)isInUseOverride {
+  return _isInUseOverride;
+}
+
+- (void)setIsInUseOverride:(BOOL)isInUseOverride {
+  _isInUseOverride = isInUseOverride;
+}
+
 - (instancetype)initWithSize:(CGSize)size device:(id<MTLDevice>)device {
   if (self = [super init]) {
     self->_size = size;

shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.h

@@ -88,7 +88,7 @@
 /**
  * Removes all cached surfaces replacing them with new ones.
  */
-- (void)replaceSurfaces:(nonnull NSArray<FlutterSurface*>*)surfaces;
+- (void)returnSurfaces:(nonnull NSArray<FlutterSurface*>*)surfaces;
 
 /**
  * Returns number of surfaces currently in cache. Used for tests.

shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm

@@ -153,7 +153,7 @@ - (void)commit:(NSArray<FlutterSurfacePresentInfo*>*)surfaces {
   FML_DCHECK([NSThread isMainThread]);
 
   // Release all unused back buffer surfaces and replace them with front surfaces.
-  [_backBufferCache replaceSurfaces:_frontSurfaces];
+  [_backBufferCache returnSurfaces:_frontSurfaces];
 
   // Front surfaces will be replaced by currently presented surfaces.
   [_frontSurfaces removeAllObjects];
@@ -266,9 +266,15 @@ - (void)presentSurfaces:(NSArray<FlutterSurfacePresentInfo*>*)surfaces
 
 // Cached back buffers will be released after kIdleDelay if there is no activity.
 static const double kIdleDelay = 1.0;
+// Once surfaces reach kEvictionAge, they will be evicted from the cache.
+// The age of 30 has been chosen to reduce potential surface allocation churn.
+// For unused surface 30 frames means only half a second at 60fps, and there is
+// idle timeout of 1 second where all surfaces are evicted.
+static const int kSurfaceEvictionAge = 30;
 
 @interface FlutterBackBufferCache () {
   NSMutableArray<FlutterSurface*>* _surfaces;
+  NSMapTable<FlutterSurface*, NSNumber*>* _surfaceAge;
 }
 
 @end
@@ -278,28 +284,65 @@ @implementation FlutterBackBufferCache
 - (instancetype)init {
   if (self = [super init]) {
     self->_surfaces = [[NSMutableArray alloc] init];
+    self->_surfaceAge = [NSMapTable weakToStrongObjectsMapTable];
   }
   return self;
 }
 
+- (int)ageForSurface:(FlutterSurface*)surface {
+  NSNumber* age = [_surfaceAge objectForKey:surface];
+  return age != nil ? age.intValue : 0;
+}
+
+- (void)setAge:(int)age forSurface:(FlutterSurface*)surface {
+  [_surfaceAge setObject:@(age) forKey:surface];
+}
+
 - (nullable FlutterSurface*)removeSurfaceForSize:(CGSize)size {
   @synchronized(self) {
+    // Purge all cached surfaces if the size has changed.
+    if (_surfaces.firstObject != nil && !CGSizeEqualToSize(_surfaces.firstObject.size, size)) {
+      [_surfaces removeAllObjects];
+    }
+
+    FlutterSurface* res;
+
+    // Returns youngest surface that is not in use. Returning youngest surface ensures
+    // that the cache doesn't keep more surfaces than it needs to, as the unused surfaces
+    // kept in cache will have their age kept increasing until purged (inside [returnSurfaces:]).
     for (FlutterSurface* surface in _surfaces) {
-      if (CGSizeEqualToSize(surface.size, size)) {
-        // By default ARC doesn't retain enumeration iteration variables.
-        FlutterSurface* res = surface;
-        [_surfaces removeObject:surface];
-        return res;
+      if (!surface.isInUse &&
+          (res == nil || [self ageForSurface:res] > [self ageForSurface:surface])) {
+        res = surface;
       }
     }
-    return nil;
+    if (res != nil) {
+      [_surfaces removeObject:res];
+    }
+    return res;
   }
 }
 
-- (void)replaceSurfaces:(nonnull NSArray<FlutterSurface*>*)surfaces {
+- (void)returnSurfaces:(nonnull NSArray<FlutterSurface*>*)returnedSurfaces {
   @synchronized(self) {
-    [_surfaces removeAllObjects];
-    [_surfaces addObjectsFromArray:surfaces];
+    for (FlutterSurface* surface in returnedSurfaces) {
+      [self setAge:0 forSurface:surface];
+    }
+    for (FlutterSurface* surface in _surfaces) {
+      [self setAge:[self ageForSurface:surface] + 1 forSurface:surface];
+    }
+
+    [_surfaces addObjectsFromArray:returnedSurfaces];
+
+    // Purge all surface with age = kSurfaceEvictionAge. Reaching this age can mean two things:
+    // - Surface is still in use and we can't return it. This can happen in some edge
+    //   cases where the compositor holds on to the surface for much longer than expected.
+    // - Surface is not in use but it hasn't been requested from the cache for a while.
+    //   This means there are too many surfaces in the cache.
+    [_surfaces filterUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(FlutterSurface* surface,
+                                                                          NSDictionary* bindings) {
+                 return [self ageForSurface:surface] < kSurfaceEvictionAge;
+               }]];
   }
 
   // performSelector:withObject:afterDelay needs to be performed on RunLoop thread

shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm

@@ -117,8 +117,13 @@ - (void)onPresent:(CGSize)frameSize withBlock:(nonnull dispatch_block_t)block {
   auto surfaceFromCache = [surfaceManager surfaceForSize:CGSizeMake(110, 110)];
   EXPECT_EQ(surfaceFromCache, surface2);
 
-  [surfaceManager presentSurfaces:@[] atTime:0 notify:nil];
-  EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul);
+  // Submit empty surfaces until the one in cache gets to age >= kSurfaceEvictionAge, in which case
+  // it should be removed.
+
+  for (int i = 0; i < 30 /* kSurfaceEvictionAge */; ++i) {
+    [surfaceManager presentSurfaces:@[] atTime:0 notify:nil];
+    EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul);
+  }
 
   [surfaceManager presentSurfaces:@[] atTime:0 notify:nil];
   EXPECT_EQ(surfaceManager.backBufferCache.count, 0ul);
@@ -163,6 +168,33 @@ - (void)onPresent:(CGSize)frameSize withBlock:(nonnull dispatch_block_t)block {
   EXPECT_EQ(surface3, surface1);
 }
 
+TEST(FlutterSurfaceManager, BackingStoreCacheSurfaceStuckInUse) {
+  TestView* testView = [[TestView alloc] init];
+  FlutterSurfaceManager* surfaceManager = CreateSurfaceManager(testView);
+
+  auto surface1 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)];
+
+  [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface1) ] atTime:0 notify:nil];
+  // Pretend that compositor is holding on to the surface. The surface will be kept
+  // in cache until the age of kSurfaceEvictionAge is reached, and then evicted.
+  surface1.isInUseOverride = YES;
+
+  auto surface2 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)];
+  [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface2) ] atTime:0 notify:nil];
+  EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul);
+
+  for (int i = 0; i < 30 /* kSurfaceEvictionAge */ - 1; ++i) {
+    auto surface3 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)];
+    [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface3) ] atTime:0 notify:nil];
+    EXPECT_EQ(surfaceManager.backBufferCache.count, 2ul);
+  }
+
+  auto surface4 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)];
+  [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface4) ] atTime:0 notify:nil];
+  // Surface in use should bet old enough at this point to be evicted.
+  EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul);
+}
+
 inline bool operator==(const CGRect& lhs, const CGRect& rhs) {
   return CGRectEqualToRect(lhs, rhs);
 }