From 4dc3896752241befcb29a3ff1ea82297fc4b7f9d Mon Sep 17 00:00:00 2001 From: Patrick Luby Date: Sun, 24 Nov 2024 20:25:59 -0500 Subject: Improve rendering speed for native controls when using Skia/Metal While debugging tdf#163945, Xcode's Instruments application uncovered the following performance bottlenecks when using Skia/Metal: 1. Very slow rendering an NSBox: Many system colors have the NSColorTypeCatalog color type. For some unkown reason, setting the NSBox's fill color to a color set to NSColorTypeCatalog causes drawing to take at least twice as long as when the fill color is set to NSColorTypeComponentBased. So, only draw with a fill color set to NSColorTypeComponentBased. 2. Excessively large offscreen buffers when drawing native controls: The temporary bitmap was set to the control region expanded by 50 * mScaling (e.g. both width and height were increased by 200 pixels when running on a Retina display). This caused temporary bitmaps to be up to several times larger than needed. Also, drawing NSBox objects to a CGBitmapContext is noticeably slow so filling all that unneeded temporary bitmap area can slow down performance when a large number of NSBox objects like the status bar are redrawn in quick succession. Using getNativeControlRegion() isn't perfect, but it does try to account for the focus ring as well as the minimum width and/or height of the native control so union the two regions set by getNativeControlRegion() and add double the focus ring width on each side just to be safe. In most cases, this should ensure that the temporary bitmap is large enough to draw the entire native control and a focus ring. 3. Unncessary copying of bitmap buffer when drawing native controls: Let Skia own the CGBitmapContext's buffer so that an SkImage can be created without Skia making a copy of the buffer. Change-Id: Ibd3abb4b9d7045c47268319772fe97a5c4dba3c6 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/177225 Tested-by: Jenkins Reviewed-by: Patrick Luby (cherry picked from commit 0807fe362819629259a9a80a48d2826e588d909c) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/177291 Reviewed-by: Adolfo Jayme Barrientos --- vcl/inc/quartz/salgdi.h | 1 + vcl/osx/salnativewidgets.cxx | 27 +++++++++++++--- vcl/skia/osx/gdiimpl.cxx | 74 +++++++++++++++++++++++++------------------- 3 files changed, 66 insertions(+), 36 deletions(-) diff --git a/vcl/inc/quartz/salgdi.h b/vcl/inc/quartz/salgdi.h index b7fb674f7553..003bf077510f 100644 --- a/vcl/inc/quartz/salgdi.h +++ b/vcl/inc/quartz/salgdi.h @@ -446,6 +446,7 @@ protected: virtual bool drawNativeControl( ControlType nType, ControlPart nPart, const tools::Rectangle& rControlRegion, ControlState nState, const ImplControlValue& aValue, const OUString& aCaption, const Color& rBackgroundColor ) override; +public: virtual bool getNativeControlRegion( ControlType nType, ControlPart nPart, const tools::Rectangle& rControlRegion, ControlState nState, const ImplControlValue& aValue, const OUString& aCaption, tools::Rectangle &rNativeBoundingRegion, tools::Rectangle &rNativeContentRegion ) override; diff --git a/vcl/osx/salnativewidgets.cxx b/vcl/osx/salnativewidgets.cxx index 8a7e81fd5d86..3340722281e8 100644 --- a/vcl/osx/salnativewidgets.cxx +++ b/vcl/osx/salnativewidgets.cxx @@ -380,6 +380,8 @@ bool AquaGraphicsBackend::drawNativeControl(ControlType nType, static void drawBox(CGContextRef context, const NSRect& rc, NSColor* pColor) { + assert(pColor); + CGContextSaveGState(context); CGContextTranslateCTM(context, rc.origin.x, rc.origin.y + rc.size.height); CGContextScaleCTM(context, 1, -1); @@ -388,12 +390,27 @@ static void drawBox(CGContextRef context, const NSRect& rc, NSColor* pColor) NSRect rect = { NSZeroPoint, NSMakeSize(rc.size.width, rc.size.height) }; NSBox* pBox = [[NSBox alloc] initWithFrame: rect]; - [pBox setBoxType: NSBoxCustom]; - [pBox setFillColor: pColor]; - SAL_WNODEPRECATED_DECLARATIONS_PUSH // setBorderType first deprecated in macOS 10.15 - [pBox setBorderType: NSNoBorder]; - SAL_WNODEPRECATED_DECLARATIONS_POP + + // Related tdf#163945: Set fill color to NSColorTypeComponentBased color type + // Many system colors have the NSColorTypeCatalog color type. For + // some unkown reason, setting the NSBox's fill color to a color set + // to NSColorTypeCatalog causes drawing to take at least twice as long + // as when the fill color is set to NSColorTypeComponentBased. So, + // only draw with a fill color set to NSColorTypeComponentBased. + NSColor* pRGBColor = pColor; + if ([pColor type] != NSColorTypeComponentBased) + { + pRGBColor = [pColor colorUsingType: NSColorTypeComponentBased]; + if (!pRGBColor) + pRGBColor = pColor; + } + [pBox setFillColor: pRGBColor]; + + // -[NSBox setBorderType: NSNoBorder] is deprecated so hide the border + // by setting the border color to transparent + [pBox setBorderColor: [NSColor clearColor]]; + [pBox setTitlePosition: NSNoTitle]; [pBox displayRectIgnoringOpacity: rect inContext: graphicsContext]; diff --git a/vcl/skia/osx/gdiimpl.cxx b/vcl/skia/osx/gdiimpl.cxx index ffe1ebc42d65..e74db4ec5475 100644 --- a/vcl/skia/osx/gdiimpl.cxx +++ b/vcl/skia/osx/gdiimpl.cxx @@ -27,6 +27,7 @@ #include #include +#include #include #include @@ -254,28 +255,51 @@ bool AquaSkiaSalGraphicsImpl::drawNativeControl(ControlType nType, ControlPart n return false; // rControlRegion is not the whole area that the control should be painted to (e.g. highlight - // around focused lineedit is outside of it). Since we draw to a temporary bitmap, we need tofind out - // the real size. Using getNativeControlRegion() might seem like the function to call, but we need - // the other direction - what is called rControlRegion here is rNativeContentRegion in that function - // what's called rControlRegion there is what we need here. Moreover getNativeControlRegion() - // in some cases returns a fixed size that does not depend on its input, so we have no way to - // actually find out what the original size was (or maybe the function is kind of broken, I don't know). - // So, add a generous margin and hope it's enough. + // around focused lineedit is outside of it). Since we draw to a temporary bitmap, we need to find out + // the real size. + // Related tdf#163945 Reduce the size of the temporary bitmap + // Previously, the temporary bitmap was set to the control region + // expanded by 50 * mScaling (e.g. both width and height were + // increased by 200 pixels when running on a Retina display). This + // caused temporary bitmaps to be up to several times larger than + // needed. Also, drawing NSBox objects to a CGBitmapContext is + // noticeably slow so filling all that unneeded temporary bitmap + // area can slow down performance when a large number of NSBox + // objects like the status bar are redrawn in quick succession. + // Using getNativeControlRegion() isn't perfect, but it does try to + // account for the focus ring as well as the minimum width and/or + // height of the native control so union the two regions set by + // getNativeControlRegion() and add double the focus ring width on + // each side just to be safe. In most cases, this should ensure + // that the temporary bitmap is large enough to draw the entire + // native control and a focus ring. tools::Rectangle boundingRegion(rControlRegion); - boundingRegion.expand(50 * mScaling); + tools::Rectangle rNativeBoundingRegion; + tools::Rectangle rNativeContentRegion; + AquaSalGraphics* pGraphics = mrShared.mpFrame->mpGraphics; + if (pGraphics + && pGraphics->getNativeControlRegion(nType, nPart, rControlRegion, nState, aValue, + OUString(), rNativeBoundingRegion, + rNativeContentRegion)) + { + boundingRegion.Union(rNativeBoundingRegion); + boundingRegion.Union(rNativeContentRegion); + } + boundingRegion.expand(2 * FOCUS_RING_WIDTH * mScaling); + // Do a scaled bitmap in HiDPI in order not to lose precision. const tools::Long width = boundingRegion.GetWidth() * mScaling; const tools::Long height = boundingRegion.GetHeight() * mScaling; const size_t bytes = width * height * 4; - sal_uInt8* data = new sal_uInt8[bytes]; - memset(data, 0, bytes); + // Let Skia own the CGBitmapContext's buffer so that an SkImage + // can be created without Skia making a copy of the buffer + sk_sp data = SkData::MakeZeroInitialized(bytes); CGContextRef context = CGBitmapContextCreate( - data, width, height, 8, width * 4, GetSalData()->mxRGBSpace, + data->writable_data(), width, height, 8, width * 4, GetSalData()->mxRGBSpace, SkiaToCGBitmapType(mSurface->imageInfo().colorType(), kPremul_SkAlphaType)); if (!context) { SAL_WARN("vcl.skia", "drawNativeControl(): Failed to allocate bitmap context"); - delete[] data; return false; } // Setup context state for drawing (performDrawNativeControl() e.g. fills background in some cases). @@ -307,14 +331,6 @@ bool AquaSkiaSalGraphicsImpl::drawNativeControl(ControlType nType, ControlPart n CGContextRelease(context); if (bOK) { - // Let SkBitmap determine when it is safe to delete the pixel buffer - SkBitmap bitmap; - if (!bitmap.installPixels(SkImageInfo::Make(width, height, - mSurface->imageInfo().colorType(), - kPremul_SkAlphaType), - data, width * 4, nullptr, nullptr)) - abort(); - preDraw(); SAL_INFO("vcl.skia.trace", "drawnativecontrol(" << this << "): " << rControlRegion << ":" << int(nType) << "/" << int(nPart)); @@ -327,22 +343,18 @@ bool AquaSkiaSalGraphicsImpl::drawNativeControl(ControlType nType, ControlPart n updateRect.GetWidth(), updateRect.GetHeight())); SkRect drawRect = SkRect::MakeXYWH(boundingRegion.getX(), boundingRegion.getY(), boundingRegion.GetWidth(), boundingRegion.GetHeight()); - assert(drawRect.width() * mScaling == bitmap.width()); // no scaling should be needed - getDrawCanvas()->drawImageRect(bitmap.asImage(), drawRect, SkSamplingOptions()); + sk_sp image = SkImages::RasterFromData( + SkImageInfo::Make(width, height, mSurface->imageInfo().colorType(), + kPremul_SkAlphaType), + data, width * 4); + assert(image + && drawRect.width() * mScaling == image->width()); // no scaling should be needed + getDrawCanvas()->drawImageRect(image, drawRect, SkSamplingOptions()); // Related: tdf#156881 flush the canvas after drawing the pixel buffer getDrawCanvas()->flush(); ++pendingOperationsToFlush; // tdf#136369 postDraw(); } - // Related: tdf#159529 eliminate possible memory leak - // Despite confirming that the release function passed to - // SkBitmap.bitmap.installPixels() does get called for every - // data array that has been allocated, Apple's Instruments - // indicates that the data is leaking. While it is likely a - // false positive, it makes leak analysis difficult so leave - // the bitmap mutable. That causes SkBitmap.asImage() to make - // a copy of the data and the data can be safely deleted here. - delete[] data; return bOK; } -- cgit