736134 - [Azure] Implement more advanced DWrite text rendering parameters

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

With cairo drawing we have a complicated set of code which selects our text rendering parameters. In Azure we currently don't support any of that, we should though.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add GlyphRenderingOptions API

Add an API for creating glyph rendering options that can be used in glyph rendering calls.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Add rendering params objects to windows platform

Add the renderingparams objects to gfxWindowsPlatform and initialize them independent of Azure content being on/off. This only takes only 0.1ms and will be needed anyway when Azure becomes the default for D2D.

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Use GlyphRenderingOptions API

This makes the font rendering code use the GlyphRenderingOptions Azure API to select the correct glyph rendering options.

Comment 4

[Bas Schouten (:bas.schouten)]

These patches mostly solve the issued. Font rendering looks a -lot- more consistent with them, although inside the azure code presumably we still need to do some work to use the right measuring mode corresponding with the chosen render mode, we can deal with that in a follow-up though.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 606255 [details] [diff] [review]
Part 1: Add GlyphRenderingOptions API

Review of attachment 606255 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/2D.h
@@ +488,5 @@
>  protected:
>    ScaledFont() {}
>  };
>  
> +class GlyphRenderingOptions : public RefCounted<GlyphRenderingOptions>

It would be good to add a bit of documentation about what this class is to be used for and why it's designed the way it is.

::: gfx/2d/DrawTargetD2D.cpp
@@ +896,5 @@
>  
> +  IDWriteRenderingParams *params = NULL;
> +  if (aRenderOptions) {
> +    if (aRenderOptions->GetType() != FONT_DWRITE) {
> +      gfxDebug() << *this << ": Ignoring incompatible GlyphRenderingOptions.";

I think we can assert this condition.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 606258 [details] [diff] [review]
Part 2: Add rendering params objects to windows platform

Review of attachment 606258 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +1407,5 @@
>              mMeasuringMode = DWRITE_MEASURING_MODE_NATURAL;
>              break;
>          }
> +
> +        nsRefPtr<IDWriteRenderingParams> defaultRenderingParams;

Do we usually use nsRefPtr's on COM objects?

@@ +1445,5 @@
> +	    (DWRITE_PIXEL_GEOMETRY)geometry, (DWRITE_RENDERING_MODE)mode,
> +            getter_AddRefs(mRenderingParams[TEXT_RENDERING_NORMAL]));
> +        GetDWriteFactory()->CreateCustomRenderingParams(gamma, contrast, level,
> +	    (DWRITE_PIXEL_GEOMETRY)geometry, DWRITE_RENDERING_MODE_CLEARTYPE_GDI_CLASSIC,
> +            getter_AddRefs(mRenderingParams[TEXT_RENDERING_GDI_CLASSIC]));

I found this hunk unreadable. Can you make it better?

Comment 7

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> Comment on attachment 606258 [details] [diff] [review]
> Part 2: Add rendering params objects to windows platform
> 
> Review of attachment 606258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxWindowsPlatform.cpp
> @@ +1407,5 @@
> >              mMeasuringMode = DWRITE_MEASURING_MODE_NATURAL;
> >              break;
> >          }
> > +
> > +        nsRefPtr<IDWriteRenderingParams> defaultRenderingParams;
> 
> Do we usually use nsRefPtr's on COM objects?

Other than where we use mozilla::RefPtr, yes.

> 
> @@ +1445,5 @@
> > +	    (DWRITE_PIXEL_GEOMETRY)geometry, (DWRITE_RENDERING_MODE)mode,
> > +            getter_AddRefs(mRenderingParams[TEXT_RENDERING_NORMAL]));
> > +        GetDWriteFactory()->CreateCustomRenderingParams(gamma, contrast, level,
> > +	    (DWRITE_PIXEL_GEOMETRY)geometry, DWRITE_RENDERING_MODE_CLEARTYPE_GDI_CLASSIC,
> > +            getter_AddRefs(mRenderingParams[TEXT_RENDERING_GDI_CLASSIC]));
> 
> I found this hunk unreadable. Can you make it better?

Will do.

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Add rendering params objects to windows platform v2

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 607216 [details] [diff] [review]
Part 2: Add rendering params objects to windows platform v2

Review of attachment 607216 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +1433,5 @@
> +                gamma : defaultRenderingParams->GetGamma();
> +
> +        level =
> +            level >= 0.0 && level <= 1.0 ?
> +level : defaultRenderingParams->GetClearTypeLevel();

This line's indentation is broken and maybe use if blocks.

Comment 10

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d21d1ad9b35
https://hg.mozilla.org/integration/mozilla-inbound/rev/191e76bacf77
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0a61adc10d

Comment 11

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/063352a44541

Comment 12

[Mounir Lamouri (:mounir)]

Previous changeset was a follow-up.

Original changesets are:
https://hg.mozilla.org/mozilla-central/rev/4d21d1ad9b35
https://hg.mozilla.org/mozilla-central/rev/191e76bacf77
https://hg.mozilla.org/mozilla-central/rev/bc0a61adc10d

Comment 13

[Gary [:streetwolf52]]

Setting "gfx.font_rendering.cleartype_params.rendering_mode" to 1 causes all text to disappear.