899435 - WIndows OMTC window resizing doesn't really work

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

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...

Comment 1

[Nick Cameron [:nrc]]

Attachment: tidy up pt1

Comment 2

[Nick Cameron [:nrc]]

Attachment: tidy up pt2 - d3d9

Comment 3

[Nick Cameron [:nrc]]

Attachment: tidy up pt3 - d3d11

Comment 4

[Nick Cameron [:nrc]]

Attachment: possible, partial solution

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?

Comment 5

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

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.

Comment 6

[Bas Schouten (:bas.schouten)]

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.

Comment 7

[Bas Schouten (:bas.schouten)]

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.

Comment 8

[Matt Woodrow (:mattwoodrow)]

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.

Comment 9

[Nick Cameron [:nrc]]

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

Comment 10

[Nick Cameron [:nrc]]

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

Comment 11

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee14a57c92b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/beecdffc9ebd
https://hg.mozilla.org/integration/mozilla-inbound/rev/112a7dc732e3

Comment 12

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8df204adb4

Comment 13

[Nick Cameron [:nrc]]

And out again :-(

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbcffb6957ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/76eced21cb7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c27f63e03ad7

Comment 14

[Nick Cameron [:nrc]]

Attachment: d3d_resize3.patch

Comment 15

[Nick Cameron [:nrc]]

Attachment: possible solution

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.

Comment 16

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6962b015a6a

Comment 17

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

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.

Comment 18

[Nick Cameron [:nrc]]

Attachment: part 4 - smooth resizing for OMTC

Comment 19

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

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

Comment 20

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=ba7ad685fde9

Comment 21

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4fa3ab3108

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d4fa3ab3108