Closed Bug 776217 Opened 7 years ago Closed 7 years ago

Orientation isn't honored by compositor thread

Categories

(Core :: Widget, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
(Not really a "fix" since it won't always work.)
Attached patch Real fix (obsolete) — Splinter Review
Fixes one bug but adds another.  I think I broken widget/gonk/nsWindow.  Off to dinner, will fix and add comments tonight.
Assignee: nobody → jones.chris.g
Attachment #644611 - Attachment is obsolete: true
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.
(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.
Attachment #644721 - Attachment is obsolete: true
Attachment #644721 - Flags: review?(roc)
Attachment #644996 - Flags: review?(roc)
(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?
(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.?
So this code can set up the correct world transform? :-)
With requested changes.
Attachment #644996 - Attachment is obsolete: true
Attachment #644996 - Flags: review?(roc)
Attachment #645185 - Flags: review?(roc)
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.
https://hg.mozilla.org/mozilla-central/rev/97040ffd8e31
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
No longer depends on: 843940
You need to log in before you can comment on or make changes to this bug.