776217 - Orientation isn't honored by compositor thread

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

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

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]

Attachment: Minimal fix for gonk, unlandable

Comment 2

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

(Not really a "fix" since it won't always work.)

Comment 3

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

Attachment: 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]

Attachment: Support gecko-implemented screen rotation with omtc

Comment 5

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

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]

(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]

Attachment: Support gecko-implemented screen rotation with omtc, v2

Comment 8

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

(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]

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

So this code can set up the correct world transform? :-)

Comment 11

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

Attachment: Support gecko-implemented screen rotation with omtc, v3

With requested changes.

Comment 12

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

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]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97040ffd8e31

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/97040ffd8e31