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

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sun, 30 Apr 2023 06:10:22 -0400
> 
> I don't have a good recipe for this. It has been happening occasionally
> for quite some time (the attached screenshots are from 2021, but the
> same thing is still occurring). Every once in a while, a part of the
> Emacs frame gets into a state where glyphs from nearby lines can be
> painted after the tail end of other lines. This seems to be related to
> scrolling (that is, while scrolling, the glyphs replicate onto nearby
> lines, but when scrolling back they stay. I usually have to force a full
> repaint to get the glyphs to disappear.

Thanks, but this is impossible to handle without some kind of recipe,
even if it only reproduces sometimes.

FWIW, it never happens for me, so maybe it's macOS-specific.

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

[Aaron Jensen]

On Sun, Apr 30, 2023 at 6:32 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Aaron Jensen <aaronjensen@gmail.com>
> > Date: Sun, 30 Apr 2023 06:10:22 -0400
> >
> > I don't have a good recipe for this. It has been happening occasionally
> > for quite some time (the attached screenshots are from 2021, but the
> > same thing is still occurring). Every once in a while, a part of the
> > Emacs frame gets into a state where glyphs from nearby lines can be
> > painted after the tail end of other lines. This seems to be related to
> > scrolling (that is, while scrolling, the glyphs replicate onto nearby
> > lines, but when scrolling back they stay. I usually have to force a full
> > repaint to get the glyphs to disappear.
>
> Thanks, but this is impossible to handle without some kind of recipe,
> even if it only reproduces sometimes.
>
> FWIW, it never happens for me, so maybe it's macOS-specific.

I'm guessing it is macOS specific. I'm not familiar enough w/ how
rendering works to even start to look at this. I do understand that
it's impossible/difficult to handle without a recipe. The only hope at
the moment is that someone is familiar enough w/ the macOS rendering
that they could spot a potential for something like this to happen.
I'll keep seeing if I can figure out what causes it.

Thanks,

Aaron

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Aaron Jensen <aaronjensen@gmail.com> writes:

> I don't have a good recipe for this. It has been happening occasionally
> for quite some time (the attached screenshots are from 2021, but the
> same thing is still occurring). Every once in a while, a part of the
> Emacs frame gets into a state where glyphs from nearby lines can be
> painted after the tail end of other lines. This seems to be related to
> scrolling (that is, while scrolling, the glyphs replicate onto nearby
> lines, but when scrolling back they stay. I usually have to force a full
> repaint to get the glyphs to disappear.
>
>
>
>
>
>
> In GNU Emacs 30.0.50 (build 1, aarch64-apple-darwin22.4.0, NS
>  appkit-2299.50 Version 13.3.1 (Build 22E261)) of 2023-04-09 built on
>  Aarons-Laptop.local
> Windowing system distributor 'Apple', version 10.3.2299
> System Description:  macOS 13.3.1
>
> Configured using:
>  'configure --disable-dependency-tracking --disable-silent-rules
>  --enable-locallisppath=/opt/homebrew/share/emacs/site-lisp
>  --infodir=/opt/homebrew/Cellar/emacs-plus@30/30.0.50/share/info/emacs
>  --prefix=/opt/homebrew/Cellar/emacs-plus@30/30.0.50 --with-xml2
>  --with-gnutls --with-native-compilation --without-compress-install
>  --without-dbus --without-imagemagick --with-modules --with-rsvg
>  --with-ns --disable-ns-self-contained 'CFLAGS=-Os -w -pipe
>  -mmacosx-version-min=13
>  -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk
>  -DFD_SETSIZE=10000 -DDARWIN_UNLIMITED_SELECT'
>  'CPPFLAGS=-I/opt/homebrew/opt/zlib/include
>  -I/opt/homebrew/opt/jpeg/include -I/opt/homebrew/opt/icu4c/include
>  -I/opt/homebrew/opt/openssl@1.1/include -isystem/opt/homebrew/include
>  -F/opt/homebrew/Frameworks
>  -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk'
>  'LDFLAGS=-L/opt/homebrew/opt/zlib/lib -L/opt/homebrew/opt/jpeg/lib
>  -L/opt/homebrew/opt/icu4c/lib -L/opt/homebrew/opt/openssl@1.1/lib
>  -L/opt/homebrew/lib -F/opt/homebrew/Frameworks
>  -Wl,-headerpad_max_install_names
>  -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk''
>
> Configured features:
> ACL GIF GLIB GMP GNUTLS JPEG JSON LCMS2 LIBXML2 MODULES NATIVE_COMP
> NOTIFY KQUEUE NS PDUMPER PNG RSVG SQLITE3 THREADS TIFF
> TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM ZLIB
>
> Important settings:
>   value of $LANG: en_US.UTF-8
>   locale-coding-system: utf-8-unix
>
> Major mode: ELisp/d
>
> Minor modes in effect:
>   global-evil-mc-mode: t
>   evil-mc-mode: t
>   treemacs-filewatch-mode: t
>   treemacs-follow-mode: t
>   treemacs-git-mode: t
>   treemacs-fringe-indicator-mode: t
>   global-git-commit-mode: t
>   transient-posframe-mode: t
>   windmove-mode: t
>   global-flycheck-mode: t
>   flycheck-mode: t
>   gcmh-mode: t
>   undo-fu-session-global-mode: t
>   undo-fu-session-mode: t
>   ws-butler-global-mode: t
>   ws-butler-mode: t
>   ns-auto-titlebar-mode: t
>   global-anzu-mode: t
>   anzu-mode: t
>   corfu-prescient-mode: t
>   corfu-history-mode: t
>   corfu-mode: t
>   form-feed-mode: t
>   eval-sexp-fu-flash-mode: t
>   eros-mode: t
>   lispyville-mode: t
>   lispy-mode: t
>   elisp-def-mode: t
>   sotlisp-mode: t
>   speed-of-thought-mode: t
>   electric-pair-mode: t
>   envrc-mode: t
>   global-evil-surround-mode: t
>   evil-surround-mode: t
>   evil-matchit-mode: t
>   evil-vimish-fold-mode: t
>   vimish-fold-mode: t
>   dtrt-indent-mode: t
>   tabspaces-mode: t
>   save-place-mode: t
>   winner-mode: t
>   savehist-mode: t
>   delete-selection-mode: t
>   yas-global-mode: t
>   yas-minor-mode: t
>   vertico-prescient-mode: t
>   prescient-persist-mode: t
>   vertico-mouse-mode: t
>   vertico-mode: t
>   mini-frame-mode: t
>   better-jumper-mode: t
>   better-jumper-local-mode: t
>   xterm-mouse-mode: t
>   pixel-scroll-precision-mode: t
>   global-auto-revert-mode: t
>   which-key-posframe-mode: t
>   which-key-mode: t
>   org-roam-db-autosync-mode: t
>   shell-dirtrack-mode: t
>   recentf-mode: t
>   repeat-mode: t
>   +popup-mode: t
>   evil-mode: t
>   evil-local-mode: t
>   nano-modeline-mode: t
>   server-mode: t
>   leader-key-leader-override-mode: t
>   global-leader-key-leader-override-mode: t
>   elpaca-use-package-mode: t
>   override-global-mode: t
>   global-display-line-numbers-mode: t
>   display-line-numbers-mode: t
>   global-eldoc-mode: t
>   eldoc-mode: t
>   show-paren-mode: t
>   electric-indent-mode: t
>   mouse-wheel-mode: t
>   tab-bar-mode: t
>   file-name-shadow-mode: t
>   global-font-lock-mode: t
>   font-lock-mode: t
>   window-divider-mode: t
>   line-number-mode: t
>   auto-fill-function: yas--auto-fill
>   transient-mark-mode: t
>   auto-composition-mode: t
>   auto-encryption-mode: t
>   auto-compression-mode: t
>   abbrev-mode: t

Hmmm, this looks like a scrolling optimization bug.
I can't build the NS port right now, but if you insert:

  return false;

right after

  /* Can't scroll the display of w32 GUI frames when position of point
     is indicated by the system caret, because scrolling the display
     will then "copy" the pixels used by the caret.  */
#ifdef HAVE_NTGUI
  if (w32_use_visible_system_caret)
    return 0;
#endif

in `scrolling_window', in dispnew.c, does the problem go away?

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

[Aaron Jensen]

On Sun, Apr 30, 2023 at 9:25 AM Po Lu <luangruo@yahoo.com> wrote:
>
> Hmmm, this looks like a scrolling optimization bug.
> I can't build the NS port right now, but if you insert:
>
>   return false;
>
> right after
>
>   /* Can't scroll the display of w32 GUI frames when position of point
>      is indicated by the system caret, because scrolling the display
>      will then "copy" the pixels used by the caret.  */
> #ifdef HAVE_NTGUI
>   if (w32_use_visible_system_caret)
>     return 0;
> #endif
>
> in `scrolling_window', in dispnew.c, does the problem go away?

Thanks, I will try this out. Is there any chance that it is related to
the other bug I just reported, bug#63186? I see that Eli's fix
affected the display matrix. It seems unlikely given the fact that
that bug was a regression.

Is there anything specific to macOS that is involved in scrolling optimization?

Thanks,

Aaron

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

[Eli Zaretskii]

> Cc: 63187@debbugs.gnu.org
> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sun, 30 Apr 2023 10:25:59 -0400
> 
> On Sun, Apr 30, 2023 at 9:25 AM Po Lu <luangruo@yahoo.com> wrote:
> >
> > Hmmm, this looks like a scrolling optimization bug.
> > I can't build the NS port right now, but if you insert:
> >
> >   return false;
> >
> > right after
> >
> >   /* Can't scroll the display of w32 GUI frames when position of point
> >      is indicated by the system caret, because scrolling the display
> >      will then "copy" the pixels used by the caret.  */
> > #ifdef HAVE_NTGUI
> >   if (w32_use_visible_system_caret)
> >     return 0;
> > #endif
> >
> > in `scrolling_window', in dispnew.c, does the problem go away?
> 
> Thanks, I will try this out. Is there any chance that it is related to
> the other bug I just reported, bug#63186?

No, no chance.  That bug was system-independent, while this one is
specific to macOS.  Also, while investigating bug#63186, I've
established that disabling scrolling_window optimization doesn't fix
the problem with redisplaying the mode line after mode-line-format was
nil.

> Is there anything specific to macOS that is involved in scrolling optimization?

Not that I know of, and so if Po Lu's suggestion fixes the problem,
we'd need to understand how come scrolling_window fails on macOS.

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

[Aaron Jensen]

On Sun, Apr 30, 2023 at 10:42 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > Cc: 63187@debbugs.gnu.org
> > From: Aaron Jensen <aaronjensen@gmail.com>
> > Date: Sun, 30 Apr 2023 10:25:59 -0400
> >
> > On Sun, Apr 30, 2023 at 9:25 AM Po Lu <luangruo@yahoo.com> wrote:
> > >
> > > Hmmm, this looks like a scrolling optimization bug.
> > > I can't build the NS port right now, but if you insert:
> > >
> > >   return false;
> > >
> > > right after
> > >
> > >   /* Can't scroll the display of w32 GUI frames when position of point
> > >      is indicated by the system caret, because scrolling the display
> > >      will then "copy" the pixels used by the caret.  */
> > > #ifdef HAVE_NTGUI
> > >   if (w32_use_visible_system_caret)
> > >     return 0;
> > > #endif
> > >
> > > in `scrolling_window', in dispnew.c, does the problem go away?
> >
> > Thanks, I will try this out. Is there any chance that it is related to
> > the other bug I just reported, bug#63186?
>
> No, no chance.  That bug was system-independent, while this one is
> specific to macOS.  Also, while investigating bug#63186, I've
> established that disabling scrolling_window optimization doesn't fix
> the problem with redisplaying the mode line after mode-line-format was
> nil.

Copy that.

> > Is there anything specific to macOS that is involved in scrolling optimization?
>
> Not that I know of, and so if Po Lu's suggestion fixes the problem,
> we'd need to understand how come scrolling_window fails on macOS.

Ok, it may take me a few days to be sure. It happens intermittently,
though lately it has been at least once in most work days.

For what it's worth, disabling it has had no discernible impact on
scroll performance on my machine (which is an M1)

Aaron

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sun, 30 Apr 2023 10:57:35 -0400
> Cc: luangruo@yahoo.com, 63187@debbugs.gnu.org
> 
> For what it's worth, disabling it has had no discernible impact on
> scroll performance on my machine (which is an M1)

scrolling_window is not about scrolling.  It's a redisplay
optimization that attempts to speed up redrawing a window by scrolling
on display the stuff already shown, when that is deemed less costly
than redrawing every screen line that has changed.

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

[Aaron Jensen]

On Sun, Apr 30, 2023 at 11:26 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Aaron Jensen <aaronjensen@gmail.com>
> > Date: Sun, 30 Apr 2023 10:57:35 -0400
> > Cc: luangruo@yahoo.com, 63187@debbugs.gnu.org
> >
> > For what it's worth, disabling it has had no discernible impact on
> > scroll performance on my machine (which is an M1)
>
> scrolling_window is not about scrolling.  It's a redisplay
> optimization that attempts to speed up redrawing a window by scrolling
> on display the stuff already shown, when that is deemed less costly
> than redrawing every screen line that has changed.

Ah, so is it used (for example) when you insert a newline in part of a
buffer? How might I reproduce a usage of it both so I can benchmark
and play around with it when it is enabled to see if I can trigger the
bug I'm seeing?

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sun, 30 Apr 2023 12:48:05 -0400
> Cc: luangruo@yahoo.com, 63187@debbugs.gnu.org
> 
> On Sun, Apr 30, 2023 at 11:26 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > scrolling_window is not about scrolling.  It's a redisplay
> > optimization that attempts to speed up redrawing a window by scrolling
> > on display the stuff already shown, when that is deemed less costly
> > than redrawing every screen line that has changed.
> 
> Ah, so is it used (for example) when you insert a newline in part of a
> buffer?

Could be, yes.  But one can never be sure, because the display engine
has many different optimizations, so it could be that this particular
case is handled elsewhere, for example in try_window_id, which also
handles cases like this one.

> How might I reproduce a usage of it both so I can benchmark and play
> around with it when it is enabled to see if I can trigger the bug
> I'm seeing?

The best idea I have is to add a printf right before the mainline of
its algorithm, after this comment:

  /* Reallocate vectors, tables etc. if necessary.  */

and pay attention which edits cause that printf.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Aaron Jensen <aaronjensen@gmail.com> writes:

> Is there anything specific to macOS that is involved in scrolling optimization?

Yes, Apple deleted the API used to perform bit blits, so Emacs uses a
workaround that I don't really understand, and seems unreliable.

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

[Eli Zaretskii]

> Cc: 63187@debbugs.gnu.org
> Date: Mon, 01 May 2023 07:58:26 +0800
> From:  Po Lu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Aaron Jensen <aaronjensen@gmail.com> writes:
> 
> > Is there anything specific to macOS that is involved in scrolling optimization?
> 
> Yes, Apple deleted the API used to perform bit blits, so Emacs uses a
> workaround that I don't really understand, and seems unreliable.

You mean, ns_scroll_run in nsterm.m?  Which parts of it do you not
understand?

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 63187@debbugs.gnu.org
>> Date: Mon, 01 May 2023 07:58:26 +0800
>> From:  Po Lu via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> Aaron Jensen <aaronjensen@gmail.com> writes:
>> 
>> > Is there anything specific to macOS that is involved in scrolling optimization?
>> 
>> Yes, Apple deleted the API used to perform bit blits, so Emacs uses a
>> workaround that I don't really understand, and seems unreliable.
>
> You mean, ns_scroll_run in nsterm.m?  Which parts of it do you not
> understand?

No, I meant the implementation of [EmacsView copyRect:] enabled under
Mac OS; see line 8655 of nsterm.m.  I don't understand how the system
synchronizes its access to the window's backing store with Emacs's.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: aaronjensen@gmail.com,  63187@debbugs.gnu.org
> Date: Mon, 01 May 2023 21:18:31 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Cc: 63187@debbugs.gnu.org
> >> Date: Mon, 01 May 2023 07:58:26 +0800
> >> From:  Po Lu via "Bug reports for GNU Emacs,
> >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >> 
> >> Aaron Jensen <aaronjensen@gmail.com> writes:
> >> 
> >> > Is there anything specific to macOS that is involved in scrolling optimization?
> >> 
> >> Yes, Apple deleted the API used to perform bit blits, so Emacs uses a
> >> workaround that I don't really understand, and seems unreliable.
> >
> > You mean, ns_scroll_run in nsterm.m?  Which parts of it do you not
> > understand?
> 
> No, I meant the implementation of [EmacsView copyRect:] enabled under
> Mac OS; see line 8655 of nsterm.m.  I don't understand how the system
> synchronizes its access to the window's backing store with Emacs's.

Alan, can you help?

If this is unworkable on macOS, we could simply disable this
optimization there.  But note that scroll_run_hook is also called from
xdisp.c, in several places, so we may need to disable it there as
well.

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

[Aaron Jensen]

On Mon, May 1, 2023 at 9:24 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Po Lu <luangruo@yahoo.com>
> > Cc: aaronjensen@gmail.com,  63187@debbugs.gnu.org
> > Date: Mon, 01 May 2023 21:18:31 +0800
> >
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> > >> Cc: 63187@debbugs.gnu.org
> > >> Date: Mon, 01 May 2023 07:58:26 +0800
> > >> From:  Po Lu via "Bug reports for GNU Emacs,
> > >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > >>
> > >> Aaron Jensen <aaronjensen@gmail.com> writes:
> > >>
> > >> > Is there anything specific to macOS that is involved in scrolling optimization?
> > >>
> > >> Yes, Apple deleted the API used to perform bit blits, so Emacs uses a
> > >> workaround that I don't really understand, and seems unreliable.
> > >
> > > You mean, ns_scroll_run in nsterm.m?  Which parts of it do you not
> > > understand?
> >
> > No, I meant the implementation of [EmacsView copyRect:] enabled under
> > Mac OS; see line 8655 of nsterm.m.  I don't understand how the system
> > synchronizes its access to the window's backing store with Emacs's.
>
> Alan, can you help?
>
> If this is unworkable on macOS, we could simply disable this
> optimization there.  But note that scroll_run_hook is also called from
> xdisp.c, in several places, so we may need to disable it there as
> well.

Is this suspect?

  {
    NSRect srcRect = NSMakeRect (x, from_y, width, height);
    NSPoint dest = NSMakePoint (x, to_y);
    EmacsView *view = FRAME_NS_VIEW (f);

    [view copyRect:srcRect to:dest];
#ifdef NS_IMPL_COCOA
    [view setNeedsDisplayInRect:srcRect];
#endif
  }

Why is the source being marked for redisplay? I would expect the
destination to be marked as such, or am I missing something?

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Mon, 1 May 2023 09:47:50 -0400
> Cc: Po Lu <luangruo@yahoo.com>, Alan Third <alan@idiocy.org>, 63187@debbugs.gnu.org
> 
>   {
>     NSRect srcRect = NSMakeRect (x, from_y, width, height);
>     NSPoint dest = NSMakePoint (x, to_y);
>     EmacsView *view = FRAME_NS_VIEW (f);
> 
>     [view copyRect:srcRect to:dest];
> #ifdef NS_IMPL_COCOA
>     [view setNeedsDisplayInRect:srcRect];
> #endif
>   }
> 
> Why is the source being marked for redisplay? I would expect the
> destination to be marked as such, or am I missing something?

IMO, both should be marked for redisplay.  Or maybe I don't understand
the significance of marking a rectangle for redisplay.

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

[Aaron Jensen]

On Mon, May 1, 2023 at 9:51 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Aaron Jensen <aaronjensen@gmail.com>
> > Date: Mon, 1 May 2023 09:47:50 -0400
> > Cc: Po Lu <luangruo@yahoo.com>, Alan Third <alan@idiocy.org>, 63187@debbugs.gnu.org
> >
> >   {
> >     NSRect srcRect = NSMakeRect (x, from_y, width, height);
> >     NSPoint dest = NSMakePoint (x, to_y);
> >     EmacsView *view = FRAME_NS_VIEW (f);
> >
> >     [view copyRect:srcRect to:dest];
> > #ifdef NS_IMPL_COCOA
> >     [view setNeedsDisplayInRect:srcRect];
> > #endif
> >   }
> >
> > Why is the source being marked for redisplay? I would expect the
> > destination to be marked as such, or am I missing something?
>
> IMO, both should be marked for redisplay.  Or maybe I don't understand
> the significance of marking a rectangle for redisplay.

For reference, this is in `ns_scroll_run'. Only the destination is
changed, right? I imagine the source would be effectively marked when
it's copied into, so marking the source in this code would be
redundant, unless I'm missing something?

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

[Aaron Jensen]

On Mon, May 1, 2023 at 9:55 AM Aaron Jensen <aaronjensen@gmail.com> wrote:
>
> On Mon, May 1, 2023 at 9:51 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > > From: Aaron Jensen <aaronjensen@gmail.com>
> > > Date: Mon, 1 May 2023 09:47:50 -0400
> > > Cc: Po Lu <luangruo@yahoo.com>, Alan Third <alan@idiocy.org>, 63187@debbugs.gnu.org
> > >
> > >   {
> > >     NSRect srcRect = NSMakeRect (x, from_y, width, height);
> > >     NSPoint dest = NSMakePoint (x, to_y);
> > >     EmacsView *view = FRAME_NS_VIEW (f);
> > >
> > >     [view copyRect:srcRect to:dest];
> > > #ifdef NS_IMPL_COCOA
> > >     [view setNeedsDisplayInRect:srcRect];
> > > #endif
> > >   }
> > >
> > > Why is the source being marked for redisplay? I would expect the
> > > destination to be marked as such, or am I missing something?
> >
> > IMO, both should be marked for redisplay.  Or maybe I don't understand
> > the significance of marking a rectangle for redisplay.
>
> For reference, this is in `ns_scroll_run'. Only the destination is
> changed, right? I imagine the source would be effectively marked when
> it's copied into, so marking the source in this code would be
> redundant, unless I'm missing something?

Also, the previous version of `ns_flush_display' was this:

static void
ns_flush_display (struct frame *f)
/* Force the frame to redisplay.  If areas have previously been marked
   dirty by setNeedsDisplayInRect (in ns_clip_to_rect), then this will call
   draw_rect: which will "expose" those areas.  */
{
  block_input ();
  [FRAME_NS_VIEW (f) displayIfNeeded];
  unblock_input ();
}

The displayIfNeeded has been removed and is now ultimately replaced
with stuff I don't understand. After looking at this code, I'm now
wondering if my problem has to do with the fact that I use child
frames for things like completions. I have recollection of the
painting artifacts appearing under areas that child frames are
temporarily covering. Is it possible that somehow those areas aren't
being properly repainted/marked for repainting?

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

[Aaron Jensen]

On Mon, May 1, 2023 at 9:47 AM Aaron Jensen <aaronjensen@gmail.com> wrote:
>
> On Mon, May 1, 2023 at 9:24 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > > From: Po Lu <luangruo@yahoo.com>
> > > Cc: aaronjensen@gmail.com,  63187@debbugs.gnu.org
> > > Date: Mon, 01 May 2023 21:18:31 +0800
> > >
> > > Eli Zaretskii <eliz@gnu.org> writes:
> > >
> > > >> Cc: 63187@debbugs.gnu.org
> > > >> Date: Mon, 01 May 2023 07:58:26 +0800
> > > >> From:  Po Lu via "Bug reports for GNU Emacs,
> > > >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > > >>
> > > >> Aaron Jensen <aaronjensen@gmail.com> writes:
> > > >>
> > > >> > Is there anything specific to macOS that is involved in scrolling optimization?
> > > >>
> > > >> Yes, Apple deleted the API used to perform bit blits, so Emacs uses a
> > > >> workaround that I don't really understand, and seems unreliable.
> > > >
> > > > You mean, ns_scroll_run in nsterm.m?  Which parts of it do you not
> > > > understand?
> > >
> > > No, I meant the implementation of [EmacsView copyRect:] enabled under
> > > Mac OS; see line 8655 of nsterm.m.  I don't understand how the system
> > > synchronizes its access to the window's backing store with Emacs's.
> >
> > Alan, can you help?
> >
> > If this is unworkable on macOS, we could simply disable this
> > optimization there.  But note that scroll_run_hook is also called from
> > xdisp.c, in several places, so we may need to disable it there as
> > well.
>
> Is this suspect?
>
>   {
>     NSRect srcRect = NSMakeRect (x, from_y, width, height);
>     NSPoint dest = NSMakePoint (x, to_y);
>     EmacsView *view = FRAME_NS_VIEW (f);
>
>     [view copyRect:srcRect to:dest];
> #ifdef NS_IMPL_COCOA
>     [view setNeedsDisplayInRect:srcRect];
> #endif
>   }
>
> Why is the source being marked for redisplay? I would expect the
> destination to be marked as such, or am I missing something?

It's not definitive, because it's always been intermittent, but I have
not seen the issue once since making this change a week ago. I'll give
it another week or two, but it certainly hasn't caused any problems
and may be worth considering for 29 before it is released.

diff --git a/src/nsterm.m b/src/nsterm.m
index ecbf80ff72d..310d2bd81c7 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2704,11 +2704,12 @@ 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
-    [view setNeedsDisplayInRect:srcRect];
+    [view setNeedsDisplayInRect:destRect];
 #endif
   }

Aaron

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Mon, 8 May 2023 23:07:00 -0400
> Cc: Po Lu <luangruo@yahoo.com>, Alan Third <alan@idiocy.org>, 63187@debbugs.gnu.org
> 
> It's not definitive, because it's always been intermittent, but I have
> not seen the issue once since making this change a week ago. I'll give
> it another week or two, but it certainly hasn't caused any problems
> and may be worth considering for 29 before it is released.

I'm just waiting for you guys to decide that it was tested long
enough, that's all.  I guess you are not yet there, right?

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

[Eli Zaretskii]

Ping!  Should the patch be installed now on the emacs-29 branch?

> Cc: luangruo@yahoo.com, alan@idiocy.org, 63187@debbugs.gnu.org
> Date: Tue, 09 May 2023 08:39:02 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Aaron Jensen <aaronjensen@gmail.com>
> > Date: Mon, 8 May 2023 23:07:00 -0400
> > Cc: Po Lu <luangruo@yahoo.com>, Alan Third <alan@idiocy.org>, 63187@debbugs.gnu.org
> > 
> > It's not definitive, because it's always been intermittent, but I have
> > not seen the issue once since making this change a week ago. I'll give
> > it another week or two, but it certainly hasn't caused any problems
> > and may be worth considering for 29 before it is released.
> 
> I'm just waiting for you guys to decide that it was tested long
> enough, that's all.  I guess you are not yet there, right?
> 
> 
> 
> 

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

[Aaron Jensen]

On Sat, May 13, 2023 at 9:54 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> Ping!  Should the patch be installed now on the emacs-29 branch?

I think it's worth it. I may have seen 1 glitch (but it was different
-- it looked like an entire blank line was duplicated. Other than
that, it's been solid, which is quite a bit better than it had been
before.

Aaron

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 13 May 2023 10:23:28 -0400
> Cc: luangruo@yahoo.com, alan@idiocy.org, 63187@debbugs.gnu.org
> 
> On Sat, May 13, 2023 at 9:54 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > Ping!  Should the patch be installed now on the emacs-29 branch?
> 
> I think it's worth it. I may have seen 1 glitch (but it was different
> -- it looked like an entire blank line was duplicated. Other than
> that, it's been solid, which is quite a bit better than it had been
> before.

Now installed on the emacs-29 branch, and closing the bug.

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

[Aaron Jensen]

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

Thank you. I'll report back if I start seeing it again, but so far so good.


Aaron


On Thu, May 18, 2023 at 7:21 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 13 May 2023 10:23:28 -0400
> Cc: luangruo@yahoo.com, alan@idiocy.org, 63187@debbugs.gnu.org
>
> On Sat, May 13, 2023 at 9:54 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> Ping! Should the patch be installed now on the emacs-29 branch?
>
> I think it's worth it. I may have seen 1 glitch (but it was different
> -- it looked like an entire blank line was duplicated. Other than that,
> it's been solid, which is quite a bit better than it had been before.
>
> Now installed on the emacs-29 branch, and closing the bug.
>

[-- Attachment #2: Type: text/html, Size: 2239 bytes --]

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

[Kai Ma]

Hi everyone, I'm also bothered by this bug recently.  And it is
happening quite frequently for me.

Unfortunately, the proposed patch and putting
  [[NSGraphicsContext currentContext] flushGraphics];
in copyRect didn't work for me.

See the screencast at https://www.youtube.com/watch?v=uLYPqsUFK5A .

I find that this bug happens more frequently if
1) the frame's alpha < 1.0, and
2) the frame is in the full screen mode, and
3) emacs is built with native compilation enabled.

(Not doing one or two of these does not prevent the bug from happening,
but seems to reduce the likelihood.)

FWIW, some version numbers:
- emacs 30.0.50 master branch
- macOS 13.4 (22F66), Intel


Aaron Jensen <aaronjensen@gmail.com> writes:

> Thank you. I'll report back if I start seeing it again, but so far so good.
>
> Aaron
>
> On Thu, May 18, 2023 at 7:21 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>
>  From: Aaron Jensen <aaronjensen@gmail.com> 
>  Date: Sat, 13 May 2023 10:23:28 -0400 
>  Cc: luangruo@yahoo.com, alan@idiocy.org, 63187@debbugs.gnu.org 
>
>  On Sat, May 13, 2023 at 9:54 AM Eli Zaretskii <eliz@gnu.org> wrote: 
>
>  Ping! Should the patch be installed now on the emacs-29 branch? 
>
>  I think it's worth it. I may have seen 1 glitch (but it was different 
>  -- it looked like an entire blank line was duplicated. Other than that, it's been solid, which is quite a bit better than it had been
>  before. 
>
>  Now installed on the emacs-29 branch, and closing the bug.
>

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

[Kai Ma]

> On Jun 8, 2023, at 13:40, Kai Ma <justksqsf@gmail.com> wrote:
> 
> 
> Hi everyone, I'm also bothered by this bug recently.  And it is
> happening quite frequently for me.
> 
> Unfortunately, the proposed patch and putting
>  [[NSGraphicsContext currentContext] flushGraphics];
> in copyRect didn't work for me.
> 
> See the screencast at https://www.youtube.com/watch?v=uLYPqsUFK5A .

Upon further debugging, the following change seems to fix the problem for me.

--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -10433,7 +10433,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
 {


This probably implies that there is another bug somewhere else in the management of IOSurface caches.


> 
> I find that this bug happens more frequently if
> 1) the frame's alpha < 1.0, and
> 2) the frame is in the full screen mode, and
> 3) emacs is built with native compilation enabled.
> 
> (Not doing one or two of these does not prevent the bug from happening,
> but seems to reduce the likelihood.)
> 
> FWIW, some version numbers:
> - emacs 30.0.50 master branch
> - macOS 13.4 (22F66), Intel
> 
> 
> Aaron Jensen <aaronjensen@gmail.com> writes:
> 
>> Thank you. I'll report back if I start seeing it again, but so far so good.
>> 
>> Aaron
>> 
>> On Thu, May 18, 2023 at 7:21 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>> From: Aaron Jensen <aaronjensen@gmail.com> 
>> Date: Sat, 13 May 2023 10:23:28 -0400 
>> Cc: luangruo@yahoo.com, alan@idiocy.org, 63187@debbugs.gnu.org 
>> 
>> On Sat, May 13, 2023 at 9:54 AM Eli Zaretskii <eliz@gnu.org> wrote: 
>> 
>> Ping! Should the patch be installed now on the emacs-29 branch? 
>> 
>> I think it's worth it. I may have seen 1 glitch (but it was different 
>> -- it looked like an entire blank line was duplicated. Other than that, it's been solid, which is quite a bit better than it had been
>> before. 
>> 
>> Now installed on the emacs-29 branch, and closing the bug.
>> 

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

[Alan Third]

On Thu, Jun 08, 2023 at 03:33:41PM +0800, Kai Ma wrote:
> 
> 
> > On Jun 8, 2023, at 13:40, Kai Ma <justksqsf@gmail.com> wrote:
> > 
> > 
> > Hi everyone, I'm also bothered by this bug recently.  And it is
> > happening quite frequently for me.
> > 
> > Unfortunately, the proposed patch and putting
> >  [[NSGraphicsContext currentContext] flushGraphics];
> > in copyRect didn't work for me.
> > 
> > See the screencast at https://www.youtube.com/watch?v=uLYPqsUFK5A .
> 
> Upon further debugging, the following change seems to fix the problem for me.
> 
> --- a/src/nsterm.m
> +++ b/src/nsterm.m
> @@ -10433,7 +10433,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
>  {
> 
> 
> This probably implies that there is another bug somewhere else in
> the management of IOSurface caches.

I did wonder about this, but I can't see where the failure would be
happening.

I've got two things it could be worth trying to see if they make any
difference.

Change the CACHE_MAX_SIZE to something greater than 2, probably 4 is a
good number. That would rule out some sort of locking problem, as it
would reduce the chance a surface would be re-used before it's been
sent to the screen.

The downside of this is probably going to be an increase in "lag"
because it's possible to have more surfaces "in-flight".

The other option is to remove this call (nsterm.m:10636):

      /* 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];

I think this is harmless, but it's in there as I *assume*, with
absolutely no proof, that it will improve performance and it seems
possible (but I think unlikely) that it may affect the sequencing of
surface operations.
-- 
Alan Third

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

[Kai Ma]

> On Jun 8, 2023, at 20:51, Alan Third <alan@idiocy.org> wrote:
> 
> I've got two things it could be worth trying to see if they make any
> difference.
> Change the CACHE_MAX_SIZE to something greater than 2, probably 4 is a
> good number. That would rule out some sort of locking problem, as it
> would reduce the chance a surface would be re-used before it's been
> sent to the screen.
> 
> The downside of this is probably going to be an increase in "lag"
> because it's possible to have more surfaces "in-flight".
> 
> The other option is to remove this call (nsterm.m:10636):
> 
>   /* 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];
> 
> I think this is harmless, but it's in there as I *assume*, with
> absolutely no proof, that it will improve performance and it seems
> possible (but I think unlikely) that it may affect the sequencing of
> surface operations.

Tested locally.  I can confirm that removing performSelectorOnMainThread (with CACHE_MAX_SIZE = 2) fixes the problem for me.  I now observe zero glitches or tearings!

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

[Kai Ma]

> On Jun 8, 2023, at 21:42, Kai Ma <justksqsf@gmail.com> wrote:
> 
> 
>> On Jun 8, 2023, at 20:51, Alan Third <alan@idiocy.org> wrote:
>> 
>> I've got two things it could be worth trying to see if they make any
>> difference.
>> Change the CACHE_MAX_SIZE to something greater than 2, probably 4 is a
>> good number. That would rule out some sort of locking problem, as it
>> would reduce the chance a surface would be re-used before it's been
>> sent to the screen.
>> 
>> The downside of this is probably going to be an increase in "lag"
>> because it's possible to have more surfaces "in-flight".
>> 
>> The other option is to remove this call (nsterm.m:10636):
>> 
>>  /* 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];
>> 
>> I think this is harmless, but it's in there as I *assume*, with
>> absolutely no proof, that it will improve performance and it seems
>> possible (but I think unlikely) that it may affect the sequencing of
>> surface operations.
> 
> Tested locally.  I can confirm that removing performSelectorOnMainThread (with CACHE_MAX_SIZE = 2) fixes the problem for me.  I now observe zero glitches or tearings!
> 

Sorry, I concluded too fast. I can still see tearings, though very rarely. So this is still not a real fix.  :-(

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

[Alan Third]

On Thu, Jun 08, 2023 at 10:57:43PM +0800, Kai Ma wrote:
> 
> 
> > On Jun 8, 2023, at 21:42, Kai Ma <justksqsf@gmail.com> wrote:
> > 
> > 
> >> On Jun 8, 2023, at 20:51, Alan Third <alan@idiocy.org> wrote:
> >> 
> >> I've got two things it could be worth trying to see if they make any
> >> difference.
> >> Change the CACHE_MAX_SIZE to something greater than 2, probably 4 is a
> >> good number. That would rule out some sort of locking problem, as it
> >> would reduce the chance a surface would be re-used before it's been
> >> sent to the screen.
> >> 
> >> The downside of this is probably going to be an increase in "lag"
> >> because it's possible to have more surfaces "in-flight".
> >> 
> >> The other option is to remove this call (nsterm.m:10636):
> >> 
> >>  /* 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];
> >> 
> >> I think this is harmless, but it's in there as I *assume*, with
> >> absolutely no proof, that it will improve performance and it seems
> >> possible (but I think unlikely) that it may affect the sequencing of
> >> surface operations.
> > 
> > Tested locally. I can confirm that removing
> > performSelectorOnMainThread (with CACHE_MAX_SIZE = 2) fixes the
> > problem for me. I now observe zero glitches or tearings!
> > 
> 
> Sorry, I concluded too fast. I can still see tearings, though very
> rarely. So this is still not a real fix. :-(

Try increasing CACHE_MAX_SIZE too. Tearing would, I believe, be
because a surface is being reused while it's still being copied to the
VRAM, so you'll see a partially modified output.

If increasing CACHE_MAX_SIZE fixes it then there are a couple of
possible options we can look at.
-- 
Alan Third

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

[Kai Ma]

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



> On Jun 9, 2023, at 01:22, Alan Third <alan@idiocy.org> wrote:
> 
> On Thu, Jun 08, 2023 at 10:57:43PM +0800, Kai Ma wrote:
>> 
>> 
>>> On Jun 8, 2023, at 21:42, Kai Ma <justksqsf@gmail.com> wrote:
>>> 
>>> 
>>>> On Jun 8, 2023, at 20:51, Alan Third <alan@idiocy.org> wrote:
>>>> 
>>>> I've got two things it could be worth trying to see if they make any
>>>> difference.
>>>> Change the CACHE_MAX_SIZE to something greater than 2, probably 4 is a
>>>> good number. That would rule out some sort of locking problem, as it
>>>> would reduce the chance a surface would be re-used before it's been
>>>> sent to the screen.
>>>> 
>>>> The downside of this is probably going to be an increase in "lag"
>>>> because it's possible to have more surfaces "in-flight".
>>>> 
>>>> The other option is to remove this call (nsterm.m:10636):
>>>> 
>>>> /* 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];
>>>> 
>>>> I think this is harmless, but it's in there as I *assume*, with
>>>> absolutely no proof, that it will improve performance and it seems
>>>> possible (but I think unlikely) that it may affect the sequencing of
>>>> surface operations.
>>> 
>>> Tested locally. I can confirm that removing
>>> performSelectorOnMainThread (with CACHE_MAX_SIZE = 2) fixes the
>>> problem for me. I now observe zero glitches or tearings!
>>> 
>> 
>> Sorry, I concluded too fast. I can still see tearings, though very
>> rarely. So this is still not a real fix. :-(
> 
> Try increasing CACHE_MAX_SIZE too. Tearing would, I believe, be
> because a surface is being reused while it's still being copied to the
> VRAM, so you'll see a partially modified output.
> 
> If increasing CACHE_MAX_SIZE fixes it then there are a couple of
> possible options we can look at.

Increasing CACHE_MAX_SIZE alone doesn’t seem to help much.
(Screencast: https://www.youtube.com/watch?v=9YD9jyP-GKw)

Increasing CACHE_MAX_SIZE + Removing performSelectorOnMainThread seems to be better but I can’t be sure.  Just observed: 

(1) M-< at the mid of a buffer, but only the first line of the view is refreshed, and other parts were still there.

(2) selecting a region doesn’t always clear the hl-line effect.


[-- Attachment #2: hl-line.png --]
[-- Type: image/png, Size: 76619 bytes --]

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

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

[Aaron Jensen]

On Thu, Jun 8, 2023 at 10:42 PM Kai Ma <justksqsf@gmail.com> wrote:
>
>
>
> > On Jun 9, 2023, at 01:22, Alan Third <alan@idiocy.org> wrote:
> >
> > On Thu, Jun 08, 2023 at 10:57:43PM +0800, Kai Ma wrote:
> >>
> >>
> >>> On Jun 8, 2023, at 21:42, Kai Ma <justksqsf@gmail.com> wrote:
> >>>
> >>>
> >>>> On Jun 8, 2023, at 20:51, Alan Third <alan@idiocy.org> wrote:
> >>>>
> >>>> I've got two things it could be worth trying to see if they make any
> >>>> difference.
> >>>> Change the CACHE_MAX_SIZE to something greater than 2, probably 4 is a
> >>>> good number. That would rule out some sort of locking problem, as it
> >>>> would reduce the chance a surface would be re-used before it's been
> >>>> sent to the screen.
> >>>>
> >>>> The downside of this is probably going to be an increase in "lag"
> >>>> because it's possible to have more surfaces "in-flight".
> >>>>
> >>>> The other option is to remove this call (nsterm.m:10636):
> >>>>
> >>>> /* 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];
> >>>>
> >>>> I think this is harmless, but it's in there as I *assume*, with
> >>>> absolutely no proof, that it will improve performance and it seems
> >>>> possible (but I think unlikely) that it may affect the sequencing of
> >>>> surface operations.
> >>>
> >>> Tested locally. I can confirm that removing
> >>> performSelectorOnMainThread (with CACHE_MAX_SIZE = 2) fixes the
> >>> problem for me. I now observe zero glitches or tearings!
> >>>
> >>
> >> Sorry, I concluded too fast. I can still see tearings, though very
> >> rarely. So this is still not a real fix. :-(
> >
> > Try increasing CACHE_MAX_SIZE too. Tearing would, I believe, be
> > because a surface is being reused while it's still being copied to the
> > VRAM, so you'll see a partially modified output.
> >
> > If increasing CACHE_MAX_SIZE fixes it then there are a couple of
> > possible options we can look at.
>
> Increasing CACHE_MAX_SIZE alone doesn’t seem to help much.
> (Screencast: https://www.youtube.com/watch?v=9YD9jyP-GKw)
>
> Increasing CACHE_MAX_SIZE + Removing performSelectorOnMainThread seems to be better but I can’t be sure.  Just observed:
>
> (1) M-< at the mid of a buffer, but only the first line of the view is refreshed, and other parts were still there.
>
> (2) selecting a region doesn’t always clear the hl-line effect.

What are you doing to make your background translucent? I've never
seen anything nearly as bad as what you have. I've only seen a glitch
maybe once since the last patch (and that may have even been something
else). It makes me wonder if there's something else different/off
about your setup.

Aaron

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

[Kai Ma]

> On Jun 9, 2023, at 10:47, Aaron Jensen <aaronjensen@gmail.com> wrote:
> 
> On Thu, Jun 8, 2023 at 10:42 PM Kai Ma <justksqsf@gmail.com> wrote:
>> 
>> Increasing CACHE_MAX_SIZE alone doesn’t seem to help much.
>> (Screencast: https://www.youtube.com/watch?v=9YD9jyP-GKw)
>> 
>> Increasing CACHE_MAX_SIZE + Removing performSelectorOnMainThread seems to be better but I can’t be sure.  Just observed:
>> 
>> (1) M-< at the mid of a buffer, but only the first line of the view is refreshed, and other parts were still there.
>> 
>> (2) selecting a region doesn’t always clear the hl-line effect.
> 
> What are you doing to make your background translucent? I've never
> seen anything nearly as bad as what you have. I've only seen a glitch
> maybe once since the last patch (and that may have even been something
> else). It makes me wonder if there's something else different/off
> about your setup.

(set-frame-parameter nil 'alpha 0.95)

Indeed I haven’t been able to reproduce this bug in emacs -q so far. I thought I didn’t do anything unconventional, but, well, polling-period seems to be the culprit.

I had in my config
  (setq polling-period 0.01)
to increase the responsiveness of C-g, and reverting the change to the default value 2.0 is effective.

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

[Alan Third]

On Fri, Jun 09, 2023 at 11:12:47AM +0800, Kai Ma wrote:
> 
> 
> > On Jun 9, 2023, at 10:47, Aaron Jensen <aaronjensen@gmail.com> wrote:
> > 
> > On Thu, Jun 8, 2023 at 10:42 PM Kai Ma <justksqsf@gmail.com> wrote:
> >> 
> >> Increasing CACHE_MAX_SIZE alone doesn’t seem to help much.
> >> (Screencast: https://www.youtube.com/watch?v=9YD9jyP-GKw)
> >> 
> >> Increasing CACHE_MAX_SIZE + Removing performSelectorOnMainThread seems to be better but I can’t be sure.  Just observed:
> >> 
> >> (1) M-< at the mid of a buffer, but only the first line of the view is refreshed, and other parts were still there.
> >> 
> >> (2) selecting a region doesn’t always clear the hl-line effect.
> > 
> > What are you doing to make your background translucent? I've never
> > seen anything nearly as bad as what you have. I've only seen a glitch
> > maybe once since the last patch (and that may have even been something
> > else). It makes me wonder if there's something else different/off
> > about your setup.
> 
> (set-frame-parameter nil 'alpha 0.95)
> 
> Indeed I haven’t been able to reproduce this bug in emacs -q so far.
> I thought I didn’t do anything unconventional, but, well,
> polling-period seems to be the culprit.
> 
> I had in my config
>   (setq polling-period 0.01)
> to increase the responsiveness of C-g, and reverting the change to
> the default value 2.0 is effective.

Well, I have no idea what that actually means in terms of the display.

It seems to me that removing the call to performSelectorOnMainThread
should be done. That may even fix Aaron's original issue too, given
that I don't know why calling setNeedsDisplayInRect twice in a row
should help, especially given it's not actually used anywhere else in
the display code.

(And it should probably be [view setNeedsDisplay:YES] if it's needed
there at all.)

If tearing becomes a wider problem probably the best option would be
to actually wait for a surface to become free before taking it off the
cache, but... I dunno. I don't really know how we do that without just
having a loop polling continuously. Sleep for a few milliseconds each
time round the loop?

-- 
Alan Third

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

[Aaron Jensen]

On Fri, Jun 9, 2023 at 2:27 PM Alan Third <alan@idiocy.org> wrote:
> It seems to me that removing the call to performSelectorOnMainThread
> should be done. That may even fix Aaron's original issue too, given
> that I don't know why calling setNeedsDisplayInRect twice in a row
> should help, especially given it's not actually used anywhere else in
> the display code.

How is it called twice? Does copyRect mark for redisplay?

Aaron

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

[Alan Third]

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

On Fri, Jun 09, 2023 at 02:46:29PM -0400, Aaron Jensen wrote:
> On Fri, Jun 9, 2023 at 2:27 PM Alan Third <alan@idiocy.org> wrote:
> > It seems to me that removing the call to performSelectorOnMainThread
> > should be done. That may even fix Aaron's original issue too, given
> > that I don't know why calling setNeedsDisplayInRect twice in a row
> > should help, especially given it's not actually used anywhere else in
> > the display code.
> 
> How is it called twice? Does copyRect mark for redisplay?

Sorry, I had misunderstood your change.

Either way, I don't see how it's made a difference.
setNeedsDisplayInRect is telling the system which parts it needs to
call drawRect on, but we don't use drawRect any more, so I would think
all it can be doing is setting the needsDisplay boolean to true.

copyRect definitely doesn't do anything with the rectangle. It deals
with the bitmap's pixel data directly, so there's no clipping or
anything else affecting it and it's changes don't need to be
committed to some backing store.

Even when it comes to actually displaying the view on the screen, we
pass in the entire bitmap to the graphics subsystem and it
(supposedly) displays it in it's entirety.

So as I understand it the rectangle passed into setNeedsDisplayInRect
doesn't do anything. I think that call in ns_scroll_run was left there
by mistake. It's literally the only call to it in the entire nsterm.m
file.

But you report that it has fixed your problem. I can't explain that
because it runs counter to my understanding of how macOS draws.

But then again, none of this is documented in any in-depth way by
Apple, so who knows what's REALLY going on.

Patch attached, but it's untested. It may even make things worse. I'm
happy to leave it up to you to decide what to do since you're in a
better position to tell if any given change actually helps.
-- 
Alan Third

[-- Attachment #2: 0001-Reduce-graphical-glitches-in-certain-circumstances-o.patch --]
[-- Type: text/x-diff, Size: 1524 bytes --]

From b34cd84c1b070248b396d2ec82018be99b529d31 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Fri, 9 Jun 2023 20:53:31 +0100
Subject: [PATCH] Reduce graphical glitches in certain circumstances on macOS
 (bug#63187)

* src/nsterm.m (ns_scroll_run): Change way we request the frame is
redrawn to the glass-we no longer invalidate regions.
([EmacsLayer display]): Get rid of this asynchronous call, it may be
causing unexpected glitches during rapid updates.
---
 src/nsterm.m | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/src/nsterm.m b/src/nsterm.m
index 3e089cc1ff1..673fdf31531 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2708,8 +2708,9 @@ Hide the window (X11 semantics)
     EmacsView *view = FRAME_NS_VIEW (f);
 
     [view copyRect:srcRect to:dest];
-#ifdef NS_IMPL_COCOA
-    [view setNeedsDisplayInRect:destRect];
+#if defined (NS_IMPL_COCOA) && MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
+    /* I don't know if we even need this...  */
+    [view setNeedsDisplay:YES];
 #endif
   }
 
@@ -10633,12 +10634,6 @@ - (void) display
       /* Put currentSurface back on the end of the cache.  */
       [cache addObject:(id)currentSurface];
       currentSurface = NULL;
-
-      /* 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];
     }
 }
 
-- 
2.39.1

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

[Aaron Jensen]

On Fri, Jun 9, 2023 at 4:00 PM Alan Third <alan@idiocy.org> wrote:
>
> On Fri, Jun 09, 2023 at 02:46:29PM -0400, Aaron Jensen wrote:
> > On Fri, Jun 9, 2023 at 2:27 PM Alan Third <alan@idiocy.org> wrote:
> > > It seems to me that removing the call to performSelectorOnMainThread
> > > should be done. That may even fix Aaron's original issue too, given
> > > that I don't know why calling setNeedsDisplayInRect twice in a row
> > > should help, especially given it's not actually used anywhere else in
> > > the display code.
> >
> > How is it called twice? Does copyRect mark for redisplay?
>
> Sorry, I had misunderstood your change.
>
> Either way, I don't see how it's made a difference.
> setNeedsDisplayInRect is telling the system which parts it needs to
> call drawRect on, but we don't use drawRect any more, so I would think
> all it can be doing is setting the needsDisplay boolean to true.
>
> copyRect definitely doesn't do anything with the rectangle. It deals
> with the bitmap's pixel data directly, so there's no clipping or
> anything else affecting it and it's changes don't need to be
> committed to some backing store.
>
> Even when it comes to actually displaying the view on the screen, we
> pass in the entire bitmap to the graphics subsystem and it
> (supposedly) displays it in it's entirety.
>
> So as I understand it the rectangle passed into setNeedsDisplayInRect
> doesn't do anything. I think that call in ns_scroll_run was left there
> by mistake. It's literally the only call to it in the entire nsterm.m
> file.
>
> But you report that it has fixed your problem. I can't explain that
> because it runs counter to my understanding of how macOS draws.
>
> But then again, none of this is documented in any in-depth way by
> Apple, so who knows what's REALLY going on.
>
> Patch attached, but it's untested. It may even make things worse. I'm
> happy to leave it up to you to decide what to do since you're in a
> better position to tell if any given change actually helps.

Thanks, I'm trying it out now. I'll report back in a bit. If it's fine
I'll try removing the setNeedsDisplay as well and try again.

Aaron

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

[Aaron Jensen]

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

On Mon, Jun 12, 2023 at 9:04 AM Aaron Jensen <aaronjensen@gmail.com> wrote:
>
> On Fri, Jun 9, 2023 at 4:00 PM Alan Third <alan@idiocy.org> wrote:
> >
> > On Fri, Jun 09, 2023 at 02:46:29PM -0400, Aaron Jensen wrote:
> > > On Fri, Jun 9, 2023 at 2:27 PM Alan Third <alan@idiocy.org> wrote:
> > > > It seems to me that removing the call to performSelectorOnMainThread
> > > > should be done. That may even fix Aaron's original issue too, given
> > > > that I don't know why calling setNeedsDisplayInRect twice in a row
> > > > should help, especially given it's not actually used anywhere else in
> > > > the display code.
> > >
> > > How is it called twice? Does copyRect mark for redisplay?
> >
> > Sorry, I had misunderstood your change.
> >
> > Either way, I don't see how it's made a difference.
> > setNeedsDisplayInRect is telling the system which parts it needs to
> > call drawRect on, but we don't use drawRect any more, so I would think
> > all it can be doing is setting the needsDisplay boolean to true.
> >
> > copyRect definitely doesn't do anything with the rectangle. It deals
> > with the bitmap's pixel data directly, so there's no clipping or
> > anything else affecting it and it's changes don't need to be
> > committed to some backing store.
> >
> > Even when it comes to actually displaying the view on the screen, we
> > pass in the entire bitmap to the graphics subsystem and it
> > (supposedly) displays it in it's entirety.
> >
> > So as I understand it the rectangle passed into setNeedsDisplayInRect
> > doesn't do anything. I think that call in ns_scroll_run was left there
> > by mistake. It's literally the only call to it in the entire nsterm.m
> > file.
> >
> > But you report that it has fixed your problem. I can't explain that
> > because it runs counter to my understanding of how macOS draws.
> >
> > But then again, none of this is documented in any in-depth way by
> > Apple, so who knows what's REALLY going on.
> >
> > Patch attached, but it's untested. It may even make things worse. I'm
> > happy to leave it up to you to decide what to do since you're in a
> > better position to tell if any given change actually helps.
>
> Thanks, I'm trying it out now. I'll report back in a bit. If it's fine
> I'll try removing the setNeedsDisplay as well and try again.

I saw a paint issue today. The "<" to the left of the indented (and
redacted) lines 65-68 was an artifact. It kept painting there even
while scrolling until I resized the window, then they all disappeared.
They appeared one at a time while scrolling, as if the painting of one
the one on line 63 was "fixed" in the window position as I was
scrolling (likely it just didn't get cleared as necessary).

[-- Attachment #2: CleanShot 2023-06-15 at 22.14.50@2x.png --]
[-- Type: image/png, Size: 80914 bytes --]

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

[Aaron Jensen]

On Thu, Jun 15, 2023 at 10:17 PM Aaron Jensen <aaronjensen@gmail.com> wrote:
>
> On Mon, Jun 12, 2023 at 9:04 AM Aaron Jensen <aaronjensen@gmail.com> wrote:
> >
> > On Fri, Jun 9, 2023 at 4:00 PM Alan Third <alan@idiocy.org> wrote:
> > >
> > > On Fri, Jun 09, 2023 at 02:46:29PM -0400, Aaron Jensen wrote:
> > > > On Fri, Jun 9, 2023 at 2:27 PM Alan Third <alan@idiocy.org> wrote:
> > > > > It seems to me that removing the call to performSelectorOnMainThread
> > > > > should be done. That may even fix Aaron's original issue too, given
> > > > > that I don't know why calling setNeedsDisplayInRect twice in a row
> > > > > should help, especially given it's not actually used anywhere else in
> > > > > the display code.
> > > >
> > > > How is it called twice? Does copyRect mark for redisplay?
> > >
> > > Sorry, I had misunderstood your change.
> > >
> > > Either way, I don't see how it's made a difference.
> > > setNeedsDisplayInRect is telling the system which parts it needs to
> > > call drawRect on, but we don't use drawRect any more, so I would think
> > > all it can be doing is setting the needsDisplay boolean to true.
> > >
> > > copyRect definitely doesn't do anything with the rectangle. It deals
> > > with the bitmap's pixel data directly, so there's no clipping or
> > > anything else affecting it and it's changes don't need to be
> > > committed to some backing store.
> > >
> > > Even when it comes to actually displaying the view on the screen, we
> > > pass in the entire bitmap to the graphics subsystem and it
> > > (supposedly) displays it in it's entirety.
> > >
> > > So as I understand it the rectangle passed into setNeedsDisplayInRect
> > > doesn't do anything. I think that call in ns_scroll_run was left there
> > > by mistake. It's literally the only call to it in the entire nsterm.m
> > > file.
> > >
> > > But you report that it has fixed your problem. I can't explain that
> > > because it runs counter to my understanding of how macOS draws.
> > >
> > > But then again, none of this is documented in any in-depth way by
> > > Apple, so who knows what's REALLY going on.
> > >
> > > Patch attached, but it's untested. It may even make things worse. I'm
> > > happy to leave it up to you to decide what to do since you're in a
> > > better position to tell if any given change actually helps.
> >
> > Thanks, I'm trying it out now. I'll report back in a bit. If it's fine
> > I'll try removing the setNeedsDisplay as well and try again.
>
> I saw a paint issue today. The "<" to the left of the indented (and
> redacted) lines 65-68 was an artifact. It kept painting there even
> while scrolling until I resized the window, then they all disappeared.
> They appeared one at a time while scrolling, as if the painting of one
> the one on line 63 was "fixed" in the window position as I was
> scrolling (likely it just didn't get cleared as necessary).

Kai, could you try this patch out. It's a total guess, but let me know
if it does any better for you.

diff --git a/src/nsterm.h b/src/nsterm.h
index b6e5a813a6d..4f6c6f7c28b 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -1384,3 +1384,11 @@ #define NSButtonTypeMomentaryPushIn
NSMomentaryPushInButton
 extern void mark_nsterm (void);

 #endif /* HAVE_NS */
+
+#define AJTRACE(...)                                          \
+  do                                                                        \
+    {                                                                       \
+        fprintf (stderr, __VA_ARGS__);                                    \
+        putc ('\n', stderr);     \
+    }                                                                       \
+  while(0)
diff --git a/src/nsterm.m b/src/nsterm.m
index 3e089cc1ff1..5a92f4cda0b 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2708,9 +2708,9 @@ Hide the window (X11 semantics)
     EmacsView *view = FRAME_NS_VIEW (f);

     [view copyRect:srcRect to:dest];
-#ifdef NS_IMPL_COCOA
-    [view setNeedsDisplayInRect:destRect];
-#endif
+// #ifdef NS_IMPL_COCOA
+//     [view setNeedsDisplayInRect:destRect];
+// #endif
   }

   unblock_input ();
@@ -10636,9 +10636,9 @@ - (void) display

       /* 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];
+      // [self performSelectorOnMainThread:@selector (getContext)
+      //                        withObject:nil
+      //                     waitUntilDone:NO];
     }
 }

@@ -10651,6 +10651,7 @@ - (void) copyContentsTo: (IOSurfaceRef) destination
   IOReturn lockStatus;
   IOSurfaceRef source = (IOSurfaceRef)[self contents];
   void *sourceData, *destinationData;
+  int seed1 = 0, seed2 = 1;
   int numBytes = IOSurfaceGetAllocSize (destination);

   NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer copyContentsTo:]");
@@ -10662,14 +10663,31 @@ - (void) copyContentsTo: (IOSurfaceRef) destination
   if (lockStatus != kIOReturnSuccess)
     NSLog (@"Failed to lock source surface: %x", lockStatus);

+  lockStatus = IOSurfaceLock (destination, kIOSurfaceLockAvoidSync, nil);
+  if (lockStatus != kIOReturnSuccess)
+    NSLog (@"Failed to lock destination surface: %x", lockStatus);
+
   sourceData = IOSurfaceGetBaseAddress (source);
   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);
+  while (seed1 != seed2)
+    {
+      seed1 = IOSurfaceGetSeed (source);
+
+      /* 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);

+      seed2 = IOSurfaceGetSeed (source);
+      if (seed1 != seed2) {
+        AJTRACE ("SEED DO NOT MATCH");
+      }
+    }
+
+  lockStatus = IOSurfaceUnlock (destination, kIOSurfaceLockAvoidSync, nil);
+  if (lockStatus != kIOReturnSuccess)
+    NSLog (@"Failed to unlock destination surface: %x", lockStatus);
   lockStatus = IOSurfaceUnlock (source, kIOSurfaceLockReadOnly, nil);
   if (lockStatus != kIOReturnSuccess)
     NSLog (@"Failed to unlock source surface: %x", lockStatus);

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

[Kai Ma]

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



> On Jun 19, 2023, at 23:46, Aaron Jensen <aaronjensen@gmail.com> wrote:
> 
> Kai, could you try this patch out. It's a total guess, but let me know
> if it does any better for you.

Thanks. I have been running this for several days.  It does not fix the problem completely, but it’s possible to set polling-period to a very small value now.

> 
> diff --git a/src/nsterm.h b/src/nsterm.h
> index b6e5a813a6d..4f6c6f7c28b 100644
> --- a/src/nsterm.h
> +++ b/src/nsterm.h
> @@ -1384,3 +1384,11 @@ #define NSButtonTypeMomentaryPushIn
> NSMomentaryPushInButton
> extern void mark_nsterm (void);
> 
> #endif /* HAVE_NS */
> +
> +#define AJTRACE(...)                                          \
> +  do                                                                      \
> +    {                                                                     \
> +        fprintf (stderr, __VA_ARGS__);                                    \
> +        putc ('\n', stderr);     \
> +    }                                                                     \
> +  while(0)
> diff --git a/src/nsterm.m b/src/nsterm.m
> index 3e089cc1ff1..5a92f4cda0b 100644
> --- a/src/nsterm.m
> +++ b/src/nsterm.m
> @@ -2708,9 +2708,9 @@ Hide the window (X11 semantics)
>     EmacsView *view = FRAME_NS_VIEW (f);
> 
>     [view copyRect:srcRect to:dest];
> -#ifdef NS_IMPL_COCOA
> -    [view setNeedsDisplayInRect:destRect];
> -#endif
> +// #ifdef NS_IMPL_COCOA
> +//     [view setNeedsDisplayInRect:destRect];
> +// #endif
>   }
> 
>   unblock_input ();
> @@ -10636,9 +10636,9 @@ - (void) display
> 
>       /* 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];
> +      // [self performSelectorOnMainThread:@selector (getContext)
> +      //                        withObject:nil
> +      //                     waitUntilDone:NO];
>     }
> }
> 
> @@ -10651,6 +10651,7 @@ - (void) copyContentsTo: (IOSurfaceRef) destination
>   IOReturn lockStatus;
>   IOSurfaceRef source = (IOSurfaceRef)[self contents];
>   void *sourceData, *destinationData;
> +  int seed1 = 0, seed2 = 1;
>   int numBytes = IOSurfaceGetAllocSize (destination);
> 
>   NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer copyContentsTo:]");
> @@ -10662,14 +10663,31 @@ - (void) copyContentsTo: (IOSurfaceRef) destination
>   if (lockStatus != kIOReturnSuccess)
>     NSLog (@"Failed to lock source surface: %x", lockStatus);
> 
> +  lockStatus = IOSurfaceLock (destination, kIOSurfaceLockAvoidSync, nil);
> +  if (lockStatus != kIOReturnSuccess)
> +    NSLog (@"Failed to lock destination surface: %x", lockStatus);
> +
>   sourceData = IOSurfaceGetBaseAddress (source);
>   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);
> +  while (seed1 != seed2)
> +    {
> +      seed1 = IOSurfaceGetSeed (source);
> +
> +      /* 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);
> 
> +      seed2 = IOSurfaceGetSeed (source);
> +      if (seed1 != seed2) {
> +        AJTRACE ("SEED DO NOT MATCH");

I haven't seen this message so far. So probably it is removing performSelectorOnMainThread that is effective.

> +      }
> +    }
> +
> +  lockStatus = IOSurfaceUnlock (destination, kIOSurfaceLockAvoidSync, nil);
> +  if (lockStatus != kIOReturnSuccess)
> +    NSLog (@"Failed to unlock destination surface: %x", lockStatus);
>   lockStatus = IOSurfaceUnlock (source, kIOSurfaceLockReadOnly, nil);
>   if (lockStatus != kIOReturnSuccess)
>     NSLog (@"Failed to unlock source surface: %x", lockStatus);


[-- Attachment #2: Type: text/html, Size: 68938 bytes --]

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

[Aaron Jensen]

On Sat, Jun 24, 2023 at 12:18 AM Kai Ma <justksqsf@gmail.com> wrote:
>
>
>
> On Jun 19, 2023, at 23:46, Aaron Jensen <aaronjensen@gmail.com> wrote:
>
> Kai, could you try this patch out. It's a total guess, but let me know
> if it does any better for you.
>
>
> Thanks. I have been running this for several days.  It does not fix the problem completely, but it’s possible to set polling-period to a very small value now.

Hmm, do you see any of the artifacts w/o changing the polling period?

> diff --git a/src/nsterm.h b/src/nsterm.h
> index b6e5a813a6d..4f6c6f7c28b 100644
> --- a/src/nsterm.h
> +++ b/src/nsterm.h
> @@ -1384,3 +1384,11 @@ #define NSButtonTypeMomentaryPushIn
> NSMomentaryPushInButton
> extern void mark_nsterm (void);
>
> #endif /* HAVE_NS */
> +
> +#define AJTRACE(...)                                          \
> +  do                                                                      \
> +    {                                                                     \
> +        fprintf (stderr, __VA_ARGS__);                                    \
> +        putc ('\n', stderr);     \
> +    }                                                                     \
> +  while(0)
> diff --git a/src/nsterm.m b/src/nsterm.m
> index 3e089cc1ff1..5a92f4cda0b 100644
> --- a/src/nsterm.m
> +++ b/src/nsterm.m
> @@ -2708,9 +2708,9 @@ Hide the window (X11 semantics)
>     EmacsView *view = FRAME_NS_VIEW (f);
>
>     [view copyRect:srcRect to:dest];
> -#ifdef NS_IMPL_COCOA
> -    [view setNeedsDisplayInRect:destRect];
> -#endif
> +// #ifdef NS_IMPL_COCOA
> +//     [view setNeedsDisplayInRect:destRect];
> +// #endif
>
>   }
>
>   unblock_input ();
> @@ -10636,9 +10636,9 @@ - (void) display
>
>       /* 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];
> +      // [self performSelectorOnMainThread:@selector (getContext)
> +      //                        withObject:nil
> +      //                     waitUntilDone:NO];
>
>     }
> }
>
> @@ -10651,6 +10651,7 @@ - (void) copyContentsTo: (IOSurfaceRef) destination
>   IOReturn lockStatus;
>   IOSurfaceRef source = (IOSurfaceRef)[self contents];
>   void *sourceData, *destinationData;
> +  int seed1 = 0, seed2 = 1;
>   int numBytes = IOSurfaceGetAllocSize (destination);
>
>   NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer copyContentsTo:]");
> @@ -10662,14 +10663,31 @@ - (void) copyContentsTo: (IOSurfaceRef) destination
>   if (lockStatus != kIOReturnSuccess)
>     NSLog (@"Failed to lock source surface: %x", lockStatus);
>
> +  lockStatus = IOSurfaceLock (destination, kIOSurfaceLockAvoidSync, nil);
> +  if (lockStatus != kIOReturnSuccess)
> +    NSLog (@"Failed to lock destination surface: %x", lockStatus);
> +
>   sourceData = IOSurfaceGetBaseAddress (source);
>   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);
> +  while (seed1 != seed2)
> +    {
> +      seed1 = IOSurfaceGetSeed (source);
> +
> +      /* 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);
>
> +      seed2 = IOSurfaceGetSeed (source);
> +      if (seed1 != seed2) {
> +        AJTRACE ("SEED DO NOT MATCH");
>
>
> I haven't seen this message so far. So probably it is removing performSelectorOnMainThread that is effective.

Could you try removing the destination lock as well and see if that
impacts anything? From what I can tell, locking the destination may be
a good idea, but I'm curious if Alan has any thoughts as to why it'd
be a bad idea.

Thanks,

Aaron

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

[Alan Third]

On Sat, Jun 24, 2023 at 09:34:18AM -0400, Aaron Jensen wrote:
> 
> Could you try removing the destination lock as well and see if that
> impacts anything? From what I can tell, locking the destination may be
> a good idea, but I'm curious if Alan has any thoughts as to why it'd
> be a bad idea.

copyContentsTo is only called by getContext, which has already locked
the destination, so there's no need to lock it again.

As I recall locking is really just setting a flag saying it's
currently being used so nobody else should touch it, it doesn't stop
you from doing things with it.

I know, for example, that it's possible to write to a surface that's
been locked by the transfer to VRAM, and that results in tearing
effects where the copy in VRAM is half updated. The VRAM copy isn't
ever copied back down to system RAM, though, so it doesn't have any
permanent consequences beyond a tearing effect at that particular
moment.
-- 
Alan Third

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

[Aaron Jensen]

On Sat, Jun 24, 2023 at 10:14 AM Alan Third <alan@idiocy.org> wrote:
>
> On Sat, Jun 24, 2023 at 09:34:18AM -0400, Aaron Jensen wrote:
> >
> > Could you try removing the destination lock as well and see if that
> > impacts anything? From what I can tell, locking the destination may be
> > a good idea, but I'm curious if Alan has any thoughts as to why it'd
> > be a bad idea.
>
> copyContentsTo is only called by getContext, which has already locked
> the destination, so there's no need to lock it again.

Ah, I see. Is the kIOSurfaceLockAvoidSync flag valuable for any reason?

> As I recall locking is really just setting a flag saying it's
> currently being used so nobody else should touch it, it doesn't stop
> you from doing things with it.
>
> I know, for example, that it's possible to write to a surface that's
> been locked by the transfer to VRAM, and that results in tearing
> effects where the copy in VRAM is half updated. The VRAM copy isn't
> ever copied back down to system RAM, though, so it doesn't have any
> permanent consequences beyond a tearing effect at that particular
> moment.

If I recall correctly, there's some code in Emacs that optimizes which
areas of the screen glyphs are drawn to. Maybe it remembers what was
the background color already and doesn't clear it again... I don't
remember where I saw it, but I think it was outside of nsterm. Does
that ring a bell? I'm thinking about how this manifests. For me, it's
always whole characters that are painted, they are always painted in
what would otherwise be whitespace, and they tend to get "copied" to
the next line in the whitespace as scrolling happens. In other words,
it doesn't just seem like a fluke write during the transfer to VRAM.
It seems like something in the engine is writing them, that the state
is getting "stuck" somehow. Does any of that ring a bell/jog anything?

Aaron

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 24 Jun 2023 10:52:08 -0400
> 
> If I recall correctly, there's some code in Emacs that optimizes which
> areas of the screen glyphs are drawn to. Maybe it remembers what was
> the background color already and doesn't clear it again... I don't
> remember where I saw it, but I think it was outside of nsterm. Does
> that ring a bell? I'm thinking about how this manifests. For me, it's
> always whole characters that are painted, they are always painted in
> what would otherwise be whitespace, and they tend to get "copied" to
> the next line in the whitespace as scrolling happens. In other words,
> it doesn't just seem like a fluke write during the transfer to VRAM.
> It seems like something in the engine is writing them, that the state
> is getting "stuck" somehow. Does any of that ring a bell/jog anything?

If you are talking about the frame's scroll_run_hook, then this is
also called by xdisp.c as part of some redisplay optimizations.

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

[Alan Third]

On Sat, Jun 24, 2023 at 10:52:08AM -0400, Aaron Jensen wrote:
> On Sat, Jun 24, 2023 at 10:14 AM Alan Third <alan@idiocy.org> wrote:
> >
> > On Sat, Jun 24, 2023 at 09:34:18AM -0400, Aaron Jensen wrote:
> > >
> > > Could you try removing the destination lock as well and see if that
> > > impacts anything? From what I can tell, locking the destination may be
> > > a good idea, but I'm curious if Alan has any thoughts as to why it'd
> > > be a bad idea.
> >
> > copyContentsTo is only called by getContext, which has already locked
> > the destination, so there's no need to lock it again.
> 
> Ah, I see. Is the kIOSurfaceLockAvoidSync flag valuable for any reason?

I don't think so... Not in our use-case. IIRC it's possible to get the
GPU to perform actions on the buffer once it's in VRAM. We don't do
that, so we don't need to ever worry about write-backs to system RAM.

I think.

> If I recall correctly, there's some code in Emacs that optimizes which
> areas of the screen glyphs are drawn to. Maybe it remembers what was
> the background color already and doesn't clear it again... I don't
> remember where I saw it, but I think it was outside of nsterm. Does
> that ring a bell? I'm thinking about how this manifests. For me, it's
> always whole characters that are painted, they are always painted in
> what would otherwise be whitespace, and they tend to get "copied" to
> the next line in the whitespace as scrolling happens. In other words,
> it doesn't just seem like a fluke write during the transfer to VRAM.
> It seems like something in the engine is writing them, that the state
> is getting "stuck" somehow. Does any of that ring a bell/jog anything?

Its quite possible. I did say before in the thread that it seems quite
possible to me that something isn't clearing the whitespace correctly.
But it's obviously rare, and I would expect that it's some piece of NS
port code rather than somewhere else.

Unless there's something that's #ifdef'd out because at some point in
the past the NS port has behaved differently...

But more likely if this is what's going on then it's going to be a bug
in the NS port. Unfortunately I don't really understand how the glyph
drawing side works, and never did.

But if that's the case, why would removing the asynchronous call to
getContext fix so many problems?

Something perhaps worth trying... Since removing the asynchronous call
to getContext fixes the problems, perhaps we need to think about the
"lazy" way we get the next buffer when the current one is displayed.
At the moment it just forgets about it until we want to draw to the
screen, at which point we call getContext and it creates the buffer if
necessary and copies the old one to the new one.

Maybe we should get the new buffer and do the copy when we set the
current buffer for display...

IIRC I avoided that because there isn't always time for the buffer to
have been sent to VRAM and unlocked before the *next* call to display,
so I wanted to leave it as long as possible between display and
getting the next buffer, but maybe this is just the wrong way to do
it.

So I suppose putting a call to getContext right after "currentSurface
== NULL" in display might be a quick and dirty way to test that.

-- 
Alan Third

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

[Aaron Jensen]

On Sat, Jun 24, 2023 at 11:41 AM Alan Third <alan@idiocy.org> wrote:
>
> On Sat, Jun 24, 2023 at 10:52:08AM -0400, Aaron Jensen wrote:
> > On Sat, Jun 24, 2023 at 10:14 AM Alan Third <alan@idiocy.org> wrote:
> > >
> > > On Sat, Jun 24, 2023 at 09:34:18AM -0400, Aaron Jensen wrote:
> > > >
> > > > Could you try removing the destination lock as well and see if that
> > > > impacts anything? From what I can tell, locking the destination may be
> > > > a good idea, but I'm curious if Alan has any thoughts as to why it'd
> > > > be a bad idea.
> > >
> > > copyContentsTo is only called by getContext, which has already locked
> > > the destination, so there's no need to lock it again.
> >
> > Ah, I see. Is the kIOSurfaceLockAvoidSync flag valuable for any reason?
>
> I don't think so... Not in our use-case. IIRC it's possible to get the
> GPU to perform actions on the buffer once it's in VRAM. We don't do
> that, so we don't need to ever worry about write-backs to system RAM.
>
> I think.
>
> > If I recall correctly, there's some code in Emacs that optimizes which
> > areas of the screen glyphs are drawn to. Maybe it remembers what was
> > the background color already and doesn't clear it again... I don't
> > remember where I saw it, but I think it was outside of nsterm. Does
> > that ring a bell? I'm thinking about how this manifests. For me, it's
> > always whole characters that are painted, they are always painted in
> > what would otherwise be whitespace, and they tend to get "copied" to
> > the next line in the whitespace as scrolling happens. In other words,
> > it doesn't just seem like a fluke write during the transfer to VRAM.
> > It seems like something in the engine is writing them, that the state
> > is getting "stuck" somehow. Does any of that ring a bell/jog anything?
>
> Its quite possible. I did say before in the thread that it seems quite
> possible to me that something isn't clearing the whitespace correctly.
> But it's obviously rare, and I would expect that it's some piece of NS
> port code rather than somewhere else.
>
> Unless there's something that's #ifdef'd out because at some point in
> the past the NS port has behaved differently...
>
> But more likely if this is what's going on then it's going to be a bug
> in the NS port. Unfortunately I don't really understand how the glyph
> drawing side works, and never did.
>
> But if that's the case, why would removing the asynchronous call to
> getContext fix so many problems?

It's possible we have two very different problems that only appear related:
1. The one i'm seeing, which is sort of ghosting of other lines into
the whitespace of nearby lines. The getContext call removal did not
fix this for me, I saw it happen once.
2. The one Kai is seeing, that is exacerbated by decreasing the
polling interval, but seems to be helped by removing the getContext
call.

> Something perhaps worth trying... Since removing the asynchronous call
> to getContext fixes the problems, perhaps we need to think about the
> "lazy" way we get the next buffer when the current one is displayed.
> At the moment it just forgets about it until we want to draw to the
> screen, at which point we call getContext and it creates the buffer if
> necessary and copies the old one to the new one.
>
> Maybe we should get the new buffer and do the copy when we set the
> current buffer for display...
>
> IIRC I avoided that because there isn't always time for the buffer to
> have been sent to VRAM and unlocked before the *next* call to display,
> so I wanted to leave it as long as possible between display and
> getting the next buffer, but maybe this is just the wrong way to do
> it.
>
> So I suppose putting a call to getContext right after "currentSurface
> == NULL" in display might be a quick and dirty way to test that.

My problem is that at this point it happens so infrequently and I have
no idea if that's because of the patches I'm trying or some other
environmental thing or just luck. I'm going to try running without the
async getContext and without the setNeedsDisplay for a while and see
if it happens. Perhaps the setNeedsDisplay is somehow causing an issue
and that's why changing it from source to dest seemed to help but it
didn't alleviate it.

If I see it again, I'll add the sync getContext call in, though I'll
admit I do not understand your paragraph above starting with IIRC. Are
you suspecting a potential problem with reading from the surface that
is in the process of being copied to vram?

Aaron

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

[Alan Third]

On Sat, Jun 24, 2023 at 12:05:45PM -0400, Aaron Jensen wrote:
> > But if that's the case, why would removing the asynchronous call to
> > getContext fix so many problems?
> 
> It's possible we have two very different problems that only appear related:
> 1. The one i'm seeing, which is sort of ghosting of other lines into
> the whitespace of nearby lines. The getContext call removal did not
> fix this for me, I saw it happen once.
> 2. The one Kai is seeing, that is exacerbated by decreasing the
> polling interval, but seems to be helped by removing the getContext
> call.

Yes, it might be two different things.

> > Something perhaps worth trying... Since removing the asynchronous call
> > to getContext fixes the problems, perhaps we need to think about the
> > "lazy" way we get the next buffer when the current one is displayed.
> > At the moment it just forgets about it until we want to draw to the
> > screen, at which point we call getContext and it creates the buffer if
> > necessary and copies the old one to the new one.
> >
> > Maybe we should get the new buffer and do the copy when we set the
> > current buffer for display...
> >
> > IIRC I avoided that because there isn't always time for the buffer to
> > have been sent to VRAM and unlocked before the *next* call to display,
> > so I wanted to leave it as long as possible between display and
> > getting the next buffer, but maybe this is just the wrong way to do
> > it.
> >
> > So I suppose putting a call to getContext right after "currentSurface
> > == NULL" in display might be a quick and dirty way to test that.
> 
> My problem is that at this point it happens so infrequently and I have
> no idea if that's because of the patches I'm trying or some other
> environmental thing or just luck. I'm going to try running without the
> async getContext and without the setNeedsDisplay for a while and see
> if it happens. Perhaps the setNeedsDisplay is somehow causing an issue
> and that's why changing it from source to dest seemed to help but it
> didn't alleviate it.

OK

> If I see it again, I'll add the sync getContext call in, though I'll
> admit I do not understand your paragraph above starting with IIRC. Are
> you suspecting a potential problem with reading from the surface that
> is in the process of being copied to vram?

Maybe... I'm really not sure what might be going on.

My suspicion is that if we try to swap between the buffers too fast,
something is going wrong between the process of flushing the drawing
to the pixel buffer and copying the pixel buffer to the next one.

So, we have two buffers, A and B. We draw to A, but before we're done
the system calls display. We send off the incomplete buffer A to VRAM,
and then take a copy of that incomplete buffer for B. At some point
the system decides to flush the graphics context to the buffer, but
it's flushing to A, and we've *already* copied A to B.

This would possibly explain why Kai lowering the polling interval
induces the issue, as it may increase the frequency at which the
screen is updated beyond the point where we're able to keep up.

To be honest though, I feel it should all be pretty linear and this
idea implies things are happening out-of-order. So I'm not convinced
I'm right.


Who knows. Maybe all we need to do is make sure we don't try to draw
to the screen while emacs is drawing to the buffer... Something like
this:

modified src/nsterm.m @@ -10622,7 +10622,7 @@ - (void) display
 {
   NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer display]");
 
-  if (context)
+  if (context && context != [NSGraphicsContext currentContext])
     {
       [self releaseContext];


...

Actually...

That change should probably be made anyway. If the NS run loop kicks
in between an ns_focus call and an ns_unfocus call, it could call
display and our display function will happily destroy the existing
context without creating a new one, so any *subsequent* drawing
operations, up until ns_unfocus, will be lost.

I'm not sure if that's a legitimate concern... 😕


I've got too many ideas about how to fix it and no way to actually try
them out, never mind the difficulty of inducing the issue if I did.

-- 
Alan Third

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

[Aaron Jensen]

On Sat, Jun 24, 2023 at 5:29 PM Alan Third <alan@idiocy.org> wrote:
>
> On Sat, Jun 24, 2023 at 12:05:45PM -0400, Aaron Jensen wrote:
> > > But if that's the case, why would removing the asynchronous call to
> > > getContext fix so many problems?
> >
> > It's possible we have two very different problems that only appear related:
> > 1. The one i'm seeing, which is sort of ghosting of other lines into
> > the whitespace of nearby lines. The getContext call removal did not
> > fix this for me, I saw it happen once.
> > 2. The one Kai is seeing, that is exacerbated by decreasing the
> > polling interval, but seems to be helped by removing the getContext
> > call.
>
> Yes, it might be two different things.
>
> > > Something perhaps worth trying... Since removing the asynchronous call
> > > to getContext fixes the problems, perhaps we need to think about the
> > > "lazy" way we get the next buffer when the current one is displayed.
> > > At the moment it just forgets about it until we want to draw to the
> > > screen, at which point we call getContext and it creates the buffer if
> > > necessary and copies the old one to the new one.
> > >
> > > Maybe we should get the new buffer and do the copy when we set the
> > > current buffer for display...
> > >
> > > IIRC I avoided that because there isn't always time for the buffer to
> > > have been sent to VRAM and unlocked before the *next* call to display,
> > > so I wanted to leave it as long as possible between display and
> > > getting the next buffer, but maybe this is just the wrong way to do
> > > it.
> > >
> > > So I suppose putting a call to getContext right after "currentSurface
> > > == NULL" in display might be a quick and dirty way to test that.
> >
> > My problem is that at this point it happens so infrequently and I have
> > no idea if that's because of the patches I'm trying or some other
> > environmental thing or just luck. I'm going to try running without the
> > async getContext and without the setNeedsDisplay for a while and see
> > if it happens. Perhaps the setNeedsDisplay is somehow causing an issue
> > and that's why changing it from source to dest seemed to help but it
> > didn't alleviate it.
>
> OK
>
> > If I see it again, I'll add the sync getContext call in, though I'll
> > admit I do not understand your paragraph above starting with IIRC. Are
> > you suspecting a potential problem with reading from the surface that
> > is in the process of being copied to vram?
>
> Maybe... I'm really not sure what might be going on.
>
> My suspicion is that if we try to swap between the buffers too fast,
> something is going wrong between the process of flushing the drawing
> to the pixel buffer and copying the pixel buffer to the next one.

Do we have any way to know when the flush is done? In other words, can
we only return the surface to the pool after that? Or is that already
done?

> So, we have two buffers, A and B. We draw to A, but before we're done
> the system calls display. We send off the incomplete buffer A to VRAM,
> and then take a copy of that incomplete buffer for B. At some point
> the system decides to flush the graphics context to the buffer, but
> it's flushing to A, and we've *already* copied A to B.

Can we avoid sending incomplete buffers? What is "done"? I don't know
much about graphics programming but I imagine we don't want to send
incomplete buffers ever, we want to finish painting the whole buffer,
then send it. I think I'm also missing understanding on what it means
to flush the graphics context to the buffer. Is that the drawing that
we're doing (like rendering text/etc?) I feel like I may need a
whiteboard session or something to get my head around this so that I
can be of any assistance other than asking dumb questions :)

> This would possibly explain why Kai lowering the polling interval
> induces the issue, as it may increase the frequency at which the
> screen is updated beyond the point where we're able to keep up.
>
> To be honest though, I feel it should all be pretty linear and this
> idea implies things are happening out-of-order. So I'm not convinced
> I'm right.
>
>
> Who knows. Maybe all we need to do is make sure we don't try to draw
> to the screen while emacs is drawing to the buffer... Something like
> this:
>
> modified src/nsterm.m @@ -10622,7 +10622,7 @@ - (void) display
>  {
>    NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer display]");
>
> -  if (context)
> +  if (context && context != [NSGraphicsContext currentContext])
>      {
>        [self releaseContext];
>
>
> ...
>
> Actually...
>
> That change should probably be made anyway. If the NS run loop kicks
> in between an ns_focus call and an ns_unfocus call, it could call
> display and our display function will happily destroy the existing
> context without creating a new one, so any *subsequent* drawing
> operations, up until ns_unfocus, will be lost.

OK, I'm adding this to my current build.

Is this in line with the type of issue I'm seeing where scrolling
works but the ghosting either replicates (or scrolls with it?) In
other words, what would you expect to see in this scenario? Would it
just stop painting entirely?

>
> I'm not sure if that's a legitimate concern... 😕
>
>
> I've got too many ideas about how to fix it and no way to actually try
> them out, never mind the difficulty of inducing the issue if I did.

I'm happy to try things if you don't mind a 1-2 week feedback cycle
(since lately that's about how long it takes for me to see an issue...

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

[Alan Third]

On Sat, Jun 24, 2023 at 05:43:04PM -0400, Aaron Jensen wrote:
> On Sat, Jun 24, 2023 at 5:29 PM Alan Third <alan@idiocy.org> wrote:
> > My suspicion is that if we try to swap between the buffers too fast,
> > something is going wrong between the process of flushing the drawing
> > to the pixel buffer and copying the pixel buffer to the next one.
> 
> Do we have any way to know when the flush is done? In other words, can
> we only return the surface to the pool after that? Or is that already
> done?

No. Part of the reason I'm very unsure about this idea is that Apple
are very clear that they don't think you should ever have to deal with
this sort of thing.

The provide functions that allow you to force the flush, but then they
say you should never have to use it.

How the macOS graphics system works is also pretty much undocumented,
so it's hard to get to grips with whether anything *is* going wrong
here. A lot of people do seem to have a deep understanding of it, but
I don't have the first clue how you go about learning from first
principles.

> > So, we have two buffers, A and B. We draw to A, but before we're done
> > the system calls display. We send off the incomplete buffer A to VRAM,
> > and then take a copy of that incomplete buffer for B. At some point
> > the system decides to flush the graphics context to the buffer, but
> > it's flushing to A, and we've *already* copied A to B.
> 
> Can we avoid sending incomplete buffers? What is "done"? I don't know
> much about graphics programming but I imagine we don't want to send
> incomplete buffers ever, we want to finish painting the whole buffer,
> then send it. I think I'm also missing understanding on what it means
> to flush the graphics context to the buffer. Is that the drawing that
> we're doing (like rendering text/etc?) I feel like I may need a
> whiteboard session or something to get my head around this so that I
> can be of any assistance other than asking dumb questions :)

When I say "done", I largely mean a call to ns_update_end. So most,
but not all iiuc, Emacs drawing is done between a call to
ns_update_begin and a call to ns_update_end.

Apple recommend you leave the final display to screen to them. At some
point, when the system decides it wants to update the screen, it
checks if we've marked the view's needsDisplay variable to true, and
if so, amongst a lot of other stuff, it calls our display function.

Now, I think this, for the most part, only happens within the NS
runloop, but it is possible to force it. I think Apple are right,
though, and we don't want to force it unless we really need to.

As for flushing the context, this is another thing Apple say we
shouldn't touch, but they do give you the tools to force it.

Imagine you want to draw some text, then a line, then a circle, or
whatever. You'd think the system would draw them directly to the
buffer as you call the functions, but apparently the system can queue
them up and apply them in one go later on. I believe this should
happen when we release the context, or in various other situations, so
it's not something I really think is likely to be the cause, but it
does sort-of match what we see happening.

> > Who knows. Maybe all we need to do is make sure we don't try to draw
> > to the screen while emacs is drawing to the buffer... Something like
> > this:
> >
> > modified src/nsterm.m @@ -10622,7 +10622,7 @@ - (void) display
> >  {
> >    NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer display]");
> >
> > -  if (context)
> > +  if (context && context != [NSGraphicsContext currentContext])
> >      {
> >        [self releaseContext];
> >
> >
> > ...
> >
> > Actually...
> >
> > That change should probably be made anyway. If the NS run loop kicks
> > in between an ns_focus call and an ns_unfocus call, it could call
> > display and our display function will happily destroy the existing
> > context without creating a new one, so any *subsequent* drawing
> > operations, up until ns_unfocus, will be lost.
> 
> OK, I'm adding this to my current build.
> 
> Is this in line with the type of issue I'm seeing where scrolling
> works but the ghosting either replicates (or scrolls with it?) In
> other words, what would you expect to see in this scenario? Would it
> just stop painting entirely?

It could be. The more I think about this change, the more I think it
might be the solution.

If you imagine we call ns_update_begin, which creates the
context/buffer/whatever else we need, then somewhere in the midst of
drawing display is called and the context is wiped out and the buffer
is sent off to the screen. Until ns_update_end is called (or anything
else that happens to call getContext, like copyRect) then nothing will
be drawn to the buffer, even though Emacs reasonably expects it has.

So yes, this might mean some parts of the screen aren't cleared, but
it could also mean other drawing actions don't take place, like
actually drawing text, or drawing underlines or whatever.

HOWEVER, as I said above, copyRect calls getContext, so if we then try
to scroll, it *will* draw to the buffer. CopyRect will *always*
successfully draw to a buffer, even if some drawing actions will have
failed already.

All my change above does is make sure that the currently selected
context (i.e. the one we're drawing to right this moment) isn't the
same one we're trying to display. unlockFocus always clears the
current context so if the current view is focused, the new check
should catch it, and if it's not, say we're drawing to another frame
or not drawing at all, then it should let it be displayed.

The biggest risk is we start missing our chance to display the buffer
to the screen, because it just skips sending the buffer. If you start
seeing delays in the screen updating then we may have to consider
forcing updates in ns_update_end or something.

I hope this fixes it. It looks like a legitimate mistake in my logic
whereas practically every other thing I can think of requires some
weird behaviour in AppKit.


Kai, it might be worth trying just that change above, while keeping
the call to performSelectorInMainThread and see if it fixes anything
for you.
-- 
Alan Third

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

[Aaron Jensen]

On Sun, Jun 25, 2023 at 8:46 AM Alan Third <alan@idiocy.org> wrote:
>
> On Sat, Jun 24, 2023 at 05:43:04PM -0400, Aaron Jensen wrote:
> > On Sat, Jun 24, 2023 at 5:29 PM Alan Third <alan@idiocy.org> wrote:
> > > My suspicion is that if we try to swap between the buffers too fast,
> > > something is going wrong between the process of flushing the drawing
> > > to the pixel buffer and copying the pixel buffer to the next one.
> >
> > Do we have any way to know when the flush is done? In other words, can
> > we only return the surface to the pool after that? Or is that already
> > done?
>
> No. Part of the reason I'm very unsure about this idea is that Apple
> are very clear that they don't think you should ever have to deal with
> this sort of thing.
>
> The provide functions that allow you to force the flush, but then they
> say you should never have to use it.
>
> How the macOS graphics system works is also pretty much undocumented,
> so it's hard to get to grips with whether anything *is* going wrong
> here. A lot of people do seem to have a deep understanding of it, but
> I don't have the first clue how you go about learning from first
> principles.
>
> > > So, we have two buffers, A and B. We draw to A, but before we're done
> > > the system calls display. We send off the incomplete buffer A to VRAM,
> > > and then take a copy of that incomplete buffer for B. At some point
> > > the system decides to flush the graphics context to the buffer, but
> > > it's flushing to A, and we've *already* copied A to B.
> >
> > Can we avoid sending incomplete buffers? What is "done"? I don't know
> > much about graphics programming but I imagine we don't want to send
> > incomplete buffers ever, we want to finish painting the whole buffer,
> > then send it. I think I'm also missing understanding on what it means
> > to flush the graphics context to the buffer. Is that the drawing that
> > we're doing (like rendering text/etc?) I feel like I may need a
> > whiteboard session or something to get my head around this so that I
> > can be of any assistance other than asking dumb questions :)
>
> When I say "done", I largely mean a call to ns_update_end. So most,
> but not all iiuc, Emacs drawing is done between a call to
> ns_update_begin and a call to ns_update_end.
>
> Apple recommend you leave the final display to screen to them. At some
> point, when the system decides it wants to update the screen, it
> checks if we've marked the view's needsDisplay variable to true, and
> if so, amongst a lot of other stuff, it calls our display function.
>
> Now, I think this, for the most part, only happens within the NS
> runloop, but it is possible to force it. I think Apple are right,
> though, and we don't want to force it unless we really need to.
>
> As for flushing the context, this is another thing Apple say we
> shouldn't touch, but they do give you the tools to force it.
>
> Imagine you want to draw some text, then a line, then a circle, or
> whatever. You'd think the system would draw them directly to the
> buffer as you call the functions, but apparently the system can queue
> them up and apply them in one go later on. I believe this should
> happen when we release the context, or in various other situations, so
> it's not something I really think is likely to be the cause, but it
> does sort-of match what we see happening.
>
> > > Who knows. Maybe all we need to do is make sure we don't try to draw
> > > to the screen while emacs is drawing to the buffer... Something like
> > > this:
> > >
> > > modified src/nsterm.m @@ -10622,7 +10622,7 @@ - (void) display
> > >  {
> > >    NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer display]");
> > >
> > > -  if (context)
> > > +  if (context && context != [NSGraphicsContext currentContext])
> > >      {
> > >        [self releaseContext];
> > >
> > >
> > > ...
> > >
> > > Actually...
> > >
> > > That change should probably be made anyway. If the NS run loop kicks
> > > in between an ns_focus call and an ns_unfocus call, it could call
> > > display and our display function will happily destroy the existing
> > > context without creating a new one, so any *subsequent* drawing
> > > operations, up until ns_unfocus, will be lost.
> >
> > OK, I'm adding this to my current build.
> >
> > Is this in line with the type of issue I'm seeing where scrolling
> > works but the ghosting either replicates (or scrolls with it?) In
> > other words, what would you expect to see in this scenario? Would it
> > just stop painting entirely?
>
> It could be. The more I think about this change, the more I think it
> might be the solution.
>
> If you imagine we call ns_update_begin, which creates the
> context/buffer/whatever else we need, then somewhere in the midst of
> drawing display is called and the context is wiped out and the buffer
> is sent off to the screen. Until ns_update_end is called (or anything
> else that happens to call getContext, like copyRect) then nothing will
> be drawn to the buffer, even though Emacs reasonably expects it has.
>
> So yes, this might mean some parts of the screen aren't cleared, but
> it could also mean other drawing actions don't take place, like
> actually drawing text, or drawing underlines or whatever.
>
> HOWEVER, as I said above, copyRect calls getContext, so if we then try
> to scroll, it *will* draw to the buffer. CopyRect will *always*
> successfully draw to a buffer, even if some drawing actions will have
> failed already.
>
> All my change above does is make sure that the currently selected
> context (i.e. the one we're drawing to right this moment) isn't the
> same one we're trying to display. unlockFocus always clears the
> current context so if the current view is focused, the new check
> should catch it, and if it's not, say we're drawing to another frame
> or not drawing at all, then it should let it be displayed.
>
> The biggest risk is we start missing our chance to display the buffer
> to the screen, because it just skips sending the buffer. If you start
> seeing delays in the screen updating then we may have to consider
> forcing updates in ns_update_end or something.
>
> I hope this fixes it. It looks like a legitimate mistake in my logic
> whereas practically every other thing I can think of requires some
> weird behaviour in AppKit.

Thank you for the continued explanation. I'm doing some additional
digging and it's making me wonder if what we are doing is even
possible reliably with Cocoa drawing and without any synchronization
primitives like MTLCommandBuffer's waitUntilCompleted. If I understand
correctly, we do something like this:

1. Draw to Surface 1 (buffer commands)
2. Assign Surface 1 to the view as its layer
3. Copy from Surface 1 to Surface 2
4. Copy rects on Surface 2 to other parts of Surface 2

As I understand, there's no way to ensure that all commands have been
flushed all the way to the point that we know that Surface 1 is
actually representative of the commands we have sent it. It seems like
macOS will ensure that it all eventually gets drawn to the screen, but
if we attempt to read from the surface, we don't know if all the
drawing is complete. This is in contrast to Metal, where we could
waitUntilCompleted before copying Surface 1 to Surface 2. Am I missing
something in this or is it just not possible to truly synchronize?

I came across some notes from the Chromium team:
https://www.chromium.org/developers/design-documents/iosurface-meeting-notes/

They (as of the notes) decided not to double buffer and just write
directly to the one surface. I don't know if that's reasonable/viable
for Emacs or not.

Lastly, as an aside, if we *could* synchronize, would it be more
efficient to not copy the entire surface just to then copy rects again
to handle scrolling? I can imagine this would add more complexity than
is worth it, I'm mainly asking for my own edification.

Thanks,

Aaron

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

[Alan Third]

On Sun, Jun 25, 2023 at 01:07:19PM -0400, Aaron Jensen wrote:
> 
> Thank you for the continued explanation. I'm doing some additional
> digging and it's making me wonder if what we are doing is even
> possible reliably with Cocoa drawing and without any synchronization
> primitives like MTLCommandBuffer's waitUntilCompleted. If I understand
> correctly, we do something like this:
> 
> 1. Draw to Surface 1 (buffer commands)
> 2. Assign Surface 1 to the view as its layer
> 3. Copy from Surface 1 to Surface 2
> 4. Copy rects on Surface 2 to other parts of Surface 2

Yep.

> As I understand, there's no way to ensure that all commands have been
> flushed all the way to the point that we know that Surface 1 is
> actually representative of the commands we have sent it.

No, you can use [NSGraphicsContext flushGraphics] and CGContextFlush
to force it.

As I recall we've tried that in both copyRect and display.

Ultimately releasing the context will have to flush any buffered
writes, if it didn't then this would be a widely known problem as
practically nothing graphical could be relied upon.

This is my point about why I can't see this being the issue. Although
it looks like it explains what we're seeing, we would have to be
hitting some extreme edge-case.

> I came across some notes from the Chromium team:
> https://www.chromium.org/developers/design-documents/iosurface-meeting-notes/
> 
> They (as of the notes) decided not to double buffer and just write
> directly to the one surface. I don't know if that's reasonable/viable
> for Emacs or not.

Set CACHE_MAX_SIZE to 1.

But on my machine this resulted in unacceptable rendering flaws on
almost all animated gifs as partially drawn buffers were sent to VRAM.

And adding more open frames just made it worse, as I recall. I never
really understood why.

It wasn't just gifs, but they were an easy test.

But then, I'll bet performance is incredible. ;)

> Lastly, as an aside, if we *could* synchronize, would it be more
> efficient to not copy the entire surface just to then copy rects again
> to handle scrolling? I can imagine this would add more complexity than
> is worth it, I'm mainly asking for my own edification.

I'm not sure if I'm following your question correctly, but yes, not
copying is usually going to be faster, but it depends how long it
takes the buffer to copy to VRAM, you might well find that you're
waiting for it to finish when you may have already been in the
position to start drawing to a second buffer.

In theory, if you set CACHE_MAX_SIZE > 1, and change

    [cache addObject:(id)currentSurface];

to

    [cache insertObject:(id)currentSurface
                atIndex:0]

It should try to pick up the most recent surface first, then
copyContentsTo will recognise that it's the same surface as last time,
and not bother with the copy. I think you'd have to enable

    [self setContents:nil];

to work all the time too, as you might be wanting to "replace" the
contents layer with itself.

I've no idea if that would really work. IIRC on my machine the surface
that was "in flight" to VRAM was never done fast enough that it would
be picked up by the next getContext call so I felt it was a waste to
check it every time. (And in fast-scrolling situations I could have as
many as three surfaces in-flight at once if I let the cache grow that
big.)
-- 
Alan Third

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

[Aaron Jensen]

On Sun, Jun 25, 2023 at 2:17 PM Alan Third <alan@idiocy.org> wrote:
>
> On Sun, Jun 25, 2023 at 01:07:19PM -0400, Aaron Jensen wrote:
> >
> > Thank you for the continued explanation. I'm doing some additional
> > digging and it's making me wonder if what we are doing is even
> > possible reliably with Cocoa drawing and without any synchronization
> > primitives like MTLCommandBuffer's waitUntilCompleted. If I understand
> > correctly, we do something like this:
> >
> > 1. Draw to Surface 1 (buffer commands)
> > 2. Assign Surface 1 to the view as its layer
> > 3. Copy from Surface 1 to Surface 2
> > 4. Copy rects on Surface 2 to other parts of Surface 2
>
> Yep.
>
> > As I understand, there's no way to ensure that all commands have been
> > flushed all the way to the point that we know that Surface 1 is
> > actually representative of the commands we have sent it.
>
> No, you can use [NSGraphicsContext flushGraphics] and CGContextFlush
> to force it.

OK, that makes sense, but it's hard to find definitive documentation
of that (and ChatGPT seems to think that it's not true, but that's
likely a hallucination):

=====START GPT=====
[NSGraphicsContext flushGraphics] (in Cocoa) and CGContextFlush (in
Core Graphics) are used to bypass this batching process and force any
pending drawing commands to be executed immediately. After calling
these methods, you can be sure that all your previously issued drawing
commands have been sent to the GPU.

However, it's important to understand that while these functions
ensure that the commands are sent to the GPU, they don't guarantee
that the GPU has finished executing them. The actual rendering work is
still done asynchronously by the GPU.

So if you need to read back the results of your rendering (e.g., from
an IOSurface), there might still be a brief period where the rendering
hasn't completed yet, even after calling flushGraphics or
CGContextFlush. If you need to ensure that all GPU rendering work is
complete before you proceed, you would typically need to use a more
low-level API (like Metal or OpenGL) that provides explicit
synchronization capabilities.

...

Regarding your question about the [NSGraphicsContext flushGraphics]
and CGContextFlush, here are the descriptions directly from Apple's
documentation:

[NSGraphicsContext flushGraphics]: "Forces any buffered drawing
commands to be sent to the destination."

CGContextFlush: "Forces all drawing operations to be completed in the
specified context."

It's important to understand these function calls ensure that drawing
commands are dispatched, not that they are completed. This is an
inference based on understanding of how graphics pipelines generally
work. For more detailed behavior and how these calls interact with
your specific use-case, you should refer to Apple's documentation and
guides, or consider reaching out to Apple's developer support.
=====END GPT=====

> As I recall we've tried that in both copyRect and display.
>
> Ultimately releasing the context will have to flush any buffered
> writes, if it didn't then this would be a widely known problem as
> practically nothing graphical could be relied upon.
>
> This is my point about why I can't see this being the issue. Although
> it looks like it explains what we're seeing, we would have to be
> hitting some extreme edge-case.
>
> > I came across some notes from the Chromium team:
> > https://www.chromium.org/developers/design-documents/iosurface-meeting-notes/
> >
> > They (as of the notes) decided not to double buffer and just write
> > directly to the one surface. I don't know if that's reasonable/viable
> > for Emacs or not.
>
> Set CACHE_MAX_SIZE to 1.
>
> But on my machine this resulted in unacceptable rendering flaws on
> almost all animated gifs as partially drawn buffers were sent to VRAM.

What did you see?
I tried with this:

https://share.cleanshot.com/vJTClHW9

And I didn't notice anything drawing related, but I have an M1 with
integrated memory.

Aaron

> And adding more open frames just made it worse, as I recall. I never
> really understood why.
>
> It wasn't just gifs, but they were an easy test.
>
> But then, I'll bet performance is incredible. ;)
>
> > Lastly, as an aside, if we *could* synchronize, would it be more
> > efficient to not copy the entire surface just to then copy rects again
> > to handle scrolling? I can imagine this would add more complexity than
> > is worth it, I'm mainly asking for my own edification.
>
> I'm not sure if I'm following your question correctly, but yes, not
> copying is usually going to be faster, but it depends how long it
> takes the buffer to copy to VRAM, you might well find that you're
> waiting for it to finish when you may have already been in the
> position to start drawing to a second buffer.
>
> In theory, if you set CACHE_MAX_SIZE > 1, and change
>
>     [cache addObject:(id)currentSurface];
>
> to
>
>     [cache insertObject:(id)currentSurface
>                 atIndex:0]
>
> It should try to pick up the most recent surface first, then
> copyContentsTo will recognise that it's the same surface as last time,
> and not bother with the copy. I think you'd have to enable
>
>     [self setContents:nil];
>
> to work all the time too, as you might be wanting to "replace" the
> contents layer with itself.
>
> I've no idea if that would really work. IIRC on my machine the surface
> that was "in flight" to VRAM was never done fast enough that it would
> be picked up by the next getContext call so I felt it was a waste to
> check it every time. (And in fast-scrolling situations I could have as
> many as three surfaces in-flight at once if I let the cache grow that
> big.)
> --
> Alan Third

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

[Alan Third]

On Sun, Jun 25, 2023 at 03:07:39PM -0400, Aaron Jensen wrote:
> On Sun, Jun 25, 2023 at 2:17 PM Alan Third <alan@idiocy.org> wrote:
> > No, you can use [NSGraphicsContext flushGraphics] and CGContextFlush
> > to force it.
> 
> OK, that makes sense, but it's hard to find definitive documentation
> of that (and ChatGPT seems to think that it's not true, but that's
> likely a hallucination):
> 
> =====START GPT=====
> [NSGraphicsContext flushGraphics] (in Cocoa) and CGContextFlush (in
> Core Graphics) are used to bypass this batching process and force any
> pending drawing commands to be executed immediately. After calling
> these methods, you can be sure that all your previously issued drawing
> commands have been sent to the GPU.
> 
> However, it's important to understand that while these functions
> ensure that the commands are sent to the GPU, they don't guarantee
> that the GPU has finished executing them. The actual rendering work is
> still done asynchronously by the GPU.
> 
> So if you need to read back the results of your rendering (e.g., from
> an IOSurface), there might still be a brief period where the rendering
> hasn't completed yet, even after calling flushGraphics or
> CGContextFlush. If you need to ensure that all GPU rendering work is
> complete before you proceed, you would typically need to use a more
> low-level API (like Metal or OpenGL) that provides explicit
> synchronization capabilities.
> 
> ...
> 
> Regarding your question about the [NSGraphicsContext flushGraphics]
> and CGContextFlush, here are the descriptions directly from Apple's
> documentation:
> 
> [NSGraphicsContext flushGraphics]: "Forces any buffered drawing
> commands to be sent to the destination."
> 
> CGContextFlush: "Forces all drawing operations to be completed in the
> specified context."
> 
> It's important to understand these function calls ensure that drawing
> commands are dispatched, not that they are completed. This is an
> inference based on understanding of how graphics pipelines generally
> work. For more detailed behavior and how these calls interact with
> your specific use-case, you should refer to Apple's documentation and
> guides, or consider reaching out to Apple's developer support.
> =====END GPT=====

The GPU isn't involved in this part. We're drawing to an IOSurface,
which is a buffer in system memory. So when we draw and flush the
graphics it's all done by the CPU in system memory.

Then we put the IOSurfaceRef in the contents variable of the layer and
at that point the GPU uses DMA to copy the buffer from system memory
to GPU memory. While this is happening the IOSurface is locked so we
know we shouldn't use it.

Once it's in GPU memory, the GPU blits it to the screenbuffer or
something, but by that time the IOSurface should be unlocked and we
can start (safely) working on it again.

> > Set CACHE_MAX_SIZE to 1.
> >
> > But on my machine this resulted in unacceptable rendering flaws on
> > almost all animated gifs as partially drawn buffers were sent to VRAM.
> 
> What did you see?
> I tried with this:
> 
> https://share.cleanshot.com/vJTClHW9
> 
> And I didn't notice anything drawing related, but I have an M1 with
> integrated memory.

Lots of white space. I thought that it was being caught either just
before or as the image was being drawn into the buffer, so some
portion of the image area was blank.
-- 
Alan Third

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

[Aaron Jensen]

On Sun, Jun 25, 2023 at 5:18 PM Alan Third <alan@idiocy.org> wrote:
>
> On Sun, Jun 25, 2023 at 03:07:39PM -0400, Aaron Jensen wrote:
> > On Sun, Jun 25, 2023 at 2:17 PM Alan Third <alan@idiocy.org> wrote:
> > > No, you can use [NSGraphicsContext flushGraphics] and CGContextFlush
> > > to force it.
> >
> > OK, that makes sense, but it's hard to find definitive documentation
> > of that (and ChatGPT seems to think that it's not true, but that's
> > likely a hallucination):
> >
> > =====START GPT=====
> > [NSGraphicsContext flushGraphics] (in Cocoa) and CGContextFlush (in
> > Core Graphics) are used to bypass this batching process and force any
> > pending drawing commands to be executed immediately. After calling
> > these methods, you can be sure that all your previously issued drawing
> > commands have been sent to the GPU.
> >
> > However, it's important to understand that while these functions
> > ensure that the commands are sent to the GPU, they don't guarantee
> > that the GPU has finished executing them. The actual rendering work is
> > still done asynchronously by the GPU.
> >
> > So if you need to read back the results of your rendering (e.g., from
> > an IOSurface), there might still be a brief period where the rendering
> > hasn't completed yet, even after calling flushGraphics or
> > CGContextFlush. If you need to ensure that all GPU rendering work is
> > complete before you proceed, you would typically need to use a more
> > low-level API (like Metal or OpenGL) that provides explicit
> > synchronization capabilities.
> >
> > ...
> >
> > Regarding your question about the [NSGraphicsContext flushGraphics]
> > and CGContextFlush, here are the descriptions directly from Apple's
> > documentation:
> >
> > [NSGraphicsContext flushGraphics]: "Forces any buffered drawing
> > commands to be sent to the destination."
> >
> > CGContextFlush: "Forces all drawing operations to be completed in the
> > specified context."
> >
> > It's important to understand these function calls ensure that drawing
> > commands are dispatched, not that they are completed. This is an
> > inference based on understanding of how graphics pipelines generally
> > work. For more detailed behavior and how these calls interact with
> > your specific use-case, you should refer to Apple's documentation and
> > guides, or consider reaching out to Apple's developer support.
> > =====END GPT=====
>
> The GPU isn't involved in this part. We're drawing to an IOSurface,
> which is a buffer in system memory. So when we draw and flush the
> graphics it's all done by the CPU in system memory.
>
> Then we put the IOSurfaceRef in the contents variable of the layer and
> at that point the GPU uses DMA to copy the buffer from system memory
> to GPU memory. While this is happening the IOSurface is locked so we
> know we shouldn't use it.
>
> Once it's in GPU memory, the GPU blits it to the screenbuffer or
> something, but by that time the IOSurface should be unlocked and we
> can start (safely) working on it again.

OK, that's clarifying, thank you.

> > > Set CACHE_MAX_SIZE to 1.
> > >
> > > But on my machine this resulted in unacceptable rendering flaws on
> > > almost all animated gifs as partially drawn buffers were sent to VRAM.
> >
> > What did you see?
> > I tried with this:
> >
> > https://share.cleanshot.com/vJTClHW9
> >
> > And I didn't notice anything drawing related, but I have an M1 with
> > integrated memory.
>
> Lots of white space. I thought that it was being caught either just
> before or as the image was being drawn into the buffer, so some
> portion of the image area was blank.

Interesting. Would:

+  if (context && context != [NSGraphicsContext currentContext])

have impacted that in any way?

Aaron

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

[Kai Ma]

> On Jun 25, 2023, at 20:46, Alan Third <alan@idiocy.org> wrote:
> 
>>> 
>>> modified src/nsterm.m @@ -10622,7 +10622,7 @@ - (void) display
>>> {
>>>   NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer display]");
>>> 
>>> -  if (context)
>>> +  if (context && context != [NSGraphicsContext currentContext])
>>>     {
>>>       [self releaseContext];
>>> 
>>> 
>>> ...
>>> 
>>> Actually...
>>> 
>>> That change should probably be made anyway. If the NS run loop kicks
>>> in between an ns_focus call and an ns_unfocus call, it could call
>>> display and our display function will happily destroy the existing
>>> context without creating a new one, so any *subsequent* drawing
>>> operations, up until ns_unfocus, will be lost.
>> 
>> OK, I'm adding this to my current build.
>> 
>> Is this in line with the type of issue I'm seeing where scrolling
>> works but the ghosting either replicates (or scrolls with it?) In
>> other words, what would you expect to see in this scenario? Would it
>> just stop painting entirely?
> 
> 
> Kai, it might be worth trying just that change above, while keeping
> the call to performSelectorInMainThread and see if it fixes anything
> for you.

Unfortunately, the problem persists (as in the YouTube video) if performSelectorInMainThread is present.

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

[Alan Third]

On Mon, Jun 26, 2023 at 03:27:41PM +0800, Kai Ma wrote:
> 
> 
> > On Jun 25, 2023, at 20:46, Alan Third <alan@idiocy.org> wrote:
> > 
> >>> 
> >>> modified src/nsterm.m @@ -10622,7 +10622,7 @@ - (void) display
> >>> {
> >>>   NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer display]");
> >>> 
> >>> -  if (context)
> >>> +  if (context && context != [NSGraphicsContext currentContext])
> >>>     {
> >>>       [self releaseContext];
> >>> 
> >>> 
> >>> ...
> >>> 
> >>> Actually...
> >>> 
> >>> That change should probably be made anyway. If the NS run loop kicks
> >>> in between an ns_focus call and an ns_unfocus call, it could call
> >>> display and our display function will happily destroy the existing
> >>> context without creating a new one, so any *subsequent* drawing
> >>> operations, up until ns_unfocus, will be lost.
> >> 
> >> OK, I'm adding this to my current build.
> >> 
> >> Is this in line with the type of issue I'm seeing where scrolling
> >> works but the ghosting either replicates (or scrolls with it?) In
> >> other words, what would you expect to see in this scenario? Would it
> >> just stop painting entirely?
> > 
> > 
> > Kai, it might be worth trying just that change above, while keeping
> > the call to performSelectorInMainThread and see if it fixes anything
> > for you.
> 
> Unfortunately, the problem persists (as in the YouTube video) if performSelectorInMainThread is present.

OK. Thanks for trying it.
-- 
Alan Third

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

[Aaron Jensen]

On Wed, Jun 28, 2023 at 3:53 PM Alan Third <alan@idiocy.org> wrote:
>
> On Mon, Jun 26, 2023 at 03:27:41PM +0800, Kai Ma wrote:
> >
> >
> > > On Jun 25, 2023, at 20:46, Alan Third <alan@idiocy.org> wrote:
> > >
> > >>>
> > >>> modified src/nsterm.m @@ -10622,7 +10622,7 @@ - (void) display
> > >>> {
> > >>>   NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "[EmacsLayer display]");
> > >>>
> > >>> -  if (context)
> > >>> +  if (context && context != [NSGraphicsContext currentContext])
> > >>>     {
> > >>>       [self releaseContext];
> > >>>
> > >>>
> > >>> ...
> > >>>
> > >>> Actually...
> > >>>
> > >>> That change should probably be made anyway. If the NS run loop kicks
> > >>> in between an ns_focus call and an ns_unfocus call, it could call
> > >>> display and our display function will happily destroy the existing
> > >>> context without creating a new one, so any *subsequent* drawing
> > >>> operations, up until ns_unfocus, will be lost.
> > >>
> > >> OK, I'm adding this to my current build.
> > >>
> > >> Is this in line with the type of issue I'm seeing where scrolling
> > >> works but the ghosting either replicates (or scrolls with it?) In
> > >> other words, what would you expect to see in this scenario? Would it
> > >> just stop painting entirely?
> > >
> > >
> > > Kai, it might be worth trying just that change above, while keeping
> > > the call to performSelectorInMainThread and see if it fixes anything
> > > for you.
> >
> > Unfortunately, the problem persists (as in the YouTube video) if performSelectorInMainThread is present.
>
> OK. Thanks for trying it.


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?

Thanks,

Aaron

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

[Alan Third]

[-- 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

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

[Aaron Jensen]

On Sun, Jul 23, 2023 at 7:20 AM Alan Third <alan@idiocy.org> wrote:
>
> 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.)

Thanks, I'll try this one out. FYI there was a typo in a comment: doible

Aaron

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

[Aaron Jensen]

Would it be worth putting this in Emacs 29 since the RC is out? This
is likely better and so far there are no obvious issues.

Aaron

On Sun, Jul 23, 2023 at 9:01 AM Aaron Jensen <aaronjensen@gmail.com> wrote:
>
> On Sun, Jul 23, 2023 at 7:20 AM Alan Third <alan@idiocy.org> wrote:
> >
> > 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.)
>
> Thanks, I'll try this one out. FYI there was a typo in a comment: doible
>
> Aaron

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Tue, 25 Jul 2023 10:47:53 -0400
> 
> Would it be worth putting this in Emacs 29 since the RC is out? This
> is likely better and so far there are no obvious issues.

Not yet, please wait until Emacs 29.1 is officially released.

Thanks.

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

[Alan Third]

On Thu, Jun 15, 2023 at 10:17:11PM -0400, Aaron Jensen wrote:
> 
> I saw a paint issue today. The "<" to the left of the indented (and
> redacted) lines 65-68 was an artifact. It kept painting there even
> while scrolling until I resized the window, then they all disappeared.
> They appeared one at a time while scrolling, as if the painting of one
> the one on line 63 was "fixed" in the window position as I was
> scrolling (likely it just didn't get cleared as necessary).

It could be worth forcing the system to display when we want it to,
rather than leaving it to decide itself... I don't think this will
make any difference, but perhaps it's worth a shot.

modified   src/nsterm.m
@@ -1089,6 +1089,8 @@ static NSRect constrain_frame_rect(NSRect frameRect, bool isFullscreen)
   [view unlockFocus];
 #if defined (NS_IMPL_GNUSTEP) || MAC_OS_X_VERSION_MIN_REQUIRED < 101400
   [[view window] flushWindow];
+#else
+  [view display];
 #endif
 
   unblock_input ();

This feels to me like things are happening out of order or
simultaneously, but I don't really see how that could be happening.
The drawing system throws out errors if you try to use it from sub
threads, so it should be obvious if there was some multi-threading
issue...

-- 
Alan Third

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

[Aaron Jensen]

On Fri, Jun 23, 2023 at 4:48 AM Alan Third <alan@idiocy.org> wrote:
>
> On Thu, Jun 15, 2023 at 10:17:11PM -0400, Aaron Jensen wrote:
> >
> > I saw a paint issue today. The "<" to the left of the indented (and
> > redacted) lines 65-68 was an artifact. It kept painting there even
> > while scrolling until I resized the window, then they all disappeared.
> > They appeared one at a time while scrolling, as if the painting of one
> > the one on line 63 was "fixed" in the window position as I was
> > scrolling (likely it just didn't get cleared as necessary).
>
> It could be worth forcing the system to display when we want it to,
> rather than leaving it to decide itself... I don't think this will
> make any difference, but perhaps it's worth a shot.
>
> modified   src/nsterm.m
> @@ -1089,6 +1089,8 @@ static NSRect constrain_frame_rect(NSRect frameRect, bool isFullscreen)
>    [view unlockFocus];
>  #if defined (NS_IMPL_GNUSTEP) || MAC_OS_X_VERSION_MIN_REQUIRED < 101400
>    [[view window] flushWindow];
> +#else
> +  [view display];
>  #endif
>
>    unblock_input ();
>
> This feels to me like things are happening out of order or
> simultaneously, but I don't really see how that could be happening.
> The drawing system throws out errors if you try to use it from sub
> threads, so it should be obvious if there was some multi-threading
> issue...

I'll try that in a bit, thanks. One interesting thing is that just
yesterday I saw this crash for the first time (with the patch I
recently posted):

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGABRT)
Exception Codes:       KERN_INVALID_ADDRESS at 0x00000000000000c0
Exception Codes:       0x0000000000000001, 0x00000000000000c0

VM Region Info: 0xc0 is not in any region.  Bytes before following
region: 105553518919488
      REGION TYPE                    START - END         [ VSIZE]
PRT/MAX SHRMOD  REGION DETAIL
      UNUSED SPACE AT START
--->
      MALLOC_NANO (reserved)   600018000000-600020000000 [128.0M]
rw-/rwx SM=NUL  ...(unallocated)

Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib               0x191db4724 __pthread_kill + 8
1   libsystem_pthread.dylib               0x191debc28 pthread_kill + 288
2   libsystem_c.dylib                     0x191cc246c raise + 32
3   emacs                                 0x1046488c8
terminate_due_to_signal + 212 (emacs.c:464)
4   emacs                                 0x104649128 emacs_abort + 20
(sysdep.c:2320)
5   emacs                                 0x104605ad4 ns_term_shutdown
+ 144 (nsterm.m:5770)
6   emacs                                 0x1044cdcdc shut_down_emacs
+ 332 (emacs.c:3017)
7   emacs                                 0x104648890
terminate_due_to_signal + 156 (emacs.c:447)
8   emacs                                 0x1044f1a20
handle_fatal_signal + 12 (sysdep.c:1783) [inlined]
9   emacs                                 0x1044f1a20
deliver_thread_signal + 112 (sysdep.c:1775) [inlined]
10  emacs                                 0x1044f1a20
deliver_fatal_thread_signal + 128 (sysdep.c:1795)
11  emacs                                 0x1044f38e0 handle_sigsegv +
64 (sysdep.c:1888)
12  libsystem_platform.dylib             0x191e1aa24 _sigtramp + 56
13  ???                            0xffff800104607dc4 ???
14  AppKit                               0x19569b8d0
-[_NSTrackingAreaAKViewHelper
updateTrackingAreasWithInvalidCursorRects:] + 284
15  AppKit                               0x1958a8964
_NSViewSubViewMutationSafeApply + 220
16  AppKit                               0x19569b974
-[_NSTrackingAreaAKViewHelper
updateTrackingAreasWithInvalidCursorRects:] + 448
17  AppKit                               0x1958a8964
_NSViewSubViewMutationSafeApply + 220
18  AppKit                               0x19569b974
-[_NSTrackingAreaAKViewHelper
updateTrackingAreasWithInvalidCursorRects:] + 448
19  AppKit                               0x195699cb4
-[_NSTrackingAreaAKManager displayCycleUpdateStructuralRegions] + 176
20  AppKit                               0x195195658
__NSWindowGetDisplayCycleObserverForUpdateStructuralRegions_block_invoke
+ 364
21  AppKit                               0x195190c30
NSDisplayCycleObserverInvoke + 168
22  AppKit                               0x19519088c NSDisplayCycleFlush + 644
23  QuartzCore                           0x1993adce4
CA::Transaction::run_commit_handlers(CATransactionPhase) + 120
24  QuartzCore                           0x1993acaa0
CA::Transaction::commit() + 320
25  AppKit                               0x195212c7c
__62+[CATransaction(NSCATransaction)
NS_setFlushesWithDisplayLink]_block_invoke + 272
26  AppKit                               0x1958eef7c
___NSRunLoopObserverCreateWithHandler_block_invoke + 64
27  CoreFoundation                       0x191ec99f0
__CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 36
28  CoreFoundation                       0x191ec98dc
__CFRunLoopDoObservers + 532
29  CoreFoundation                       0x191ec8f14 __CFRunLoopRun + 776
30  CoreFoundation                       0x191ec84b8 CFRunLoopRunSpecific + 612
31  HIToolbox                             0x19b712c40
RunCurrentEventLoopInMode + 292
32  HIToolbox                             0x19b7128d0
ReceiveNextEventCommon + 220
33  HIToolbox                             0x19b7127d4
_BlockUntilNextEventMatchingListInModeWithFilter + 76
34  AppKit                               0x1950e9d44 _DPSNextEvent + 636
35  AppKit                               0x1950e8ee0
-[NSApplication(NSEvent)
_nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 716
36  AppKit                               0x1950dd344 -[NSApplication run] + 464
37  emacs                                 0x104605c24 -[EmacsApp run]
+ 264 (nsterm.m:5823)
38  emacs                                 0x10461842c ns_read_socket_1
+ 560 (nsterm.m:4703)
39  emacs                                 0x10461602c ns_flush_display
+ 44 (nsterm.m:5311)
40  emacs                                 0x104428a04 flush_frame + 32
(frame.h:1760) [inlined]
41  emacs                                 0x104428a04
echo_area_display + 1080 (xdisp.c:13203)
42  emacs                                 0x1044284d8 message3_nolog +
548 (xdisp.c:12104)
43  emacs                                 0x104428078 message3 + 132
(xdisp.c:12034)
44  emacs                                 0x10455456c Fmessage + 68
(editfns.c:3192)
45  emacs                                 0x10455ca4c Ffuncall + 396
(eval.c:3008)
46  emacs                                 0x10455f750 Fapply + 996 (eval.c:2679)
47  emacs                                 0x10455ca4c Ffuncall + 396
(eval.c:3008)
48  subr--trampoline-6d657373616765_message_0.eln        0x1196f7f20
F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_0 + 88
49  magit-process-0a318d45-3f4ff10f.eln        0x128a6262c
F6d616769742d72756e2d6769742d6173796e63_magit_run_git_async_0 + 220
50  emacs                                 0x10455ca4c Ffuncall + 396
(eval.c:3008)
51  magit-fetch-8acc7b48-029daf61.eln        0x12acf664c
F6d616769742d6769742d6665746368_magit_git_fetch_0 + 104
52  emacs                                 0x10455ca4c Ffuncall + 396
(eval.c:3008)
53  magit-fetch-8acc7b48-029daf61.eln        0x12acf6abc
F6d616769742d66657463682d616c6c_magit_fetch_all_0 + 76
54  emacs                                 0x10455ca4c Ffuncall + 396
(eval.c:3008)
55  emacs                                 0x1045a8a0c exec_byte_code +
3236 (bytecode.c:809)
56  emacs                                 0x10455ca4c Ffuncall + 396
(eval.c:3008)
57  emacs                                 0x10455a760 eval_sub + 2356
(eval.c:2483)
58  emacs                                 0x104560e50 Fprogn + 28
(eval.c:436) [inlined]
59  emacs                                 0x104560e50 funcall_lambda +
1356 (eval.c:3246)
60  emacs                                 0x10455ca4c Ffuncall + 396
(eval.c:3008)
61  emacs                                 0x1045a8a0c exec_byte_code +
3236 (bytecode.c:809)
62  emacs                                 0x10455ca4c Ffuncall + 396
(eval.c:3008)
63  emacs                                 0x104557b14
Ffuncall_interactively + 68 (callint.c:250)
64  emacs                                 0x10455ca4c Ffuncall + 396
(eval.c:3008)
65  emacs                                 0x104559000
Fcall_interactively + 5332 (callint.c:342)
66  simple-fab5b0cf-e1c8f2a9.eln         0x107dd4efc
F636f6d6d616e642d65786563757465_command_execute_0 + 652
67  emacs                                 0x10455ca4c Ffuncall + 396
(eval.c:3008)
68  emacs                                 0x1044d1bbc call1 + 20
(lisp.h:3241) [inlined]
69  emacs                                 0x1044d1bbc command_loop_1 +
1384 (keyboard.c:1504)
70  emacs                                 0x10455d7fc
internal_condition_case + 96 (eval.c:1486)
71  emacs                                 0x1044d1640 command_loop_2 +
52 (keyboard.c:1133)
72  emacs                                 0x10455d09c internal_catch +
88 (eval.c:1209)
73  emacs                                 0x104648d1c
command_loop.cold.1 + 80 (keyboard.c:1111)
74  emacs                                 0x1044d0d74 command_loop +
156 (keyboard.c:1110)
75  emacs                                 0x1044d0c2c recursive_edit_1
+ 152 (keyboard.c:720)
76  emacs                                 0x1044d0fe0 Frecursive_edit
+ 372 (keyboard.c:803)
77  emacs                                 0x1044cfdb4 main + 8324 (emacs.c:2530)
78  dyld                                 0x191a93f28 start + 2236

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

[Alan Third]

On Mon, May 01, 2023 at 04:25:05PM +0300, Eli Zaretskii wrote:
> > From: Po Lu <luangruo@yahoo.com>
> > Cc: aaronjensen@gmail.com,  63187@debbugs.gnu.org
> > Date: Mon, 01 May 2023 21:18:31 +0800
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > >> Cc: 63187@debbugs.gnu.org
> > >> Date: Mon, 01 May 2023 07:58:26 +0800
> > >> From:  Po Lu via "Bug reports for GNU Emacs,
> > >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > >> 
> > >> Aaron Jensen <aaronjensen@gmail.com> writes:
> > >> 
> > >> > Is there anything specific to macOS that is involved in scrolling optimization?
> > >> 
> > >> Yes, Apple deleted the API used to perform bit blits, so Emacs uses a
> > >> workaround that I don't really understand, and seems unreliable.
> > >
> > > You mean, ns_scroll_run in nsterm.m?  Which parts of it do you not
> > > understand?
> > 
> > No, I meant the implementation of [EmacsView copyRect:] enabled under
> > Mac OS; see line 8655 of nsterm.m.  I don't understand how the system
> > synchronizes its access to the window's backing store with Emacs's.
> 
> Alan, can you help?
> 
> If this is unworkable on macOS, we could simply disable this
> optimization there.  But note that scroll_run_hook is also called from
> xdisp.c, in several places, so we may need to disable it there as
> well.

This is one of the many reasons I walked away from the NS port. ;)

I'm 99% certain that [EmacsView copyRect:] is correct. I went over it
numerous times. It's just doing a simple copy of one rectangle of a
graphics buffer to another, identically sized, rectangle. It handles
overlapping rectangles and there's really not very much logic in it to
go wrong.

Po Lu asks about how it synchronises with the system, and it doesn't
really have to. The system draws into the same buffer that copyRect
accesses. It does make me wonder if there's a rare occasion when
copyRect is called and the system hasn't flushed all it's drawing
instructions to the buffer yet. It's probably possible to force it to
flush by doing something like:

    [[NSGraphicsContext currentContext] flushGraphics];

at the top of copyRect. Preferably only on macOS 10.14 and above, but
it probably makes no difference on other platforms since scrollRect
should force a flush anyway.

My memory of Aaron's previous bug report about this was that it looked
like, in some cases at least, the rectangle being copied was too wide
and copyRect, not being very smart, was just wrapping off the edge of
the buffer onto the next row of pixels, but I couldn't find any reason
for that to happen since afaik all the callers check the Emacs window
size so should never ask to copy a too-wide rectangle.

-- 
Alan Third

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

[Aaron Jensen]

On Mon, May 1, 2023 at 1:26 PM Alan Third <alan@idiocy.org> wrote:
>
> This is one of the many reasons I walked away from the NS port. ;)

You are missed :)

> I'm 99% certain that [EmacsView copyRect:] is correct. I went over it
> numerous times. It's just doing a simple copy of one rectangle of a
> graphics buffer to another, identically sized, rectangle. It handles
> overlapping rectangles and there's really not very much logic in it to
> go wrong.
>
> Po Lu asks about how it synchronises with the system, and it doesn't
> really have to. The system draws into the same buffer that copyRect
> accesses. It does make me wonder if there's a rare occasion when
> copyRect is called and the system hasn't flushed all it's drawing
> instructions to the buffer yet. It's probably possible to force it to
> flush by doing something like:
>
>     [[NSGraphicsContext currentContext] flushGraphics];
>
> at the top of copyRect. Preferably only on macOS 10.14 and above, but
> it probably makes no difference on other platforms since scrollRect
> should force a flush anyway.
>
> My memory of Aaron's previous bug report about this was that it looked
> like, in some cases at least, the rectangle being copied was too wide
> and copyRect, not being very smart, was just wrapping off the edge of
> the buffer onto the next row of pixels, but I couldn't find any reason
> for that to happen since afaik all the callers check the Emacs window
> size so should never ask to copy a too-wide rectangle.

This one seems to be more about longer lines ghosting onto the tail
end of shorter lines. You could imagine that if you had a long line
and a short line, then scrolled such that the short line was where the
long line was but only repainted the area there were glyphs, the new
short line would include the tail end of the long line:

window line 2: xxxxxxxxxxxxxx
window line 3: yyyyyyyyyy
->
window 1: xxxxxxxxxxxxxx
window 2: yyyyyyyyyyxxxx

What do you think about what I mentioned later in the thread about:

[view setNeedsDisplayInRect:srcRect];

Why would that be srcRect and not destRect? Could that cause this somehow?

Thanks,

Aaron

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

[Alan Third]

On Mon, May 01, 2023 at 06:40:15PM -0400, Aaron Jensen wrote:
> On Mon, May 1, 2023 at 1:26 PM Alan Third <alan@idiocy.org> wrote:
> >
> > My memory of Aaron's previous bug report about this was that it looked
> > like, in some cases at least, the rectangle being copied was too wide
> > and copyRect, not being very smart, was just wrapping off the edge of
> > the buffer onto the next row of pixels, but I couldn't find any reason
> > for that to happen since afaik all the callers check the Emacs window
> > size so should never ask to copy a too-wide rectangle.
> 
> This one seems to be more about longer lines ghosting onto the tail
> end of shorter lines. You could imagine that if you had a long line
> and a short line, then scrolled such that the short line was where the
> long line was but only repainted the area there were glyphs, the new
> short line would include the tail end of the long line:
> 
> window line 2: xxxxxxxxxxxxxx
> window line 3: yyyyyyyyyy
> ->
> window 1: xxxxxxxxxxxxxx
> window 2: yyyyyyyyyyxxxx

Yeah, that's why I'm wondering if there's a timing thing where the
toolkit hasn't completed it's drawing before we copy the pixels. Emacs
might ask to clear the end of a line, then copy those "cleared" pixels
using copyRect before the initial clear has actually been completed.

This is just conjecture, though.

The other option is that it is copying too much and grabbing things it
shouldn't, but even though I think we've seen that before, I don't
know how it would happen.

Although, on reflection, your description sounds more like it's just
not clearing the end of lines correctly, which wouldn't necessarily
have anything to do with scrolling as such...

> What do you think about what I mentioned later in the thread about:
> 
> [view setNeedsDisplayInRect:srcRect];
> 
> Why would that be srcRect and not destRect? Could that cause this somehow?

I suspect that's a hang-over from the old rendering scheme where we
would have had to flag up that the source would need redrawn later.

scrollRect already marks the destination as needing updated on screen,
but iirc for whatever reason the source was never flagged as needing
redrawn, which it almost always did when scrolling.

The 10.14+ drawing scheme doesn't really require individual areas to
be marked as needing redrawn since the entire back buffer is sent to
the screen on update anyway. It could be worth replacing that with a
call to [NSView setNeedsDisplay:], or getting rid of it and making
copyRect do it instead. I don't know if it's even necessary since
I think Emacs will always do other drawing actions as part of a scroll
which will mark the view as needing sent to the screen.
-- 
Alan Third

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

[Eli Zaretskii]

> Date: Tue, 2 May 2023 11:14:56 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, Po Lu <luangruo@yahoo.com>,
> 	63187@debbugs.gnu.org
> 
> Although, on reflection, your description sounds more like it's just
> not clearing the end of lines correctly, which wouldn't necessarily
> have anything to do with scrolling as such...

Can that happen in this case?  You will see in the code that (AFAIU)
it copies a rectangle whose width is the total width of the window, so
why would (not) clearing ends of lines be an issue?

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

[Alan Third]

On Tue, May 02, 2023 at 03:21:46PM +0300, Eli Zaretskii wrote:
> > Date: Tue, 2 May 2023 11:14:56 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: Eli Zaretskii <eliz@gnu.org>, Po Lu <luangruo@yahoo.com>,
> > 	63187@debbugs.gnu.org
> > 
> > Although, on reflection, your description sounds more like it's just
> > not clearing the end of lines correctly, which wouldn't necessarily
> > have anything to do with scrolling as such...
> 
> Can that happen in this case?  You will see in the code that (AFAIU)
> it copies a rectangle whose width is the total width of the window, so
> why would (not) clearing ends of lines be an issue?

It depends where the actual problem is occurring. It's hard to tell
from the screenshots whether the rogue glyphs appear spontaneously in
the middle of the window, or as a new line is scrolled onto the
screen.

If it's the latter then obviously the procedure is to clear the line
that's to be drawn, then draw the new content, however if the clearing
isn't done correctly then you end up with the end of the line that was
there previously still visible to the right of the new content.

As it scrolls further, Emacs no doubt is clever enough to know that it
doesn't need to clear more than it drew for the last line, so the
rogue characters at the end stay for the subsequent line, and so on
until it hits a line that is long enough to wipe it all out.

I hope I'm explaining this well enough.

Either way, I don't know why it would be happening.
-- 
Alan Third

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

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Alan Third <alan@idiocy.org> writes:

> On Tue, May 02, 2023 at 03:21:46PM +0300, Eli Zaretskii wrote:
>> > Date: Tue, 2 May 2023 11:14:56 +0100
>> > From: Alan Third <alan@idiocy.org>
>> > Cc: Eli Zaretskii <eliz@gnu.org>, Po Lu <luangruo@yahoo.com>,
>> > 	63187@debbugs.gnu.org
>> > 
>> > Although, on reflection, your description sounds more like it's just
>> > not clearing the end of lines correctly, which wouldn't necessarily
>> > have anything to do with scrolling as such...
>> 
>> Can that happen in this case?  You will see in the code that (AFAIU)
>> it copies a rectangle whose width is the total width of the window, so
>> why would (not) clearing ends of lines be an issue?
>
> It depends where the actual problem is occurring. It's hard to tell
> from the screenshots whether the rogue glyphs appear spontaneously in
> the middle of the window, or as a new line is scrolled onto the
> screen.
>
> If it's the latter then obviously the procedure is to clear the line
> that's to be drawn, then draw the new content, however if the clearing
> isn't done correctly then you end up with the end of the line that was
> there previously still visible to the right of the new content.
>
> As it scrolls further, Emacs no doubt is clever enough to know that it
> doesn't need to clear more than it drew for the last line, so the
> rogue characters at the end stay for the subsequent line, and so on
> until it hits a line that is long enough to wipe it all out.
>
> I hope I'm explaining this well enough.
>
> Either way, I don't know why it would be happening.

Could we add some tracing (or assertions) to the NS port, to help
understand this problem better?  I've also seen some rogue characters
from time to time (not frequently, and my Emacs sessions last long).  I
think it all started since the migration to IOSurface, but I'm not sure.
I don't have clear steps to reproduce yet.

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

[Eli Zaretskii]

> Date: Tue, 2 May 2023 23:36:34 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: aaronjensen@gmail.com, luangruo@yahoo.com, 63187@debbugs.gnu.org
> 
> On Tue, May 02, 2023 at 03:21:46PM +0300, Eli Zaretskii wrote:
> > > Date: Tue, 2 May 2023 11:14:56 +0100
> > > From: Alan Third <alan@idiocy.org>
> > > Cc: Eli Zaretskii <eliz@gnu.org>, Po Lu <luangruo@yahoo.com>,
> > > 	63187@debbugs.gnu.org
> > > 
> > > Although, on reflection, your description sounds more like it's just
> > > not clearing the end of lines correctly, which wouldn't necessarily
> > > have anything to do with scrolling as such...
> > 
> > Can that happen in this case?  You will see in the code that (AFAIU)
> > it copies a rectangle whose width is the total width of the window, so
> > why would (not) clearing ends of lines be an issue?
> 
> It depends where the actual problem is occurring. It's hard to tell
> from the screenshots whether the rogue glyphs appear spontaneously in
> the middle of the window, or as a new line is scrolled onto the
> screen.

AFAIU, a new line scrolled into the screen is not handled by
scroll_run_hook.  Such a line is drawn anew, because there's no way we
have it in the glyph matrices and/or on the glass, by definition.

> As it scrolls further, Emacs no doubt is clever enough to know that it
> doesn't need to clear more than it drew for the last line, so the
> rogue characters at the end stay for the subsequent line, and so on
> until it hits a line that is long enough to wipe it all out.

I think you are describing something that happens only on TTY
terminals, if ever.  Emacs doesn't work that way on GUI terminals, and
if scroll_run_hook is indeed the culprit (that remains yet to be
proven, AFAIU), it is used in situations that are not necessarily
"scrolling" from the user POV.  This hook is run when Emacs determines
that some part of the display is exactly what is needed to be
displayed, but it is higher or lower on the screen than where it
should be.  Then (subject to some criteria of whether this
optimization is worth our while), we call this hook, which is supposed
to copy the pixels on the screen in the vertical direction, either up
or down.  It always copies the entire width of the window, so clearing
to EOL should not be the issue here, since the "cleared" pixels are
moved as well.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Alan Third <alan@idiocy.org> writes:

> Po Lu asks about how it synchronises with the system, and it doesn't
> really have to. The system draws into the same buffer that copyRect
> accesses. It does make me wonder if there's a rare occasion when
> copyRect is called and the system hasn't flushed all it's drawing
> instructions to the buffer yet. It's probably possible to force it to
> flush by doing something like:
>
>     [[NSGraphicsContext currentContext] flushGraphics];
>
> at the top of copyRect. Preferably only on macOS 10.14 and above, but
> it probably makes no difference on other platforms since scrollRect
> should force a flush anyway.

Hmm... Aaron, could you try that?

BTW, I've just noticed that ns_scroll_run invalidates the source
rectangle, not the destination.  Shouldn't things be the other way
around?  I see Aaron saw that as well.

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

[Aaron Jensen]

On Mon, May 1, 2023 at 8:07 PM Po Lu <luangruo@yahoo.com> wrote:
>
> Alan Third <alan@idiocy.org> writes:
>
> > Po Lu asks about how it synchronises with the system, and it doesn't
> > really have to. The system draws into the same buffer that copyRect
> > accesses. It does make me wonder if there's a rare occasion when
> > copyRect is called and the system hasn't flushed all it's drawing
> > instructions to the buffer yet. It's probably possible to force it to
> > flush by doing something like:
> >
> >     [[NSGraphicsContext currentContext] flushGraphics];
> >
> > at the top of copyRect. Preferably only on macOS 10.14 and above, but
> > it probably makes no difference on other platforms since scrollRect
> > should force a flush anyway.
>
> Hmm... Aaron, could you try that?

I'll try it after I try running w/ the invalidate change. I'll report
back if that doesn't fix it and try the flush.

Aaron