From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Alan Third Newsgroups: gmane.emacs.bugs Subject: bug#63187: 30.0.50; Tail of longer lines painted after end of nearby lines on macOS Date: Sun, 23 Jul 2023 12:20:21 +0100 Message-ID: References: <047EC69F-EAC7-458F-A288-13DDC77DE187@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="Ih3RP+YmDNTlFk/h" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="14074"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Po Lu , Kai Ma , Eli Zaretskii , 63187@debbugs.gnu.org To: Aaron Jensen Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Jul 23 13:21:17 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qNX9I-0003Th-GO for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 23 Jul 2023 13:21:16 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qNX96-0006PE-8o; Sun, 23 Jul 2023 07:21:04 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qNX93-0006Oo-VZ for bug-gnu-emacs@gnu.org; Sun, 23 Jul 2023 07:21:01 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qNX93-0003P3-Nn for bug-gnu-emacs@gnu.org; Sun, 23 Jul 2023 07:21:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qNX93-0002rZ-KF for bug-gnu-emacs@gnu.org; Sun, 23 Jul 2023 07:21:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Alan Third Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 23 Jul 2023 11:21:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 63187 X-GNU-PR-Package: emacs Original-Received: via spool by 63187-submit@debbugs.gnu.org id=B63187.169011123310962 (code B ref 63187); Sun, 23 Jul 2023 11:21:01 +0000 Original-Received: (at 63187) by debbugs.gnu.org; 23 Jul 2023 11:20:33 +0000 Original-Received: from localhost ([127.0.0.1]:38737 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qNX8a-0002qj-Bl for submit@debbugs.gnu.org; Sun, 23 Jul 2023 07:20:33 -0400 Original-Received: from dane.soverin.net ([185.233.34.30]:45993) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qNX8X-0002qN-5X for 63187@debbugs.gnu.org; Sun, 23 Jul 2023 07:20:31 -0400 Original-Received: from smtp.soverin.net (c04smtp-lb01.int.sover.in [10.10.4.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dane.soverin.net (Postfix) with ESMTPS id 4R816v44Lvz2yC1; Sun, 23 Jul 2023 11:20:23 +0000 (UTC) Original-Received: from smtp.soverin.net (smtp.soverin.net [10.10.4.99]) by soverin.net (Postfix) with ESMTPSA id 4R816t5pClzFf; Sun, 23 Jul 2023 11:20:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=idiocy.org; s=soverin; t=1690111223; bh=bZ+TpIKsdBwpr3WM6fVqy0oI0mnwbwzg4khjYHM9vt4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=kJqJCCQDjpjKkjcqvmHDYjgKuReXq3f+AAGV2PM6Ap4pyBDA6AHqFBcnBqZ6llIuo nnGEpXKOcu3RN3j30lKwzE4xiBqWRxIJZngpLcqJpNyR6MsGBNQnQLCjwphUu4+cpH yE0mTau9gD8zHtIKBjkA9+qlbOaXDdw/DsmSyAa4AHfX+0Klv50qfvnI+njRRRWSG8 tW3jNgZazpfPePOoAruX4x7kjY28us5qvu+bpDySELOQVmhUXEfaGl0+v74GK/l1Lu a+bsPS8u9oGj/H2jWShQJ/4eSaGToR/6QitWK+EiL3XlcH1EEwvbROLPM+EqNK73EK RSMJH54H95K3g== Original-Received: from alan by faroe.holly.idiocy.org with local (Exim 4.96) (envelope-from ) id 1qNX8P-000GVb-1o; Sun, 23 Jul 2023 12:20:21 +0100 X-Soverin-Authenticated: true Mail-Followup-To: Alan Third , Aaron Jensen , Kai Ma , 63187@debbugs.gnu.org, Eli Zaretskii , Po Lu Content-Disposition: inline In-Reply-To: X-CMAE-Score: 0 X-CMAE-Analysis: v=2.4 cv=LJ6gAra9 c=1 sm=1 tr=0 ts=64bd0cf7 a=ws7JD89P4LkA:10 a=L6B7qcsLAri9DC4yUXYA:9 a=CjuIK1q_8ugA:10 a=hIj89exaAAAA:8 a=rIQEmj2j8KJhexac9BUA:9 a=lS9wXHQM5UdnNJ4u63Ry:22 X-Cloudmark-Reporter: YLCCbXqKVpYmfE05kdJ/qIAUBLs= X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:265890 Archived-At: --Ih3RP+YmDNTlFk/h Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 --Ih3RP+YmDNTlFk/h Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-Simplify-the-EmacsLayer-double-buffering-code-bug-63.patch" >From 4a486d74acb5f8f17266d65629aa7f884efb2834 Mon Sep 17 00:00:00 2001 From: Alan Third 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 --Ih3RP+YmDNTlFk/h--