607653 - avoid temporary fbos/textures on transformed layers, when possible

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

Chris Jones recently report about GL state for fennec, and said that we should probably try to avoid temporary FBO's textures when it is possible...

We also profile this stuff on Maemo, and found that it is necessary in order to get good performance on basic thebesLayer scrolling usecase.

I've checked and we have opaque surface but transform.IsIdentity() == false and we go with temporary buffer..
child clip != empty.

19:36 < Bas> romaxa: Okay, then yeah, you can't really get out of that intermediate surface, we could add code to translate the clip rect in case of integer translations.

Comment 1

[Oleg Romashin (:romaxa)]

not sure if it is duplicate for 602435 or not

Comment 2

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

Not necessarily, but this might overlap with bug 599507 and bug 602200.

Comment 3

[Oleg Romashin (:romaxa)]

hmmm... very interesting bugs... wondering why they are not yet landed...
chris do you know any other bugs with patches which might help free GPU from extra copies/composites?

I have made small hack for current problem , by simple caching offscreen buffer in containerogl... and I have almost free CPU but GPU seems overloaded now :(...

Comment 4

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

Nah, just those and the dependency tree of 598864.

Comment 5

[Oleg Romashin (:romaxa)]

good.. I've tested patches from bug 599507 and bug 602200 + patch from bug 592464

and this problem not reproducible anymore, and text looks sharp after zoom, and not problem with textures deletion.

Comment 6

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

Great. We'll get that stuff landed as soon as the tree reopens to non-beta7 changes.

After those land, we can make further improvements to eliminate temporary buffers in more cases. In particular we currently use a temporary surface if we have a non-identity transform and any child has a cliprect; but when the transform is just a translation+scale, we can simply transform the cliprects instead.

Comment 7

[Oleg Romashin (:romaxa)]

Attachment: Cache like this

btw would it be possible to cache texture  surface somehow? will it be acceptable for us?

Comment 8

[Oleg Romashin (:romaxa)]

Attachment: Right file

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Better FBO caching texture patch from Bug 599507 with small update

Patch taken from:
https://bugzilla.mozilla.org/attachment.cgi?id=478416&action=edit
and disabled glTextImage2D for cached texture

without this patch most pages almost unusable...

Comment 10

[Oleg Romashin (:romaxa)]

Would it be possible to render without any FBO at all? just apply transformation correctly for each layer and clip them somehow without temporary buffer?

Comment 11

[Matt Woodrow (:mattwoodrow)]

We clip using glScissor which can only clip to aligned rectangles.

For transformations that consist of only scaling, translation and rotation in multiples of 90 degrees you could transform the clip rect without too much problem.

I'm not sure how to handle more complex transforms, we'd need another way of clipping. Creating a mask layer perhaps?

Comment 12

[Oleg Romashin (:romaxa)]

Attachment: about:blank fennec gDumpPaintList

While fennec scrolling about:blank page it is creating container which is needs temp FBO texture surface 980,544
Here is the gDumpPaintList log

I think it is really really bad to create temp FBO for about:blank page ...

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: lemonde.fr gDumpPaintList

This is output from lemonde page which is rendering containers and creating temporary FBO objects.

I'm printing:
ContainerLayerOGL::ContainerRender::148 slow and memory consume: container with temp FBO, 
when container needsFramebuffer

Comment 14

[Oleg Romashin (:romaxa)]

and mUseIntermediateSurface == true because of

ContainerLayer::DefaultComputeEffectiveTransforms::345  clipRect !clipRect->IsEmpty() && !child->GetVisibleRegion().IsEmpty() 
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp#343

Comment 15

[Oleg Romashin (:romaxa)]

On lemonde.fr we have:
ContainerLayer::DefaultComputeEffectiveTransforms::332 opac:1, HasMultChild:1, effTransFIsIdent:0
ContainerLayer::DefaultComputeEffectiveTransforms::348  clipRect !clipRect->IsEmpty() && !child->GetVisibleRegion().IsEmpty() !!! useIntermediateSurface, clip[0,260,680,3911], visible[0,119,680,81]

On about:blank it is always like this:
ContainerLayer::DefaultComputeEffectiveTransforms::332 opac:1, HasMultChild:0, effTransFIsIdent:0
ContainerLayer::DefaultComputeEffectiveTransforms::348  clipRect !clipRect->IsEmpty() && !child->GetVisibleRegion().IsEmpty() !!! useIntermediateSurface, clip[0,0,980,544], visible[0,0,980,544]

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: Detect simple transform and apply that on clipRect and pass down to layers

Comment 17

[Oleg Romashin (:romaxa)]

Attachment: Simple transform detection fixed and check for isMatr2D

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: Fixed comment

sorry for spam

Comment 19

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

Why aren't you just changing ContainerLayer::DefaultComputeEffectiveTransforms?

Comment 20

[Oleg Romashin (:romaxa)]

Attachment: Moved to ContainerLayer::DefaultComputeEffectiveTransform

Comment 21

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

+      gfxMatrix contTransform;

Call it 'containerTransform'

IsSimpleTransform isn't defined in this patch. It's also not a very well-named API. How about calling it PreservesAxisAlignedRectangles?

+            child->SetClipRect(&trRect);

You must not overwrite layer state like this. That state is owned by the client (layout).

Instead, you need to make GetEffectiveClipRect return the right thing. Probably the best thing to do is to do nothing in DefaultComputeEffectiveTransforms except set a flag on the container to indicate that its transform needs to be applied to the cliprects of descendants. Then in GetEffectiveClipRect we can walk up the parent layers until we reach the root or a layer with an intermediate surface, and for every parent we find with that flag set, transform the clip rect.

Comment 22

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

(In reply to comment #21)
> Then in GetEffectiveClipRect we can
> walk up the parent layers until we reach the root or a layer with an
> intermediate surface, and for every parent we find with that flag set,
> transform the clip rect.

That's not quite right, but something like that.

Comment 23

[Oleg Romashin (:romaxa)]

Attachment: Fixed according to comments

Comment 24

[Oleg Romashin (:romaxa)]

Attachment: Transform Clip rect for OGL containers (Fixed typo problem)

Fixed typo problem
-retRect.SetRect(PRInt32(clipGfx.X()),
+retRect.SetRect(PRInt32(transformedClipRect.X()),

Comment 25

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

I thought we decided on IRC to return a struct containing an nsIntRect and a boolean to tell the caller if there's clipping or not. It would be nice to produce a single patch that just does that refactoring and then layer the rest of the changes in a separate patch on top of that.

This would break !MOZ_IPC. You need the same clip-transformation code for !MOZ_IPC. Better move #ifdef MOZ_IPC into GetEffectiveClipRect.

You'll need to update the callers of GetEffectiveClipRect in D3D9 and D3D10 layers.

+  if (mEffectiveTransform.Is2D(&contTransform) && contTransform.PreservesAxisAlignedRectangles()) {
+    mIsPreservesAxisAlignedRectanglesTransform = PR_TRUE;
+  }

This name is ugly...

But actually, I've been pretty stupid. Your original patch was much better. Sorry!!!

Comment 26

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

Hmm, well, that patch still needs work too.

+    if (passTransformDownAndClip) {
+      layerToRender->GetLayer()->ComputeEffectiveTransforms(transform);
+    }

Don't do this. We should only do one pass of ComputeEffectiveTransforms, like now.

In DefaultComputeEffectiveTransforms, if the container transform has PreservesAxisAlignedRectangles, we don't need to set useIntermediateSurface just because a child layer has a clip.

Then in ContainerRender (and the RenderLayer methods for D3D9 and D3D10), when you set the scissor rect for the child layer, if this container doesn't have an intermediate surface, transform the scissor rect by the container's GetEffectiveTransform.

Comment 27

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

Is this going to fix bug 602435 incidentally?

Comment 29

[Oleg Romashin (:romaxa)]

Attachment: Fixed according last comment

Comment 30

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

+    if (!needsFramebuffer) {
+      gfxMatrix contTransform;
+      transform.Is2D(&contTransform);

This could fail. ContainerLayer::DefaultComputeEffectiveTransforms doesn't guarantee that useIntermediateSurface is true when preservesAxisAlignedRectangles is false.

Comment 31

[Oleg Romashin (:romaxa)]

Attachment: Add check for PreservesAxisAlignedRectangles transform

Comment 32

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

This patch isn't correct. You're ignoring the clip rect set on the container layer, when you set the scissor rect for the child layer.

This is getting a bit frustrating. It feels like you're submitting random code for me to tell you whether it works or not. You need to think harder about what the invariants are here and prove to yourself that your patch works in all cases.

Comment 33

[Oleg Romashin (:romaxa)]

> This is getting a bit frustrating. It feels like you're submitting random code
> for me to tell you whether it works or not. You need to think harder about what
> the invariants are here and prove to yourself that your patch works in all
> cases.

This is not true, I'm checking that temp FBO is not created, and checking it on some pages, like www.lemonde.fr about:blank, dp.ru, and many other pages.
This code works, but I'm not sure about all possible testcases which could be broken by this.

Comment 34

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

We can't prove that code is correct by testing it.

When we write code, we have to have some kind of proof --- even a very informal proof in our heads --- that the code we have written is correct.

Then we test it in case we made a mistake.

Comment 35

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

+    gfxMatrix contTransform;
+    PRBool preservesAxisAlignedRectangles =
+      mEffectiveTransform.Is2D(&contTransform) && contTransform.PreservesAxisAlignedRectangles();
+    if (preservesAxisAlignedRectangles) {
+      for (Layer* child = GetFirstChild(); child; child = child->GetNextSibling()) {
+        child->ComputeEffectiveTransforms(mEffectiveTransform);
+      }
+    } else if (!mEffectiveTransform.IsIdentity()) {

Here, we don't need to check mEffectiveTransform.IsIdentity() anymore. If it's the identity, then it preserves axis-aligned rectangles.

Comment 36

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

Actually, the rest of your patch seems correct for the GL backend. Later on we have
    scissorRect.IntersectRect(scissorRect, cachedScissor);

which takes care of combining the passed-in scissor rect with the scissor rect for the child layer.

Comment 37

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

     }
 
+    if (!needsFramebuffer) {
+      gfxMatrix contTransform;
+      if (transform.Is2D(&contTransform) && contTransform.PreservesAxisAlignedRectangles()) {
+        gfxRect r(scissorRect.x, scissorRect.y, scissorRect.width, scissorRect.height);
+        gfxRect trScissor = contTransform.TransformBounds(r);
+        scissorRect.SetRect(trScissor.pos.x,
+                            trScissor.pos.y,
+                            trScissor.size.width,
+                            trScissor.size.height);
+      }
+    }

Actually this should go inside the "if (clipRect)" case. Then things are a bit clearer.

In fact, the Is2D call and the PreservesAxisAlignedRectangles check can be hoisted all the way outside the loop over the children.

Things will be even clearer if in the loop over children, you skip children with a fully-empty cliprect or an empty visible region, just like DefaultComputeEffectiveTransforms does. Then you should be able to *prove* that if DefaultComputeEffectiveTransforms set !useIntermediateSurface (so !needsFramebuffer in RenderLayers), the call to transform.Is2D cannot fail.

Comment 38

[Oleg Romashin (:romaxa)]

Attachment: Hopefully fixed comment #37

Hope I understood last paragraph of comment 37 and covered all comments in this update

Comment 39

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

       for (Layer* child = GetFirstChild(); child; child = child->GetNextSibling()) {
         const nsIntRect *clipRect = child->GetEffectiveClipRect();
         /* We can't (easily) forward our transform to children with a non-empty clip
          * rect since it would need to be adjusted for the transform.
          * TODO: This is easily solvable for translation/scaling transforms.
          */
         if (clipRect && !clipRect->IsEmpty() && !child->GetVisibleRegion().IsEmpty()) {
+          if (preservesAxisAlignedRectangles) {
+            child->ComputeEffectiveTransforms(mEffectiveTransform);
+            continue;
+          }

Why did you change this to check preservesAxisAlignedRectangles inside the loop? You were correct before. Now you're calling ComputeEffectiveTransforms on some children twice :-(. Why do you want to do that?

+  PRBool preservesAxisAlignedRectangles =
+    !needsFramebuffer &&
+    transform.Is2D(&contTransform) &&
+    contTransform.PreservesAxisAlignedRectangles();

You don't need this check.

See the last part of comment #37. You should skip children with a fully-empty cliprect or an empty visible region.

Then, if the child layer has a clip rect, and its nonempty, and !needsFramebuffer, then you *know* preservesAxisAlignedRectangles must be true.

Comment 40

[Oleg Romashin (:romaxa)]

Attachment: r4.

>loop? You were correct before. Now you're calling ComputeEffectiveTransforms on
>some children twice :-(. Why do you want to do that?

Not sure how it is possible with current patch

Last part is fixed.

Comment 41

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

(In reply to comment #40)
> Not sure how it is possible with current patch

Because below that loop, we call ComputeEffectiveTransformsForChildren, which calls ComputeEffectiveTransforms on all children.

Comment 42

[Oleg Romashin (:romaxa)]

Attachment: So return earlier?

Comment 43

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

No. Why do you insist on calling ComputeEffectiveTransforms on the child in that loop?

We should call ComputeEffectiveTransforms after useIntermediateSurface has been set to its final value.

Comment 44

[Oleg Romashin (:romaxa)]

(In reply to comment #43)
> No. Why do you insist on calling ComputeEffectiveTransforms on the child in
> that loop?
Because it worked.

> 
> We should call ComputeEffectiveTransforms after useIntermediateSurface has been
> set to its final value.
so in the end I  should call instead of 
ComputeEffectiveTransformsForChildren(idealTransform);
call
ComputeEffectiveTransformsForChildren(mEffectiveTransform);
?

Comment 45

[Oleg Romashin (:romaxa)]

> No. Why do you insist on calling ComputeEffectiveTransforms on the child in
> that loop?
oh, seems I  misunderstood you previous comment about checking clip and visible region and applied it to wrong place (comment 37)

Comment 46

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

(In reply to comment #44)
> (In reply to comment #43)
> > No. Why do you insist on calling ComputeEffectiveTransforms on the child in
> > that loop?
> Because it worked.

That is not a good answer :-(. You need to understand why it should work, *then* test it.

Comment 47

[Oleg Romashin (:romaxa)]

Attachment: r5. no ComputeEffectiveTransforms in first loop

Comment 48

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

+    if (!mEffectiveTransform.IsIdentity() && !preservesAxisAlignedRectangles) {

Why did this IsIdentity call come back?

Comment 49

[Oleg Romashin (:romaxa)]

Attachment: r6. Fixed comment 35/48

Comment 50

[Oleg Romashin (:romaxa)]

Comment on attachment 492584 [details] [diff] [review]
r6. Fixed comment 35/48

oh, this will not work

Comment 51

[Oleg Romashin (:romaxa)]

Attachment: r7. just do nothing when preservesAxisAlignedRectangles,

Also removed transform.Is2D out from NS_ASSERTION... (otherwise it works only for debug build... my bad)

Comment 52

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

+  PRBool preservesAxisAlignedRectangles = PR_FALSE;

Just declare preservesAxisAlignedRectangles inside the 'else' block.

+    PRBool is2d = transform.Is2D(&contTransform);

You probably need to put #ifdefs around "PRBool is2d =" so there's no warning about an unused local variable in non-debug builds.

Other than that, the GL code looks fine now. Now the D3D9/D3D10 code needs to be updated.

Comment 53

[Oleg Romashin (:romaxa)]

Attachment: r8. Common + opengGL part

Comment 54

[Oleg Romashin (:romaxa)]

Attachment: D3D part

Comment 55

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

         // Scissor rect should be an intersection of the old and current scissor.
         r.left = NS_MAX<PRInt32>(oldScissor.left, r.left);
         r.right = NS_MIN<PRInt32>(oldScissor.right, r.right);
         r.top = NS_MAX<PRInt32>(oldScissor.top, r.top);
         r.bottom = NS_MIN<PRInt32>(oldScissor.bottom, r.bottom);
+        if (clipRect && !clipRect->IsEmpty() &&
+            !layerToRender->GetLayer()->GetEffectiveVisibleRegion().IsEmpty()) {
+          gfxRect r(r.left, r.top, r.left + r.right, r.top + r.bottom);
+          gfxRect trScissor = contTransform.TransformBounds(r);
+          r.left = trScissor.pos.x;
+          r.top = trScissor.pos.y;
+          r.right = trScissor.XMost();
+          r.bottom = trScissor.YMost();

This is wrong. oldScissor should not be transformed. You need to transform r before combining with oldScissor.

Comment 56

[Oleg Romashin (:romaxa)]

Attachment: D3D part v3

Comment 57

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

+          gfxRect r(r.left, r.top, r.left + r.right, r.top + r.bottom);
+          gfxRect trScissor = contTransform.TransformBounds(r);
+          r.left = trScissor.pos.x;
+          r.top = trScissor.pos.y;
+          r.right = trScissor.XMost();
+          r.bottom = trScissor.YMost();

This is wrong. You're changing the "r" in this block which doesn't affect the r in the outer block. Don't have two local variables with the same name in the same function!

Comment 58

[Oleg Romashin (:romaxa)]

Attachment: D3D better names r4

Sorry, very bad style to name variables like "r", "p" et.c.

Comment 59

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

Actually there's one more thing for both the GL and D3D patches: when the clip rect is empty or the visible region is empty, instead of just skipping the clipping, skip painting the entire child.

Comment 60

[Oleg Romashin (:romaxa)]

Attachment: comment 59 fix

tested with 2 prev patches. compiling on OGL

Comment 61

[Oleg Romashin (:romaxa)]

Attachment: Combined patch

Comment 62

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

Sorry, one more thing. We should think a bit more carefully about the rounding  and possible overflows when we convert the transformed gfxRect to an nsIntRect.

+        scissorRect.SetRect(trScissor.pos.x,
+                            trScissor.pos.y,
+                            trScissor.size.width,
+                            trScissor.size.height);

I think we should call trScissor.Round() to round the coordinates. Then we should call nsLayoutUtils::GfxRectToIntRect to convert to an nsIntRect (but you can't call into layout directly, so move nsLayoutUtils::GfxRectToIntRect to replace the one in gfxThebesUtils). If the call fails, that means there is a super-large scale on the surface, so we should probably just not apply the clip.

Comment 63

[Oleg Romashin (:romaxa)]

Attachment: Fixed comment 62

Have not tested compilation (will check in morning), but approximately should be like this.
hope it is ok remove that function from layoutUtils and just use gfxUtils everywhere.

Comment 64

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

Comment on attachment 492841 [details] [diff] [review]
Fixed  comment 62

+  return gfxRect(aOut->x, aOut->y, aOut->width, aOut->height) == aIn
+    ? NS_OK : NS_ERROR_FAILURE;

Drop ? NS_OK : NS_ERROR_FAILURE

The rest looks good.

Comment 65

[Oleg Romashin (:romaxa)]

> The rest looks good.
oh, yep, you right

Comment 66

[Oleg Romashin (:romaxa)]

Attachment: Combined patch r2

combined all r+ patches
Try server
http://hg.mozilla.org/try/rev/58f189126d38

Comment 67

[Oleg Romashin (:romaxa)]

Comment on attachment 492925 [details] [diff] [review]
Combined patch r2

bad patch, D3D compilation fail

Comment 68

[Oleg Romashin (:romaxa)]

Attachment: Fixed D3D compilation problems

Try build
http://hg.mozilla.org/try/rev/4147771b7652

Comment 69

[Oleg Romashin (:romaxa)]

Comment on attachment 492966 [details] [diff] [review]
Fixed D3D compilation problems

Would it be possible to get it in?

Comment 70

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

Comment on attachment 492966 [details] [diff] [review]
Fixed D3D compilation problems

Yes, we definitely want this for performance

Comment 71

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/8fff1c186b30

Comment 72

[Dave Townsend [:mossop]]

Backed out due to reftest failures on OSX: http://hg.mozilla.org/mozilla-central/rev/563dd73da056

Comment 73

[(no longer active)]

This seems to break non-libxul builds because gfxUtils::GfxRectToIntRect is not visible to libgklayout.  Please add the THEBES_API modifier to the function declaration when relanding this patch.

Comment 74

[Oleg Romashin (:romaxa)]

hmm, gfxUtils have THEBES_API modifier... I think something else wrong is here..

Comment 75

[Oleg Romashin (:romaxa)]

Attachment: Fix SVG orange, should be used with attachment.cgi?id=492966

Some parts of this patch incorrectly switched from nsresult to PRBool (typo/copy/paste)
http://hg.mozilla.org/try/rev/4147771b7652#l17.15
http://hg.mozilla.org/try/rev/4147771b7652#l19.15

Comment 76

[Oleg Romashin (:romaxa)]

Will check another try
http://hg.mozilla.org/try/rev/118671523573

Comment 77

[(no longer active)]

(In reply to comment #74)
> hmm, gfxUtils have THEBES_API modifier... I think something else wrong is
> here..

Hmm, right, I don't know what was wrong, but the linker errors were solved for me after a full rebuild.  Sorry for the noise...

Comment 78

[(no longer active)]

Please do not specify checkin-needed until you have all of the necessary reviews and approvals.

Comment 79

[Oleg Romashin (:romaxa)]

ok all approvals are available now

Comment 80

[Oleg Romashin (:romaxa)]

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

Comment 81

[Justin Lebar (not reading bugmail)]

Backed out due to persistent orange, bug 615235.

http://hg.mozilla.org/mozilla-central/rev/510e6e2ae3fd

Comment 82

[Oleg Romashin (:romaxa)]

ok, my fault, did not check all MozillaTry failures properly, and recognized Win Opt reftest failure as the same SVG failure.

Will try to reproduce this with OGL backend... not sure if it will be possible for me to build Win D3D version.

Comment 83

[Oleg Romashin (:romaxa)]

ok, got it reproducible in local OGL linux GTK build. need to check where is the problem

Comment 84

[Oleg Romashin (:romaxa)]

Things looks a bit strange here:
I've made try builds for Linux and windows with accelerate = true;
and linux build has plugin.layers enabled and plugin rendered, but scaling seems wrong in reftest:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1291098691.1291099771.4178.gz#err1

with windows build (plugin layers disabled) I don't see any plugin rendered at all... test and ref-test both has green area only... is it expected?
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1291110267.1291112888.26687.gz#err0

Comment 85

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

(In reply to comment #84)
> Things looks a bit strange here:
> I've made try builds for Linux and windows with accelerate = true;
> and linux build has plugin.layers enabled and plugin rendered, but scaling
> seems wrong in reftest:
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1291098691.1291099771.4178.gz#err1

There is no scaling in this test. This test is clipping two unscaled plugin instances.

> with windows build (plugin layers disabled) I don't see any plugin rendered at
> all... test and ref-test both has green area only... is it expected?
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1291110267.1291112888.26687.gz#err0

No, that's pretty broken.

The rendering of windowless-clipping-1-ref.html in http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1291098691.1291099771.4178.gz#err1 is the correct rendering.

Comment 86

[Oleg Romashin (:romaxa)]

> > http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1291110267.1291112888.26687.gz#err0
> 
> No, that's pretty broken.

so we are trying to make that be "correctly" broken..
ok, then I should probably try to fix this bug with linux reftest and HW acceleration enabled...

Comment 87

[Oleg Romashin (:romaxa)]

roc: is it ok to apply non-layers part (gfxUtils::GfxRectToIntRect) ?

Comment 88

[Oleg Romashin (:romaxa)]

Attachment: GfxRectToIntRect only part1

Comment 89

[Oleg Romashin (:romaxa)]

Attachment: Safe layers part2 (while->for) code cleanup

Comment 90

[Oleg Romashin (:romaxa)]

Here is the part which is breaking windowless clipping test http://hg.mozilla.org/try/rev/49786eb913e3 - still need some investigation

Comment 91

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

(In reply to comment #87)
> roc: is it ok to apply non-layers part (gfxUtils::GfxRectToIntRect) ?

Yes!

Comment 92

[Oleg Romashin (:romaxa)]

Safe parts landed 
http://hg.mozilla.org/mozilla-central/rev/566832c6b284
http://hg.mozilla.org/mozilla-central/rev/557ce7926eba

Comment 93

[Oleg Romashin (:romaxa)]

ok, after some investigation I found that plugins rendered correctly if I open windowless-clipping-1.html and windowless-clipping-1-ref.html separately...
The only problem is timings... seems our MozPaintWait system does not work well now. (not sure was it broken with some PluginLayers for Win landings...)

Comment 94

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

See bug 617152 which fixes problems with the harness.

Comment 95

[Oleg Romashin (:romaxa)]

Ok, tested on latest m-c with bug 617152 fixed:
Still orange reproducible:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292873187.1292876288.10907.gz

Comment 96

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

Are you sure that's not just bug 618788?

Comment 97

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

Hmm. Can you attach the remaining patch? I wonder if you broke clipping on D3D9 or D3D10.

Comment 98

[Oleg Romashin (:romaxa)]

Attachment: remaining patch

Comment 99

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

+          gfxRect cliprect(r.left, r.top, r.left + r.right, r.top + r.bottom);

This is quite wrong. You can't add r.left and r.right together to get anything useful... same bug in both D3D9 and D3D10

Comment 100

[Oleg Romashin (:romaxa)]

Comment on attachment 498871 [details] [diff] [review]
remaining patch

(In reply to comment #99)
> +          gfxRect cliprect(r.left, r.top, r.left + r.right, r.top + r.bottom);
ouch...

Comment 101

[Oleg Romashin (:romaxa)]

Attachment: Fixed cliprect setup, PUSH me

Comment 102

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

Comment on attachment 498969 [details] [diff] [review]
Fixed cliprect setup, PUSH me

Better. Let's see if this lets you pass reftests.

Comment 103

[Oleg Romashin (:romaxa)]

Comment on attachment 498969 [details] [diff] [review]
Fixed cliprect setup, PUSH me

Ok, it pass reftests now

Comment 104

[Oleg Romashin (:romaxa)]

don't know when tree is going to be green again... 
please push https://bugzilla.mozilla.org/attachment.cgi?id=498969

Comment 105

[Oleg Romashin (:romaxa)]

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

Comment 106

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 498969 [details] [diff] [review]
Fixed cliprect setup, PUSH me

>+#ifdef DEBUG
>+    PRBool is2d =
>+#endif
>+    GetEffectiveTransform().Is2D(&contTransform);
>+    NS_ASSERTION(is2d, "Transform must be 2D");

Thank you!