Last Comment Bug 717393 - [Azure]Subpixel AA is disabled
: [Azure]Subpixel AA is disabled
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
: 718204 738106 (view as bug list)
Depends on: 755741 756424 756427
Blocks: 715768
  Show dependency treegraph
 
Reported: 2012-01-11 13:44 PST by Leman Bennett [Omega]
Modified: 2012-05-20 13:41 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add helper for creating DWriteGlyphRuns (5.39 KB, patch)
2012-05-13 19:08 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 2: Refactor clip management and add ClippedMask texture (11.96 KB, patch)
2012-05-13 19:10 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 3: Cache clipped geometry (4.80 KB, patch)
2012-05-13 19:11 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 4: Add code to draw subpixel-AA to transparent surfaces (22.24 KB, patch)
2012-05-13 19:14 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 5: Manage subpixel AA permissions (5.16 KB, patch)
2012-05-13 19:24 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review

Description Leman Bennett [Omega] 2012-01-11 13:44:36 PST
Bas already knows but I'll file anyway.

Subpixel AA is not available when Azure-Thebes wrapper is enabled.
Comment 1 Joe Drew (not getting mail) 2012-01-14 10:08:25 PST
*** Bug 718204 has been marked as a duplicate of this bug. ***
Comment 2 Bas Schouten (:bas.schouten) 2012-05-13 19:08:51 PDT
Created attachment 623551 [details] [diff] [review]
Part 1: Add helper for creating DWriteGlyphRuns
Comment 3 Bas Schouten (:bas.schouten) 2012-05-13 19:10:32 PDT
Created attachment 623554 [details] [diff] [review]
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) 2012-05-13 19:11:13 PDT
Created attachment 623555 [details] [diff] [review]
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) 2012-05-13 19:14:36 PDT
Created attachment 623558 [details] [diff] [review]
Part 4: Add code to draw subpixel-AA to transparent surfaces
Comment 6 Bas Schouten (:bas.schouten) 2012-05-13 19:24:16 PDT
Created attachment 623563 [details] [diff] [review]
Part 5: Manage subpixel AA permissions
Comment 7 Jeff Muizelaar [:jrmuizel] 2012-05-14 10:40:22 PDT
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] 2012-05-14 11:17:52 PDT
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) 2012-05-14 14:19:22 PDT
(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) 2012-05-14 14:20:21 PDT
(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 11 Bas Schouten (:bas.schouten) 2012-05-14 14:33:13 PDT
*** Bug 738106 has been marked as a duplicate of this bug. ***
Comment 14 Leman Bennett [Omega] 2012-05-16 14:40:53 PDT
This is causing graphics corruption in chrome and certain content javascript overlays.
Comment 15 Jim Jeffery not reading bug-mail 1/2/11 2012-05-16 15:39:57 PDT
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) 2012-05-16 17:25:39 PDT
(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) 2012-05-16 17:33:00 PDT
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) 2012-05-16 17:35:10 PDT
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) 2012-05-16 17:55:06 PDT
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 [:streetwolf] 2012-05-16 18:20:02 PDT
I already did:  https://bugzilla.mozilla.org/show_bug.cgi?id=755741
Comment 21 Ed Morley [:emorley] 2012-05-17 03:13:07 PDT
(In reply to Bas Schouten (:bas) from comment #18)
> Pushed the regenerated files. 

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

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