Closed Bug 607936 Opened 9 years ago Closed 9 years ago

Make possible to apply transform on topLevel LayerManager scene

Categories

(Core :: Graphics, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b12

People

(Reporter: romaxa, Assigned: romaxa)

References

Details

Attachments

(3 files, 6 obsolete files)

Qt Fennec in software mode rotates using software rotate Qt functionality (not very fast)

Qt Fennec with HW acceleration enabled does whole rendering inside LayerManagerOGL, and Qt not able to rotate anything...

I think we should provide API for applying TopLevel transformation on GL scene.

For example when double buffering done using mBackBufferTexture, then apply texture with rotate transform or something like that.
Here is the implementation without double buffer rotation.
Problem with double buffer is  - performance.
Attachment #487292 - Flags: feedback?(roc)
Comment on attachment 487292 [details] [diff] [review]
Rotate apply topLevel matrix on viewMatrix in setupPipeline

Looks OK to me, but will need review from someone more familiar with the GL backend
Attachment #487292 - Flags: feedback?(roc) → feedback+
Attached patch World transform OGL (obsolete) — Splinter Review
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #489098 - Flags: review?(vladimir)
Attached patch Qt dependent part (obsolete) — Splinter Review
Attachment #489099 - Flags: review?(vladimir)
vlad any review comments?
Attached patch Qt World transform apply (obsolete) — Splinter Review
Attachment #489099 - Attachment is obsolete: true
Attachment #497433 - Flags: review?(vladimir)
Attachment #489099 - Flags: review?(vladimir)
Attached patch LayerManagerOGL WorldTransform (obsolete) — Splinter Review
Attachment #489098 - Attachment is obsolete: true
Attachment #497434 - Flags: review?(vladimir)
Attachment #489098 - Flags: review?(vladimir)
Blocks: 619056
vlad: could you check these patches plz?
Comment on attachment 497433 [details] [diff] [review]
Qt World transform apply

vlad is busy, so this part I think better to ask someone else to review
Attachment #497433 - Flags: review?(vladimir) → review?(mozilla)
Attachment #497434 - Flags: review?(vladimir) → review?(matt.woodrow+bugzilla)
Comment on attachment 497434 [details] [diff] [review]
LayerManagerOGL WorldTransform


>+    if (!needsFramebuffer) {
>+      aManager->WorldTransformRect(scissorRect);
>+    }
>+

Won't this be incorrect if the world transform is a matrix that doesn't preserve axis aligned rectangles?

If we know the world transform will always be one of a few fixed possibilities, then maybe some sort of SetOrientation api might make more sense, where we construct the transform ourselves and can limit it to ones that preserve axis aligned rectangles.

Otherwise I think we need to render to an intermediate buffer without transforming (use the existing FBO code), and then transform by the world matrix on the final blit (without clipping enabled).

I guess even explicit documentation and some assertions could work with the current code if you're worried about the overhead of an extra blit on mobile.
intermediate buffer - is definitely not an option here.
Also we need in some cases translate transformation too (statusbar rendering).

would it be enough in SetWorldTransform set check like this:
gfxMatrix matr;
if (!From2D(matr).PreservesAxisAlignedRectangles())
   NS_ERROR("SetWorldTransform accept only PreservesAxisAlignedRectangles transformation");
   return;
}
?
also document it in header.
Yes, that sounds fine to me.
Attachment #497434 - Attachment is obsolete: true
Attachment #505371 - Flags: review?(matt.woodrow+bugzilla)
Attachment #497434 - Flags: review?(matt.woodrow+bugzilla)
Comment on attachment 505371 [details] [diff] [review]
Updated patch. added Matrix check and simple comment

Sounds like matt is not available for some time...
Joe could you r this patch?
Attachment #505371 - Flags: review?(joe)
Comment on attachment 505371 [details] [diff] [review]
Updated patch. added Matrix check and simple comment


>-    aManager->SetupPipeline(visibleRect.width, visibleRect.height);
>+    aManager->SetupPipeline(visibleRect.width, visibleRect.height, PR_FALSE, PR_FALSE);

Two bool parameters make this really hard to read. Maybe a flags enum would be easier to follow?
Why is this PR_FALSE for the first param? This would have been PR_TRUE under the old code.

Since we don't have tests running for mobile yet, maybe you should try hard coding a 180 degree flip
and push it to tryserver to get desktop results.

> 
>     if (needsFramebuffer) {
>       scissorRect.MoveBy(- visibleRect.TopLeft());
>     } else {
>+      // Transform scissorRect here
>+      aManager->WorldTransformRect(scissorRect);
>+
>       if (!aPreviousFrameBuffer) {

Why aren't we transforming the scissor rect when a framebuffer is created? The viewport should still have the world transform applied right?


>-LayerManagerOGL::SetupPipeline(int aWidth, int aHeight)
>+LayerManagerOGL::SetWorldTransform(const gfxMatrix& aMatrix)
>+{
>+  if (!aMatrix.PreservesAxisAlignedRectangles()) {
>+    NS_ERROR("SetWorldTransform accept only PreservesAxisAlignedRectangles matrix");

NS_ASSERTION would be fine here.

"SetWorldTransform only accepts matrices that satisfy PreservesAxisAlignedRectangles"
> coding a 180 degree flip
> and push it to tryserver to get desktop results.

What should I get with that? IIUC I'll see all tests failed, due to rotated upside down image..
I have tested this patch in different combinations with/without bug 607653 patches, and on Qt/Meegotouch platform. on Lemonde.fr page

> 
> > 
> >     if (needsFramebuffer) {
> >       scissorRect.MoveBy(- visibleRect.TopLeft());
> >     } else {
> >+      // Transform scissorRect here
> >+      aManager->WorldTransformRect(scissorRect);
> >+
> >       if (!aPreviousFrameBuffer) {
> 
> Why aren't we transforming the scissor rect when a framebuffer is created? The

We should not do that, we apply WorldTransform in SetupPipeline call, + SetLayerTransform, but in current code we are not supposed to transform scissor anyhow, because GetEffectiveTransform at least does not PreservesAxisAlignedRectangles in that case.

Also I just to be sure tested this patch on page where we need temp FBO created (lemonde.fr without bug 607653 patches ) and clipping is wrong if I transform scissors when needsFramebuffer == true.

> viewport should still have the world transform applied right?
Attachment #505371 - Attachment is obsolete: true
Attachment #508395 - Flags: review?(matt.woodrow+bugzilla)
Attachment #505371 - Flags: review?(matt.woodrow+bugzilla)
Attachment #505371 - Flags: review?(joe)
Comment on attachment 508395 [details] [diff] [review]
Updated patch, apply global transform


> 
> What should I get with that? IIUC I'll see all tests failed, due to rotated
> upside down image..

Since both the tests and references are flipped, they should still pass.

 
> We should not do that, we apply WorldTransform in SetupPipeline call, +
> SetLayerTransform, but in current code we are not supposed to transform scissor
> anyhow, because GetEffectiveTransform at least does not
> PreservesAxisAlignedRectangles in that case.

Right, sorry, I missed that SetupPipeline for an intermediate isn't transforming by the world transform.
Attachment #508395 - Flags: review?(matt.woodrow+bugzilla) → review+
Comment on attachment 497433 [details] [diff] [review]
Qt World transform apply

this patch is obsolete due to bug 626595
Attachment #497433 - Flags: review?(mozilla)
> Since both the tests and references are flipped, they should still pass.
> 
yes it does, I set Rotate(M_PI) in LayerManager constructor and that works
Comment on attachment 508395 [details] [diff] [review]
Updated patch, apply global transform

Tests are green can we push it ?
Attachment #508395 - Flags: approval2.0?
     if (needsFramebuffer) {
       scissorRect.MoveBy(- visibleRect.TopLeft());
     } else {
+      // Transform scissorRect here
+      aManager->WorldTransformRect(scissorRect);
+

If the ancestor has an intermediate surface, this isn't right. Is it?

+LayerManagerOGL::SetupPipeline(int aWidth, int aHeight, PRBool aWorldTransform)

boolean parameters kinda suck. How about at least using a flags word?

+LayerManagerOGL::WorldTransformRect(nsIntRect& aRect)
+{
+  gfxRect grect(aRect.x, aRect.y, aRect.width, aRect.height);
+  grect = mWorldMatrix.TransformBounds(grect);
+  aRect.SetRect(grect.pos.x, grect.pos.y, grect.size.width, grect.size.height);

I think this really only works if the world transform matrix is a 90-degree rotation and/or a flip. We should document that in the API and assert it.
Attachment #497433 - Attachment is obsolete: true
Attachment #508395 - Attachment is obsolete: true
Attachment #509115 - Flags: review?(roc)
Attachment #508395 - Flags: approval2.0?
Comment on attachment 509115 [details] [diff] [review]
Fixed roc comments

+  if (!aMatrix.PreservesAxisAlignedRectangles()) {
+    NS_ASSERTION(aMatrix.PreservesAxisAlignedRectangles(),
+                 "SetWorldTransform only accepts matrices that satisfy PreservesAxisAlignedRectangles");
+    return;
+  }
+  if (aMatrix.HasNonIntegerScale()) {
+    NS_ASSERTION(!aMatrix.HasNonIntegerScale(),
+                 "SetWorldTransform only accepts matrices with integer scale");
+    return;
+  }

You don't need the ASSERTION and the if. You probably just want the assertion.

r+ with that.
Attachment #509115 - Flags: review?(roc) → review+
Fixed last comment, moving r+, requesting approval.
Attachment #509115 - Attachment is obsolete: true
Attachment #509202 - Flags: review+
Attachment #509202 - Flags: approval2.0?
sent bunch to try server:
http://hg.mozilla.org/try/rev/3d498f0b90d3

I think it should be good.
Comment on attachment 509115 [details] [diff] [review]
Fixed roc comments

oh, let's keep this review visible
Attachment #509115 - Attachment is obsolete: false
Blocks: 630186
Comment on attachment 509202 [details] [diff] [review]
Only assertions, no if.

I would prefer if this had some tests, and if we go to add any users of this code, I will insist upon it.
Attachment #509202 - Flags: approval2.0? → approval2.0+
we are going to add user in Bug 630186. possibly it make sense add some test for this API there too.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/c8a2a8063a1c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
I backed this patch as part of this pushlog <http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7e12e3e16e6c> because of the oranges it caused <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296852850.1296854923.2345.gz&fulltext=1>.

I'm not sure which one of the bugs was at fault, so I just backed them all out.  The assignee needs to investigate and make sure that his patch has not been the culprit, and then re-add checkin-needed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded all but one of the patches in that push.  For this bug, that's:
http://hg.mozilla.org/mozilla-central/rev/c96db95315d3
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
thanks.
You need to log in before you can comment on or make changes to this bug.