The default bug view has changed. See this FAQ.

Orientation isn't honored by compositor thread

RESOLVED FIXED in mozilla17

Status

()

Core
Widget
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

Trunk
mozilla17
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Created attachment 644611 [details] [diff] [review]
Minimal fix for gonk, unlandable
(Not really a "fix" since it won't always work.)
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.
Assignee: nobody → jones.chris.g
Attachment #644611 - Attachment is obsolete: true
Created attachment 644721 [details] [diff] [review]
Support gecko-implemented screen rotation with omtc
Attachment #644708 - Attachment is obsolete: true
Attachment #644721 - Flags: review?(roc)
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.
Created attachment 644996 [details] [diff] [review]
Support gecko-implemented screen rotation with omtc, v2
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? :-)
Created attachment 645185 [details] [diff] [review]
Support gecko-implemented screen rotation with omtc, v3

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.
Attachment #645185 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/97040ffd8e31
https://hg.mozilla.org/mozilla-central/rev/97040ffd8e31
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 843940

Updated

4 years ago
No longer depends on: 843940
You need to log in before you can comment on or make changes to this bug.