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] Disable blending for inner fills This adds capabilities to PaintParamsKeyBuilder to replace both the last block ID (returning what used to be there for verification) and to replace block IDs within a key if they have the same structure as the ID being replaced. These rules ensure that the extracted uniforms are the same for the new key as the old key, and that the tree structure does not change (meaning that the edits can be done in linear time directly to the buffer instead of having to re-arrange entire sub trees). Now Device will use the new rewriting ability to get a new opaquePaintId that it uses for the inner fill draws. This means that opaque src-over draws that had analytic AA will still have their inner fill converted to src blending. Bug: 478239991 Change-Id: Iaba21ca6446ba2b3525a3df1151e6a8ac842a186 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1202799 Reviewed-by: Robert Phillips <robertphillips@google.com> Commit-Queue: 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] Remove Insertion helper struct The Layer was the only thing used from Insertion, and the comparison logic for selecting the most-recent layer in ClipStack can use the layer's CompressedPaintersOrder directly. Bug: 419535595 Change-Id: Ia14e764944a7b885eb56eb79a7529c3121f58775 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1283261 Reviewed-by: Thomas Smith <thomsmit@google.com> 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>

[graphite] Support kSwapRB xfer op w/o relying on raster pipeline This is important for two reasons: 1. There are some texture formats that need to be swizzled that don't have a matching swizzled color type. Having it as an xfer op means it can be performed outside of the raster pipeline. This removes the hacks on TF::kARGB4 requiring a BGRA read swizzle in the shader. 2. This moves TextureFormatXferFn closer to being able to avoid RP for all cases where the only modifications are bit shuffling. Bug: 509890794 Change-Id: I26776475232ae55e6fb932b3210c633efb6f8254 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1311556 Reviewed-by: Robert Phillips <robertphillips@google.com> Commit-Queue: Michael Ludwig <michaelludwig@google.com>

[graphite] Refactor layer/binding-list/draw recording functions `recordBackwards` becomes `searchBackwards` and returns the Layer and BindingList that the Draw should be added to. `recordForwards` is renamed `findOrCreateBindingInLayer` to emphasize it's only used after the pilot draw. The `Draw` creation logic is handled in `recordDraw` after the final BindingList is found. Splits Layer::add() into a `addNewBinding()` function on Layer for creating and inserting new BindingLists and a separate BidningList::addDraw() for inserting the Draw into a BindingList. Bug: 419535595 Change-Id: Ic8b52ca9047f6094e6b526b891846953286973e2 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1283258 Reviewed-by: Thomas Smith <thomsmit@google.com>