bug#63187: 30.0.50; Tail of longer lines painted after end of nearby lines on macOS

[Alan Third <alan@idiocy.org>]

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]

On Thu, Jul 20, 2023 at 10:02:53PM -0400, Aaron Jensen wrote:
> 
> I've been using this for about a month now and have seen no artifacts:
> 
> diff --git a/src/nsterm.m b/src/nsterm.m
> index 78089906752..d23fb650ab8 100644
> --- a/src/nsterm.m
> +++ b/src/nsterm.m
> @@ -2708,9 +2708,6 @@ Hide the window (X11 semantics)
>      EmacsView *view = FRAME_NS_VIEW (f);
> 
>      [view copyRect:srcRect to:dest];
> -#ifdef NS_IMPL_COCOA
> -    [view setNeedsDisplayInRect:destRect];
> -#endif
>    }
> 
>    unblock_input ();
> @@ -10435,7 +10432,7 @@ @implementation EmacsLayer
>     cache.  If no free surfaces are found in the cache then a new one
>     is created.  */
> 
> -#define CACHE_MAX_SIZE 2
> +#define CACHE_MAX_SIZE 1
> 
>  - (id) initWithColorSpace: (CGColorSpaceRef)cs
>  {
> @@ -10621,7 +10618,7 @@ - (void) display
>  {
>    NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer display]");
> 
> -  if (context)
> +  if (context && context != [NSGraphicsContext currentContext])
>      {
>        [self releaseContext];
> 
> 
> I'm not sure what the ramifications are for CACHE_MAX_SIZE 1 on slower
> machines, but I don't notice any performance issues on my M1.
> 
> Alan, what do you think we should do? Is there anything else you think
> I should test for the next bit of time?

I dug out my mac and built this and it still flickers with animated
gifs. It's pretty easy to make happen, so it must be some hardware
performance thing.

Anyway, I've tried simplifying the double buffering code and put in
all the wee changes I've thought about. Who knows if this will work
any better...

(It may be worth making the single/double buffering a run-time option
as theoretically the single buffering will perform better, although
always at the increased risk of tearing effects etc.)
-- 
Alan Third

[-- Attachment #2: 0001-Simplify-the-EmacsLayer-double-buffering-code-bug-63.patch --]
[-- Type: text/x-diff, Size: 12556 bytes --]

From 4a486d74acb5f8f17266d65629aa7f884efb2834 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Sun, 23 Jul 2023 12:00:30 +0100
Subject: [PATCH] Simplify the EmacsLayer double buffering code (bug#63187)

* src/nsterm.h (EmacsLayer): Remove cache and replace with two
IOSurface variables.
* src/nsterm.m (ns_scroll_run): Remove redundant code.
([EmacsView copyRect:to:]): Ensure the context is flushed before we
start messign directly with the pixel buffer.
([EmacsLayer initWithColorSpace:]):
([EmacsLayer dealloc]):
([EmacsLayer releaseSurfaces]):
([EmacsLayer checkDimensions]):
([EmacsLayer getContext]):
([EmacsLayer releaseContext]):
([EmacsLayer display]):
([EmacsLayer copyContentsTo:]): Remove cache and replace with two
IOSurface variables.
---
 src/nsterm.h |   3 +-
 src/nsterm.m | 147 +++++++++++++++++++--------------------------------
 2 files changed, 56 insertions(+), 94 deletions(-)

diff --git a/src/nsterm.h b/src/nsterm.h
index b6e5a813a6d..22dbf2d8f27 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -742,9 +742,8 @@ #define NSTRACE_UNSILENCE()
 #if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
 @interface EmacsLayer : CALayer
 {
-  NSMutableArray *cache;
   CGColorSpaceRef colorSpace;
-  IOSurfaceRef currentSurface;
+  IOSurfaceRef frontSurface, backSurface;
   CGContextRef context;
 }
 - (id) initWithColorSpace: (CGColorSpaceRef)cs;
diff --git a/src/nsterm.m b/src/nsterm.m
index 78089906752..c23238b7dec 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2704,11 +2704,10 @@ Hide the window (X11 semantics)
   {
     NSRect srcRect = NSMakeRect (x, from_y, width, height);
     NSPoint dest = NSMakePoint (x, to_y);
-    NSRect destRect = NSMakeRect (x, from_y, width, height);
     EmacsView *view = FRAME_NS_VIEW (f);
 
     [view copyRect:srcRect to:dest];
-#ifdef NS_IMPL_COCOA
+#if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MAX_ALLOWED < 101400
     [view setNeedsDisplayInRect:destRect];
 #endif
   }
@@ -8664,8 +8663,10 @@ - (void)copyRect:(NSRect)srcRect to:(NSPoint)dest
                                NSHeight (srcRect));
 
 #if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
-  double scale = [[self window] backingScaleFactor];
   CGContextRef context = [(EmacsLayer *)[self layer] getContext];
+  CGContextFlush (context);
+
+  double scale = [[self window] backingScaleFactor];
   int bpp = CGBitmapContextGetBitsPerPixel (context) / 8;
   void *pixels = CGBitmapContextGetData (context);
   int rowSize = CGBitmapContextGetBytesPerRow (context);
@@ -10421,21 +10422,16 @@ @implementation EmacsLayer
    of the output.  To avoid this problem we can check if the surface
    is "in use", and if it is then avoid using it.  Unfortunately to
    avoid writing to a surface that's in use, but still maintain the
-   ability to draw to the screen at any time, we need to keep a cache
-   of multiple surfaces that we can use at will.
+   ability to draw to the screen at any time, we need to mantain
+   multiple surfaces that we can use at will.
 
-   The EmacsLayer class maintains this cache of surfaces, and
-   handles the conversion to a CGGraphicsContext that AppKit can use
-   to draw on.
+   The EmacsLayer class maintains these surfaces, and handles the
+   conversion to a CGGraphicsContext that AppKit can use to draw on.
 
-   The cache is simple: if a free surface is found it is removed from
-   the cache and set as the "current" surface.  Emacs draws to the
-   surface and when the layer wants to update the screen we set it's
-   contents to the surface and then add it back on to the end of the
-   cache.  If no free surfaces are found in the cache then a new one
-   is created.  */
+   We simply have a "back" surface, which we can draw to, and a
+   "front" buffer that is currently being displayed on the screen.  */
 
-#define CACHE_MAX_SIZE 2
+#define EMACSLAYER_DOUBLE_BUFFERED 1
 
 - (id) initWithColorSpace: (CGColorSpaceRef)cs
 {
@@ -10443,14 +10439,9 @@ - (id) initWithColorSpace: (CGColorSpaceRef)cs
 
   self = [super init];
   if (self)
-    {
-      cache = [[NSMutableArray arrayWithCapacity:CACHE_MAX_SIZE] retain];
-      [self setColorSpace:cs];
-    }
+    [self setColorSpace:cs];
   else
-    {
-      return nil;
-    }
+    return nil;
 
   return self;
 }
@@ -10470,8 +10461,6 @@ - (void) setColorSpace: (CGColorSpaceRef)cs
 - (void) dealloc
 {
   [self releaseSurfaces];
-  [cache release];
-
   [super dealloc];
 }
 
@@ -10481,18 +10470,16 @@ - (void) releaseSurfaces
   [self setContents:nil];
   [self releaseContext];
 
-  if (currentSurface)
+  if (frontSurface)
     {
-      CFRelease (currentSurface);
-      currentSurface = nil;
+      CFRelease (frontSurface);
+      frontSurface = nil;
     }
 
-  if (cache)
+  if (backSurface)
     {
-      for (id object in cache)
-        CFRelease ((IOSurfaceRef)object);
-
-      [cache removeAllObjects];
+      CFRelease (backSurface);
+      backSurface = nil;
     }
 }
 
@@ -10503,8 +10490,7 @@ - (BOOL) checkDimensions
 {
   int width = NSWidth ([self bounds]) * [self contentsScale];
   int height = NSHeight ([self bounds]) * [self contentsScale];
-  IOSurfaceRef s = currentSurface ? currentSurface
-    : (IOSurfaceRef)[cache firstObject];
+  IOSurfaceRef s = backSurface ? backSurface : frontSurface;
 
   return !s || (IOSurfaceGetWidth (s) == width
                 && IOSurfaceGetHeight (s) == height);
@@ -10517,41 +10503,21 @@ - (CGContextRef) getContext
   CGFloat scale = [self contentsScale];
 
   NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer getContext]");
-  NSTRACE_MSG ("IOSurface count: %lu", [cache count] + (currentSurface ? 1 : 0));
 
   if (![self checkDimensions])
     [self releaseSurfaces];
 
   if (!context)
     {
-      IOSurfaceRef surface = NULL;
       int width = NSWidth ([self bounds]) * scale;
       int height = NSHeight ([self bounds]) * scale;
 
-      for (id object in cache)
-        {
-          if (!IOSurfaceIsInUse ((IOSurfaceRef)object))
-            {
-              surface = (IOSurfaceRef)object;
-              [cache removeObject:object];
-              break;
-            }
-        }
-
-      if (!surface && [cache count] >= CACHE_MAX_SIZE)
-        {
-          /* Just grab the first one off the cache.  This may result
-             in tearing effects.  The alternative is to wait for one
-             of the surfaces to become free.  */
-          surface = (IOSurfaceRef)[cache firstObject];
-          [cache removeObject:(id)surface];
-        }
-      else if (!surface)
+      if (!backSurface)
         {
           int bytesPerRow = IOSurfaceAlignProperty (kIOSurfaceBytesPerRow,
                                                     width * 4);
 
-          surface = IOSurfaceCreate
+          backSurface = IOSurfaceCreate
             ((CFDictionaryRef)@{(id)kIOSurfaceWidth:[NSNumber numberWithInt:width],
                 (id)kIOSurfaceHeight:[NSNumber numberWithInt:height],
                 (id)kIOSurfaceBytesPerRow:[NSNumber numberWithInt:bytesPerRow],
@@ -10559,25 +10525,23 @@ - (CGContextRef) getContext
                 (id)kIOSurfacePixelFormat:[NSNumber numberWithUnsignedInt:'BGRA']});
         }
 
-      if (!surface)
+      if (!backSurface)
         {
           NSLog (@"Failed to create IOSurface for frame %@", [self delegate]);
           return nil;
         }
 
-      IOReturn lockStatus = IOSurfaceLock (surface, 0, nil);
+      IOReturn lockStatus = IOSurfaceLock (backSurface, 0, nil);
       if (lockStatus != kIOReturnSuccess)
         NSLog (@"Failed to lock surface: %x", lockStatus);
 
-      [self copyContentsTo:surface];
+      [self copyContentsTo:backSurface];
 
-      currentSurface = surface;
-
-      context = CGBitmapContextCreate (IOSurfaceGetBaseAddress (currentSurface),
-                                       IOSurfaceGetWidth (currentSurface),
-                                       IOSurfaceGetHeight (currentSurface),
+      context = CGBitmapContextCreate (IOSurfaceGetBaseAddress (backSurface),
+                                       IOSurfaceGetWidth (backSurface),
+                                       IOSurfaceGetHeight (backSurface),
                                        8,
-                                       IOSurfaceGetBytesPerRow (currentSurface),
+                                       IOSurfaceGetBytesPerRow (backSurface),
                                        colorSpace,
                                        (kCGImageAlphaPremultipliedFirst
                                         | kCGBitmapByteOrder32Host));
@@ -10585,13 +10549,13 @@ - (CGContextRef) getContext
       if (!context)
         {
           NSLog (@"Failed to create context for frame %@", [self delegate]);
-          IOSurfaceUnlock (currentSurface, 0, nil);
-          CFRelease (currentSurface);
-          currentSurface = nil;
+          IOSurfaceUnlock (backSurface, 0, nil);
+          CFRelease (backSurface);
+          backSurface = nil;
           return nil;
         }
 
-      CGContextTranslateCTM(context, 0, IOSurfaceGetHeight (currentSurface));
+      CGContextTranslateCTM(context, 0, IOSurfaceGetHeight (backSurface));
       CGContextScaleCTM(context, scale, -scale);
     }
 
@@ -10608,10 +10572,11 @@ - (void) releaseContext
   if (!context)
     return;
 
+  CGContextFlush (context);
   CGContextRelease (context);
   context = NULL;
 
-  IOReturn lockStatus = IOSurfaceUnlock (currentSurface, 0, nil);
+  IOReturn lockStatus = IOSurfaceUnlock (backSurface, 0, nil);
   if (lockStatus != kIOReturnSuccess)
     NSLog (@"Failed to unlock surface: %x", lockStatus);
 }
@@ -10621,63 +10586,61 @@ - (void) display
 {
   NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer display]");
 
-  if (context)
+  if (context && context != [[NSGraphicsContext currentContext] CGContext])
     {
       [self releaseContext];
 
-#if CACHE_MAX_SIZE == 1
+#ifdef EMACSLAYER_DOUBLE_BUFFERED
+      IOSurfaceRef tmp = frontSurface;
+      frontSurface = backSurface;
+      backSurface = tmp;
+
+      [self setContents:(id)frontSurface];
+#else
       /* This forces the layer to see the surface as updated.  */
       [self setContents:nil];
-#endif
-
-      [self setContents:(id)currentSurface];
 
-      /* Put currentSurface back on the end of the cache.  */
-      [cache addObject:(id)currentSurface];
-      currentSurface = NULL;
+      /* Since we're not doible buffering, just use the back
+         surface. */
+      [self setContents:(id)backSurface];
 
-      /* Schedule a run of getContext so that if Emacs is idle it will
-         perform the buffer copy, etc.  */
-      [self performSelectorOnMainThread:@selector (getContext)
-                             withObject:nil
-                          waitUntilDone:NO];
+#endif
     }
 }
 
 
-/* Copy the contents of lastSurface to DESTINATION.  This is required
+/* Copy the contents of frontSurface to DESTINATION.  This is required
    every time we want to use an IOSurface as its contents are probably
    blanks (if it's new), or stale.  */
 - (void) copyContentsTo: (IOSurfaceRef) destination
 {
   IOReturn lockStatus;
-  IOSurfaceRef source = (IOSurfaceRef)[self contents];
-  void *sourceData, *destinationData;
+  void *frontSurfaceData, *destinationData;
   int numBytes = IOSurfaceGetAllocSize (destination);
 
   NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer copyContentsTo:]");
 
-  if (!source || source == destination)
+  if (!frontSurface || frontSurface == destination)
     return;
 
-  lockStatus = IOSurfaceLock (source, kIOSurfaceLockReadOnly, nil);
+  lockStatus = IOSurfaceLock (frontSurface, kIOSurfaceLockReadOnly, nil);
   if (lockStatus != kIOReturnSuccess)
     NSLog (@"Failed to lock source surface: %x", lockStatus);
 
-  sourceData = IOSurfaceGetBaseAddress (source);
+  frontSurfaceData = IOSurfaceGetBaseAddress (frontSurface);
   destinationData = IOSurfaceGetBaseAddress (destination);
 
   /* Since every IOSurface should have the exact same settings, a
      memcpy seems like the fastest way to copy the data from one to
      the other.  */
-  memcpy (destinationData, sourceData, numBytes);
+  memcpy (destinationData, frontSurfaceData, numBytes);
 
-  lockStatus = IOSurfaceUnlock (source, kIOSurfaceLockReadOnly, nil);
+  lockStatus = IOSurfaceUnlock (frontSurface, kIOSurfaceLockReadOnly, nil);
   if (lockStatus != kIOReturnSuccess)
     NSLog (@"Failed to unlock source surface: %x", lockStatus);
 }
 
-#undef CACHE_MAX_SIZE
+#undef EMACSLAYER_DOUBLE_BUFFERED
 
 @end /* EmacsLayer */
 
-- 
2.40.1