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

[Alan Third <alan@idiocy.org>]

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