avoid temporary fbos/textures on transformed layers, when possible

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 26 obsolete attachments)

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
(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Blocks: 598864
(Assignee)

Comment 1

8 years ago
not sure if it is duplicate for 602435 or not
Not necessarily, but this might overlap with bug 599507 and bug 602200.
(Assignee)

Comment 3

8 years ago
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.
(Assignee)

Comment 5

8 years ago
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.
(Assignee)

Comment 7

8 years ago
Created attachment 487848 [details] [diff] [review]
Cache like this

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

Comment 8

8 years ago
Created attachment 487849 [details] [diff] [review]
Right file
Attachment #487848 - Attachment is obsolete: true
(Assignee)

Comment 9

8 years ago
Created attachment 488290 [details] [diff] [review]
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...
Attachment #487849 - Attachment is obsolete: true
Attachment #488290 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #488290 - Flags: review? → review?(vladimir)
(Assignee)

Comment 10

8 years ago
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?
(Assignee)

Comment 12

8 years ago
Created attachment 489105 [details]
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 ...
(Assignee)

Updated

8 years ago
Attachment #489105 - Attachment description: about:blank fennec → about:blank fennec gDumpPaintList
(Assignee)

Comment 13

8 years ago
Created attachment 489106 [details]
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
(Assignee)

Comment 14

8 years ago
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
(Assignee)

Comment 15

8 years ago
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]
(Assignee)

Comment 16

8 years ago
Created attachment 489137 [details] [diff] [review]
Detect simple transform and apply that on clipRect and pass down to layers
Attachment #489137 - Flags: review?(roc)
(Assignee)

Comment 17

8 years ago
Created attachment 489139 [details] [diff] [review]
Simple transform detection fixed and check for isMatr2D
Attachment #489137 - Attachment is obsolete: true
Attachment #489139 - Flags: review?(roc)
Attachment #489137 - Flags: review?(roc)
(Assignee)

Comment 18

8 years ago
Created attachment 489141 [details] [diff] [review]
Fixed comment

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?
(Assignee)

Comment 20

8 years ago
Created attachment 489439 [details] [diff] [review]
Moved to ContainerLayer::DefaultComputeEffectiveTransform
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.
(Assignee)

Comment 23

8 years ago
Created attachment 489747 [details] [diff] [review]
Fixed according to comments
Attachment #489439 - Attachment is obsolete: true
Attachment #489747 - Flags: review?(roc)
Attachment #489439 - Flags: review?(roc)
(Assignee)

Comment 24

8 years ago
Created attachment 489748 [details] [diff] [review]
Transform Clip rect for OGL containers (Fixed typo problem)

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)
(Assignee)

Updated

8 years ago
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?
(Assignee)

Comment 29

8 years ago
Created attachment 491842 [details] [diff] [review]
Fixed according last comment
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.
(Assignee)

Comment 31

8 years ago
Created attachment 492264 [details] [diff] [review]
Add check for PreservesAxisAlignedRectangles transform
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.
(Assignee)

Comment 33

8 years ago
> 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.
(Assignee)

Comment 38

8 years ago
Created attachment 492322 [details] [diff] [review]
Hopefully fixed comment #37

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.
(Assignee)

Comment 40

8 years ago
Created attachment 492453 [details] [diff] [review]
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.
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.
(Assignee)

Comment 42

8 years ago
Created attachment 492470 [details] [diff] [review]
So return earlier?
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.
(Assignee)

Comment 44

8 years ago
(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);
?
(Assignee)

Comment 45

8 years ago
> 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.
(Assignee)

Comment 47

8 years ago
Created attachment 492498 [details] [diff] [review]
r5. no  ComputeEffectiveTransforms in first loop
Attachment #492470 - Attachment is obsolete: true
+    if (!mEffectiveTransform.IsIdentity() && !preservesAxisAlignedRectangles) {

Why did this IsIdentity call come back?
(Assignee)

Comment 49

8 years ago
Created attachment 492584 [details] [diff] [review]
r6. Fixed comment 35/48
Attachment #492498 - Attachment is obsolete: true
(Assignee)

Comment 50

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

oh, this will not work
(Assignee)

Comment 51

8 years ago
Created attachment 492589 [details] [diff] [review]
r7. just do nothing when preservesAxisAlignedRectangles,

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.
(Assignee)

Comment 53

8 years ago
Created attachment 492607 [details] [diff] [review]
r8. Common + opengGL part
Attachment #492589 - Attachment is obsolete: true
Attachment #492607 - Flags: review?(roc)
Attachment #492589 - Flags: review?(roc)
(Assignee)

Comment 54

8 years ago
Created attachment 492608 [details] [diff] [review]
D3D part
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.
(Assignee)

Comment 56

8 years ago
Created attachment 492752 [details] [diff] [review]
D3D part v3
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!
(Assignee)

Comment 58

8 years ago
Created attachment 492775 [details] [diff] [review]
D3D better names r4

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.
(Assignee)

Comment 60

8 years ago
Created attachment 492801 [details] [diff] [review]
comment 59 fix

tested with 2 prev patches. compiling on OGL
Attachment #492801 - Flags: review?(roc)
(Assignee)

Comment 61

8 years ago
Created attachment 492810 [details] [diff] [review]
Combined patch
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.
(Assignee)

Comment 63

8 years ago
Created attachment 492841 [details] [diff] [review]
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.
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+
(Assignee)

Comment 65

8 years ago
> The rest looks good.
oh, yep, you right
(Assignee)

Comment 66

8 years ago
Created attachment 492925 [details] [diff] [review]
Combined patch r2

combined all r+ patches
Try server
http://hg.mozilla.org/try/rev/58f189126d38
Attachment #492841 - Attachment is obsolete: true
Attachment #492925 - Flags: review?(roc)
(Assignee)

Comment 67

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

bad patch, D3D compilation fail
Attachment #492925 - Flags: review?(roc)
(Assignee)

Comment 68

8 years ago
Created attachment 492966 [details] [diff] [review]
Fixed D3D compilation problems

Try build
http://hg.mozilla.org/try/rev/4147771b7652
Attachment #492925 - Attachment is obsolete: true
Attachment #492966 - Flags: review?(roc)
(Assignee)

Comment 69

8 years ago
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+
(Assignee)

Comment 71

8 years ago
http://hg.mozilla.org/mozilla-central/rev/8fff1c186b30
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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 → ---

Comment 73

8 years ago
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.
(Assignee)

Comment 74

8 years ago
hmm, gfxUtils have THEBES_API modifier... I think something else wrong is here..
(Assignee)

Comment 75

8 years ago
Created attachment 493198 [details] [diff] [review]
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
(Assignee)

Comment 76

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

Comment 77

8 years ago
(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...
(Assignee)

Updated

8 years ago
Attachment #488290 - Attachment is obsolete: true
Attachment #488290 - Flags: review?(vladimir)
(Assignee)

Updated

8 years ago
Attachment #493198 - Attachment description: Fix switch → Fix SVG orange, should be used with attachment.cgi?id=492966
Attachment #493198 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 78

8 years ago
Please do not specify checkin-needed until you have all of the necessary reviews and approvals.
Keywords: checkin-needed
(Assignee)

Comment 79

8 years ago
ok all approvals are available now
Keywords: checkin-needed
(Assignee)

Comment 80

8 years ago
http://hg.mozilla.org/mozilla-central/rev/a5ec817740f0
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

8 years ago
Depends on: 615235
Backed out due to persistent orange, bug 615235.

http://hg.mozilla.org/mozilla-central/rev/510e6e2ae3fd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 82

8 years ago
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.
(Assignee)

Comment 83

8 years ago
ok, got it reproducible in local OGL linux GTK build. need to check where is the problem
(Assignee)

Comment 84

8 years ago
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.
(Assignee)

Comment 86

8 years ago
> > 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...
(Assignee)

Comment 87

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

Comment 88

8 years ago
Created attachment 494955 [details] [diff] [review]
GfxRectToIntRect only part1
(Assignee)

Updated

8 years ago
Attachment #494955 - Attachment description: GfxRectToIntRect part only → GfxRectToIntRect only part1
(Assignee)

Comment 89

8 years ago
Created attachment 494956 [details] [diff] [review]
Safe layers part2 (while->for) code cleanup
(Assignee)

Comment 90

8 years ago
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!
(Assignee)

Comment 93

8 years ago
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.
(Assignee)

Updated

8 years ago
Blocks: 619056
(Assignee)

Comment 95

8 years ago
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.
(Assignee)

Comment 98

8 years ago
Created attachment 498871 [details] [diff] [review]
remaining patch
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
(Assignee)

Comment 100

8 years ago
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)
(Assignee)

Comment 101

8 years ago
Created attachment 498969 [details] [diff] [review]
Fixed cliprect setup, PUSH me
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+
(Assignee)

Comment 103

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

Ok, it pass reftests now
Attachment #498969 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Attachment #498969 - Attachment description: Fixed cliprect setup → Fixed cliprect setup, PUSH me
(Assignee)

Comment 104

8 years ago
don't know when tree is going to be green again... 
please push https://bugzilla.mozilla.org/attachment.cgi?id=498969
Keywords: checkin-needed
(Assignee)

Comment 105

8 years ago
http://hg.mozilla.org/mozilla-central/rev/ab173acf8a1f
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 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!

Updated

7 years ago
Depends on: 654950
You need to log in before you can comment on or make changes to this bug.