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