Closed Bug 772726 Opened 11 years ago Closed 11 years ago

nsCanvasRenderingContext2DAzure::DrawWindow always uses Thebes for content drawing even if Azure-content is enabled

Categories

(Core :: Graphics: Canvas2D, defect)

Product:
Core
Component:
Graphics: Canvas2D
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(15 files, 4 obsolete files)

fix
3.43 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
part 2: fix Transform3D crash
1.27 KB, patch
mattwoodrow
: review+
roc
: checkin+
Details | Diff | Splinter Review
Part 4: Fix SampleTextTexturesPS(Masked) shaders
76.20 KB, patch
jrmuizel
: review+
roc
: checkin+
Details | Diff | Splinter Review
part 3 v2 real patch
5.77 KB, patch
bas.schouten
: review+
roc
: checkin+
Details | Diff | Splinter Review
Part 6: Add NudgeToIntegers operation to Azure Matrix, add MultiplyAndNudgeToIntegers to gfxContext, and call it from SVG when we render path geometry
5.15 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
Part 7: Avoid specifying "repeat" mode when rendering CSS gradients if we don't need it
8.69 KB, patch
padenot
: review+
Details | Diff | Splinter Review
Part 9: Since Azure doesn't support EXTEND_NONE, when blitting a post-3D-transform temporary surface we need to make sure the output is clipped to the area of the surface
7.84 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
Part 5: Mark gradient tests as pass/fuzzy
3.51 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
Part 10: Reduce scale to avoid floating-point accuracy errors that cause small artifacts with D2D Azure content drawing
1.21 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
Part 12: Mark test failure with Azure content drawing, due to apparent D2D drawing bug (bug 789402)
1.62 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
Part 13: Fix handling of transforms in gfxTextObjectPaint
27.04 KB, patch
eflores
: review+
jwatt
: review+
Details | Diff | Splinter Review
Part 8: Add Rect::NudgeToIntegers and share improved nudging code among all nudging APIs
6.62 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
Part 14: Nudge pattern transform components to integers to avoid rounding errors. Also nudge rects to integers when we retransform them due to a CTM change
2.27 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
Part 15: Change check for Azure content drawing to handle cases where Azure is preffed on but isn't being used
2.30 KB, patch
nrc
: review+
Details | Diff | Splinter Review
Patch for the D2D clipping issue
21.84 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review 
Patch coming up.

This is quite scary. Where reftests are using USE_WIDGET_LAYERS this shouldn't affect anything. When they aren't, and Azure-content is preffed on, this will switch them from using Thebes to using Azure!
In particular, it seems to me that Windows 7 reftests will switch from using cairo to using Azure!
Attached patch fix — — Splinter Review 

        Attachment #640896 -
        Flags: review?(bas.schouten)

    
    

    
  
Comment on attachment 640896 [details] [diff] [review]
fix

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

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +3962,5 @@
>  
> +  Matrix matrix = mTarget->GetTransform();
> +  // gfxContext-over-Azure needs to know the transform, so remove the
> +  // transform from the DrawTarget and reapply it via the gfxContext
> +  mTarget->SetTransform(Matrix());

Why do we need to remove it here? We actually do mDT->SetTransform(Matrix()) in the gfxContext constructor. We should only need to store the old one.

@@ +3966,5 @@
> +  mTarget->SetTransform(Matrix());
> +  nsRefPtr<gfxContext> thebes;
> +  if (gfxPlatform::UseAzureContentDrawing()) {
> +    thebes = new gfxContext(mTarget);
> +    mTarget->SetTransform(Matrix());

What's this additional SetTransform doing? Doesn't seem useful as far as I can tell.

        Attachment #640896 -
        Flags: review?(bas.schouten) → review+

    
    

    
  
Good points. Removed unnecessary SetTransforms and fixed comment.

    
    

    
  


  
Note that this code is bogus --- the "fast path" where we composite directly to an imagesurface is never actually used --- but that's an existing bug.

        Attachment #641709 -
        Flags: review?(matt.woodrow)

    
    

    
  
Comment on attachment 641713 [details] [diff] [review]
Part 3: Optimize DrawTargetD2D::GetClippedGeometry

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

::: gfx/2d/DrawTargetD2D.cpp
@@ +1619,5 @@
> +    factory()->CreateRectangleGeometry(&rectangleIntersection, byRef(rectGeom));
> +    mCurrentClippedGeometry = rectGeom.forget();
> +    return mCurrentClippedGeometry;
> +  }
> +

This looks fine to me, but we -could- do it by only iterating over the clips once. By in the other iteration keeping a Rect for as long as we haven't hit the first path. And then when we hit the first non-rectangular clip we create the first geometry and continue in the regular way.

That would also reduce the amount of combine with geometry with a large clip stack with mostly rects and a geometry at the end.

        Attachment #641713 -
        Flags: review?(bas.schouten) → review+

        Attachment #641714 -
        Flags: review?(jmuizelaar) → review+

        Attachment #641709 -
        Flags: review?(matt.woodrow) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 3 v2 (obsolete)
        — 
          — Splinter Review
      

    


  
Rewrite part 3 the way Bas suggested. Also fixes the intersection code to actually compute intersection instead of union!

        Attachment #641713 -
        Attachment is obsolete: true

        Attachment #643256 -
        Flags: review?(bas.schouten)
Whiteboard: [leave open]

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> Created attachment 643256 [details] [diff] [review]
> part 3 v2
> 
> Rewrite part 3 the way Bas suggested. Also fixes the intersection code to
> actually compute intersection instead of union!

Did you forget to qref? This patch still seems to take the bounds of the union. It seems identical to the old patch.

    
    

    
  


  

        Attachment #643256 -
        Attachment is obsolete: true

        Attachment #643256 -
        Flags: review?(bas.schouten)

        Attachment #643348 -
        Flags: review?(bas.schouten)

        Attachment #643348 -
        Flags: review?(bas.schouten) → review+

    
    

    
  
Down to 8 reftest failures:
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/526463-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/css-gradients/repeating-linear-1a.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/css-gradients/repeating-linear-1b.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/css-gradients/repeating-radial-1a.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/css-gradients/repeating-radial-1c.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/css-gradients/repeating-radial-1e.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/svg/marker-orientation-01.svg | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/transform-3d/rotatex-perspective-3a.html | image comparison (==)

We already agreed to fuzz away the gradient issues.

526463-1.html and marker-orientation-01.svg are only minor differences but I'll look into them.

layout/reftests/transform-3d/rotatex-perspective-3a.html is producing very different results. I definitely need to look into that.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> Checked in part 3:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5d77941eba28

Push backed out for failures in meter-native-style.html:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14292529&tree=Mozilla-Inbound

Couldn't see you in #developers and didn't seem obvious which changeset caused it, so had to back all three out.

Given this failure and the bustage on the initial landing that required a fixup, please can you consider https://groups.google.com/d/topic/mozilla.dev.platform/nrbJE1ixkIs/discussion next time using inbound.

        Attachment #658006 -
        Flags: review?(bas.schouten) → review+

    
    

    
  


  
Requesting review from Paul since I'm messing with his gradient cache.

        Attachment #659229 -
        Flags: review?(paul)

    
    

    
  


  
This fixes a major bug in BasicLayers 3D transforms which is caught when Azure content drawWindow is turned on.

        Attachment #659233 -
        Flags: review?(matt.woodrow)

    
    

    
  
We might want to push part 9 to branches.

    
    

    
  


  
The test failure I'm working around is very minor pixel differences. As far as I can tell the test still tests what it's supposed to test.

        Attachment #659236 -
        Flags: review?(dbaron)

    
    

    
  


  
Sorry, previous patch was the wrong patch.

        Attachment #659236 -
        Attachment is obsolete: true

        Attachment #659236 -
        Flags: review?(dbaron)

        Attachment #659238 -
        Flags: review?(dbaron)

    
    

    
  


  
This test depends on an SVG <pattern in a transformed SVG element exactly filling a specific rectangle of screen pixels when it copies the pattern's temporary surface to the destination. With Azure D2D, one edge of the rectangle is slightly transparent, apparently because the transform for the pattern is slightly off. My debugging showed that the correct transform component value is 0.001 and the value in Azure/D2D is the closest 'float' value to 0.001, so it seems D2D is overreacting to that error, and there's no much we can do about that.

Reducing the scale makes the problem go away, and I'm pretty sure it preserves the value of the test.

        Attachment #659242 -
        Flags: review?(jwatt)

    
    

    
  


  
D2D seems to have a bizarre one-pixel error in the top-left of the destination surface when rendering a rotated rectangle. I filed bug 789402 for that (with a simple testcase using <canvas>). That error shows up in a reftest, so here I'm using fuzz to ignore it.

        Attachment #659243 -
        Flags: review?(bas.schouten)

    
    

    
  
With those patches, I think I've covered all the test failures. This try push should show whether I really have:
https://tbpl.mozilla.org/?tree=Try&rev=bdc60a9fe657

        Attachment #659242 -
        Flags: review?(jwatt) → review+

    
    

    
  
Still more failures. I noticed that https://tbpl.mozilla.org/php/getParsedLog.php?id=15048762&tree=Try&full=1 (Ru) still has a lot of failures.

    
    

    
  
Comment on attachment 659229 [details] [diff] [review]
Part 7: Avoid specifying "repeat" mode when rendering CSS gradients if we don't need it

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

Seems fine. Thank you for cleaning the interface of RegisterEntry.

        Attachment #659229 -
        Flags: review?(paul) → review+

    
    

    
  
Comment on attachment 659238 [details] [diff] [review]
Part 11: Reduce use of text in reftest to avoid small pixel differences with Azure D2D content drawing

It seems pretty weird to have to change this; the final DOM in the two testcases is identical; it's just one gets there via a dynamic change that causes part of it to paint at a different time.  Why should we end up with differences in a case like that?  And if we do, can't we also end up with differences in tests all over the place due to incremental loading?

        Attachment #659243 -
        Flags: review?(bas.schouten) → review+

        Attachment #659235 -
        Flags: review?(bas.schouten) → review+

    
    

    
  
(In reply to David Baron [:dbaron] from comment #38)
> It seems pretty weird to have to change this; the final DOM in the two
> testcases is identical; it's just one gets there via a dynamic change that
> causes part of it to paint at a different time.  Why should we end up with
> differences in a case like that?  And if we do, can't we also end up with
> differences in tests all over the place due to incremental loading?

I suspect this is a case where a quirk of the rasterizer (D2D in this case) makes pixels dependent on the repaint region or something like that. Or possibly it's something to do with layer-based mask drawing vs Azure-based mask drawing. But I'll look into it some more.

    
    

    
  
Comment on attachment 659233 [details] [diff] [review]
Part 9: Since Azure doesn't support EXTEND_NONE, when blitting a post-3D-transform temporary surface we need to make sure the output is clipped to the area of the surface

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

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +631,5 @@
>   * @param aDest         Desintation context.
>   * @param aBounds       Area represented by aSource.
>   * @param aTransform    Transformation matrix.
> + * @param aDestRect     Output: rectangle in which to draw returned surface on aDest
> + *                      (same size as aDest). Only filled in if this returns

What is the same size as aDest?

        Attachment #659233 -
        Flags: review?(matt.woodrow) → review+

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> (In reply to David Baron [:dbaron] from comment #38)
> > It seems pretty weird to have to change this; the final DOM in the two
> > testcases is identical; it's just one gets there via a dynamic change that
> > causes part of it to paint at a different time.  Why should we end up with
> > differences in a case like that?  And if we do, can't we also end up with
> > differences in tests all over the place due to incremental loading?
> 
> I suspect this is a case where a quirk of the rasterizer (D2D in this case)
> makes pixels dependent on the repaint region or something like that. Or
> possibly it's something to do with layer-based mask drawing vs Azure-based
> mask drawing. But I'll look into it some more.

If we're going to punt on it, I'd sort of rather mark it fuzzy-if(d2d) than change the test like this.

    
    

    
  
Comment on attachment 659238 [details] [diff] [review]
Part 11: Reduce use of text in reftest to avoid small pixel differences with Azure D2D content drawing

Bas found a way to modify Azure-D2D to avoid this issue.

        Attachment #659238 -
        Attachment is obsolete: true

        Attachment #659238 -
        Flags: review?(dbaron)

        Attachment #660314 -
        Flags: review? → review?(bas.schouten)

    
    

    
  


  
This is Bas's patch for the D2D clipping issue. Not sure if he wants to get it reviewed and landed here or in some other bug.

    
    

    
  
Comment on attachment 660316 [details] [diff] [review]
Part 15: Change check for Azure content drawing to handle cases where Azure is preffed on but isn't being used

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

Did we want to rename UseAzureContentDrawing to something like IsAzureContentPreffere?

        Attachment #660316 -
        Flags: review?(ncameron) → review+

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #50)
> https://tbpl.mozilla.org/?tree=Try&rev=75214fa20ca1

This is, finally, green. On Windows 7 at least, but we shouldn't be using Azure content drawing on Windows XP so I don't expect problems there.

    
    

    
  
(In reply to Nick Cameron [:nrc] from comment #51)
> Did we want to rename UseAzureContentDrawing to something like
> IsAzureContentPreffere?

Probably, but that can be a separate patch in its own bug :-). You might want to rename UseProgressiveTilePainting similarly.

        Attachment #660313 -
        Flags: review?(eflores) → review+

    
    

    
  
Comment on attachment 660313 [details] [diff] [review]
Part 13: Fix handling of transforms in gfxTextObjectPaint

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

::: layout/svg/base/src/nsSVGGlyphFrame.cpp
@@ +1109,5 @@
>  
>    switch (mPaintType) {
>    case eStyleSVGPaintType_None:
>      pattern = new gfxPattern(gfxRGBA(0.0f, 0.0f, 0.0f, 0.0f));
> +    mPatternMatrix = gfxMatrix();

It's not really clear to me why mPatternMatrix even exists. Can you add a comment here and below noting why you're setting it to the identity matrix?

Also, if we're setting it for eStyleSVGPaintType_None and eStyleSVGPaintType_Color, should we also be setting it for eStyleSVGPaintType_ObjectFill and eStyleSVGPaintType_ObjectStroke below?

::: layout/svg/base/src/nsSVGGlyphFrame.h
@@ +321,1 @@
>        gfxMatrix mContextMatrix;

You mean "user space of the element using the pattern"?

        Attachment #660313 -
        Flags: review+ → review?(eflores)

    
    

    
  
Um, sorry, didn't mean to cancel eflores's r+.

    
    

    
  
Comment on attachment 660313 [details] [diff] [review]
Part 13: Fix handling of transforms in gfxTextObjectPaint

r=jwatt for the SVG changes.

        Attachment #660313 -
        Flags: review?(jwatt) → review+

        Attachment #660314 -
        Flags: review?(bas.schouten) → review+

    
    

    
  
Comment on attachment 660315 [details] [diff] [review]
Part 14: Nudge pattern transform components to integers to avoid rounding errors. Also nudge rects to integers when we retransform them due to a CTM change

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

::: gfx/thebes/gfxContext.cpp
@@ +2092,5 @@
>      Matrix toNewUS = mDT->GetTransform() * invMatrix;
>  
>      if (toNewUS.IsRectilinear() && mPathIsRect) {
>        mRect = toNewUS.TransformBounds(mRect);
> +      mRect.NudgeToIntegers();

It seems to me like we can't just do this? It's perfectly allowed to have a non-pixel-aligned rectangle post-transform. That's also Cairo behaviour.

    
    

    
  
The bug appears also on Windows 8 64-bit.

    
    

    
  
(In reply to Bas Schouten (:bas) from comment #57)
> It seems to me like we can't just do this? It's perfectly allowed to have a
> non-pixel-aligned rectangle post-transform. That's also Cairo behaviour.

NudgeToIntegers is safe to call on non-pixel-aligned rectangles; it won't affect them --- unless the edges are within epsilon of pixel boundaries, in which case it will snap to those boundaries. That's why NudgeToInteger has an error threshold. That is the point of NudgeToIntegers :-).

        Attachment #660315 -
        Flags: review?(bas.schouten) → review+

        Attachment #660320 -
        Flags: review?(jmuizelaar)

    
    

    
  
Comment on attachment 660320 [details] [diff] [review]
Patch for the D2D clipping issue

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

This feels really complicated and sort of makes me sad. I don't have much to suggest though.

::: gfx/2d/DrawTargetD2D.cpp
@@ +115,5 @@
> +      factory()->CreatePathGeometry(byRef(pathGeom));
> +      RefPtr<ID2D1GeometrySink> sink;
> +      pathGeom->Open(byRef(sink));
> +      mClippedArea->CombineWithGeometry(rectGeom, D2D1_COMBINE_MODE_INTERSECT, nullptr, sink);
> +      sink->Close();

Could this be a helper function that returns a RefPtr<ID2D1PathGeometry> Intersect()

::: gfx/2d/DrawTargetD2D.h
@@ +218,5 @@
>    RefPtr<ID3D10Texture2D> mTexture;
>    RefPtr<ID3D10Texture2D> mCurrentClipMaskTexture;
>    RefPtr<ID2D1Geometry> mCurrentClippedGeometry;
> +  // This is only valid if mCurrentClippedGeometry is non-null. And will
> +  // only the intersection of all pixel-aligned retangular clips. This is in

"And will only" makes no sense.

@@ +246,1 @@
>      RefPtr<PathD2D> mPath;

Add a comment about how the choice between mTransform or mIsPixelAligned is decided by mPath.

        Attachment #660320 -
        Flags: review?(jmuizelaar) → review+
Whiteboard: [leave open]
Depends on: 793175

    
  
Depends on: 793416

    
  
Depends on: 793690

    
    

    
  
Comment on attachment 660313 [details] [diff] [review]
Part 13: Fix handling of transforms in gfxTextObjectPaint

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

[Bugzilla] Your Outstanding Requests

ARE YOU HAPPY NOW, BUGZILLA?

        Attachment #660313 -
        Flags: review?(eflores) → review+

    
  
Depends on: 863635

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