WIndows OMTC window resizing doesn't really work

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

Trunk
mozilla26
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

With both d3d9 and 11 OMTC, when resizing the window it looks ugly as hell due to the graphics in the window being scaled rather than clipped and this happening in a jumpy way.

I have identified and fixed a whole bunch of minor bugs which affect this, but I think there is something fundamentally broken about our model for resizing. I have a hack fix for that, but maybe we want a better one.

Patches and more discussion coming up...
Posted patch tidy up pt1Splinter Review
Attachment #782965 - Flags: review?(matt.woodrow)
Attachment #782967 - Flags: review?(bas)
Attachment #782968 - Flags: review?(bas)
Posted patch possible, partial solution (obsolete) — Splinter Review
So, this is where it gets interesting. The issue here is that while we are busy compositing the size of the window changes and so if we present, then we are doing so with the wrong size. This solution (which I think is a bit of a hack, but maybe we can think of it as double checked locking, without the locking :-s ) checks to see if the size has changed, and if so, don't present. It works. We should probably schedule another composition immediately in this case, but it works without, maybe not as smoothly.

An alternative would be to do a sync composite on resize, which would prevent the concurrent change of window size (I think). But, I think that is a big change, because at the moment (AFAICT) we don't respond to WM_SIZE messages, we just implicitly deal with resizing. I'm not sure if we can hook into the invalidation we are presumably doing and force a sync composite. Does that sound feasible? Better?

Any other ideas?
Attachment #782970 - Flags: feedback?(roc)
Attachment #782970 - Flags: feedback?(matt.woodrow)
Attachment #782970 - Flags: feedback?(bas)
Comment on attachment 782970 [details] [diff] [review]
possible, partial solution

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

I think this is OK, but sync composites during resizing would probably make things even better.

Off the top of my head I don't know exactly when is the right time during resizing to do those composites. Win32 resizing is pretty complex, with lots of WM_ messages involved. You could use Spy++ to see the message sequence and try doing sync composites during various messages.
Attachment #782970 - Flags: feedback?(roc) → feedback+
Attachment #782967 - Flags: review?(bas) → review+
Comment on attachment 782968 [details] [diff] [review]
tidy up pt3 - d3d11

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +570,3 @@
>    UpdateRenderTarget();
>  
> +  if (mSize.width == 0 || mSize.height == 0) {

All this mSize stuff is kind of ugly, but oh well.

@@ +677,3 @@
>                                DXGI_FORMAT_B8G8R8A8_UNORM,
>                                0);
> +#ifdef MOZ_METRO

I don't think we need this do we?

@@ +833,5 @@
>                          map.RowPitch,
>                          gfxASurface::ImageFormatARGB32);
>  
>    mTarget->SetSource(tmpSurface);
> +  mTarget->SetOperator(gfxContext::OPERATOR_SOURCE);

Hrm, this is a tricky thing, this'll likely be slow on Direct2D which you might very well have here. But it is more correct strictly speaking.
Attachment #782968 - Flags: review?(bas) → review+
Comment on attachment 782970 [details] [diff] [review]
possible, partial solution

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

I'm not sure if I'm in favor of doing this approach for now. I feel we should be doing a sync composite. I suspect we'll want to do this sync composite on the first WM_PAINT message after we notice the size changed.
Attachment #782970 - Flags: feedback?(bas)
Comment on attachment 782965 [details] [diff] [review]
tidy up pt1

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

::: gfx/layers/d3d11/CompositorD3D11.h
@@ +140,5 @@
> +                        "You should not do that outside of BeginFrame, so the best we can do is return "
> +                        "the last size we got, that might not be up to date. So you probably shouldn't "
> +                        "use this method.");
> +    return mSize;
> +  }

Since this is going to assert every time, does that mean there are no callers?

And is that specific to windows, or is that true for all the compositors?

It seems like we should just get rid of this entirely.
(In reply to Bas Schouten (:bas.schouten) from comment #6)
> Comment on attachment 782968 [details] [diff] [review]
> tidy up pt3 - d3d11
> 
> Review of attachment 782968 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +677,3 @@
> >                                DXGI_FORMAT_B8G8R8A8_UNORM,
> >                                0);
> > +#ifdef MOZ_METRO
> 
> I don't think we need this do we?
>
I don't think we need it, but I'm not sure about building without the Win8 sdk and we have these guards further up in the same file and in the equivalent place in LayerManagerD3D10, so it was more for consistency.
 
> @@ +833,5 @@
> >                          map.RowPitch,
> >                          gfxASurface::ImageFormatARGB32);
> >  
> >    mTarget->SetSource(tmpSurface);
> > +  mTarget->SetOperator(gfxContext::OPERATOR_SOURCE);
> 
> Hrm, this is a tricky thing, this'll likely be slow on Direct2D which you
> might very well have here. But it is more correct strictly speaking.

You told me to add it for d3d9 :-) I think we only use this for tests anyway (and maybe DrawWindow or something) so I think correctness is more important than speed.

(In reply to Bas Schouten (:bas.schouten) from comment #7)
> Comment on attachment 782970 [details] [diff] [review]
> possible, partial solution
> 
> Review of attachment 782970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure if I'm in favor of doing this approach for now. I feel we
> should be doing a sync composite. I suspect we'll want to do this sync
> composite on the first WM_PAINT message after we notice the size changed.

WM_PAINT is not good enough - we already do a sync composite for WM_PAINT. There are two problems - one is that we only get WM_PAINT when the window gets larger, not when it gets small. And two, we don't get it often enough - we still have time to do a broken async composite in between the WM_PAINT messages. We probably need to listen for another message to tackle the first problem and leave the detection code in for the second.
(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> Comment on attachment 782965 [details] [diff] [review]
> tidy up pt1
> 
> Review of attachment 782965 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d11/CompositorD3D11.h
> @@ +140,5 @@
> > +                        "You should not do that outside of BeginFrame, so the best we can do is return "
> > +                        "the last size we got, that might not be up to date. So you probably shouldn't "
> > +                        "use this method.");
> > +    return mSize;
> > +  }
> 
> Since this is going to assert every time, does that mean there are no
> callers?
> 
> And is that specific to windows, or is that true for all the compositors?
> 
> It seems like we should just get rid of this entirely.

It is specifically tied to tiled thebes layers, so at the moment only on Android. But when we implement it everywhere this would bite someone if not for this assert. I haven't tested on desktop GL, but I think it is only a Windows problem. There are indeed no callers on Windows with the other fix in this patch.
Attachment #782965 - Flags: review?(matt.woodrow) → review+
Blocks: 899785
Blocks: 899787
No longer blocks: 756606, 874721
Depends on: 756606, 874721
Whiteboard: [leave open]
Version: 21 Branch → Trunk
Attachment #782965 - Flags: checkin+
Attachment #782967 - Flags: checkin+
Attachment #782968 - Flags: checkin+
Posted patch d3d_resize3.patch (obsolete) — Splinter Review
Attachment #784815 - Flags: feedback?(roc)
Attachment #784815 - Attachment is obsolete: true
Attachment #784815 - Flags: feedback?(roc)
Posted patch possible solution (obsolete) — Splinter Review
How would you feel about doing this? It solves the problem by causing us to get a WM_PAINT when the window shrinks as well as when it grows, and thus a sync composite. But it invalidates the whole window, from how I understand the docs (http://msdn.microsoft.com/en-us/library/windows/desktop/ff729176%28v=vs.85%29.aspx). I'm not sure if that is bad - it sounds bad, but paint flashing makes it look like we do that anyway.

If we do this, we probably want the checks in the compositor too, although I didn't notice any mistakes with this alone, I think it is still possible. There are also window classes that I didn't look at which might need adjusting, and maybe we should avoid this for the invisible window case etc.
Attachment #784817 - Flags: feedback?(roc)
Comment on attachment 784817 [details] [diff] [review]
possible solution

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

We always recomposite the whole window with D3D anyway, so I don't think this really hurts when D3D is enabled.

I think this is a good idea. Probably we should make it conditional on OMTC for now, though.
Attachment #784817 - Flags: feedback?(roc) → feedback+
Attachment #782970 - Attachment is obsolete: true
Attachment #784817 - Attachment is obsolete: true
Attachment #782970 - Flags: feedback?(matt.woodrow)
Attachment #787328 - Flags: review?(roc)
Comment on attachment 787328 [details] [diff] [review]
part 4 - smooth resizing for OMTC

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

::: widget/windows/nsWindow.cpp
@@ +6569,5 @@
> +nsWindow::WindowUsesOMTC()
> +{
> +  ULONG_PTR style = ::GetClassLongPtr(mWnd, GCL_STYLE);
> +  if (!style) {
> +    NS_WARNING("Cpould not get window class style");

Fix "Cpould" typo
Attachment #787328 - Flags: review?(roc) → review+
Depends on: 902929
Attachment #787328 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/3d4fa3ab3108
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
No longer depends on: 902929
You need to log in before you can comment on or make changes to this bug.