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

Allow subpixel with specified background color in offscreen targets. #2701

Merged
merged 1 commit into from May 1, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Allow subpixel with specified background color in offscreen targets.

When a subpixel text run has a specified background color, we can
safely draw it into an off-screen surface.

Fix a bug in the renderer where the shader color mode was being
set before the shader was bound.

Add support in wrench for specifying the background color of
a text run.

Add a reftest that checks offscreen surface + bg color subpixel
text is the same as subpixel text on a normal background.

Fixes #2670.
  • Loading branch information
gw3583 committed Apr 30, 2018
commit 676c7ba07247a3de3b69445b4cd0d3d868b8ca34
@@ -224,6 +224,10 @@ impl FontInstance {
platform_options: Option<FontInstancePlatformOptions>,
variations: Vec<FontVariation>,
) -> Self {
// If a background color is enabled, it only makes sense
// for it to be completely opaque.
debug_assert!(bg_color.a == 0 || bg_color.a == 255);

FontInstance {
font_key,
size,
@@ -728,7 +728,7 @@ impl TextRunPrimitiveCpu {
display_list: &BuiltDisplayList,
frame_building_state: &mut FrameBuildingState,
) {
if !allow_subpixel_aa {
if !allow_subpixel_aa && self.font.bg_color.a == 0 {

This comment has been minimized.

@Gankra

Gankra Apr 30, 2018

Contributor

shouldn't this be < 1.0 ?

This comment has been minimized.

@glennw

glennw Apr 30, 2018

Author Member

We use the != 0 check in a few places, and the comment where the field is declared says:

    /// When bg_color.a is != 0 and render_mode is FontRenderMode::Subpixel,
    /// the text will be rendered with bg_color.r/g/b as an opaque estimated
    /// background color.

So I think it's fine to leave it as is? Perhaps I should add an assert that the alpha in this case is either 0 or 255 though, since any other value doesn't really make sense here?

This comment has been minimized.

@Gankra

Gankra Apr 30, 2018

Contributor

yeah, the assert sounds like a good compromise!

This comment has been minimized.

@glennw

glennw Apr 30, 2018

Author Member

OK, rebased + added an assert in FontInstance::new.

self.font.render_mode = self.font.render_mode.limit_by(FontRenderMode::Alpha);
}

@@ -2891,6 +2891,13 @@ impl Renderer {
}

for batch in &alpha_batch_container.alpha_batches {
self.shaders
.get(&batch.key)
.bind(
&mut self.device, projection,
&mut self.renderer_errors,
);

if batch.key.blend_mode != prev_blend_mode {
match batch.key.blend_mode {
BlendMode::None => {
@@ -2924,13 +2931,6 @@ impl Renderer {
prev_blend_mode = batch.key.blend_mode;
}

self.shaders
.get(&batch.key)
.bind(
&mut self.device, projection,
&mut self.renderer_errors,
);

// Handle special case readback for composites.
if let BatchKind::Brush(BrushBatchKind::MixBlend { task_id, source_id, backdrop_id }) = batch.key.kind {
// composites can't be grouped together because
@@ -0,0 +1,8 @@
---
root:
items:
- text: "A"
origin: 30 220
size: 200
color: black
font: "FreeSans.ttf"
@@ -0,0 +1,15 @@
# verify that drawing a text run on an off-screen surface with a
# specified background color gives the same result as drawing a
# subpixel text run directly on the background.
---
root:
items:
- type: stacking-context
transform-style: preserve-3d
items:
- text: "A"
origin: 30 220
size: 200
color: black
font: "FreeSans.ttf"
bg-color: white
@@ -58,3 +58,4 @@ platform(linux) == two-shadows.yaml two-shadows.png
fuzzy(1,68) platform(linux) == shadow-transforms.yaml shadow-transforms.png
fuzzy(1,71) platform(linux) == raster-space.yaml raster-space.png
!= allow-subpixel.yaml allow-subpixel-ref.yaml
== bg-color.yaml bg-color-ref.yaml
@@ -467,6 +467,7 @@ impl Wrench {
size: Au,
flags: FontInstanceFlags,
render_mode: Option<FontRenderMode>,
bg_color: Option<ColorU>,
) -> FontInstanceKey {
let key = self.api.generate_font_instance_key();
let mut update = ResourceUpdates::new();
@@ -475,6 +476,9 @@ impl Wrench {
if let Some(render_mode) = render_mode {
options.render_mode = render_mode;
}
if let Some(bg_color) = bg_color {
options.bg_color = bg_color;
}
update.add_font_instance(key, font_key, size, Some(options), None, Vec::new());
self.api.update_resources(update);
key
@@ -198,7 +198,7 @@ pub struct YamlFrameReader {
image_map: HashMap<(PathBuf, Option<i64>), (ImageKey, LayoutSize)>,

fonts: HashMap<FontDescriptor, FontKey>,
font_instances: HashMap<(FontKey, Au, FontInstanceFlags), FontInstanceKey>,
font_instances: HashMap<(FontKey, Au, FontInstanceFlags, Option<ColorU>), FontInstanceKey>,
font_render_mode: Option<FontRenderMode>,
allow_mipmaps: bool,

@@ -544,19 +544,21 @@ impl YamlFrameReader {
&mut self,
font_key: FontKey,
size: Au,
bg_color: Option<ColorU>,
flags: FontInstanceFlags,
wrench: &mut Wrench,
) -> FontInstanceKey {
let font_render_mode = self.font_render_mode;

*self.font_instances
.entry((font_key, size, flags))
.entry((font_key, size, flags, bg_color))
.or_insert_with(|| {
wrench.add_font_instance(
font_key,
size,
flags,
font_render_mode,
bg_color,
)
})
}
@@ -1117,6 +1119,8 @@ impl YamlFrameReader {
) {
let size = item["size"].as_pt_to_au().unwrap_or(Au::from_f32_px(16.0));
let color = item["color"].as_colorf().unwrap_or(*BLACK_COLOR);
let bg_color = item["bg-color"].as_colorf().map(|c| c.into());

let mut flags = FontInstanceFlags::empty();
if item["synthetic-italics"].as_bool().unwrap_or(false) {
flags |= FontInstanceFlags::SYNTHETIC_ITALICS;
@@ -1146,6 +1150,7 @@ impl YamlFrameReader {
let font_key = self.get_or_create_font(desc, wrench);
let font_instance_key = self.get_or_create_font_instance(font_key,
size,
bg_color,
flags,
wrench);

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.