Michael Ludwig

[text] Introduce PackedGPUGlyphID to add more metadata to SkPackedGlyphID Both Ganesh and Graphite share a lot of common structure to their glyph handling code, so the changes outlined below are applied pretty similarly to both codebases. 1. Adds a new shared type PackedGPUGlyphID that wraps SkPackedGlyphID and packs into the free bits the rest of the information that atlas-backed glyphs require, which is the mask format, the amount of padding, and whether or not the data is a coverage or distance value. 2. Pulls back the presence of MaskFormat from the GlyphVector concepts and functions as it's now handled internally by each backend's GlyphData classes and embedded into the packed GPU IDs that they make. 3. Each backend's TextStrike implementation stores PackedGPUGlyphIDs as the keys to GlyphEntries instead of SkPackedGlyphIDs. This ensures that an atlas will only have a cache hit if all of the mask/padding/data-type properties are consistent with what was in the atlas and what is requested by the subrun being drawn. 4. Each backend's GlyphData implementation takes in these additional properties in its constructor (propagating to initBackendData() calls). It then resolves the mask format and padding with the configuration of the backend's atlas manager so that all subsequent PackedGPUGlyphIDs that it makes represent the final configuration. 5. The atlas manager's now require the format and padding to be pre-resolved in many of their functions, such that 565 has been lifted to RGBA8 and 1px of padding is added for direct masks if the caps require all direct masks to have padding (this automatically makes transformed mask subrun glyphs and direct mask subrun glyphs key the same when fSupportBilerpAtlas is true). Backend-specific changes for Graphite: The properties that GlyphData requires for filling out the PackedGPUGlyphID are basically the `sktext::gpu::RendererData` that was being stored in the SubRunData object. I added the srcPadding to it and then made it accessible from GlyphData, so it could be removed from SubRunData. This keeps the number of arguments to the various functions fairly concise. As part of this, RendererData moved from SubRunContainer.h to GlyphVector.h Backend-specific changes for Ganesh: Ganesh didn't use RendererData, so its GlyphData just takes the parameters in directly and from the AtlasTextOp. Its GlyphData class also had an unused declared constructor that I removed, and GrAtlasManager::addToAtlas() could be made private (which made enforcing resolvedMaskFormat easier). Bug: 514078656 Change-Id: I07977910f64d84e15fe6e4c687d8635afb356b7c Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1292556 Reviewed-by: Alexis Cruz-Ayala <alexisdavidc@google.com> Reviewed-by: Robert Phillips <robertphillips@google.com> Commit-Queue: Michael Ludwig <michaelludwig@google.com>

Trace detailed ResourceCache summaries for Ganesh and Graphite Adds Perfettoe counter tracing for GPU memory in Ganesh and Graphite, updating counters roughly once each frame based on stats that were already collected in the ResourceCaches. For Ganesh, this adds a fWrappedBytes total to help get an idea of the amount of wrapped memory that is reported. However, there is no good way to track the memory represented by all BackendTexture objects. For Graphite, a new ThreadSafeResourceStats helper is added to the SharedContext so that all ResourceCaches and Providers can push updates to the fields. This will go away when there's a single cache, but for now it combines everything into a single set of counters. Other than handling wrapped vs. backend textures, the traced counters are meant to be compatible between ganesh and graphite, storing the total amount of GPU memory, the budgeted (non-purgeable), purgable (which is always budgeted), and the unbudgeted (but not wrapped) bytes. Bug: 535175078 Change-Id: I6e5d82bcff9fe49b0183eec751cf61051a7dd821 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1292496 Reviewed-by: Robert Phillips <robertphillips@google.com> Commit-Queue: Michael Ludwig <michaelludwig@google.com>

Reland "[text] Introduce PackedGPUGlyphID to add more metadata to SkPackedGlyphID" This reverts commit 48b58ee222f14b2b14a08c2e8574fae8256b26e0. Reason for revert: Fixing GlyphData max size Original change's description: > Revert "[text] Introduce PackedGPUGlyphID to add more metadata to SkPackedGlyphID" > > This reverts commit f82e81ea65ed4cd9549f9416e2cc4e983eada195. > > Reason for revert: Breaking tree, and Michael (CL author) is not here to discuss path forward > > Failure Link: https://ci.chromium.org/raw/build/logs.chromium.org/skia/79e459c81677dc11/+/annotations > Original change's description: > > [text] Introduce PackedGPUGlyphID to add more metadata to SkPackedGlyphID > > > > Both Ganesh and Graphite share a lot of common structure to their glyph > > handling code, so the changes outlined below are applied pretty > > similarly to both codebases. > > > > 1. Adds a new shared type PackedGPUGlyphID that wraps SkPackedGlyphID > > and packs into the free bits the rest of the information that > > atlas-backed glyphs require, which is the mask format, the amount of > > padding, and whether or not the data is a coverage or distance value. > > > > 2. Pulls back the presence of MaskFormat from the GlyphVector concepts > > and functions as it's now handled internally by each backend's GlyphData > > classes and embedded into the packed GPU IDs that they make. > > > > 3. Each backend's TextStrike implementation stores PackedGPUGlyphIDs as > > the keys to GlyphEntries instead of SkPackedGlyphIDs. This ensures that > > an atlas will only have a cache hit if all of the mask/padding/data-type > > properties are consistent with what was in the atlas and what is > > requested by the subrun being drawn. > > > > 4. Each backend's GlyphData implementation takes in these additional > > properties in its constructor (propagating to initBackendData() calls). > > It then resolves the mask format and padding with the configuration of > > the backend's atlas manager so that all subsequent PackedGPUGlyphIDs > > that it makes represent the final configuration. > > > > 5. The atlas manager's now require the format and padding to be > > pre-resolved in many of their functions, such that 565 has been lifted > > to RGBA8 and 1px of padding is added for direct masks if the caps > > require all direct masks to have padding (this automatically makes > > transformed mask subrun glyphs and direct mask subrun glyphs key the > > same when fSupportBilerpAtlas is true). > > > > Backend-specific changes for Graphite: > > > > The properties that GlyphData requires for filling out the > > PackedGPUGlyphID are basically the `sktext::gpu::RendererData` that was > > being stored in the SubRunData object. I added the srcPadding to it and > > then made it accessible from GlyphData, so it could be removed from > > SubRunData. This keeps the number of arguments to the various functions > > fairly concise. > > > > As part of this, RendererData moved from SubRunContainer.h to > > GlyphVector.h > > > > Backend-specific changes for Ganesh: > > > > Ganesh didn't use RendererData, so its GlyphData just takes the > > parameters in directly and from the AtlasTextOp. Its GlyphData class > > also had an unused declared constructor that I removed, and > > GrAtlasManager::addToAtlas() could be made private (which made enforcing > > resolvedMaskFormat easier). > > > > Bug: 514078656 > > Change-Id: I07977910f64d84e15fe6e4c687d8635afb356b7c > > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1292556 > > Reviewed-by: Alexis Cruz-Ayala <alexisdavidc@google.com> > > Reviewed-by: Robert Phillips <robertphillips@google.com> > > Commit-Queue: Michael Ludwig <michaelludwig@google.com> > > Bug: 514078656 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Change-Id: Iec81ee8afd60d7fd9864bdc9dff8d4cca2c95d51 > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1314716 > Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com> > Commit-Queue: Michael Ludwig <michaelludwig@google.com> Bug: 514078656 Fixed: 514078656 Change-Id: I98e05b49519f829b82ced452e07d5b97f8195be4 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1314717 Commit-Queue: Robert Phillips <robertphillips@google.com> Reviewed-by: Robert Phillips <robertphillips@google.com> Auto-Submit: Michael Ludwig <michaelludwig@google.com>

[graphite] MakeWithFilter returns non-budgeted, non-scratch textures Most scratch resources call sites in Graphite know up front what the final surface is going to be and are able to take a Budgeted parameter for use when creating that instance. At the end of their operations, the returned surface's makeTemporaryImage() then automatically gets the Budgeted::kNo value and is instantiated as part of TextureProxy::Make(). Image filters have a much more complex control flow and it's difficult to identity when the underlying texture is the "last" step. This is partly because it's spread out across several components such as FilterResult and the SkBlurEngine (so the blur engine needs to know whether or not its final passes are also the final steps of the filter graph). But even when there are more steps in the filter graph, it's possible that a later step is able to just chain operations on to the FilterResult without requiring a new texture. As such, for the MakeWithFilter() factory, we ended up with an SkImage that was always Budgeted::kYes and not forced to be instantiated. If the returned SkImage was not used in the Recording that evaluated the filter, it would have an open read/write interval and the ScratchResourceManager would instantiate it with a non-shareable resource (although it would still be counted towards our internal budget). However, if the returned image was also drawn in the initial Recording, then it would for all intents and purposes appear like a regular image filter draw and the ScratchResourceManager would instantiate it with a scratch resource. If subsequent recordings reused the image, and also required some scratch textures, the image's contents might get overwritten. This adds a new test ImageFilterMakeWithFilter_ScratchReuse_Graphite that both reproduced the corrupted rendering, and adds verification that the properties of the underlying texture match what we want for a user-owned SkImage (non-budgeted, non-shareable, and instantiated). The rendering corruption could have been addressed by just forcing the underlying proxy to be instantiated before returning the image from MakeWithFilter, but then we'd still be seeing it as budgeted with no real way to drop refs when we were over budget. To get around that, this adds a new makeNonBudgeted() function that modifies the TextureProxy's fBudgeted field if it can, or performs a copy if it can't. Bug: b/549741762 Change-Id: I2bd927bf83346002d95f1f598d217f5f034f407d Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1334556 Reviewed-by: Nathan Sanchez <nathanasanchez@google.com> Reviewed-by: Robert Phillips <robertphillips@google.com> Commit-Queue: Michael Ludwig <michaelludwig@google.com>

[graphite] Finish optimizing transfer functions This updates the TextureFormat tests to verify the optimize_transfer() analysis for all of its format+colortype+alphatype combinations. It confirms that if the data transfer left bits unmodified that the TextureFormatXferfn's isIdentity() returns true. It also confirms the opposite, that if isIdentity() returns true, that it also left the bits unmodified and matches the expected output. It requires that raster pipeline is only used for several key conversions, such as data type and precision changes, or applying color space or luminance calculations. Everything else should be able to be handled by the extended xfer ops. It also confirms that if raster pipeline is used, that it's not also trying to use extended xfer ops. The exception to this is the allowed use of kPadAlpha and kDropAlpha, since RP has no support for 3 channel formats and the only option is to mix the transfer approaches. In order for these tests to pass, the optimize_transfer logic had to be updated to catch a few cases: 1. When going from gray to red, it can be treated as an identity transfer since the red channel's value will equal the gray value. 2. If there's no color channels (i.e. alpha-only), then we can get rid of the colorspace conversions 3. When creating the raster pipeline, the Make() function can now manipulate the extended xfer ops mask. This allows it to consume ops that can also be implemented as a swizzle to avoid mixing strategies. Bug: 509890794 Change-Id: I4773d2dbd530c9584eddc75ea766867ce1c2d08f Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1344797 Reviewed-by: Thomas Smith <thomsmit@google.com> Commit-Queue: Michael Ludwig <michaelludwig@google.com> Reviewed-by: Robert Phillips <robertphillips@google.com>

[graphite] Use approx-sized MSAA and D/S attachments when possible When rendering to many exact-sized textures, requiring depth attachments and color MSAA attachments to match the size would create many "redundant" transient attachments. Since in ideal circumstances, these attachments have no real memory, their dimensions are somewhat irrelevant, but are still part of the texture key and contribute to the number of Resources in the ResourceCache and allocated by VMA. This adds a new enum, AttachmentSizePolicy, that generalizes the differentResolveAttachmentSizeSupport() that was used to support both approx-sized MSAA attachments and smaller render-area MSAA attachments for Dawn. This behavior now is represented by the kMSAARenderArea option. It is restricted to when there is MSAA since the mechanisms it uses (Intel's partial resolve extension, or emulation via separate internal render passes) require the "resolve" action to work. This means single-sampled depth attachments must still be exact sized for Dawn. In Vulkan, the VulkanFramebuffer is updated to have its size set by the main texture (e.g. single-sampled color texture), which then allows the other attachments to be larger than the framebuffer. Vulkan can universally use the kApprox option to create fewer resources. In Metal, there is a (undocumented) but reported validation error if the MSAA color attachment dimensions are not exactly the same as the resolve texture's dimensions. There are no validation errors if the depth/stencil attachment is larger than the resolve texture, but given that it also lacks any documented requirements, I opted to just keep Metal as using the kExact policy instead of an additional kApproxDepthOnly option. Bug: b/542445906 Change-Id: I00479042bc74a623d49276cb1a4d5cf687cb52a0 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1325116 Reviewed-by: Nathan Sanchez <nathanasanchez@google.com> Commit-Queue: Michael Ludwig <michaelludwig@google.com>

[ganesh] Check results of internal flushes for internalWritePixels success This moves the caps' dependent pre-flush out of newWritePixelsTask and performs the flush inside internalWritePixels. Also checks for the success of the flush at the end of internalWritePixels when the source pixel data isn't owned. These use a shared helper function. Adds a unit test that can trigger writing stale scratch texture contents if the internalWritePixels' flush failed when performing an upload to the scratch texture when taking a write-as-draw path for the primary writePixels target. Bug: 536068737 Fixed: 536068737 Change-Id: I75c01c1db94c3c38ba302473a0ba502e5cff2c94 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1302397 Reviewed-by: Thomas Smith <thomsmit@google.com> Commit-Queue: Michael Ludwig <michaelludwig@google.com> Reviewed-by: Nathan Sanchez <nathanasanchez@google.com>

[graphite] Test format transfers with unknown-alpha swizzle handling This CL updates the unit tests to apply the same rgb1 swizzle to the texture's read swizzle that is done when wrapping textures that are to be interpreted as an unknown alpha type. This exposes the TextureFormatXferFn to some more swizzle combinations so its gray and alpha handling had to be updated. It updates the CPU colortype/alphatype handling to match the combinations that can appear when dealing with a valid SkBitmap. It was getting tricky from a testing POV to have the CPU data also behave as if "unknown alpha" was forced to opaque when nothing in the raster backend actually does that. This CL also updates the compare_pixels() function to be able to ignore the alpha channel. Ignoring alpha occurs for CPU->GPU transfers when the final read swizzle has A=1, since the actual transferred values don't matter. This also updates the value generation of input pixel data to be inject "padding"/junk bits into a masked out alpha channel. This doesn't come up currently, but as more optimizations are landed to detect more identity conversions, this will be important. It makes it easier to verify that the actual transfer either overrides them or carries them forwards unmodified because it can assume the read swizzle handles it. Bug: 509890794 Change-Id: If1dcca63790b6b589aeafff3a0ae26acaca165a0 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1342676 Commit-Queue: Michael Ludwig <michaelludwig@google.com> Reviewed-by: Thomas Smith <thomsmit@google.com> Reviewed-by: Robert Phillips <robertphillips@google.com>

Reland "[graphite] Use approx-sized MSAA and D/S attachments when possible" This reverts commit 9aec3850d46dd7a881e1bbbd32fadb08b344e6c3. Reason for revert: fixing dimension usages, suppress swiftshader and adreno 6xx GPUs but otherwise leave it enabled by default as the other previous failures are gone. Original change's description: > Revert "[graphite] Use approx-sized MSAA and D/S attachments when possible" > > This reverts commit 00541e88a9e40705eb2acbf44e35a7d77c8e136e. > > Reason for revert: breaks on pixel 5, 6, and older drivers on 7pro and all nvidia GPUs on the tree. > > Failure Link: <LINK TO FAILURE> > > Original change's description: > > [graphite] Use approx-sized MSAA and D/S attachments when possible > > > > When rendering to many exact-sized textures, requiring depth attachments > > and color MSAA attachments to match the size would create many > > "redundant" transient attachments. Since in ideal circumstances, these > > attachments have no real memory, their dimensions are somewhat > > irrelevant, but are still part of the texture key and contribute to the > > number of Resources in the ResourceCache and allocated by VMA. > > > > This adds a new enum, AttachmentSizePolicy, that generalizes the > > differentResolveAttachmentSizeSupport() that was used to support both > > approx-sized MSAA attachments and smaller render-area MSAA attachments > > for Dawn. This behavior now is represented by the kMSAARenderArea > > option. It is restricted to when there is MSAA since the mechanisms it > > uses (Intel's partial resolve extension, or emulation via separate > > internal render passes) require the "resolve" action to work. This means > > single-sampled depth attachments must still be exact sized for Dawn. > > > > In Vulkan, the VulkanFramebuffer is updated to have its size set by the > > main texture (e.g. single-sampled color texture), which then allows the > > other attachments to be larger than the framebuffer. Vulkan can > > universally use the kApprox option to create fewer resources. > > > > In Metal, there is a (undocumented) but reported validation error if the > > MSAA color attachment dimensions are not exactly the same as the resolve > > texture's dimensions. There are no validation errors if the > > depth/stencil attachment is larger than the resolve texture, but given > > that it also lacks any documented requirements, I opted to just keep > > Metal as using the kExact policy instead of an additional > > kApproxDepthOnly option. > > > > Bug: b/542445906 > > Change-Id: I00479042bc74a623d49276cb1a4d5cf687cb52a0 > > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1325116 > > Reviewed-by: Nathan Sanchez <nathanasanchez@google.com> > > Commit-Queue: Michael Ludwig <michaelludwig@google.com> > > Bug: b/542445906 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Change-Id: I7c57b85221b2807443794cce3f27200f0806f4dc > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1355377 > Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com> > Commit-Queue: Michael Ludwig <michaelludwig@google.com> Bug: b/542445906 Change-Id: I85a78d4d5fa6b1103a572ee94eb6f1590d5b51b0 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1355044 Commit-Queue: Michael Ludwig <michaelludwig@google.com> Reviewed-by: Nathan Sanchez <nathanasanchez@google.com>

[graphite] Reduce _ALWAYS tracing overhead Some _ALWAYS traces are removed as they don't hold a lot of value unless doing a really deep dive. They also tended to require the snprintf() string generation for the hybrid atrace/perfetto system. Once things are 100% perfetto, these can be enabled automatically by categories. When tracing pipeline labels, we stop tracing the nummber of pipelines since that is implicit in the number of reported instants, and avoids the same string building overhead. Lastly, this updates Graphite's Resource getLabel() to return a const std::string& so that when it's passed to a perfetto::DynamicString there is no need to call strlen, which hopefully speeds up any copy that might have to happen when the event is written. If we want to take it further, we could intentionally leak the pipeline labels so that they have process lifetime (unfortunately Context lifetime or Resource lifetime is not safe enough). However, this would only benefit perfetto tracing which auto-interns strings but would not help atrace. Alternatively, in a perfetto world we can define internable data by extending `TrackEventInternedDataIndex` in order to get an interned id so we could call that any time we create a pipeline (a bit unclear as to what happens if tracing is disabled at the time we need to intern the data). Bug: 522157577 Change-Id: Ib092ef72d74d685096783d1c26a27bc5e561b590 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1359016 Reviewed-by: Noelle Scobie <nscobie@google.com> Commit-Queue: Michael Ludwig <michaelludwig@google.com>