717393 - [Azure]Subpixel AA is disabled

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Leman Bennett [Omega]]

Bas already knows but I'll file anyway.

Subpixel AA is not available when Azure-Thebes wrapper is enabled.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add helper for creating DWriteGlyphRuns

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Refactor clip management and add ClippedMask texture

This patch refactors clip management a little to be more readible. It also adds the ClipMaskTexture which is a cached texture that contains the mask defined by the current clip.

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Cache clipped geometry

As we start using GetClippedGeometry more with the patch to fix this bug, caching it is a good idea.

Comment 5

[Bas Schouten (:bas.schouten)]

Attachment: Part 4: Add code to draw subpixel-AA to transparent surfaces

Comment 6

[Bas Schouten (:bas.schouten)]

Attachment: Part 5: Manage subpixel AA permissions

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 623551 [details] [diff] [review]
Part 1: Add helper for creating DWriteGlyphRuns

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

::: gfx/2d/HelpersD2D.h
@@ +227,5 @@
> +  mat.dy = aMatrix._32;
> +  return mat;
> +}
> +
> +class AutoDWriteGlyphRun : public DWRITE_GLYPH_RUN

Not a huge fan of the Auto in the name but can't think of anything better.

@@ +270,5 @@
> +  run->allocate(aGlyphs.mNumGlyphs);
> +
> +  FLOAT *advances = const_cast<FLOAT*>(run->glyphAdvances);
> +  UINT16 *indices = const_cast<UINT16*>(run->glyphIndices);
> +  DWRITE_GLYPH_OFFSET *offsets = const_cast<DWRITE_GLYPH_OFFSET*>(run->glyphOffsets);

The const_casts here are sort of ugly. Can we do anything about them?

@@ +272,5 @@
> +  FLOAT *advances = const_cast<FLOAT*>(run->glyphAdvances);
> +  UINT16 *indices = const_cast<UINT16*>(run->glyphIndices);
> +  DWRITE_GLYPH_OFFSET *offsets = const_cast<DWRITE_GLYPH_OFFSET*>(run->glyphOffsets);
> +
> +  memset(advances, 0, sizeof(FLOAT) * aGlyphs.mNumGlyphs);

It might be clearer to get rid of the memset() and do advances[i] = 0;

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 623558 [details] [diff] [review]
Part 4: Add code to draw subpixel-AA to transparent surfaces

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

::: gfx/2d/DrawTargetD2D.cpp
@@ +895,5 @@
> +      aOptions.mCompositionOp == OP_OVER && aPattern.GetType() == PATTERN_COLOR) {
> +    if (FillGlyphsManual(font, aBuffer,
> +                         static_cast<const ColorPattern*>(&aPattern)->mColor,
> +                         params, aOptions)) {
> +      return;

Do we expect this to fail? If not we should probably at least produce a Warning()

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> Comment on attachment 623551 [details] [diff] [review]
> @@ +270,5 @@
> > +  run->allocate(aGlyphs.mNumGlyphs);
> > +
> > +  FLOAT *advances = const_cast<FLOAT*>(run->glyphAdvances);
> > +  UINT16 *indices = const_cast<UINT16*>(run->glyphIndices);
> > +  DWRITE_GLYPH_OFFSET *offsets = const_cast<DWRITE_GLYPH_OFFSET*>(run->glyphOffsets);
> 
> The const_casts here are sort of ugly. Can we do anything about them?

I can't think of anything, the constness is set in the inherited MS class.

> 
> @@ +272,5 @@
> > +  FLOAT *advances = const_cast<FLOAT*>(run->glyphAdvances);
> > +  UINT16 *indices = const_cast<UINT16*>(run->glyphIndices);
> > +  DWRITE_GLYPH_OFFSET *offsets = const_cast<DWRITE_GLYPH_OFFSET*>(run->glyphOffsets);
> > +
> > +  memset(advances, 0, sizeof(FLOAT) * aGlyphs.mNumGlyphs);
> 
> It might be clearer to get rid of the memset() and do advances[i] = 0;

I did this for performance reasons, I'm fine with going the other thing but I thought this would perform slightly better.

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Comment on attachment 623558 [details] [diff] [review]
> Part 4: Add code to draw subpixel-AA to transparent surfaces
> 
> Review of attachment 623558 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetD2D.cpp
> @@ +895,5 @@
> > +      aOptions.mCompositionOp == OP_OVER && aPattern.GetType() == PATTERN_COLOR) {
> > +    if (FillGlyphsManual(font, aBuffer,
> > +                         static_cast<const ColorPattern*>(&aPattern)->mColor,
> > +                         params, aOptions)) {
> > +      return;
> 
> Do we expect this to fail? If not we should probably at least produce a
> Warning()

There's some realistic scenarios where this could fail (when outline drawing is recommended by DWrite for very large fonts and such). Failing is not a big issue as we'll just draw normally without subpixel-AA.

Comment 12

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ea09dad0f1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d3b467dcd3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4678c61c8974
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6be2987c6ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/c14773057d57

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4ea09dad0f1a
https://hg.mozilla.org/mozilla-central/rev/9d3b467dcd3e
https://hg.mozilla.org/mozilla-central/rev/4678c61c8974
https://hg.mozilla.org/mozilla-central/rev/c6be2987c6ec
https://hg.mozilla.org/mozilla-central/rev/c14773057d57

Comment 14

[Leman Bennett [Omega]]

This is causing graphics corruption in chrome and certain content javascript overlays.

Comment 15

[Jim Jeffery not reading bug-mail 1/2/11]

I also suspect its crashing my ATI video chip HD3200, system locks hard and system going in/out of video card recovery, except it never recovers forcing a 'hard powerdown'

Comment 16

[Bas Schouten (:bas.schouten)]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #15)
> I also suspect its crashing my ATI video chip HD3200, system locks hard and
> system going in/out of video card recovery, except it never recovers forcing
> a 'hard powerdown'

If that's normally never happens and it isn't blacklisted it's probably best if you raise a separate bug with your graphics info :(

(In reply to Leman Bennett (Omega X) from comment #14)
> This is causing graphics corruption in chrome and certain content javascript
> overlays.

Any chance you could point me at some STR? It would be great if you could open a bug on it with a screenshot.

Comment 17

[Bas Schouten (:bas.schouten)]

I found the likely cause, I didn't rerun the shader compiler to regenerate the shaders. Pushing a follow-up now.

Comment 18

[Bas Schouten (:bas.schouten)]

Pushed the regenerated files. I chose not to make a separate bug as that would probably confuse things. (This regeneration is really part of this bug and is a recompilation of code reviewed in this bug). If someone disagrees with that I apologize.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3bd5a8cb3b

Comment 19

[Joe Drew (not getting mail)]

Can you file a bug for (and possibly implement) the build to fail if the shaders and the bytecode get out of sync?

Comment 20

[Gary [:streetwolf52]]

I already did:  https://bugzilla.mozilla.org/show_bug.cgi?id=755741

Comment 21

[Ed Morley [:emorley]]

(In reply to Bas Schouten (:bas) from comment #18)
> Pushed the regenerated files. 

https://hg.mozilla.org/mozilla-central/rev/5e3bd5a8cb3b