Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] Use separate atlases and shaders for color and alpha #41780

Merged
merged 13 commits into from
May 10, 2023

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented May 5, 2023

Creates a separate atlas context and cache for color and alpha bitmap glyphs. Removes SDF shader and uses separate shader for full color glyphs.

Requires #41754

Fixes flutter/flutter#116818
Fixes flutter/flutter#126101

This also fixes #39383 but for light text on a dark background. This problem crops up when we switch to a full color atlas. In this context, the chosen glyph color is important. But with the alpha channel only atlas, its irrelevant.

See diff:

image

Example app:

// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/material.dart';
import 'package:flutter/widgets.dart';

void main() =>
  runApp(
    Container(
      alignment: Alignment.center,
     child: Text(
      '''
 (Unicode Conference)، الذي سيعقد في 10-12 آذار 1997 بمدينة مَايِنْتْس، ألمانيا. و سيجمع المؤتمر بين خبراء من كافة قطاعات الصناعة على الشبكة العالمية انترنيت ويونيكود، حيث ستتم، على الصعيدين الدولي والمحلي على حد سواء مناقشة سبل استخدام يونكود في النظم القائمة وفيما يخص التطبيقات الحاسوبية، الخطوط، تصميم النصوص والحوسبة متعددة اللغات.

مَمِمّمَّمِّ😀 😃 😄 😁 😆''',
textDirection: TextDirection.rtl, style: TextStyle(fontSize: 24, color: Colors.white),
     ),
    ),
  );

@jonahwilliams
Copy link
Member Author

jonahwilliams commented May 5, 2023

Still need to investigate if this fixes flutter/flutter#126101 . EDIT: it does

@jonahwilliams jonahwilliams changed the title [Impeller][WIP] Use separate atlases and shaders for color and alpha [Impeller] Use separate atlases and shaders for color and alpha May 6, 2023
@jonahwilliams jonahwilliams marked this pull request as ready for review May 6, 2023 01:40
@bdero
Copy link
Member

bdero commented May 8, 2023

This also fixes #39383 but for light text on a dark background.

How did you fix this? Did you verify that this doesn't regress the case that #39383 solves?

@bdero
Copy link
Member

bdero commented May 8, 2023

Skia seems to correct the alpha based on the paint color. I think the AA alpha dropoff becomes more aggressive as the color gets darker.

@bdero
Copy link
Member

bdero commented May 8, 2023

We have it tuned to draw black text on a white background right now because black text that's too thick looks worse/more jarring than white text that's too thin.

@jonahwilliams
Copy link
Member Author

Note that I did not actually change how glyphs are rendered, except in the case where there are emoji present - then previously we would promote to a full color atlas. Upon checking, it does appear that dark text gets darker, but the rendering is indentical in the case where there is no emoji.

So not exactly regressed, but not quite fixed yet. Let me double check the color that is being using in Skia. Skia only uses a single color in their atlas regardless of light/dark background so we should be able to also.

@jonahwilliams
Copy link
Member Author

Though it looks the same if I always use SK_ColorWHITE?

@bdero
Copy link
Member

bdero commented May 8, 2023

Skia's internal atlas (assuming it's actually storing one) is probably not going to have the color-based alpha correction applied to it, lest they would have to generate different glyphs based on a color source state hash. My guess would be that the alpha is linear in their internal atlas. Then, when they sample it, they just map it through a simple curve (like an exponential that's tuned based on some computed property of the color -- like brightness).

@jonahwilliams
Copy link
Member Author

I don't think they're doing that. Here is the text fragment shader that gets generated:

#include <metal_stdlib>
#include <simd/simd.h>
using namespace metal;

struct sampler2D {
    texture2d<half> tex;
    sampler smp;
};
half4 sample(sampler2D i, float2 p, float b=-0.475) { return i.tex.sample(i.smp, p, bias(b)); }
half4 sample(sampler2D i, float3 p, float b=-0.475) { return i.tex.sample(i.smp, p.xy / p.z, bias(b)); }
half4 sampleLod(sampler2D i, float2 p, float lod) { return i.tex.sample(i.smp, p, level(lod)); }
half4 sampleLod(sampler2D i, float3 p, float lod) {
    return i.tex.sample(i.smp, p.xy / p.z, level(lod));
}
half4 sampleGrad(sampler2D i, float2 p, float2 dPdx, float2 dPdy) {
    return i.tex.sample(i.smp, p, gradient2d(dPdx, dPdy));
}

struct Inputs {
    float2 vTextureCoords_S0  [[user(locn0)]];
    float vTexIndex_S0  [[user(locn1)]];
    half4 vinColor_S0  [[user(locn2)]];
};
struct Outputs {
    half4 sk_FragColor [[color(0)]];
};
struct uniformBuffer {
    float4 sk_RTAdjust;
    float2 uAtlasSizeInv_S0;
};
struct Globals {
    sampler2D uTextureSampler_0_S0;
    constant uniformBuffer* _anonInterface0;
};
fragment Outputs fragmentMain(Inputs _in [[stage_in]], texture2d<half> uTextureSampler_0_S0_Tex [[texture(0)]], sampler uTextureSampler_0_S0_Smplr [[sampler(0)]], constant uniformBuffer& _anonInterface0 [[buffer(0)]], bool _frontFacing [[front_facing]], float4 _fragCoord [[position]]) {
    Globals _globals{{uTextureSampler_0_S0_Tex, uTextureSampler_0_S0_Smplr}, &_anonInterface0};
    (void)_globals;
    Outputs _out;
    (void)_out;
    half4 outputColor_S0 = _in.vinColor_S0;
    half4 texColor = sample(_globals.uTextureSampler_0_S0, _in.vTextureCoords_S0).xxxx;
    half4 outputCoverage_S0 = texColor;
    {
        _out.sk_FragColor = outputColor_S0 * outputCoverage_S0;
    }
    return _out;
}

They have an R8 glyph atlas with at scale rendered glyphs, and the sampler is a NN sampler

@jonahwilliams
Copy link
Member Author

Anyway, it turns out this does not fix the light text on dark background - and we're still bolder than Skia on the same font.

SkPaint glyph_paint;
glyph_paint.setColor(glyph_color);
glyph_paint.setColor(SK_ColorWHITE);
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to always be SK_ColorWhite, otherwise we end up darkening the dark text on light backgrounds. However this change does fix the issue that non-color glyphs were inconsistently rendered depending on whether or not we had promoted the atlas to full color.

Copy link
Member

@bdero bdero May 8, 2023

Choose a reason for hiding this comment

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

IIUC, this is going to make white text look correct, but black text look too thick, regressing the more common cases solved for in flutter/flutter#119805 and flutter/flutter#119234.

@bdero
Copy link
Member

bdero commented May 8, 2023

I don't think they're doing that.

In this case, it's probably getting baked in when the atlas is generated? I assume different subpixel AA variations of the same glyph are also getting baked into the atlas with this solution.

Anyway, it turns out this does not fix the light text on dark background - and we're still bolder than Skia on the same font.

You mean text looks thinner than Skia for light text with a dark background, right? Black text on a white background should look nearly identical to Skia at ToT.

@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #41780 at sha 242427d

@jonahwilliams
Copy link
Member Author

(but of course I need to figure out if the goldens got thinner (expected) or thicker (unexpected))

@bdero
Copy link
Member

bdero commented May 9, 2023

My worry is about the behavior of text drawn to the U8 atlas before and after this PR. I haven't tried running this PR yet, but my expectation is that all text drawn to the U8 atlas will become thicker, regressing flutter/flutter#119805 and flutter/flutter#119234.

@jonahwilliams
Copy link
Member Author

Put the color atlas check back, just in case we need it. I've also confirmed that with the color changed back we haven't adjusted how font was rendered provided that we were already using the A8 atlas . Fonts should still be thinner in general if they were included in the full color atlas before. None of our goldens cover this specific case though.

cast.2.webm
cast.3.webm

Copy link
Member

@bdero bdero left a comment

Choose a reason for hiding this comment

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

LGTM!

@jonahwilliams jonahwilliams added autosubmit Merge PR when tree becomes green via auto submit App and removed will affect goldens labels May 9, 2023
@auto-submit auto-submit bot merged commit 688782f into flutter:main May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 10, 2023
zanderso pushed a commit to engine-flutter-autoroll/flutter that referenced this pull request May 10, 2023
zanderso pushed a commit to flutter/flutter that referenced this pull request May 10, 2023
…sions) (#126455)

Manual roll requested by aaclarke@google.com

Cannot build log URL because revision "892f88d2513f" is invalid: Luci
builds of "Linux Fuchsia FEMU" for
892f88d2513f4a60ebd6d1496c4ac272e53c55ba was STARTED

2023-05-10 zanderso@users.noreply.github.com Revert "Move linux fuchsia
engine v2 build to prod." (flutter/engine#41902)
2023-05-10 godofredoc@google.com Migrate mac host clang tidy to engine
v2. (flutter/engine#41824)
2023-05-10 jonahwilliams@google.com [Impeller] delete special handling
of RRect. (flutter/engine#41872)
2023-05-10 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
JiOACcaGrDphuHIql... to a3rrULFaXlwS8mfjW... (flutter/engine#41898)
2023-05-10 109111084+yaakovschectman@users.noreply.github.com Only
register top level window message listener upon registering service
binding (flutter/engine#41733)
2023-05-10 skia-flutter-autoroll@skia.org Roll Skia from 8c936fb9ba8e to
32f4cfc2460b (27 revisions) (flutter/engine#41891)
2023-05-10 yjbanov@google.com [web] dialog a11y fixes
(flutter/engine#41681)
2023-05-10 skia-flutter-autoroll@skia.org Roll Dart SDK from
2cf089614e1c to 7ad028c26344 (2 revisions) (flutter/engine#41882)
2023-05-10 jonahwilliams@google.com [Impeller] Increase minimum size of
alpha glyph atlas. (flutter/engine#41880)
2023-05-10 jonahwilliams@google.com [Impeller] Use separate atlases and
shaders for color and alpha (flutter/engine#41780)
2023-05-10 godofredoc@google.com Move linux fuchsia engine v2 build to
prod. (flutter/engine#41865)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from JiOACcaGrDph to a3rrULFaXlwS

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC aaclarke@google.com,rmistry@google.com,zra@google.com on the
revert to ensure that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 10, 2023
zanderso pushed a commit to flutter/flutter that referenced this pull request May 10, 2023
…126468)

flutter/engine@10ac36c...406b354

2023-05-10 mdebbar@google.com [web] Re-enable history tests on Safari
(flutter/engine#41901)
2023-05-10 flar@google.com switch from MockCanvas to DisplayListBuilder
in layer unit tests (flutter/engine#41889)
2023-05-10 zanderso@users.noreply.github.com Revert "Move linux fuchsia
engine v2 build to prod." (flutter/engine#41902)
2023-05-10 godofredoc@google.com Migrate mac host clang tidy to engine
v2. (flutter/engine#41824)
2023-05-10 jonahwilliams@google.com [Impeller] delete special handling
of RRect. (flutter/engine#41872)
2023-05-10 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
JiOACcaGrDphuHIql... to a3rrULFaXlwS8mfjW... (flutter/engine#41898)
2023-05-10 109111084+yaakovschectman@users.noreply.github.com Only
register top level window message listener upon registering service
binding (flutter/engine#41733)
2023-05-10 skia-flutter-autoroll@skia.org Roll Skia from 8c936fb9ba8e to
32f4cfc2460b (27 revisions) (flutter/engine#41891)
2023-05-10 yjbanov@google.com [web] dialog a11y fixes
(flutter/engine#41681)
2023-05-10 skia-flutter-autoroll@skia.org Roll Dart SDK from
2cf089614e1c to 7ad028c26344 (2 revisions) (flutter/engine#41882)
2023-05-10 jonahwilliams@google.com [Impeller] Increase minimum size of
alpha glyph atlas. (flutter/engine#41880)
2023-05-10 jonahwilliams@google.com [Impeller] Use separate atlases and
shaders for color and alpha (flutter/engine#41780)
2023-05-10 godofredoc@google.com Move linux fuchsia engine v2 build to
prod. (flutter/engine#41865)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from JiOACcaGrDph to a3rrULFaXlwS

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC aaclarke@google.com,rmistry@google.com,zra@google.com on the
revert to ensure that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
…sions) (flutter#126455)

Manual roll requested by aaclarke@google.com

Cannot build log URL because revision "892f88d2513f" is invalid: Luci
builds of "Linux Fuchsia FEMU" for
892f88d2513f4a60ebd6d1496c4ac272e53c55ba was STARTED

2023-05-10 zanderso@users.noreply.github.com Revert "Move linux fuchsia
engine v2 build to prod." (flutter/engine#41902)
2023-05-10 godofredoc@google.com Migrate mac host clang tidy to engine
v2. (flutter/engine#41824)
2023-05-10 jonahwilliams@google.com [Impeller] delete special handling
of RRect. (flutter/engine#41872)
2023-05-10 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
JiOACcaGrDphuHIql... to a3rrULFaXlwS8mfjW... (flutter/engine#41898)
2023-05-10 109111084+yaakovschectman@users.noreply.github.com Only
register top level window message listener upon registering service
binding (flutter/engine#41733)
2023-05-10 skia-flutter-autoroll@skia.org Roll Skia from 8c936fb9ba8e to
32f4cfc2460b (27 revisions) (flutter/engine#41891)
2023-05-10 yjbanov@google.com [web] dialog a11y fixes
(flutter/engine#41681)
2023-05-10 skia-flutter-autoroll@skia.org Roll Dart SDK from
2cf089614e1c to 7ad028c26344 (2 revisions) (flutter/engine#41882)
2023-05-10 jonahwilliams@google.com [Impeller] Increase minimum size of
alpha glyph atlas. (flutter/engine#41880)
2023-05-10 jonahwilliams@google.com [Impeller] Use separate atlases and
shaders for color and alpha (flutter/engine#41780)
2023-05-10 godofredoc@google.com Move linux fuchsia engine v2 build to
prod. (flutter/engine#41865)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from JiOACcaGrDph to a3rrULFaXlwS

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC aaclarke@google.com,rmistry@google.com,zra@google.com on the
revert to ensure that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
…lutter#126468)

flutter/engine@10ac36c...406b354

2023-05-10 mdebbar@google.com [web] Re-enable history tests on Safari
(flutter/engine#41901)
2023-05-10 flar@google.com switch from MockCanvas to DisplayListBuilder
in layer unit tests (flutter/engine#41889)
2023-05-10 zanderso@users.noreply.github.com Revert "Move linux fuchsia
engine v2 build to prod." (flutter/engine#41902)
2023-05-10 godofredoc@google.com Migrate mac host clang tidy to engine
v2. (flutter/engine#41824)
2023-05-10 jonahwilliams@google.com [Impeller] delete special handling
of RRect. (flutter/engine#41872)
2023-05-10 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
JiOACcaGrDphuHIql... to a3rrULFaXlwS8mfjW... (flutter/engine#41898)
2023-05-10 109111084+yaakovschectman@users.noreply.github.com Only
register top level window message listener upon registering service
binding (flutter/engine#41733)
2023-05-10 skia-flutter-autoroll@skia.org Roll Skia from 8c936fb9ba8e to
32f4cfc2460b (27 revisions) (flutter/engine#41891)
2023-05-10 yjbanov@google.com [web] dialog a11y fixes
(flutter/engine#41681)
2023-05-10 skia-flutter-autoroll@skia.org Roll Dart SDK from
2cf089614e1c to 7ad028c26344 (2 revisions) (flutter/engine#41882)
2023-05-10 jonahwilliams@google.com [Impeller] Increase minimum size of
alpha glyph atlas. (flutter/engine#41880)
2023-05-10 jonahwilliams@google.com [Impeller] Use separate atlases and
shaders for color and alpha (flutter/engine#41780)
2023-05-10 godofredoc@google.com Move linux fuchsia engine v2 build to
prod. (flutter/engine#41865)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from JiOACcaGrDph to a3rrULFaXlwS

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC aaclarke@google.com,rmistry@google.com,zra@google.com on the
revert to ensure that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
3 participants