Closed
Bug 776217
Opened 12 years ago
Closed 12 years ago
Orientation isn't honored by compositor thread
Categories
(Core :: Widget, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: cjones, Assigned: cjones)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
(Not really a "fix" since it won't always work.)
Assignee | ||
Comment 3•12 years ago
|
||
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
Assignee | ||
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 6•12 years ago
|
||
(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.
Assignee | ||
Comment 7•12 years ago
|
||
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?
Assignee | ||
Comment 9•12 years ago
|
||
(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? :-)
Assignee | ||
Comment 11•12 years ago
|
||
With requested changes.
Attachment #644996 -
Attachment is obsolete: true
Attachment #644996 -
Flags: review?(roc)
Attachment #645185 -
Flags: review?(roc)
Assignee | ||
Comment 12•12 years ago
|
||
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+
Assignee | ||
Comment 13•12 years ago
|
||
Comment 14•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•