Tk Source Code

View Ticket
Login
Ticket UUID: 685ac3072790118c07db73c1cb667e8bd37bddf8
Title: Aqua: XCopyArea() broken for non-pixmap source (as used by Tktable)
Type: Bug Version: 8.6.10
Submitter: chrstphrchvz Created on: 2020-04-08 08:32:55
Subsystem: 82. X11 Emulation Assigned To: nobody
Priority: 5 Medium Severity: Minor
Status: Open Last Modified: 2022-09-01 07:58:44
Resolution: None Closed By: nobody
    Closed on:
Description:

Tktable bug report from 2010: https://sourceforge.net/p/tktable/bugs/309/

In Tktable, a cell's text "overflows" when the text is wider or taller than the cell. Under Tk Cocoa, any cells with overflowing text will have their text erroneously appear behind the text of every later cell (i.e. a cell in a later column in the same row, or a cell in a later row) with overflowing text, unless -drawmode slow is used:


The problem is more clearly seen by overflowing cells with whitespace and/or non-overlapping text:

Examining the drawing code in TableDisplay() (tkTable.c), there is almost no platform-specific code. The clipWind pixmap is used when cells have overflowing text: the background and contents already drawn to the cell are copied into clipWind using XCopyArea(), the text is then drawn into clipWind, and then only the text-containing portion of the cell is copied from clipWind into window using XCopyArea(). The assumption is that existing contents of clipWind are overwritten by the first XCopyArea() for each cell that uses it, but this fails on Aqua, except for -drawmode slow, where window is a pixmap rather than the actual window (the pixmap gets copied to the actual window with XCopyArea() after the entire table is drawn into it).

So this would indicate XCopyArea() is broken on Aqua, at least when the source is not a pixmap, which seems to likely be the case. When the source of an XCopyArea() operation is an actual window rather than a pixmap, it relies on the known-broken TkMacOSXBitmapRepFromDrawableRect(). (A comment was added above this function not too long ago saying it is only used by the unused XGetImage(), but that appears to not be the only remaining use.)

As I understand it, TkMacOSXBitmapRepFromDrawableRect() is broken in the sense that there does not appear to be any way to read from a Mac window's "backing store" and get a bitmap or other representation of what has already been drawn so far. The available methods cacheDisplayInRect:toBitmapImageRep:, dataWithPDFInsideRect:, etc. all involve entering drawRect: and drawing window contents from scratch. Though it seems that none of these methods are intended to be called from drawRect:. The acknowledged limitation is that drawRect: can no longer be entered recursively. But even if it were possible to recurse into drawRect:, it would never yield what we want; drawing the table from scratch would only result in TableDisplay()XCopyArea()TkMacOSXBitmapRepFromDrawableRect()drawRect: being called all over again.

Probably the only case where XCopyArea() could work with a window as source is if the destination is a window in the same NSView, and then use the deprecated scrollRect:by: method. (Were someone able to solve the above problem of not being able to retrieve already-drawn contents from the backing store, they could also implement a scrollRect:by: replacement.)

Or, have Tk draw everything into an image (which Tk is free to read from), and then setting that image as what is drawn into the backing store, or directly used instead of a backing store. This seems possible (CALayer/layer-backed view using NSImage/CGImageRef?), but I don't know how radical of a change it is and all the disadvantages it has (e.g. loss of GPU accelerated functions) or what it may limit Tk from doing in the future. Though I wonder if it would better accommodate Tk's somewhat eager/Aqua-unaware drawing behavior (see my comments on [06d8246baf]).

Otherwise, this issue is in some sense unfixable; XCopyArea() and anything using it (e.g. XCopyPlane()) is undefined on Aqua when the source is an actual window.

Is it important for Tk Aqua to support this and similar X11 emulation functions, or is it not worth the effort?

Tktable has been dormant for several years. But it seems possible for it to implement a workaround; I will post some ideas to the Tktable bug report.

User Comments: chrstphrchvz added on 2022-09-01 07:58:44:

The NSImage drawing approach I discussed here is reborn as the CGBitmapContext/CGImage approach proposed in [3e9e82bcfe4b]; part of it is to get XCopyArea() and XGetImage() working again.


chrstphrchvz added on 2021-04-12 04:45:42:

CFRelease(bitmapRep) should probably have instead been [bitmapRep release] since bitmapRep is a kind of NSObject *.


chrstphrchvz added on 2021-04-11 03:07:45:

Without seeing a stack trace for the crash reported on tcl-mac, here's why I think it happens:

image create photo -type window relies on XGetImage(), which in Tk Aqua in turn relies on CreateCGImageFromDrawableRect() (tkMacOSXImage.c).

This line in CreateCGImageFromDrawableRect():

    } else if (TkMacOSXGetNSViewForDrawable(mac_drawable) != nil) {

used to be (when it was in TkMacOSXBitmapRepFromDrawableRect() ()):

    } else if ((view = TkMacOSXDrawableView(mac_drawable)) != NULL) {

but in [b3409e6717] became (I believe mistakenly):

    } else if (TkMacOSXDrawableView(mac_drawable) != NULL) {
This currently leaves view to always be nil even when TkMacOSXGetNSViewForDrawable(mac_drawable) != nil. And since any image create photo -type window usage more than likely happens in the normal Tcl event loop and not while in drawRect:, [NSView focusView] == nil. Therefore view == [NSView focusView] and we end up doing [view cacheDisplayInRect:view_rect toBitmapImageRep:bitmapRep] which just sets bitmapRep to NULL, causing CFRelease(bitmapRep) to crash ("illegal hardware instruction").

In 0003-Aqua-Restore-initWithFocusedViewRect-usage.patch I restore the assignment to view:

    } else if ((view = TkMacOSXGetNSViewForDrawable(mac_drawable)) != nil) {

This change alone could be applied right away: it will not resolve the larger issue of not being able to capture windows, but will at least error ("Window … cannot be transformed into a pixmap (possibly obscured?)") rather than crash.


chrstphrchvz added on 2021-04-11 01:59:29:

The issue of no longer being able to capture a window on Aqua using TkImg has been reported by another user on the tcl-mac mailing list: https://sourceforge.net/p/tcl/mailman/tcl-mac/thread/c3e00748-e0d6-fc08-ce85-6160de94ddb8%40mastersoft.com/#msg37258979


chrstphrchvz added on 2020-11-15 11:26:31:

A better solution was found for the Tktable issue in the description (see [b505e5f6a9]), although the issues with Tk Aqua's XCopyArea() and XGetImage() remain.

I have attached refreshed patches for reinstating initWithFocusedViewRect:, which apply to the latest core-8-6-branch ([cfb1c35782]). Here is a demo script for testing screenshot of a widget with TkImg:

package require Tk
package require Img

. configure -width 200 -height 100 -background red ttk::button .b -text "Capture this button" -command {do_capture .b} place .b -x 20 -y 30

proc {do_capture} {w} { set capture_photo [image create photo -format window -data $w] toplevel .t$capture_photo pack [label .t$capture_photo.l -image $capture_photo] }

Running this and pressing "Capture this button" should open a new toplevel with the screenshot of the button when it was clicked; the red background surrounding the button should not leak into the screenshot. Someone with both a Retina and a non-Retina display should be able to check that the demo works even after moving the root toplevel between displays. I have only had success with this demo on macOS 10.13, and not on (modified) 10.15, which may indicate that initWithFocusedViewRect: no longer works as of 10.14. But this implementation should be better than the current one, which doesn't work on any macOS version.

From Marc's comment on [fcd6717d8b] (2020-08-23 02:40:15):

I probably misread the code, but it looked like the Retinaciousness was being stored in a global variable, so it would not be sensitive to a window being moved from one display to another. More importantly, though, what do you see as the issue with simply asking an NSWindow for its backingStoreScaleFactor whenever that is relevant?

I believe this is referring to the tkMacOSXNSWindowHasBackingScaleFactor global variable, which only indicates whether the backingScaleFactor of NSWindows can be accessed (which is only possible on macOS 10.7 and later). The backingScaleFactor of an NSWindow is being asked for when needed, using the TkMacOSXNSWindowBackingScaleFactor() wrapper function (although if running macOS 10.6, the function will always return 1.0); the backingScaleFactor is not then cached elsewhere by Tk.


chrstphrchvz added on 2020-08-18 02:25:48:

In response to my comment from 2020-05-24 23:23:57: only calling initWithFocusedViewRect: while in drawRect: will only work for usage that can wait until inside drawRect: before calling it, which is the case for Tktable. But lockFocus and unlockFocus are still necessary if something needs to retrieve from the backing store while outside of a drawRect: call for the drawable's view, such as capturing a window with TkImg (it cannot wait until or try again when in drawRect:).


chrstphrchvz added on 2020-08-18 02:05:51:

I have attached patches based on what I currently had in the iwfvr-no-cdir branch. Patches #3 and #6 are the important ones; the rest are comment tweaks and optional refactoring.


chrstphrchvz added on 2020-08-15 02:02:34:

In case I don't get around to attaching patches soon, please have a look at my work so far on a branch on my GitHub fork: https://github.com/chrstphrchvz/tk/commits/iwfvr-no-cdir

One set of bugs I would like to simultaneously address is the instances where the imageBounds argument for TkMacOSXDrawCGImage() is invalid: it is being initialized with (MacDrawable *)srcDraw->size which is NSZeroSize for non-pixmaps. This leads to TkMacOSXDrawCGImage() complaining about "Mismatch of sub CGImage bounds." Although there may not be anything broken in TkMacOSXDrawCGImage(), one suggestion I have would be to simplify it so that it doesn't even accept imageBounds nor srcBounds. I would leave TkMacOSXDrawCGImage() to mainly be a wrapper around CGContextDrawImage(), meaning that any cropping of the source image would need to be done before calling it, and the image will be implicitly rescaled by CGContextDrawImage() if its size does not equal dstBounds.size. Only XPutImage() would need cropping logic added (using CGImageCreateWithImageInRect()); other instances can/already use TkMacOSXBitmapRepFromDrawableRect() to crop the source.

To separate out and reuse some refactoring from the NSImageCALayer drawing experiment, I also propose renaming TkMacOSXBitmapRepFromDrawableRect() to TkMacOSXCreateCGImageFromDrawableRect() and having it return a CGImageRef instead.


marc_culler (claiming to be Marc Culler) added on 2020-08-14 15:45:33:
A patch would be great!

chrstphrchvz added on 2020-08-12 22:07:00:

I made a separate installation of macOS 10.13 High Sierra (the last version officially supporting my 2010 MBP), and using initWithFocusedViewRect: on it appears to work without the crashes or glitches I observed on 10.15.

I think initWithFocusedViewRect: usage can be reinstated at least for 10.13 and earlier. Maybe it can be safely used on supported Macs running 10.14 and later, if someone else can verify (and if Tk would rather not workaround issues due to running macOS on unsupported Macs). At a minimum, I would still like to remove cacheDisplayInRect: usage, as that is algorithmically inappropriate for Tk, and deliberately allow nil to be returned instead. I should be able to provide patches for this.

As I did not change very much to get initWithFocusedViewRect: working, I'm prompted to think I may have incorrectly assumed that this issue is the same one reported for Tktable 10 years ago. There may instead be two issues: a new one introduced as recently as Tk 8.6.9.1 (in response to initWithFocusedViewRect: being deprecated on 10.14 Mojave); and a 10-year-old one whose steps to reproduce and workaround (-drawmode slow) coincidentally apply to the new issue, and which may have actually been addressed by Tk at some point (without anyone following up on the Tktable ticket). All the reports of the new issue that I'm aware of are with Tk 8.9.6.1 or later; and I never confirmed exactly what the old issue looked like—the reporter of the SourceForge ticket never provided a screenshot of the issue, and the Tktable maintainer only described it as "something about the cocoa text drawing is defying the general clipping routines", which isn't quite how I would describe the new issue.


chrstphrchvz added on 2020-08-09 18:50:23:

I have pushed my work so far on NSImageCALayer drawing to a branch on GitHub (careful, commits may get rebased and disappear): https://github.com/chrstphrchvz/tk/commits/calayer-drawing

Overview of the NSImageCALayer drawing approach so far:

An NSImage is created with the size of the toplevel's TKContentView instance; for convenience a reference to it is stored in the tkLayerImage property of the TKContentView. updateLayer and wantsUpdateLayer are overridden for TKContentView: updateLayer is where tkLayerImage is used to set the contents of the TKContentView's layer; and wantsUpdateLayer returns YES so that updateLayer is called instead of drawRect:. setWantsLayer:YES is called for each TKContentView instance.

TkMacOSXSetupDrawingContext() and TkMacOSXRestoreDrawingContext() are modified to call lockFocus and unlockFocus respectively on tkLayerImage. This means that widgets should be able to draw into tkLayerImage immediately rather than only during deferred drawing as with drawRect:. TkMacOSXRestoreDrawingContext() calls addTkDirtyRect: for the bounds of the TKContentView instance so that updateLayer is eventually called. Unlike drawRect:, no widget redrawing routines execute during updateLayer. updateLayer calls clearTkDirtyRect when it finishes.

TkMacOSXBitmapRepFromDrawableRect() (renamed to TkMacOSXCreateCGImageFromDrawableRect()) is modified to take advantage of being able to retrieve prior drawing from tkLayerImage as a CGImage, which is then usable by functions including XGetImage() and XCopyArea().

Tweaks were made to other functions including TkMacOSXDrawCGImage(), which may be useful outside of this experimental branch for addressing existing bugs.

Here are Retina display screenshots of overflowing Tktable text without -drawmode slow, and a button widget captured using TkImg and then displayed in a separate toplevel:

A few issues I'm aware of:

  • Everything runs much slower; things that used to take a split second may now take several seconds.
  • Widgets are often not drawn when toplevels are first mapped, and only appear e.g. once hovered over.
  • Artifacts of widgets drawn in an intermediate location may appear when toplevels are resized.
  • There is a delay hovering/un-hovering over links in widget demo. This delay seems to increase with the size of the toplevel.
  • There appear to be problems with calling update and/or uplevel 1 (not returning control to topmost Tcl loop); e.g. clicking a widget demo link does nothing, and then closing Tk causes a bunch of errors to print regarding the application having been closed.
  • Tk will crash if this warning appears:
    It does not make sense to draw an image when [NSGraphicsContext currentContext] is nil. This is a programming error. Break on void _NSWarnForDrawingImageWithNoCurrentContext(void) to debug. This will be logged only once. This may break in the future.
    But the warning occurs during the call to [image lockFocus], which should be setting the current graphics context.
  • It might be a good idea to tell everything to redraw when the backingScaleFactor changes.


chrstphrchvz added on 2020-08-02 19:16:10:

I tried switching Tk back to using initWithFocusedViewRect:, and I'm not sure it can work. On my MacBook Pro 2010 running (modified) macOS 10.15.6, calling this method enough times eventually triggers a crash in ERROR_CGBlt_copyBytes_BufferIsNotReadable or ERROR_CGBlt_copyBytes_BufferIsNotBigEnough. I haven't tried seeing what happens on another machine/graphics card, so I can't rule out that these errors are due to using graphics card drivers taken from an older macOS version and modified for macOS 10.15 (which doesn't support this Mac or its graphics card).

I am going down the rabbit hole of getting Tk to draw to an NSImage (or possibly some other image type) which then provides a CALayer to use as an NSView's contents, rather than drawing directly into an NSView. Having access to an image should allow doing things either issue-prone or not possible with an opaque backing store. This also involves eliminating scrollRect:by: usage. Besides this Tktable example, I am also trying to use TkImg to get an image of a window, which involves getting the XGetImage() and TkMacOSXBitmapRepFromDrawableRect() functions to finally work for real windows (i.e. not just pixmaps). This experiment is revealing quite a few other smaller bugs along the way. For now the goal of this experiment is correctness rather than performance; so far this approach runs significantly slower. Some of the functionality may only exist in macOS versions more recent than 10.6.


chrstphrchvz added on 2020-05-25 14:04:14:

I still have yet to try anything, but there are reports of initWithFocusedViewRect: no longer working when linking to 10.14+ SDK.


marc_culler (claiming to be Marc Culler) added on 2020-05-25 00:05:24:
That sounds like an excellent idea, if it works.  There might be some
unanticipated problem, but it is definitely worth testing.  Apple's
minimalist documentation does not give one much to go on, so trial and
error is really the only option as far as I can tell.

The nature of the backing store changed with the Mojave release, according
to Apple.  But is not clear to me what the implications of that may be.

chrstphrchvz added on 2020-05-24 23:23:57:

If Tk does go back to using initWithFocusedViewRect:, I wonder if it should just check for [NSApp isDrawing] rather than doing [view lockFocusIfCanDraw]/[view unlockFocus], since those are presumably already done by whichever display method that calls drawRect: (and are also deprecated).


marc_culler (claiming to be Marc Culler) added on 2020-05-24 23:05:29:
I agree that switching back to initWithFocusedViewRect: would be a good idea,
assuming that it actually works.

At the time it seemed like a good goal to avoid all deprecated APIs, especially
when a replacement is provided.  But the replacement did not work.  And now
we are using other deprecated apis ([view scrollRect:by:]) which do not have
any replacement.  In the case of [view scrollRect:by:] no deprecation warning
is issued when building with target system 10.9, which is a common choice
these days.  So it would seem that Apple is likely to keep that api available
at least until they decide to completely abandon 10.9.  Probably the situation
with initWithFocusedViewRect is similar.

chrstphrchvz added on 2020-05-24 20:21:08:

Apparently, there has been a way to read from a Mac window's backing store without calling drawRect: again: the now-deprecated initWithFocusedViewRect:. I wonder if Tk shouldn't have switched away from using that method just because it was deprecated (even though this issue was present when that was still used).

I'm under the impression that Apple has been deprecating APIs not to signal that they intend to remove them (unless they explicitly state so), but rather because Apple found too many programs out in the wild which they believed were using APIs incorrectly or should instead use some strongly-preferred (and possibly more performant) alternative—in, say, 99.9% of programs. But the strongly-suggested alternative may neither be drop-in replacements for deprecated APIs, nor even provide the functionality that the remaining 0.1% of programs truly require.

I think Tk has good reason to keep using the initWithFocusedViewRect: since cacheDisplayInRect: definitely does not do what Tk wants. I think scrollRect:by: was likely deprecated for the same reason as initWithFocusedViewRect:: not because Apple wants to remove scrollRect:by:, but because 99.9% of programs out there would be better off using NSScrollView—even though it would be inappropriate for the remaining 0.1% of programs, including Tk (which insists on the strict separation between scrolled widgets and scrollbars into two distinct widgets, and exposes functionality currently expected by Tk programs but would likely be impossible to provide using NSScrollView).

Besides initWithFocusedViewRect: being deprecated, are there any other reasons to not go back to using it, or why cacheDisplayInRect: should still be used instead?


chrstphrchvz added on 2020-04-13 15:21:25:

…using drawmode -slow causes "fuzzy text"

I have opened [e2e9ce70b2] regarding this.


chrstphrchvz added on 2020-04-11 08:36:13:

I used the basic.tcl demo for the screenshots. I either shrink rows/columns or add text in a cell until the text overflows.

The Tktable man page says -drawmode slow "is slow for larger tables", but that was likely what was observed a long time ago on X11, perhaps more so if using X forwarding. On Aqua, I haven't observed -drawmode slow to be at all slower (maybe the whole table is always being redrawn either way); some things that do noticeably impact performance are the total number of visible cells and amount of text they contain (and not necessarily the visible size of the table; better illustrated by the debug.tcl or maxsize.tcl demos and zooming the window).

The -drawmode slow workaround has been known since 2010, but who knows how well users are aware of it (possibly only those who encountered the bug and then either found a previous report of it mentioning the workaround, or reported it again and had the workaround shared with them). However a more recent reporter of this bug says that using drawmode -slow causes "fuzzy text", which I have not observed.


marc_culler (claiming to be Marc Culler) added on 2020-04-11 00:27:50:
The first question that occurs to me is whether -drawmode slow is *actually*
slow, or is that just a name that was chosen for the option.  If it is as fast,
or only a tolerable amount slower, then the obvious workaround would be to
always use that mode when running in Aqua.

If I were to look at this it would be helpful to have an easy (for me) way to
produce an example of the problem.  Could you perhaps post the script that was
used to generate the images in this ticket?

Attachments: