Closed Bug 607653 Opened 9 years ago Closed 9 years ago

avoid temporary fbos/textures on transformed layers, when possible

Categories

(Core :: Graphics, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: romaxa, Assigned: romaxa)

References

Details

Attachments

(7 files, 26 obsolete files)

25.82 KB, text/plain
Details
190.47 KB, text/plain
Details
26.08 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.83 KB, patch
roc
: review+
Details | Diff | Splinter Review
15.12 KB, patch
Details | Diff | Splinter Review
6.44 KB, patch
Details | Diff | Splinter Review
5.97 KB, patch
roc
: review+
Details | Diff | Splinter Review
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.
Blocks: 598864
not sure if it is duplicate for 602435 or not
Not necessarily, but this might overlap with bug 599507 and bug 602200.
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 :(...
Nah, just those and the dependency tree of 598864.
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.
Depends on: 602200
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.
Attached patch Cache like this (obsolete) — Splinter Review
btw would it be possible to cache texture  surface somehow? will it be acceptable for us?
Attached patch Right file (obsolete) — Splinter Review
Attachment #487848 - Attachment is obsolete: true
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...
Attachment #487849 - Attachment is obsolete: true
Attachment #488290 - Flags: review?
Attachment #488290 - Flags: review? → review?(vladimir)
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?
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?
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 ...
Attachment #489105 - Attachment description: about:blank fennec → about:blank fennec 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
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
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]
Attachment #489137 - Attachment is obsolete: true
Attachment #489139 - Flags: review?(roc)
Attachment #489137 - Flags: review?(roc)
Attached patch Fixed comment (obsolete) — Splinter Review
sorry for spam
Assignee: nobody → romaxa
Attachment #489139 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #489141 - Flags: review?(roc)
Attachment #489139 - Flags: review?(roc)
Why aren't you just changing ContainerLayer::DefaultComputeEffectiveTransforms?
Attachment #489141 - Attachment is obsolete: true
Attachment #489439 - Flags: review?(roc)
Attachment #489141 - Flags: review?(roc)
+      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.
(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.
Attached patch Fixed according to comments (obsolete) — Splinter Review
Attachment #489439 - Attachment is obsolete: true
Attachment #489747 - Flags: review?(roc)
Attachment #489439 - Flags: review?(roc)
Fixed typo problem
-retRect.SetRect(PRInt32(clipGfx.X()),
+retRect.SetRect(PRInt32(transformedClipRect.X()),
Attachment #489747 - Attachment is obsolete: true
Attachment #489748 - Flags: review?(roc)
Attachment #489747 - Flags: review?(roc)
Attachment #489748 - Attachment description: Fixed typo problem → Transform Clip rect for OGL containers (Fixed typo problem)
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!!!
Attachment #489141 - Attachment is obsolete: false
Attachment #489141 - Flags: review?(roc)
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.
Is this going to fix bug 602435 incidentally?
Attached patch Fixed according last comment (obsolete) — Splinter Review
Attachment #489141 - Attachment is obsolete: true
Attachment #489748 - Attachment is obsolete: true
Attachment #491842 - Flags: review?(roc)
Attachment #489141 - Flags: review?(roc)
Attachment #489748 - Flags: review?(roc)
+    if (!needsFramebuffer) {
+      gfxMatrix contTransform;
+      transform.Is2D(&contTransform);

This could fail. ContainerLayer::DefaultComputeEffectiveTransforms doesn't guarantee that useIntermediateSurface is true when preservesAxisAlignedRectangles is false.
Attachment #491842 - Attachment is obsolete: true
Attachment #492264 - Flags: review?(roc)
Attachment #491842 - Flags: review?(roc)
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.
> 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.
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.
+    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.
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.
     }
 
+    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.
Attached patch Hopefully fixed comment #37 (obsolete) — Splinter Review
Hope I understood last paragraph of comment 37 and covered all comments in this update
Attachment #492264 - Attachment is obsolete: true
Attachment #492264 - Flags: review?(roc)
       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.
Attached patch r4. (obsolete) — Splinter Review
>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.
Attachment #492322 - Attachment is obsolete: true
Attachment #492453 - Flags: review?(roc)
(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.
Attached patch So return earlier? (obsolete) — Splinter Review
Attachment #492453 - Attachment is obsolete: true
Attachment #492453 - Flags: review?(roc)
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.
(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);
?
> 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)
(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.
Attachment #492470 - Attachment is obsolete: true
+    if (!mEffectiveTransform.IsIdentity() && !preservesAxisAlignedRectangles) {

Why did this IsIdentity call come back?
Attached patch r6. Fixed comment 35/48 (obsolete) — Splinter Review
Attachment #492498 - Attachment is obsolete: true
Comment on attachment 492584 [details] [diff] [review]
r6. Fixed comment 35/48

oh, this will not work
Also removed transform.Is2D out from NS_ASSERTION... (otherwise it works only for debug build... my bad)
Attachment #492584 - Attachment is obsolete: true
Attachment #492589 - Flags: review?(roc)
+  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.
Attached patch r8. Common + opengGL part (obsolete) — Splinter Review
Attachment #492589 - Attachment is obsolete: true
Attachment #492607 - Flags: review?(roc)
Attachment #492589 - Flags: review?(roc)
Attached patch D3D part (obsolete) — Splinter Review
Attachment #492608 - Flags: review?(roc)
         // 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.
Attached patch D3D part v3 (obsolete) — Splinter Review
Attachment #492608 - Attachment is obsolete: true
Attachment #492752 - Flags: review?(roc)
Attachment #492608 - Flags: review?(roc)
+          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!
Attached patch D3D better names r4 (obsolete) — Splinter Review
Sorry, very bad style to name variables like "r", "p" et.c.
Attachment #492752 - Attachment is obsolete: true
Attachment #492775 - Flags: review?(roc)
Attachment #492752 - Flags: review?(roc)
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.
Attached patch comment 59 fix (obsolete) — Splinter Review
tested with 2 prev patches. compiling on OGL
Attachment #492801 - Flags: review?(roc)
Attached patch Combined patch (obsolete) — Splinter Review
Attachment #492810 - Flags: review?(roc)
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.
Attached patch Fixed comment 62 (obsolete) — Splinter Review
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.
Attachment #492607 - Attachment is obsolete: true
Attachment #492775 - Attachment is obsolete: true
Attachment #492801 - Attachment is obsolete: true
Attachment #492810 - Attachment is obsolete: true
Attachment #492841 - Flags: review?(roc)
Attachment #492775 - Flags: review?(roc)
Attachment #492810 - Flags: review?(roc)
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.
Attachment #492841 - Flags: review?(roc) → review+
> The rest looks good.
oh, yep, you right
Attached patch Combined patch r2 (obsolete) — Splinter Review
combined all r+ patches
Try server
http://hg.mozilla.org/try/rev/58f189126d38
Attachment #492841 - Attachment is obsolete: true
Attachment #492925 - Flags: review?(roc)
Comment on attachment 492925 [details] [diff] [review]
Combined patch r2

bad patch, D3D compilation fail
Attachment #492925 - Flags: review?(roc)
Try build
http://hg.mozilla.org/try/rev/4147771b7652
Attachment #492925 - Attachment is obsolete: true
Attachment #492966 - Flags: review?(roc)
Comment on attachment 492966 [details] [diff] [review]
Fixed D3D compilation problems

Would it be possible to get it in?
Attachment #492966 - Flags: approval2.0?
Comment on attachment 492966 [details] [diff] [review]
Fixed D3D compilation problems

Yes, we definitely want this for performance
Attachment #492966 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/8fff1c186b30
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Backed out due to reftest failures on OSX: http://hg.mozilla.org/mozilla-central/rev/563dd73da056
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
hmm, gfxUtils have THEBES_API modifier... I think something else wrong is here..
(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...
Attachment #488290 - Attachment is obsolete: true
Attachment #488290 - Flags: review?(vladimir)
Attachment #493198 - Attachment description: Fix switch → Fix SVG orange, should be used with attachment.cgi?id=492966
Attachment #493198 - Flags: review?(roc)
Keywords: checkin-needed
Please do not specify checkin-needed until you have all of the necessary reviews and approvals.
Keywords: checkin-needed
ok all approvals are available now
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/a5ec817740f0
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 615235
Backed out due to persistent orange, bug 615235.

http://hg.mozilla.org/mozilla-central/rev/510e6e2ae3fd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
ok, got it reproducible in local OGL linux GTK build. need to check where is the problem
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
(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.
> > 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...
roc: is it ok to apply non-layers part (gfxUtils::GfxRectToIntRect) ?
Attachment #494955 - Attachment description: GfxRectToIntRect part only → GfxRectToIntRect only part1
Here is the part which is breaking windowless clipping test http://hg.mozilla.org/try/rev/49786eb913e3 - still need some investigation
(In reply to comment #87)
> roc: is it ok to apply non-layers part (gfxUtils::GfxRectToIntRect) ?

Yes!
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...)
See bug 617152 which fixes problems with the harness.
Blocks: 619056
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
Hmm. Can you attach the remaining patch? I wonder if you broke clipping on D3D9 or D3D10.
Attached patch remaining patch (obsolete) — Splinter Review
Attachment #498871 - Flags: review?(roc)
+          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 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...
Attachment #498871 - Attachment is obsolete: true
Attachment #498871 - Flags: review?(roc)
Attachment #498969 - Flags: review?(roc)
Comment on attachment 498969 [details] [diff] [review]
Fixed cliprect setup, PUSH me

Better. Let's see if this lets you pass reftests.
Attachment #498969 - Flags: review?(roc) → review+
Comment on attachment 498969 [details] [diff] [review]
Fixed cliprect setup, PUSH me

Ok, it pass reftests now
Attachment #498969 - Flags: approval2.0?
Attachment #498969 - Attachment description: Fixed cliprect setup → Fixed cliprect setup, PUSH me
don't know when tree is going to be green again... 
please push https://bugzilla.mozilla.org/attachment.cgi?id=498969
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/ab173acf8a1f
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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!
Depends on: 654950
You need to log in before you can comment on or make changes to this bug.