[Mesa-dev] [PATCH] wayland/egl: initialize window surface size to window size

[Juan A. Suarez Romero]

On Tue, 2018-06-05 at 12:41 +0100, Daniel Stone wrote:
> Hi Juan,
> 
> On 5 June 2018 at 09:51, Juan A. Suarez Romero <jasuarez at igalia.com> wrote:
> > On Mon, 2018-06-04 at 13:22 +0100, Daniel Stone wrote:
> > > The first query will correctly return (w1,h1). The second query will
> > > incorrectly also return (w1,h1), even though the surface will never
> > > have that size. The third query will return (w2,h2) as the
> > > last-attached size, even though the surface will be (w3,h3) on next
> > > draw. The fourth query will correctly return (w3,h3).
> > > 
> > > I would like this to be consistent: my suggestion is that a query for
> > > the surface size always returns the size that the surface will become
> > > on the next eglSwapBuffers.
> > 
> > I've been re-reading again EGL 1.5 spec, and found this interesting parts that
> > can bring some clarification to this mess:
> 
> 'Mess' is right. :) Probably because the spec is clearly written from
> platforms which actually have a native window size outside of client
> control. e.g. on X11, the window manager can resize a window and from
> the client's point of view this is something which just happens to it:
> neither the application code nor EGL has any say at all on it. Wayland
> doesn't have this constraint, and puts the client (in this case, the
> EGL implementation) in full control of the size at all times.
> 

:)

> > - Section 3.10.1.1 ("Native Window Resizing"): if the native window
> > corresponding to _surface_ has been resized prior to the swap, _surface_ must be
> > resized to match. _surface_ will normally be resized by the EGL implementation
> > at the time the native window is resized. If the implementation cannot do this
> > transparently to the client, then *eglSwapBuffers* must detect the change and
> > resize surface prior to copying its pixels to the native window.
> > 
> > - Section 3.5.6 ("Surface Attributes"): querying `EGL_WIDTH` and `EGL_HEIGHT`
> > returns respectively the width and height, in pixels, of the surface. For a
> > window or pixmap surface, these values are initially equal to the width and
> > height of the native window or pixmap with respect to which surface was created.
> > If a native window is resized, the corresponding window surface will eventually
> > be resized by the implementation to match (as discussed in section 3.10.1). If
> > there is a discrepancy because EGL has not yet resized the window surface, the
> > size returned by *eglQuerySurface* will always be that of the EGL surface, not
> > the corresponding native window.
> > 
> > 
> > From the previous parts, I extract the following conclusions:
> > 
> > - Surface size value is indeed initialized with the native window size, as we
> > are doing in our patch.
> 
> Agree.
> 

Nice.

> > - In Wayland/Mesa, the surface is not resized at the time the native window is
> > resized; this is done in *eglSwapBuffers*, following 3.10.1.1.
> 
> Disagree, given that we are in charge of exactly when the native
> window is 'resized'. It's unclear to me what 'resize the surface prior
> to copying its pixels to the native window' even means: does this
> require we do a scale blit, or a crop blit, or?
> 
> The semantics Mesa has interpreted until recently is that
> wl_egl_resize_window() constitutes a resize _request_, and that the
> last submitted request is acted upon and latched at the first draw
> call. This then constitutes the native surface size up until the next
> first draw call. I think these are good semantics, and we have to be
> quite picky about what exactly we implement, since clients rely on
> them. Not just cosmetically (atomic resizing of subsurface chains),
> but also because if you get your window sizing wrong on the desktop,
> you'll catch a fatal error from the compositor (X11 just shrugs and
> does nothing).
> 
> I think these are good semantics (as to exactly when resize takes
> effect), but for the moment we only update the EGL_{WIDTH,HEIGHT}
> surface query results after the first draw call. I would suggest that
> in the period after eglSwapBuffers but before the first draw call, we
> also update those values, and that we update them after eglSwapBuffers
> has executed as well. This makes the semantics that any native window
> resize requests take effect immediately after eglSwapBuffers (or just
> immediately, if no swap has ever been executed) and before the first
> draw call (or buffer_age query ... cf. 9ca6711faa03) for a given
> frame; in between the first draw call and eglSwapBuffers, the native
> resize requests will be queued until after eglSwapBuffers.
> 


I see. The surface size should be updated inmediately, after a window resize is
done, as you said.

I'll send then a new patch version. Actually I'll split this patch in two: one
to correctly initialize the surface size (there is no doubt on this, and this
patch alone will fix the test), and another patch to correctly update the
surface size when the window is resized.


> > - If our EGL implementation has not resized the window surface, then
> > *eglQuerySurface* should return the current size of the surface, not the future
> > size, as the future size is the native window size. This is the main difference:
> > *eqlQuerySurface* does not return the size the surface will become on next
> > eglSwapBuffers, but the current size.
> 
> 3.5.6 ('EGL has not yet resized the window surface') and 3.10.1.1
> ('will normally be resized by the EGL implementation at the time the
> native window is resized') seem to contradict each other here. But the
> reason for 3.5.6 wording is almost certainly coming from X11, where
> the native window size changes without notification to the EGL
> implementation, and the first time it's plausible for the
> implementation to know that a resize happened is inside
> eglSwapBuffers. As that doesn't apply here, I prefer to go with
> 3.10.1.1's 'immediately' wording, and that (unless we are mid-draw)
> the EGL surface immediately reflects the native window size.
> 
> > If the above conclusions are correct, then the calls to eglQuerySurface() in the
> >  example code should return:
> > 
> > - (w1,h1): this is the initial surface window size.
> > - (w1,h1): even when the window was resized to (w2,h2), in Mesa this does not
> > take effect until calling eglSwapBuffers(). So current size is still (w1,h1)
> 
> In Mesa, this takes effect during the glClear() call, not during
> eglSwapBuffers; making the same query immediately after glClear()
> would return (w2,h2). I would propose we return the same value here
> for consistency.
> 
> > - (w2,h2): like the previous call, the native window was resized to (w3,h3), but
> >  eglSwapBuffers() was not called yet.
> 
> Ditto.
> 
> > - (w2,h2) or (w3,h3)?: I have doubts here. On one side, we should return
> > (w2,h2), as eglSwapBuffers() was not called yet. On the other hand, not sure if
> > glClear() should update the size. Maybe this is something dependent on the
> > implementation.
> 
> I don't share your interpretation that waiting for an eglSwapBuffers()
> call is required for a resize to take effect.
> 

Right. 

> > > Yes, that's correct, and I believe eglMakeCurrent() does perform that
> > > attachment. The NVIDIA/EGLStreams implementation is extremely broken
> > > in that regard, in a way which does break genuine real-world
> > > applications. We had a long discussion on wayland-devel@ when the
> > > EGLStreams implementation was first posted, detailing the ways in
> > > which their implementation causes genuine problems for real-world
> > > users, and can provoke client deadlocks. I am very opposed to
> > > supporting or codifying that behaviour.
> > 
> > So I understand eglSwapBuffers() is required to do the attachment.
> 
> It is required to do an attachment, yeah.
> 

Nice. The other failing test in CTS is not calling eglSwapBuffers() before doing
the first wl_gl_window_get_attached_size(), and hence when we call it we get
null values.

So it seems the fix must be done in CTS, bycalling eglSwapBuffers() to ensure
the window has an attached buffer, before getting the size.

> > In the example above, if we replace eglQuerySurface() by
> > wl_egl_window_get_attached_size(), what would be the expected values?
> 
> - (0, 0) as no attachment has ever been made
> - (0, 0) due to the same reason
> - (w2, h2) as we have called eglSwapBuffers at this point, and that
> used (w2,h2) for its buffer size as it was the most current value at
> the time the frame's first draw call (in this case glClear) was made
> - (w2, h2) due to the same reason
> 
> I'm really sorry about the length this thread is taking to resolve.
> You've stepped on quite a landmine with a very broken test and a
> poorly-documented spec. I would happily review and help merge any
> effort to codify this, either in libwayland-egl itself, or in an
> update to EGL_KHR_platform_wayland.
> 

No worries! Actually, I'm very very much grateful for your pantience and help.


BR,
	J.A.


> Cheers,
> Daniel
>