Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion impeller/renderer/backend/gles/blit_command_gles.cc
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ bool BlitCopyBufferToTextureCommandGLES::Encode(
0u, // border
data.external_format, // external format
data.type, // type
tex_data // data
nullptr // data
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be null because otherwise the driver might attempt to read out of bounds, the texture data is actually initialized below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, definitely missed that.

);
texture_gles.MarkSliceInitialized(slice);
}
Expand Down
24 changes: 24 additions & 0 deletions impeller/renderer/blit_pass_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -104,5 +104,29 @@ TEST_P(BlitPassTest, ChecksInvalidSliceParameters) {
std::nullopt, "", /*slice=*/0));
}

TEST_P(BlitPassTest, CanBlitSmallRegionToUninitializedTexture) {
auto context = GetContext();
auto cmd_buffer = context->CreateCommandBuffer();
auto blit_pass = cmd_buffer->CreateBlitPass();

TextureDescriptor dst_format;
dst_format.storage_mode = StorageMode::kDevicePrivate;
dst_format.format = PixelFormat::kR8G8B8A8UNormInt;
dst_format.size = {1000, 1000};
auto dst = context->GetResourceAllocator()->CreateTexture(dst_format);

DeviceBufferDescriptor src_format;
src_format.size = 4;
src_format.storage_mode = StorageMode::kHostVisible;
auto src = context->GetResourceAllocator()->CreateBuffer(src_format);

ASSERT_TRUE(dst);

EXPECT_TRUE(blit_pass->AddCopy(DeviceBuffer::AsBufferView(src), dst,
IRect::MakeLTRB(0, 0, 1, 1), "", /*slice=*/0));
EXPECT_TRUE(blit_pass->EncodeCommands(GetContext()->GetResourceAllocator()));
EXPECT_TRUE(context->GetCommandQueue()->Submit({std::move(cmd_buffer)}).ok());
}

} // namespace testing
} // namespace impeller
39 changes: 27 additions & 12 deletions impeller/typographer/backends/skia/typographer_context_skia.cc
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,12 @@ static size_t PairsFitInAtlasOfSize(
)) {
return pairs.size() - i;
}
glyph_positions.emplace_back(Rect::MakeXYWH(location_in_atlas.x(), //
location_in_atlas.y(), //
glyph_size.width, //
glyph_size.height //
// The size of the glyph was padded by 1px on all sides. Shift the glyph
// origin by 1px so the padding surrounds the glyph.
glyph_positions.emplace_back(Rect::MakeXYWH(location_in_atlas.x() + 1, //
location_in_atlas.y() + 1, //
glyph_size.width, //
glyph_size.height //
));
}

Expand Down Expand Up @@ -180,10 +182,12 @@ static bool CanAppendToExistingAtlas(
)) {
return false;
}
glyph_positions.emplace_back(Rect::MakeXYWH(location_in_atlas.x(), //
location_in_atlas.y(), //
glyph_size.width, //
glyph_size.height //
// The size of the glyph was padded by 1px on all sides. Shift the glyph
// origin by 1px so the padding surrounds the glyph.
glyph_positions.emplace_back(Rect::MakeXYWH(location_in_atlas.x() + 1, //
location_in_atlas.y() + 1, //
glyph_size.width, //
glyph_size.height //
));
}

Expand Down Expand Up @@ -234,7 +238,8 @@ static void DrawGlyph(SkCanvas* canvas,
const Glyph& glyph,
bool has_color) {
const auto& metrics = scaled_font.font.GetMetrics();
const auto position = SkPoint::Make(0, 0);
const auto position =
SkPoint::Make(1.0f / scaled_font.scale, 1.0f / scaled_font.scale);
SkGlyphID glyph_id = glyph.index;

SkFont sk_font(
Expand Down Expand Up @@ -280,6 +285,12 @@ static bool UpdateAtlasBitmap(const GlyphAtlas& atlas,
continue;
}

FML_DCHECK(pos->GetOrigin() != Point(0, 0));
// Add 1px of padding around glyph so that linearly sampled skew/rotate
// glyphs do not sample uninitialized data.
size.width += 2;
size.height += 2;

SkBitmap bitmap;
HostBufferAllocator allocator(host_buffer);
bitmap.setInfo(GetImageInfo(atlas, size));
Expand All @@ -297,9 +308,11 @@ static bool UpdateAtlasBitmap(const GlyphAtlas& atlas,

DrawGlyph(canvas, pair.scaled_font, pair.glyph, has_color);

if (!blit_pass->AddCopy(allocator.TakeBufferView(), texture,
IRect::MakeXYWH(pos->GetLeft(), pos->GetTop(),
size.width, size.height))) {
// Note: the position is shifted by (1, 1) to include the padding pixels.
if (!blit_pass->AddCopy(
allocator.TakeBufferView(), texture,
IRect::MakeXYWH(pos->GetLeft() - 1, pos->GetTop() - 1, size.width,
size.height))) {
return false;
}
}
Expand Down Expand Up @@ -414,6 +427,7 @@ std::shared_ptr<GlyphAtlas> TypographerContextSkia::CreateGlyphAtlas(
if (atlas_size.IsEmpty()) {
return nullptr;
}

// ---------------------------------------------------------------------------
// Step 4b: Find location of font-glyph pairs in the atlas. We have this from
// the last step. So no need to do create another rect packer. But just do a
Expand Down Expand Up @@ -461,6 +475,7 @@ std::shared_ptr<GlyphAtlas> TypographerContextSkia::CreateGlyphAtlas(
if (!new_texture) {
return nullptr;
}

// The texture needs to be cleared to transparent black so that linearly
// samplex rotated/skewed glyphs do not grab uninitialized data. We could
// instead use a render pass to clear to transparent black, but there are
Expand Down