Last Comment Bug 736134 - [Azure] Implement more advanced DWrite text rendering parameters
: [Azure] Implement more advanced DWrite text rendering parameters
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla14
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-15 09:34 PDT by Bas Schouten (:bas.schouten)
Modified: 2012-03-21 15:25 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add GlyphRenderingOptions API (12.74 KB, patch)
2012-03-15 09:36 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Review
Part 2: Add rendering params objects to windows platform (5.44 KB, patch)
2012-03-15 09:37 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review-
Details | Diff | Review
Part 3: Use GlyphRenderingOptions API (10.26 KB, patch)
2012-03-15 09:38 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Review
Part 2: Add rendering params objects to windows platform v2 (5.58 KB, patch)
2012-03-19 10:57 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Review

Description Bas Schouten (:bas.schouten) 2012-03-15 09:34:50 PDT
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) 2012-03-15 09:36:00 PDT
Created attachment 606255 [details] [diff] [review]
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) 2012-03-15 09:37:48 PDT
Created attachment 606258 [details] [diff] [review]
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) 2012-03-15 09:38:49 PDT
Created attachment 606261 [details] [diff] [review]
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) 2012-03-15 09:40:21 PDT
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] 2012-03-16 08:16:06 PDT
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] 2012-03-16 08:21:21 PDT
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) 2012-03-17 08:59:49 PDT
(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) 2012-03-19 10:57:41 PDT
Created attachment 607216 [details] [diff] [review]
Part 2: Add rendering params objects to windows platform v2
Comment 9 Jeff Muizelaar [:jrmuizel] 2012-03-19 11:46:11 PDT
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 11 Mounir Lamouri (:mounir) 2012-03-20 03:57:30 PDT
https://hg.mozilla.org/mozilla-central/rev/063352a44541
Comment 13 Gary [:streetwolf] 2012-03-20 09:48:02 PDT
Setting "gfx.font_rendering.cleartype_params.rendering_mode" to 1 causes all text to disappear.

Note You need to log in before you can comment on or make changes to this bug.