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

[Tapani Pälli]

On 05.06.2018 11:51, Juan A. Suarez Romero wrote:
> On Mon, 2018-06-04 at 13:22 +0100, Daniel Stone wrote:
>> Hi Juan,
>>
>> On 4 June 2018 at 12:43, Juan A. Suarez Romero <jasuarez at igalia.com> wrote:
>>> On Fri, 2018-06-01 at 16:32 +0100, Daniel Stone wrote:
>>>> I think you're right, and this needs more rework to be consistent.
>>>>
>>>> wl_egl_window_get_attached_size() always returns the size of the last
>>>> attached buffer (if there have been any); that is good and consistent.
>>>
>>> Let's focus now on this, as I already sent a V2 patch to properly initialize
>>> surface Width/Height values.
>>
>> Right, but in doing so it introduces an inconsistency. As below:
>> wl_egl_create_window(w1, h1);
>> eglCreateSurface();
>> eglQuerySurface(EGL_{WIDTH,HEIGHT});
>> wl_egl_resize_window(w2, h2);
>> eglQuerySurface(EGL_{WIDTH,HEIGHT});
>> glClear();
>> eglSwapBuffers();
>> wl_egl_resize_window(w3, h3);
>> eglQuerySurface(EGL_{WIDTH,HEIGHT});
>> glClear();
>> eglQuerySurface(EGL_{WIDTH,HEIGHT});
>> eglSwapBuffers();
>>
>> 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:
> 
> - 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.
> 
> - 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.
> 
> - 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.

This sounds correct to me and is exactly what x11 implementation does, 
during query hook we ask X server the current size of the surface to 
make sure we are in sync with what window system. Mismatch is what the 
resize tests are trying to catch.


> 
> 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)
> - (w2,h2): like the previous call, the native window was resized to (w3,h3), but
>   eglSwapBuffers() was not called yet.
> - (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.
> 
> If NVidia is resizing the surface at the time the native window is resized,
> without waiting for eglSwapBuffers, then the returned values should be (w1,h1),
> (w2,h2), (w3,h3) and (w3,h3), respectively.
> 
> 
>>> In dEQP-EGL.functional.resize.surface_size.* , this is what the test does
>>> initially:
>>>
>>> wl_egl_window_create(w1, h1);
>>> eglCreateWindowSurface();
>>> egl.makeCurrent()
>>> wl_egl_window_get_attached_size();
>>>
>>> The failing part is the results for wl_egl_window_get_attached_size(): test
>>> assumes it will return (w1, h1) (and this is what Nvidia implementation seems to
>>> return), but Mesa returns (0, 0).
>>>
>>> If I understand correctly, Mesa returns (0, 0) because there is no buffer
>>> attached. If we call before eglSwapBuffers() (which internally calls
>>> wl_surface_attach()) and afterwards wl_egl_window_get_attached_size(), then Mesa
>>> would return (w1, h1) because now the buffer is attached to the window.
>>>
>>> Am I correct?
>>
>> Yep, that is correct.
>>
>>> Because it seems like NVidia implementation is returning (w1,h1) because at that
>>> moment it has an attached buffer. Wondering if eglMakeCurrent() is performing
>>> such attachment.
>>
>> 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.
> 
> In the example above, if we replace eglQuerySurface() by
> wl_egl_window_get_attached_size(), what would be the expected values?
> 
> 
> BR,
> 
> 	J.A.
> 
>> Cheers,
>> Daniel
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>