602200 - Canvas content is blurred and washed out with D3D10 layers

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Csaba Kozák [:WonderCsabo]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20101004 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20101004 Firefox/4.0b7pre

All of canvas content seems to be blurred and washed out with D3D10 layers used.

Reproducible: Always

Steps to Reproduce:
1. Turn on D3D10 layers
2. Open a page with canvas content like this:
http://www.phoboslab.org/biolab/
Actual Results:  
The canvas seems to be blurred and washed out

Expected Results:  
The canvas should be drawn like with D3D9 layers

Comment 1

[Csaba Kozák [:WonderCsabo]]

Attachment: Screenshot demonstrating the issue

Comment 2

[Alex]

Attachment: Same issue on OS X

I'm seeing the same drawing problem with OpenGL layers on OS X, so it looks like it's a common issue.

Comment 3

[Csaba Kozák [:WonderCsabo]]

Yeah, i tried OGL layers on Windows, and it reproduces the problem, too. I guess it's the same on Linux w/ OGL layers, too.

But it's fine w/ D3D9 layers or w/ layers OFF.

Comment 4

[Bas Schouten (:bas.schouten)]

Please note that D3D9 is currently using point sampling and does not respect the 'mFilter' set on a CanvasLayer, this might cause D3D9 to hide the bug.

Comment 5

[Csaba Kozák [:WonderCsabo]]

Sorry, i'm not that expert to figure out that. Thanks!

Should i rename the bug to "... with accelerated layers" ?

Comment 6

[Bas Schouten (:bas.schouten)]

The problem here is that the transform set on the canvas layer specifies a non-integer (712.5 in my case for this page). This causes the quad to be positioned incorrectly. This is dependent on the width/height of the page and is probably caused by something dividing it by two. Before the transform on the CanvasLayer is set we should probably round the values.

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Round position of CanvasLayers

I think this fix is correct, since we always position our canvas layers pixel aligned. I could be wrong though, so please have a careful look at this for me tn :).

Comment 8

[Joe Drew (not getting mail)]

We should add a test for this bug too.

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to comment #8)
> We should add a test for this bug too.

I'm not sure what sort of test would have value here. If the layout guys have any suggestion we should probably do it.

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 481251 [details] [diff] [review]
Round position of CanvasLayers

roc should probably take a look.

We do a similar thing for video layers, do we want to round there too?

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to comment #9)
> (In reply to comment #8)
> > We should add a test for this bug too.
> 
> I'm not sure what sort of test would have value here. If the layout guys have
> any suggestion we should probably do it.

Does a simple reftest with a canvas at fractional pixel location not show the problem?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BasicImageLayer and BasicCanvasLayer both do aContext->PixelSnappedRectangleAndSetPattern() to draw their contents. I think we should do something similar in the other backends.

Comment 13

[Bas Schouten (:bas.schouten)]

(In reply to comment #12)
> BasicImageLayer and BasicCanvasLayer both do
> aContext->PixelSnappedRectangleAndSetPattern() to draw their contents. I think
> we should do something similar in the other backends.

We can't handle this for transforms unless we move transforms into software. There's no way for us (that I can think of) to snap to pixels at the end of the vertex shader stage.

Comment 14

[Bas Schouten (:bas.schouten)]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > We should add a test for this bug too.
> > 
> > I'm not sure what sort of test would have value here. If the layout guys have
> > any suggestion we should probably do it.
> 
> Does a simple reftest with a canvas at fractional pixel location not show the
> problem?

It probably does, but I'm not sure how useful that test would be. We should probably define behavior and then add a test which fully confirms that behavior is as we expect it.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #13)
> (In reply to comment #12)
> > BasicImageLayer and BasicCanvasLayer both do
> > aContext->PixelSnappedRectangleAndSetPattern() to draw their contents. I think
> > we should do something similar in the other backends.
> 
> We can't handle this for transforms unless we move transforms into software.
> There's no way for us (that I can think of) to snap to pixels at the end of the
> vertex shader stage.

You can just adjust the transform matrix to get the snapping effect. In effect, that's all PixelSnappedRectangleAndSetPattern does.

Comment 16

[Bas Schouten (:bas.schouten)]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > BasicImageLayer and BasicCanvasLayer both do
> > > aContext->PixelSnappedRectangleAndSetPattern() to draw their contents. I think
> > > we should do something similar in the other backends.
> > 
> > We can't handle this for transforms unless we move transforms into software.
> > There's no way for us (that I can think of) to snap to pixels at the end of the
> > vertex shader stage.
> 
> You can just adjust the transform matrix to get the snapping effect. In effect,
> that's all PixelSnappedRectangleAndSetPattern does.

Hrm, I saw that. That is one of the ugliest things I've ever seen though. I'd be more inclined to add a function to gfx3DMatrix or something that functions can call that desire this behavior. I don't think doing this for all layers indiscriminately is a particularly good idea, in fact, it's probably a bad idea. It seems like pretty unpredictable behavior, additionally when for example there's some transform animation going on it could transition from being pixel snapped to being non-pixel snapped.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #16)
> Hrm, I saw that. That is one of the ugliest things I've ever seen though. I'd
> be more inclined to add a function to gfx3DMatrix or something that functions
> can call that desire this behavior.

Sure. Maybe something like gfxMatrix::SnapToTransformRectToPixels(const gfxRect& aRect)? Then if the matrix contains at most a scale and a translation, transform aRect by *this, snap the result to pixel edges, and change *this to the matrix that maps aRect to the snapped result. Then add gfx3DMatrix which does the same thing if *this is a 2D matrix.

> I don't think doing this for all layers
> indiscriminately is a particularly good idea, in fact, it's probably a bad
> idea. It seems like pretty unpredictable behavior, additionally when for
> example there's some transform animation going on it could transition from
> being pixel snapped to being non-pixel snapped.

OK, but unfortunately in general we can't tell whether the current page layout represents the end of an animation (in which case we should snap, to avoid blurriness) or the mid-point of an animation (in which case we would perhaps like to not snap). In theory we can tell for declarative animations, but we can never tell for scripted animations.

However, it seems pretty clear we should assume that we're at the end of an animation, and snap. That's what we do all over the place in layout, and this is no different.

Now, I actually think we should be doing this snapping in layout, in FrameLayerBuilder where we set the transforms on all layers, instead of in the layer backends. Does that sound good? The only tricky thing is that in a few cases (mainly printing) we don't want to snap at all because integer "device pixels" aren't really pixels, so we'd need to detect that via new layer manager API, e.g. a ShouldSnapTransforms() method. That would be true for everything except BasicLayers, where we would check if snapping is enabled on the target gfxContext.

Comment 18

[Bas Schouten (:bas.schouten)]

(In reply to comment #17)
> (In reply to comment #16)
> Now, I actually think we should be doing this snapping in layout, in
> FrameLayerBuilder where we set the transforms on all layers, instead of in the
> layer backends. Does that sound good? The only tricky thing is that in a few
> cases (mainly printing) we don't want to snap at all because integer "device
> pixels" aren't really pixels, so we'd need to detect that via new layer manager
> API, e.g. a ShouldSnapTransforms() method. That would be true for everything
> except BasicLayers, where we would check if snapping is enabled on the target
> gfxContext.

Yes, this sounds fine! I'm not against snapping at all, I'd just rather not see it happen in the layers backend.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Here's what I suggest we do:
-- For each layer L, identify the nearest ancestor layer that is either the root layer or a layer whose transform is not just a translation+scale. Call that the snap-target S(L). We will snap L's rendering to the pixel grid of S(L).
-- If L has an intrinsic size (W,H) (e.g. ImageLayers or CanvasLayers), transform the rectangle (0,0)-(W,H) up to S(L)'s coordinate system using the ideal transforms. Snap the resulting rectangle to nearest pixel edges. Let T(L) be the translation+scale transform that maps (0,0)-(W,H) to that snapped rectangle.
-- If L has no intrinsic size, transform the point (0,0) up to S(L)'s coordinate system using the ideal transforms. Snap the resulting point to nearest pixel edges. Let T(L) be the translation+scale transform that maps (0,0) to the snapped point and scales by the same amount as the transforms up to S(L).
-- Set the layer-tree transform on each L to be T(L)*Inverse(T(P(L))).

Layout can do all this. I had thought layout wouldn't be able to make good decisions without knowing where the temporary surfaces are being created by the layer manager, but this approach should work regardless.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually there is another reason to do this snapping in the layer system: cross-process layers. Once bug 602431 is fixed, the shadow layer tree can have extra transforms added to it by the compositor (e.g. compositor-driven scrolling or animation). We should be snapping those transforms and doing the snapping taking into account those extra transforms.

So I suggest once bug 602431 is fixed, we fix this bug by doing comment #19 in GetEffectiveTransform (added in bug 602431).

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Just FTR, shadow-layer postprocessing for bug 602431 happens in layout code, but I don't think that really changes what you're proposing (I might have misinterpreted "in the layer system" as well).

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But it happens in the compositing parent. The important thing is that when the content process constructs its layer tree, it cannot know what the final transforms are.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 481251 [details] [diff] [review]
Round position of CanvasLayers

We're going to do this differently

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Here's my plan:
1) Remove existing pixel-snapping logic from BasicLayers
2) Add mEffectiveTransform matrix to all layers. Compute this matrix for all layers in a pass over the layer tree just before we paint. Use the definition in comment #19. For BasicLayers, the initial transform in the target context can be treated as an extra transform on the root layer, so we can snap all the way to the destination surface (if snapping is enabled at all). GetEffectiveTranform will return mEffectiveTransform.
3) Convert D3D9 and D3D10 layers to use GetEffectiveTransform (and maybe GetEffectiveClipRect and GetEffectiveVisibleRegion while I'm at it).

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes BasicLayers and OGL. I'll attach a separate patch on top to address D3D9 and D3D10. This applies on top of bug 599507.

The basic idea is that layout does not snap any layout transforms. Instead we add a pass over the layer tree just before rendering which computes an "effective transform" for each layer. This is the transform which should be used to render the layer into the nearest intermediate buffer for an ancestor layer, or the destination if there is no such buffer. This transform takes into account snapping and also ShadowLayer's "extra transform". It also includes the scale factor applied for variable-resolution ThebesLayers.

This is done in recursive calls to ComputeEffectiveTransforms. ComputeEffectiveTransforms takes a parameter which is the composition of all layout-specified transforms from the parent layer to the destination buffer --- the "true transform". Layer-specific implementations then call InternalComputeEffectiveTransform to combine in the layer's own transform, any ShadowLayer "extra transform", and any layer-specific "local transform" such as the scaling to account for ThebesLayer resolution. Callers of InternalComputeEffectiveTransform can also pass in a rectangle that should be snapped to pixels in the destination buffer; InternalComputeEffectiveTransform tweaks the effective transform to achieve such snapping (if possible).

ImageLayers and CanvasLayers snap the rectangle of the image/canvas. ThebesLayers, ColorLayers and ContainerLayers just snap the point (0,0) and preserve the scaling factors of the "true transform".

Snapping can be disabled globally by a layer manager flag; BasicLayers uses this to disable snapping when drawing to a context that has snapping disabled (i.e., printing).

Rendering logic had to change a tiny bit to account for the fact that GetEffectiveTransform now transforms directly to the nearest buffer. Mainly this meant removing the incoming matrix parameter in LayerOGL::RenderLayer, and just using GetEffectiveTransform alone.

In BasicLayers, we might be drawing into a target context that already has a transform set on it. We incorporate that transform into the GetEffectiveTransform computed for all the layers. This means those transforms are properly snapped to pixels in the destination surface, and it's why we can call ctx->SetMatrix(GetEffectiveTransform()) without breaking things.

I had to hoist mBounds from CanvasLayer implementations up to CanvasLayer itself so we could use a single implementation of ComputeEffectiveTransforms for all CanvasLayer implementations.

In ContainerComputeEffectiveTransforms in ContainerLayerOGL.cpp, things are a bit tricky. We need to know the final "effective opacity" of the layer (including opacity possibly inherited from ancestors) to determine whether we need an intermediate surface, while computing effective transforms. So I added LayerOGL::GetEffectiveOpacity to calculate that, but it requires that mUseIntermediateSurface has been set on all ancestor layers. The good news is that GetEffectiveOpacity and GetEffectiveTransform eliminate the need to pass a matrix and opacity around in RenderLayer.

The D3D9/D3D10 patch will convert them to use GetEffectiveTransform and probably introduce GetEffectiveOpacity as well.

This will definitely need a tryserver run before landing.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

I'm going to do some more work on this patch so we can share code across layer maangers for D3D.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

This patch updates all layer backends. It should be complete.

This creates generic Layer APIs GetEffectiveTransform and GetEffectiveOpacity.

The effective opacities are computed on-the-fly using another new API ContainerLayer::UseIntermediateSurface().

The values of UseIntermediateSurface and GetEffectiveTransform are precomputed in a pass over the layer tree by ComputeEffectiveTransforms as above.

For non-BasicLayers, ComputeEffectiveTransforms uses a shared implementation DefaultComputeEffectiveTransforms. That method sets mUseIntermediateSurface if
a) the container layer has an effective opacity < 1 and has more than one child or b) the container layer has a non-identity transform and a child has a nonempty cliprect. I realize this will need to change for 3D transforms but hopefully we'll only need changes in one place now.

I also took the liberty of moving some common code in D3D layers out to LayerD3D9::SetShaderTransformAndOpacity and LayerD3D10::SetShaderTransformAndOpacity to avoid repeating my changes.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

Make HasMultipleChildren ignore invisible children.

There are some tryserver test failures I'm looking at, so not re-requesting review just yet.

Comment 30

[Bas Schouten (:bas.schouten)]

Comment on attachment 485263 [details] [diff] [review]
fix v2

It doesn't exactly give me warm fuzzy feelings inside, it doesn't make the Layers code any more transparent. But since I don't have any better ideas I guess this is the best possible solution. I'm worried about the additional computation done and I wonder if we can avoid it more aggressively in some cases. But I guess we can do that if it ever shows up on profiles. I looked at the D3D layer managers, and also skimmed over the OGL/Basic, however my understanding of them is much more limited. Vlad should probably have a good look at the OGL one.

>From: Robert O'Callahan <[e-mail]>
>Bug 602200. Share code to compute effective transforms and opacity, and snap effective transforms. r=bas,sr=vlad
>
>+  gfxMatrix matrix2D;
>+  if (mManager->IsSnappingEffectiveTransforms() &&
>+      trueTransformToSurface.Is2D(&matrix2D) && !matrix2D.IsSingular() &&
>+      !matrix2D.HasNonAxisAlignedTransform()) {

I feel it might we worth it to avoid this math for integer translation matrices and integer snap rects. It seems like that would be a lot of our cases.

>+    gfxMatrix snappedMatrix;
>+    gfxPoint topLeft = matrix2D.Transform(aSnapRect.TopLeft());
>+    topLeft.Round();
>+    if (aSnapRect.IsEmpty()) {
>+      snappedMatrix.xx = matrix2D.xx;
>+      snappedMatrix.yy = matrix2D.yy;
>+    } else {
>+      gfxPoint bottomRight = matrix2D.Transform(aSnapRect.BottomRight());
>+      bottomRight.Round();
>+      snappedMatrix.xx = (bottomRight.x - topLeft.x)/aSnapRect.Width();
>+      snappedMatrix.yy = (bottomRight.y - topLeft.y)/aSnapRect.Height();
>+    }
>+    snappedMatrix.x0 = topLeft.x - aSnapRect.pos.x*snappedMatrix.xx;
>+    snappedMatrix.y0 = topLeft.y - aSnapRect.pos.y*snappedMatrix.yy;

I understand what's happening here. But it could be documented a little bit to preserve the rationale. i.e. first create matrix that transforms snaprect to non-pixel aligned position integer size rectangle. and then later adjust translation to place the rectangle at pixel boundaries.

>+  /**
>+   * This returns the effective transform computed by
>+   * ComputeEffectiveTransforms. Typically this is a transform that transforms
>+   * this layer all the way to some intermediate surface or destination
>+   * surface. For non-BasicLayers this will be a transform to the nearest
>+   * ancestor with UseIntermediateSurface() (or to the root, if there is no
>+   * such ancestor), but for BasicLayers it's different.
>+   */
>+  const gfx3DMatrix& GetEffectiveTransform() { return mEffectiveTransform; }

This function isn't const itself? As far as I can see it doesn't have to return a const version of the matrix? If we do want this to return a const reference for some reason how about we provide a non-const version for the terrible looking const casting in D3D10 layers?

Comment 31

[Bas Schouten (:bas.schouten)]

I've been looking at it some more, and I'm wondering if we're properly preventing cumulative rounding errors leading to displacement when a lot of containers use intermediate surfaces? I might be missing something.

It seems to me like you'd want to pass in a matrix to ComputeEffectiveTransformsForChildren in the useIntermediateSurface case that compensates for the amount of sub-pixel rounding that occurs when blending the container to the background?

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #30)
> I feel it might we worth it to avoid this math for integer translation matrices
> and integer snap rects. It seems like that would be a lot of our cases.

Yeah. But I don't want to do that unless it starts showing up on profiles.

> >+    gfxMatrix snappedMatrix;
> >+    gfxPoint topLeft = matrix2D.Transform(aSnapRect.TopLeft());
> >+    topLeft.Round();
> >+    if (aSnapRect.IsEmpty()) {
> >+      snappedMatrix.xx = matrix2D.xx;
> >+      snappedMatrix.yy = matrix2D.yy;
> >+    } else {
> >+      gfxPoint bottomRight = matrix2D.Transform(aSnapRect.BottomRight());
> >+      bottomRight.Round();
> >+      snappedMatrix.xx = (bottomRight.x - topLeft.x)/aSnapRect.Width();
> >+      snappedMatrix.yy = (bottomRight.y - topLeft.y)/aSnapRect.Height();
> >+    }
> >+    snappedMatrix.x0 = topLeft.x - aSnapRect.pos.x*snappedMatrix.xx;
> >+    snappedMatrix.y0 = topLeft.y - aSnapRect.pos.y*snappedMatrix.yy;
> 
> I understand what's happening here. But it could be documented a little bit to
> preserve the rationale. i.e. first create matrix that transforms snaprect to
> non-pixel aligned position integer size rectangle. and then later adjust
> translation to place the rectangle at pixel boundaries.

All that's really happening here is a) first compute scale factors that will transform aSnapRect to the snapped rect then b) compute transform that will transform aSnapRect to the snapped rect, given those scale factors.

> This function isn't const itself?

I'll make it const.

> As far as I can see it doesn't have to return
> a const version of the matrix? If we do want this to return a const reference
> for some reason how about we provide a non-const version for the terrible
> looking const casting in D3D10 layers?

Logically, the matrix is const; callers must not modify it. So I think it should be declared const. The const-cast in D3D10 sucks but it belongs there, since the D3D10 API has the wrong signature.

(In reply to comment #31)
> I've been looking at it some more, and I'm wondering if we're properly
> preventing cumulative rounding errors leading to displacement when a lot of
> containers use intermediate surfaces? I might be missing something.
> 
> It seems to me like you'd want to pass in a matrix to
> ComputeEffectiveTransformsForChildren in the useIntermediateSurface case that
> compensates for the amount of sub-pixel rounding that occurs when blending the
> container to the background?

You're right. I'll fix it.

Comment 33

[Bas Schouten (:bas.schouten)]

(In reply to comment #32)
> (In reply to comment #30)
> All that's really happening here is a) first compute scale factors that will
> transform aSnapRect to the snapped rect then b) compute transform that will
> transform aSnapRect to the snapped rect, given those scale factors.
I'd say:

a) first compute scale factors that scale aSnapRect to the snapped rect
b) compute translation factors that will move aSnapRect to the snapped rect given those scale factors

In any case, I think a comment there would be helpful if others looked at it in the future.

> Logically, the matrix is const; callers must not modify it. So I think it
> should be declared const. The const-cast in D3D10 sucks but it belongs there,
> since the D3D10 API has the wrong signature.

I wish you weren't right but I think you are. We're stuck with it.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #32)
> (In reply to comment #30)
> > I feel it might we worth it to avoid this math for integer translation matrices
> > and integer snap rects. It seems like that would be a lot of our cases.
> 
> Yeah. But I don't want to do that unless it starts showing up on profiles.

Actually I just did it, since it's just one line.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v4

I found a bug causing reftest failures. InternalComputeEffectiveTransform was multiplying the true transforms in the wrong order. Oops.

I also have InternalComputeEffectiveTransform returning a "residual transform" which ContainerLayer::ComputeEffectiveTransforms can apply to its children.

There are still a few reftest failures on Mac with this patch. The test results look OK, I suspect the problem is inconsistent color channel rounding in tests with partial opacity applied to ThebesLayers. I may have to wait until I get back to my Mac on Tuesday to figure that out.

Can still get review though. The Mac issue is probably minor, perhaps something we'll need to work around.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: ThebesLayers vs CanvasLayers

Here's a testcase that the patch doesn't handle correctly.

This is actually quite a deep problem. What happens is that we render content to the ThebesLayer assuming that 0,0 in ThebesLayer coordinates is aligned at pixel boundaries. In this case the left border-edge is rendered over x coordinates 0.4-1.4, which gets snapped to 0-1. But the transform means that overall on-screen the left border-edge covers 0.8-1.8, so the "correct" snapped drawing would be at 1-2. Because the canvas is drawn with that ideal snapping, it starts at x=2, so we get a visible gap between the border and the canvas.

To get the ideal rendering of the border, we could apply the 'residual transform' in the patch as we draw into the ThebesLayer, which would cause the border to snap to 1-2 in the ThebesLayer. But that seems like a really bad solution because it means changes to the transform would require the entire ThebesLayer to be rerendered. We really want to avoid that.

Given we can't apply a fixup transform when drawing into the ThebesLayer, rendering of a transformed ThebesLayer cannot exactly match what we'd get without the layer. I think we can live with that though.

The only way to fix this testcase, then, is to snap transforms for non-Thebes leaf layers consistently with the way we snap for ThebesLayers. This requires some kind of interface change since currently CanvasLayers and ImageLayers have no idea what their offset is from the 0,0 in sibling ThebesLayers.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v6

This fixes the issues illustrated in the previous testcase (and adds that testcase as a reftest).

I refactored Layer::InternalComputeEffectiveTransform into two methods: GetLocalTransform() and SnapTransform(). Then for CanvasLayers and ImageLayers I emulate the way they would be snapped if we were to draw the image via a ThebesLayer: we snap the transform the container passed down to us the way a ThebesLayer would, and separately snap the layer's own transform the way gfxContext would when drawing into a ThebesLayer.

I also took out the changes that moved handling of X/YResolution from ThebesLayerBuffer to the ThebesLayer's effective-transform. They weren't tested and may not be needed anyway. If we discover they are needed, we can do that in a separate patch.

This patch also tweaks a couple of reftests that failed on Mac. On Mac, it seems that filling a surface with a rgb(160,160,160) and then blending it to a target using CGContextSetAlpha(0.5) can produce different results than painting a solid color rgba(160,160,160,0.5). I modified the reftests to avoid the solid-color path by having more than just a single color in relevant layers.

I'm running this all through tryserver again now.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v7

Bas pointed out that I wasn't using 'residual'. This fixes that, and adds a test to ensure we are using the residual.

Comment 40

[Bas Schouten (:bas.schouten)]

Comment on attachment 486191 [details] [diff] [review]
fix v7

I think Vlad should verify the OpenGL parts, and maybe have a quick look over the BasicLayers parts, both of these I'm not that familiar with. Other than that it looks good!

Comment 41

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 486191 [details] [diff] [review]
fix v7

looks reasonable here

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Matt, you said you saw a reftest failure on 602200-2.html on Mac+GL with this patch, right?

I can't reproduce it ... my Mac doesn't run GL, and the test fails on my Linux+GL box because the canvas renders upside down --- which is not this bug!!

Comment 43

[Matt Woodrow (:mattwoodrow)]

I had failures on both -2 and -3. -3 was happening even without GL layers, -2 was only with GL enabled. I can do a build with the patch applied tomorrow and grab the data URLs and debug if you have any suggestions on where to look.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK. I did run the tests on my Mac without GL and didn't see the failure you mentioned (I got some 2-pixel reftest failures, but that was just aggressive subpixel-AA getting clipped, so I don't think that's what you meant).

I just ran the tests on Linux without GL and got no failures too.

I'd love to catch you on IRC and debug this with you.

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v8

Fixed some failing tests.

Comment 46

[Matt Woodrow (:mattwoodrow)]

Attachment: fix v9

Fix tests on Mac/OpenGL

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This may have caused a crashtest assertion on Linux try:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1288963124.1288963471.3125.gz&fulltext=1

Comment 48

[Matt Woodrow (:mattwoodrow)]

That looks like bug 597915

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/f35c89eac392

Comment 50

[Martin Eklund]

I get this problem (but vertically instead of horizontally) on Firefox 4.0b7:
https://developer.mozilla.org/samples/canvas-tutorial/2_1_canvas_rect.html

(I suspect it is not related to drawing of primitives,
 since I get the same with pixels plotted with putImageData(),
 they get 'smeared' into 2 pixels vertically)


This is on Win 7 64bit, NVIDIA GeForce GT 430 (Driver 8.17.12.6099)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7) Gecko/20100101 Firefox/4.0b7

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can you retest with a nightly build please?

Comment 52

[Martin Eklund]

Yep! It seems fixed in nightly (tested 13 dec)