Last Comment Bug 776217 - Orientation isn't honored by compositor thread
: Orientation isn't honored by compositor thread
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on:
Blocks: 745148
  Show dependency treegraph
 
Reported: 2012-07-21 00:52 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2013-02-25 16:06 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Minimal fix for gonk, unlandable (2.11 KB, patch)
2012-07-21 01:32 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Real fix (34.92 KB, patch)
2012-07-21 19:13 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Support gecko-implemented screen rotation with omtc (35.75 KB, patch)
2012-07-21 22:22 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Support gecko-implemented screen rotation with omtc, v2 (35.79 KB, patch)
2012-07-23 11:23 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Support gecko-implemented screen rotation with omtc, v3 (40.31 KB, patch)
2012-07-23 21:07 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-21 00:52:34 PDT
When we change orientation, content reflows properly and we repaint the correct dimensions.  However, the compositor isn't notified of screen rotation, so it keeps compositing without the transform induced by the rotation.

There are a variety of ways to fix this, but I think the cleanest may be to send the orientation as part of layers transactions and let the compositor apply the world transform we currently use.  This is relatively simple and non-racy.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-21 01:32:37 PDT
Created attachment 644611 [details] [diff] [review]
Minimal fix for gonk, unlandable
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-21 01:33:09 PDT
(Not really a "fix" since it won't always work.)
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-21 19:13:52 PDT
Created attachment 644708 [details] [diff] [review]
Real fix

Fixes one bug but adds another.  I think I broken widget/gonk/nsWindow.  Off to dinner, will fix and add comments tonight.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-21 22:22:09 PDT
Created attachment 644721 [details] [diff] [review]
Support gecko-implemented screen rotation with omtc
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-22 15:26:08 PDT
Comment on attachment 644721 [details] [diff] [review]
Support gecko-implemented screen rotation with omtc

Review of attachment 644721 [details] [diff] [review]:
-----------------------------------------------------------------

This approach seems reasonable...

::: gfx/layers/ipc/PLayers.ipdl
@@ +32,5 @@
> +};
> +
> +union MaybeTargetConfig {
> +  null_t;
> +  TargetConfig;

Why MaybeTargetConfig? Can't we always provide a TargetConfig?

::: widget/WidgetUtils.h
@@ +18,5 @@
> +  ROTATION_90,
> +  ROTATION_180,
> +  ROTATION_270,
> +
> +  SENTINEL_ROTATION

I'd call this ROTATION_COUNT

::: widget/xpwidgets/nsBaseWidget.h
@@ +188,5 @@
>    class AutoLayerManagerSetup {
>    public:
>      AutoLayerManagerSetup(nsBaseWidget* aWidget, gfxContext* aTarget,
> +                          BasicLayerManager::BufferMode aDoubleBuffering,
> +                          const nsIntRect& aNaturalWidgetBounds = nsIntRect(),

Can't we get aWidget's bounds and use that?

@@ +189,5 @@
>    public:
>      AutoLayerManagerSetup(nsBaseWidget* aWidget, gfxContext* aTarget,
> +                          BasicLayerManager::BufferMode aDoubleBuffering,
> +                          const nsIntRect& aNaturalWidgetBounds = nsIntRect(),
> +                          ScreenRotation aRotation = mozilla::ROTATION_0);

Seems like aNaturalWidgetBounds will never be used for ROTATION_0? If so, please document.
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 10:31:14 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Comment on attachment 644721 [details] [diff] [review]
> Support gecko-implemented screen rotation with omtc
> 
> ::: gfx/layers/ipc/PLayers.ipdl
> @@ +32,5 @@
> > +};
> > +
> > +union MaybeTargetConfig {
> > +  null_t;
> > +  TargetConfig;
> 
> Why MaybeTargetConfig? Can't we always provide a TargetConfig?
> 

We can either mark explicitly when we don't provide a config (what this patch does), or propagate a gentleman's agreement that rotation==0 && bounds.IsEmpty() means no config.  I prefer the former, but don't feel strongly about it, so will change.

> ::: widget/WidgetUtils.h
> @@ +18,5 @@
> > +  ROTATION_90,
> > +  ROTATION_180,
> > +  ROTATION_270,
> > +
> > +  SENTINEL_ROTATION
> 
> I'd call this ROTATION_COUNT
> 

OK.

> ::: widget/xpwidgets/nsBaseWidget.h
> @@ +188,5 @@
> >    class AutoLayerManagerSetup {
> >    public:
> >      AutoLayerManagerSetup(nsBaseWidget* aWidget, gfxContext* aTarget,
> > +                          BasicLayerManager::BufferMode aDoubleBuffering,
> > +                          const nsIntRect& aNaturalWidgetBounds = nsIntRect(),
> 
> Can't we get aWidget's bounds and use that?
> 

We don't have a getter for the "natural bounds" (not taking rotation into account).  I'm not sure it makes sense to expose that publicly.

> @@ +189,5 @@
> >    public:
> >      AutoLayerManagerSetup(nsBaseWidget* aWidget, gfxContext* aTarget,
> > +                          BasicLayerManager::BufferMode aDoubleBuffering,
> > +                          const nsIntRect& aNaturalWidgetBounds = nsIntRect(),
> > +                          ScreenRotation aRotation = mozilla::ROTATION_0);
> 
> Seems like aNaturalWidgetBounds will never be used for ROTATION_0? If so,
> please document.

That's right.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 11:23:40 PDT
Created attachment 644996 [details] [diff] [review]
Support gecko-implemented screen rotation with omtc, v2
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-23 15:36:20 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> We can either mark explicitly when we don't provide a config (what this
> patch does), or propagate a gentleman's agreement that rotation==0 &&
> bounds.IsEmpty() means no config.  I prefer the former, but don't feel
> strongly about it, so will change.

Why can't we pass a real config all the time? I.e. rotation==0 and the regular widget bounds.

> We don't have a getter for the "natural bounds" (not taking rotation into
> account).  I'm not sure it makes sense to expose that publicly.

Why not?
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 15:42:00 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> > We don't have a getter for the "natural bounds" (not taking rotation into
> > account).  I'm not sure it makes sense to expose that publicly.
> 
> Why not?

It's an internal implementation detail of widget/ code.  I'm somewhat sad we need to leak this out into the compositor, but it's the only correct fix.

Why would any code possibly need to know the natural widget bounds vs. the bounds we redraw, reflow, tell content about, etc.?
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-23 17:30:17 PDT
So this code can set up the correct world transform? :-)
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 21:07:54 PDT
Created attachment 645185 [details] [diff] [review]
Support gecko-implemented screen rotation with omtc, v3

With requested changes.
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-24 01:06:58 PDT
I had to add this code to LayerManagerD3D10.cpp to fix windows build bustage

-    ShadowLayerForwarder::BeginTransaction();
+    ShadowLayerForwarder::BeginTransaction(mWidget->GetNaturalBounds(),
+                                           ROTATION_0);
     

but seems trivial enough not to post another patch iteration.
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-24 12:01:54 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/97040ffd8e31
Comment 14 Ed Morley [:emorley] 2012-07-25 08:11:31 PDT
https://hg.mozilla.org/mozilla-central/rev/97040ffd8e31

Note You need to log in before you can comment on or make changes to this bug.