From f81ac3b1996cad041d558fb503d165fb686d6520 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Fri, 18 Nov 2022 16:33:25 -0800 Subject: [PATCH] Fix glyph sampling overlap (#37764) --- .../shader_lib/impeller/transform.glsl | 26 ++++++-------- impeller/entity/contents/text_contents.cc | 27 +++++++-------- impeller/entity/shaders/glyph_atlas.frag | 30 ++++++++-------- impeller/entity/shaders/glyph_atlas.vert | 34 ++++++++++--------- impeller/entity/shaders/glyph_atlas_sdf.frag | 29 ++++++++++------ impeller/entity/shaders/glyph_atlas_sdf.vert | 28 ++++++++------- impeller/fixtures/struct_def_bug.vert | 18 +++++----- .../backends/skia/text_render_context_skia.cc | 10 ++++-- 8 files changed, 104 insertions(+), 98 deletions(-) diff --git a/impeller/compiler/shader_lib/impeller/transform.glsl b/impeller/compiler/shader_lib/impeller/transform.glsl index f4ee335286dbd..444419db4b087 100644 --- a/impeller/compiler/shader_lib/impeller/transform.glsl +++ b/impeller/compiler/shader_lib/impeller/transform.glsl @@ -13,22 +13,16 @@ vec2 IPVec2TransformPosition(mat4 matrix, vec2 point) { // Returns the transformed gl_Position for a given glyph position in a glyph // atlas. -vec4 IPPositionForGlyphPosition(mat4 mvp, vec2 unit_vertex, vec2 glyph_position, vec2 glyph_size) { - vec4 translate = mvp[0] * glyph_position.x - + mvp[1] * glyph_position.y - + mvp[3]; - mat4 translated_mvp = mat4( - mvp[0], - mvp[1], - mvp[2], - vec4( - translate.xyz, - mvp[3].w - ) - ); - return translated_mvp * - vec4(unit_vertex.x * glyph_size.x, - unit_vertex.y * glyph_size.y, 0.0, 1.0); +vec4 IPPositionForGlyphPosition(mat4 mvp, + vec2 unit_position, + vec2 glyph_position, + vec2 glyph_size) { + vec4 translate = + mvp[0] * glyph_position.x + mvp[1] * glyph_position.y + mvp[3]; + mat4 translated_mvp = + mat4(mvp[0], mvp[1], mvp[2], vec4(translate.xyz, mvp[3].w)); + return translated_mvp * vec4(unit_position.x * glyph_size.x, + unit_position.y * glyph_size.y, 0.0, 1.0); } #endif diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index d2d1c0d9bcfc8..1d6727d30565d 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -80,6 +80,7 @@ static bool CommonRender( SamplerDescriptor sampler_desc; sampler_desc.min_filter = MinMagFilter::kLinear; sampler_desc.mag_filter = MinMagFilter::kLinear; + sampler_desc.mip_filter = MipFilter::kNone; typename FS::FragInfo frag_info; frag_info.text_color = ToVector(color.Premultiply()); @@ -103,9 +104,9 @@ static bool CommonRender( // interpolated vertex information is also used in the fragment shader to // sample from the glyph atlas. - const std::vector unit_vertex_points = { - {0, 0}, {1, 0}, {0, 1}, {1, 1}}; - const std::vector indices = {0, 1, 2, 1, 2, 3}; + const std::array unit_points = {Point{0, 0}, Point{1, 0}, + Point{0, 1}, Point{1, 1}}; + const std::array indices = {0, 1, 2, 1, 2, 3}; VertexBufferBuilder vertex_builder; @@ -127,7 +128,7 @@ static bool CommonRender( for (const auto& run : frame.GetRuns()) { auto font = run.GetFont(); - auto glyph_size_ = ISize::Ceil(font.GetMetrics().GetBoundingBox().size); + auto glyph_size_ = font.GetMetrics().GetBoundingBox().size; auto glyph_size = Point{static_cast(glyph_size_.width), static_cast(glyph_size_.height)}; auto metrics_offset = @@ -141,22 +142,20 @@ static bool CommonRender( return false; } - auto atlas_position = - atlas_glyph_pos->origin + Point{1 / atlas_glyph_pos->size.width, - 1 / atlas_glyph_pos->size.height}; + auto atlas_position = atlas_glyph_pos->origin; auto atlas_glyph_size = Point{atlas_glyph_pos->size.width, atlas_glyph_pos->size.height}; auto offset_glyph_position = glyph_position.position + metrics_offset; - for (const auto& point : unit_vertex_points) { + for (const auto& point : unit_points) { typename VS::PerVertexData vtx; - vtx.unit_vertex = point; - vtx.glyph_position = offset_glyph_position; - vtx.glyph_size = glyph_size; - vtx.atlas_position = atlas_position; - vtx.atlas_glyph_size = atlas_glyph_size; + vtx.unit_position = point; + vtx.destination_position = offset_glyph_position + Point(0.5, 0.5); + vtx.destination_size = glyph_size - Point(1.0, 1.0); + vtx.source_position = atlas_position + Point(0.5, 0.5); + vtx.source_glyph_size = atlas_glyph_size - Point(1.0, 1.0); if constexpr (std::is_same_v) { - vtx.color_glyph = + vtx.has_color = glyph_position.glyph.type == Glyph::Type::kBitmap ? 1.0 : 0.0; } vertex_builder.AppendVertex(std::move(vtx)); diff --git a/impeller/entity/shaders/glyph_atlas.frag b/impeller/entity/shaders/glyph_atlas.frag index 6fa09ad2dde2f..4c9175bd9b16a 100644 --- a/impeller/entity/shaders/glyph_atlas.frag +++ b/impeller/entity/shaders/glyph_atlas.frag @@ -7,27 +7,25 @@ uniform sampler2D glyph_atlas_sampler; uniform FragInfo { vec2 atlas_size; vec4 text_color; -} frag_info; +} +frag_info; -in vec2 v_unit_vertex; -in vec2 v_atlas_position; -in vec2 v_atlas_glyph_size; -in float v_color_glyph; +in vec2 v_unit_position; +in vec2 v_source_position; +in vec2 v_source_glyph_size; +in float v_has_color; out vec4 frag_color; void main() { - vec2 scale_perspective = v_atlas_glyph_size / frag_info.atlas_size; - vec2 offset = v_atlas_position / frag_info.atlas_size; - if (v_color_glyph == 1.0) { - frag_color = texture( - glyph_atlas_sampler, - v_unit_vertex * scale_perspective + offset - ); + vec2 uv_size = v_source_glyph_size / frag_info.atlas_size; + vec2 offset = v_source_position / frag_info.atlas_size; + if (v_has_color == 1.0) { + frag_color = + texture(glyph_atlas_sampler, v_unit_position * uv_size + offset); } else { - frag_color = texture( - glyph_atlas_sampler, - v_unit_vertex * scale_perspective + offset - ).aaaa * frag_info.text_color; + frag_color = + texture(glyph_atlas_sampler, v_unit_position * uv_size + offset).aaaa * + frag_info.text_color; } } diff --git a/impeller/entity/shaders/glyph_atlas.vert b/impeller/entity/shaders/glyph_atlas.vert index e332d67d3be7f..365fc72b13b1e 100644 --- a/impeller/entity/shaders/glyph_atlas.vert +++ b/impeller/entity/shaders/glyph_atlas.vert @@ -6,24 +6,26 @@ uniform FrameInfo { mat4 mvp; -} frame_info; +} +frame_info; -in vec2 unit_vertex; -in vec2 glyph_position; -in vec2 glyph_size; -in vec2 atlas_position; -in vec2 atlas_glyph_size; -in float color_glyph; +in vec2 unit_position; +in vec2 destination_position; +in vec2 destination_size; +in vec2 source_position; +in vec2 source_glyph_size; +in float has_color; -out vec2 v_unit_vertex; -out vec2 v_atlas_position; -out vec2 v_atlas_glyph_size; -out float v_color_glyph; +out vec2 v_unit_position; +out vec2 v_source_position; +out vec2 v_source_glyph_size; +out float v_has_color; void main() { - gl_Position = IPPositionForGlyphPosition(frame_info.mvp, unit_vertex, glyph_position, glyph_size); - v_unit_vertex = unit_vertex; - v_atlas_position = atlas_position; - v_atlas_glyph_size = atlas_glyph_size; - v_color_glyph = color_glyph; + gl_Position = IPPositionForGlyphPosition( + frame_info.mvp, unit_position, destination_position, destination_size); + v_unit_position = unit_position; + v_source_position = source_position; + v_source_glyph_size = source_glyph_size; + v_has_color = has_color; } diff --git a/impeller/entity/shaders/glyph_atlas_sdf.frag b/impeller/entity/shaders/glyph_atlas_sdf.frag index 38518cee4247d..9947566d150df 100644 --- a/impeller/entity/shaders/glyph_atlas_sdf.frag +++ b/impeller/entity/shaders/glyph_atlas_sdf.frag @@ -7,28 +7,35 @@ uniform sampler2D glyph_atlas_sampler; uniform FragInfo { vec2 atlas_size; vec4 text_color; -} frag_info; +} +frag_info; -in vec2 v_unit_vertex; -in vec2 v_atlas_position; -in vec2 v_atlas_glyph_size; +in vec2 v_unit_position; +in vec2 v_source_position; +in vec2 v_source_glyph_size; out vec4 frag_color; void main() { - vec2 scale_perspective = v_atlas_glyph_size / frag_info.atlas_size; - vec2 offset = v_atlas_position / frag_info.atlas_size; + vec2 scale_perspective = v_source_glyph_size / frag_info.atlas_size; + vec2 offset = v_source_position / frag_info.atlas_size; // Inspired by Metal by Example's SDF text rendering shader: // https://github.com/metal-by-example/sample-code/blob/master/objc/12-TextRendering/TextRendering/Shaders.metal // Outline of glyph is the isocontour with value 50% float edge_distance = 0.5; - // Sample the signed-distance field to find distance from this fragment to the glyph outline - float sample_distance = texture(glyph_atlas_sampler, v_unit_vertex * scale_perspective + offset).a; - // Use local automatic gradients to find anti-aliased anisotropic edge width, cf. Gustavson 2012 + // Sample the signed-distance field to find distance from this fragment to the + // glyph outline + float sample_distance = + texture(glyph_atlas_sampler, v_unit_position * scale_perspective + offset) + .a; + // Use local automatic gradients to find anti-aliased anisotropic edge width, + // cf. Gustavson 2012 float edge_width = length(vec2(dFdx(sample_distance), dFdy(sample_distance))); - // Smooth the glyph edge by interpolating across the boundary in a band with the width determined above - float insideness = smoothstep(edge_distance - edge_width, edge_distance + edge_width, sample_distance); + // Smooth the glyph edge by interpolating across the boundary in a band with + // the width determined above + float insideness = smoothstep(edge_distance - edge_width, + edge_distance + edge_width, sample_distance); frag_color = frag_info.text_color * insideness; } diff --git a/impeller/entity/shaders/glyph_atlas_sdf.vert b/impeller/entity/shaders/glyph_atlas_sdf.vert index 830566304a123..cfbb2d353e7c6 100644 --- a/impeller/entity/shaders/glyph_atlas_sdf.vert +++ b/impeller/entity/shaders/glyph_atlas_sdf.vert @@ -6,21 +6,23 @@ uniform FrameInfo { mat4 mvp; -} frame_info; +} +frame_info; -in vec2 unit_vertex; -in vec2 glyph_position; -in vec2 glyph_size; -in vec2 atlas_position; -in vec2 atlas_glyph_size; +in vec2 unit_position; +in vec2 destination_position; +in vec2 destination_size; +in vec2 source_position; +in vec2 source_glyph_size; -out vec2 v_unit_vertex; -out vec2 v_atlas_position; -out vec2 v_atlas_glyph_size; +out vec2 v_unit_position; +out vec2 v_source_position; +out vec2 v_source_glyph_size; void main() { - gl_Position = IPPositionForGlyphPosition(frame_info.mvp, unit_vertex, glyph_position, glyph_size); - v_unit_vertex = unit_vertex; - v_atlas_position = atlas_position; - v_atlas_glyph_size = atlas_glyph_size; + gl_Position = IPPositionForGlyphPosition( + frame_info.mvp, unit_position, destination_position, destination_size); + v_unit_position = unit_position; + v_source_position = source_position; + v_source_glyph_size = source_glyph_size; } diff --git a/impeller/fixtures/struct_def_bug.vert b/impeller/fixtures/struct_def_bug.vert index 800dc5358ef17..2781034a7c3c0 100644 --- a/impeller/fixtures/struct_def_bug.vert +++ b/impeller/fixtures/struct_def_bug.vert @@ -10,25 +10,25 @@ uniform FrameInfo { in vec2 unit_vertex; in mat4 glyph_position; // <--- Causes multiple slots to be used and is a failure. -in vec2 glyph_size; -in vec2 atlas_position; -in vec2 atlas_glyph_size; +in vec2 destination_size; +in vec2 source_position; +in vec2 source_glyph_size; out vec2 v_unit_vertex; -out vec2 v_atlas_position; -out vec2 v_atlas_glyph_size; +out vec2 v_source_position; +out vec2 v_source_glyph_size; out vec2 v_atlas_size; out vec4 v_text_color; void main() { gl_Position = frame_info.mvp * glyph_position - * vec4(unit_vertex.x * glyph_size.x, - unit_vertex.y * glyph_size.y, 0.0, 1.0); + * vec4(unit_vertex.x * destination_size.x, + unit_vertex.y * destination_size.y, 0.0, 1.0); v_unit_vertex = unit_vertex; - v_atlas_position = atlas_position; - v_atlas_glyph_size = atlas_glyph_size; + v_source_position = source_position; + v_source_glyph_size = source_glyph_size; v_atlas_size = frame_info.atlas_size; v_text_color = frame_info.text_color; } diff --git a/impeller/typographer/backends/skia/text_render_context_skia.cc b/impeller/typographer/backends/skia/text_render_context_skia.cc index c1b6405bc193b..1641171f56a75 100644 --- a/impeller/typographer/backends/skia/text_render_context_skia.cc +++ b/impeller/typographer/backends/skia/text_render_context_skia.cc @@ -71,15 +71,19 @@ static size_t PairsFitInAtlasOfSize(const FontGlyphPair::Vector& pairs, glyph_positions.clear(); glyph_positions.reserve(pairs.size()); + // TODO(114563): We might be able to remove this per-glyph padding if we fix + // the underlying causes of the overlap. + constexpr auto padding = 2; + for (size_t i = 0; i < pairs.size(); i++) { const auto& pair = pairs[i]; const auto glyph_size = ISize::Ceil(pair.font.GetMetrics().GetBoundingBox().size * pair.font.GetMetrics().scale); SkIPoint16 location_in_atlas; - if (!rect_packer->addRect(glyph_size.width, // - glyph_size.height, // - &location_in_atlas // + if (!rect_packer->addRect(glyph_size.width + padding, // + glyph_size.height + padding, // + &location_in_atlas // )) { return pairs.size() - i; }