-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Use a device buffer for SkBitmap allocation, use Linear texture on Metal backend. #41374
Changes from 19 commits
cff1248
b5eb0c2
82ff8ad
d8ca1a1
0511d65
725ea87
4c5cf04
0df598c
706386a
e9aa30e
7e7d217
81ad980
1ede1d0
69ec6b0
dfb3f3c
f832c97
9ac1722
ffe0d88
70597bd
6b3d886
07e12f2
78fddf7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,11 +10,13 @@ | |
#include "flutter/fml/trace_event.h" | ||
#include "impeller/base/allocation.h" | ||
#include "impeller/core/allocator.h" | ||
#include "impeller/core/device_buffer.h" | ||
#include "impeller/typographer/backends/skia/typeface_skia.h" | ||
#include "third_party/skia/include/core/SkBitmap.h" | ||
|
||
#include "third_party/skia/include/core/SkCanvas.h" | ||
#include "third_party/skia/include/core/SkFont.h" | ||
#include "third_party/skia/include/core/SkFontMetrics.h" | ||
#include "third_party/skia/include/core/SkPixelRef.h" | ||
#include "third_party/skia/include/core/SkRSXform.h" | ||
#include "third_party/skia/include/core/SkSurface.h" | ||
#include "third_party/skia/src/core/SkIPoint16.h" // nogncheck | ||
|
@@ -139,7 +141,9 @@ ISize OptimumAtlasSizeForFontGlyphPairs( | |
const FontGlyphPair::Set& pairs, | ||
std::vector<Rect>& glyph_positions, | ||
const std::shared_ptr<GlyphAtlasContext>& atlas_context) { | ||
static constexpr auto kMinAtlasSize = 8u; | ||
// This size needs to be above the minimum required aligment for linear | ||
// textures. This is 256 for older intel macs and decreases on iOS devices. | ||
static constexpr auto kMinAtlasSize = 256u; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Look this up based on caps? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might actually be better to keep a larger minimum atlas size since we reuse the texture across frames now, but only if the size is unchanged. The memory hit is fairly minor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but what happens if we run on some backend where the minimum size is even larger? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it only matters for metal There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose its possible that a future iOS or macOS versions requires a larger minimum alignment. I'll update this to compute the exact minimum alignment. That said, separately I think we should use a much larger minimum texture size |
||
static constexpr auto kMaxAtlasSize = 4096u; | ||
|
||
TRACE_EVENT0("impeller", __FUNCTION__); | ||
|
@@ -346,9 +350,12 @@ static bool UpdateAtlasBitmap(const GlyphAtlas& atlas, | |
return true; | ||
} | ||
|
||
static std::shared_ptr<SkBitmap> CreateAtlasBitmap(const GlyphAtlas& atlas, | ||
const ISize& atlas_size) { | ||
static std::pair<std::shared_ptr<SkBitmap>, std::shared_ptr<DeviceBuffer>> | ||
CreateAtlasBitmap(const GlyphAtlas& atlas, | ||
std::shared_ptr<Allocator> allocator, | ||
const ISize& atlas_size) { | ||
TRACE_EVENT0("impeller", __FUNCTION__); | ||
auto font_allocator = FontImpellerAllocator(std::move(allocator)); | ||
auto bitmap = std::make_shared<SkBitmap>(); | ||
SkImageInfo image_info; | ||
|
||
|
@@ -363,17 +370,18 @@ static std::shared_ptr<SkBitmap> CreateAtlasBitmap(const GlyphAtlas& atlas, | |
break; | ||
} | ||
|
||
if (!bitmap->tryAllocPixels(image_info)) { | ||
return nullptr; | ||
bitmap->setInfo(image_info); | ||
if (!bitmap->tryAllocPixels(&font_allocator)) { | ||
return std::make_pair(nullptr, nullptr); | ||
} | ||
|
||
auto surface = SkSurface::MakeRasterDirect(bitmap->pixmap()); | ||
if (!surface) { | ||
return nullptr; | ||
return std::make_pair(nullptr, nullptr); | ||
} | ||
auto canvas = surface->getCanvas(); | ||
if (!canvas) { | ||
return nullptr; | ||
return std::make_pair(nullptr, nullptr); | ||
} | ||
|
||
bool has_color = atlas.GetType() == GlyphAtlas::Type::kColorBitmap; | ||
|
@@ -384,13 +392,16 @@ static std::shared_ptr<SkBitmap> CreateAtlasBitmap(const GlyphAtlas& atlas, | |
return true; | ||
}); | ||
|
||
return bitmap; | ||
auto device_buffer = font_allocator.GetDeviceBuffer(); | ||
if (!device_buffer.has_value()) { | ||
return std::make_pair(nullptr, nullptr); | ||
} | ||
return std::make_pair(bitmap, device_buffer.value()); | ||
} | ||
|
||
static bool UpdateGlyphTextureAtlas(std::shared_ptr<SkBitmap> bitmap, | ||
const std::shared_ptr<Texture>& texture) { | ||
TRACE_EVENT0("impeller", __FUNCTION__); | ||
|
||
FML_DCHECK(bitmap != nullptr); | ||
auto texture_descriptor = texture->GetTextureDescriptor(); | ||
|
||
|
@@ -404,14 +415,12 @@ static bool UpdateGlyphTextureAtlas(std::shared_ptr<SkBitmap> bitmap, | |
} | ||
|
||
static std::shared_ptr<Texture> UploadGlyphTextureAtlas( | ||
const std::shared_ptr<Allocator>& allocator, | ||
std::shared_ptr<SkBitmap> bitmap, | ||
Allocator& allocator, | ||
const std::shared_ptr<DeviceBuffer>& device_buffer, | ||
const std::shared_ptr<SkBitmap>& bitmap, | ||
const ISize& atlas_size, | ||
PixelFormat format) { | ||
TRACE_EVENT0("impeller", __FUNCTION__); | ||
if (!allocator) { | ||
return nullptr; | ||
} | ||
|
||
FML_DCHECK(bitmap != nullptr); | ||
const auto& pixmap = bitmap->pixmap(); | ||
|
@@ -421,32 +430,27 @@ static std::shared_ptr<Texture> UploadGlyphTextureAtlas( | |
texture_descriptor.format = format; | ||
texture_descriptor.size = atlas_size; | ||
|
||
// If the alignment isn't a multiple of the pixel format, we cannot use | ||
// a linear texture and instead must blit to a new texture. | ||
if (pixmap.rowBytes() * pixmap.height() != | ||
texture_descriptor.GetByteSizeOfBaseMipLevel()) { | ||
return nullptr; | ||
} | ||
|
||
auto texture = allocator->CreateTexture(texture_descriptor); | ||
FML_DCHECK(allocator.MinimumBytesPerRow(format) <= pixmap.rowBytes()); | ||
auto texture = device_buffer->AsTexture(allocator, texture_descriptor, | ||
texture_descriptor.GetBytesPerRow()); | ||
Comment on lines
+457
to
+458
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this punish non-metal implementations right now? I think it doesn't because before we did the copy explicitly in this code, and now it gets done implicitly if needed there, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup! |
||
if (!texture || !texture->IsValid()) { | ||
return nullptr; | ||
} | ||
texture->SetLabel("GlyphAtlas"); | ||
|
||
auto mapping = std::make_shared<fml::NonOwnedMapping>( | ||
reinterpret_cast<const uint8_t*>(bitmap->getAddr(0, 0)), // data | ||
texture_descriptor.GetByteSizeOfBaseMipLevel(), // size | ||
[bitmap](auto, auto) mutable { bitmap.reset(); } // proc | ||
); | ||
|
||
if (!texture->SetContents(mapping)) { | ||
return nullptr; | ||
} | ||
return texture; | ||
} | ||
|
||
std::shared_ptr<GlyphAtlas> TextRenderContextSkia::CreateGlyphAtlas( | ||
GlyphAtlas::Type type, | ||
std::shared_ptr<GlyphAtlasContext> atlas_context, | ||
const std::shared_ptr<const Capabilities>& capabilities, | ||
FrameIterator frame_iterator) const { | ||
TRACE_EVENT0("impeller", __FUNCTION__); | ||
if (!IsValid()) { | ||
|
@@ -502,15 +506,18 @@ std::shared_ptr<GlyphAtlas> TextRenderContextSkia::CreateGlyphAtlas( | |
// --------------------------------------------------------------------------- | ||
// Step 5: Draw new font-glyph pairs into the existing bitmap. | ||
// --------------------------------------------------------------------------- | ||
auto bitmap = atlas_context->GetBitmap(); | ||
auto [bitmap, device_buffer] = atlas_context->GetBitmap(); | ||
if (!UpdateAtlasBitmap(*last_atlas, bitmap, new_glyphs)) { | ||
return nullptr; | ||
} | ||
|
||
// --------------------------------------------------------------------------- | ||
// Step 6: Update the existing texture with the updated bitmap. | ||
// This is only necessary on backends that don't support creating | ||
// a texture that shares memory with the underlying device buffer. | ||
// --------------------------------------------------------------------------- | ||
if (!UpdateGlyphTextureAtlas(bitmap, last_atlas->GetTexture())) { | ||
if (!capabilities->SupportsSharedDeviceBufferTextureMemory() && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's the payoff, right? LGTM. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically this and below where we construct a texture from the device buffer, that required a second copy before but now its "free" on iOS/macOS |
||
!UpdateGlyphTextureAtlas(bitmap, last_atlas->GetTexture())) { | ||
return nullptr; | ||
} | ||
return last_atlas; | ||
|
@@ -552,11 +559,12 @@ std::shared_ptr<GlyphAtlas> TextRenderContextSkia::CreateGlyphAtlas( | |
// --------------------------------------------------------------------------- | ||
// Step 7: Draw font-glyph pairs in the correct spot in the atlas. | ||
// --------------------------------------------------------------------------- | ||
auto bitmap = CreateAtlasBitmap(*glyph_atlas, atlas_size); | ||
auto [bitmap, device_buffer] = CreateAtlasBitmap( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are shadowing the declarations in 509, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, those are in a different block |
||
*glyph_atlas, GetContext()->GetResourceAllocator(), atlas_size); | ||
if (!bitmap) { | ||
return nullptr; | ||
} | ||
atlas_context->UpdateBitmap(bitmap); | ||
atlas_context->UpdateBitmap(bitmap, device_buffer); | ||
|
||
// --------------------------------------------------------------------------- | ||
// Step 8: Upload the atlas as a texture. | ||
|
@@ -574,8 +582,9 @@ std::shared_ptr<GlyphAtlas> TextRenderContextSkia::CreateGlyphAtlas( | |
format = PixelFormat::kR8G8B8A8UNormInt; | ||
break; | ||
} | ||
auto texture = UploadGlyphTextureAtlas(GetContext()->GetResourceAllocator(), | ||
bitmap, atlas_size, format); | ||
auto texture = | ||
UploadGlyphTextureAtlas(*GetContext()->GetResourceAllocator().get(), | ||
device_buffer, bitmap, atlas_size, format); | ||
if (!texture) { | ||
return nullptr; | ||
} | ||
|
@@ -588,4 +597,43 @@ std::shared_ptr<GlyphAtlas> TextRenderContextSkia::CreateGlyphAtlas( | |
return glyph_atlas; | ||
} | ||
|
||
FontImpellerAllocator::FontImpellerAllocator( | ||
std::shared_ptr<impeller::Allocator> allocator) | ||
: allocator_(std::move(allocator)) {} | ||
|
||
std::optional<std::shared_ptr<impeller::DeviceBuffer>> | ||
FontImpellerAllocator::GetDeviceBuffer() const { | ||
return buffer_; | ||
} | ||
|
||
bool FontImpellerAllocator::allocPixelRef(SkBitmap* bitmap) { | ||
const SkImageInfo& info = bitmap->info(); | ||
if (kUnknown_SkColorType == info.colorType() || info.width() < 0 || | ||
info.height() < 0 || !info.validRowBytes(bitmap->rowBytes())) { | ||
return false; | ||
} | ||
|
||
DeviceBufferDescriptor descriptor; | ||
descriptor.storage_mode = StorageMode::kHostVisible; | ||
descriptor.size = ((bitmap->height() - 1) * bitmap->rowBytes()) + | ||
(bitmap->width() * bitmap->bytesPerPixel()); | ||
|
||
auto device_buffer = allocator_->CreateBuffer(descriptor); | ||
|
||
struct ImpellerPixelRef final : public SkPixelRef { | ||
ImpellerPixelRef(int w, int h, void* s, size_t r) | ||
: SkPixelRef(w, h, s, r) {} | ||
|
||
~ImpellerPixelRef() override {} | ||
}; | ||
|
||
auto pixel_ref = sk_sp<SkPixelRef>( | ||
new ImpellerPixelRef(info.width(), info.height(), | ||
device_buffer->OnGetContents(), bitmap->rowBytes())); | ||
|
||
bitmap->setPixelRef(std::move(pixel_ref), 0, 0); | ||
buffer_ = std::move(device_buffer); | ||
return true; | ||
} | ||
|
||
} // namespace impeller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should support it eventually, right? Probably worth adding a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some research but I think is metal only