From 666947c94da7a869e3077168cfaf965ccc947244 Mon Sep 17 00:00:00 2001 From: oldosfan Date: Sat, 1 Oct 2022 02:50:58 +0000 Subject: [PATCH] Minor fixes to EGL renderer and toplevel resizing * compositor.h (struct _RenderFuncs): Accept damage in finish_render. * egl.c (EglInitFuncs): Load eglSwapBuffersWithDamageEXT. (FinishRender): Accept damage and swap only damaged area if possible. (UpdateBuffer): Always update textures. * explicit_synchronization.c (XLSyncRelease): Fix comment. * renderer.c (RenderFinishRender): Accept damage argument. * subcompositor.c (ViewWidth, ViewHeight): Fix computation of destination width and height when there is a content scale; those values have already been adjusted into window coordinates. (SubcompositorUpdate, SubcompositorExpose): Pass damage to RenderFinishRender. * surface.c (ApplyDamage): Fix some aspects of damage calculation. * xdg_surface.c (IsRoleMapped): Fix crash when destroying client with subsurfaces. (Commit, NoteBounds, ResizeForMap, XLXdgRoleSendConfigure) (XLXdgRoleSetBoundsSize, XLXdgRoleResizeForMap): Really fix window state reverting to maximized during unmaximization. This had two causes: the first was that the window geometry would be set even before ack_configure, which was fixed by moving setting the window geometry into NoteBounds, and the second was that NoteBounds would sometimes resize the window back to its old dimensions if a commit (or subsurface update) happened between SetBoundsSize and the configure event arriving, also confusing the window manager. * xdg_toplevel.c (NoteConfigureTime): Fix coding style. (HandleConfigureEvent): Call SetBoundsSize before posting the configure event. --- compositor.h | 7 ++- egl.c | 51 +++++++++++++---- explicit_synchronization.c | 6 +- renderer.c | 4 +- subcompositor.c | 15 +++-- surface.c | 43 ++++++++++++--- xdg_surface.c | 110 +++++++++++++++++++++++++++++++++---- xdg_toplevel.c | 13 ++--- 8 files changed, 197 insertions(+), 52 deletions(-) diff --git a/compositor.h b/compositor.h index 6b083bf..7a7a79a 100644 --- a/compositor.h +++ b/compositor.h @@ -287,8 +287,9 @@ struct _RenderFuncs void (*composite) (RenderBuffer, RenderTarget, Operation, int, int, int, int, int, int, DrawParams *); - /* Finish rendering, and swap changes to display. May be NULL. */ - void (*finish_render) (RenderTarget); + /* Finish rendering, and swap changes in given damage to display. + May be NULL. */ + void (*finish_render) (RenderTarget, pixman_region32_t *); /* Return the age of the target. Value is a number not less than -1, describing the "age" of the contents of the target. @@ -414,7 +415,7 @@ extern void RenderFillBoxesWithTransparency (RenderTarget, pixman_box32_t *, extern void RenderClearRectangle (RenderTarget, int, int, int, int); extern void RenderComposite (RenderBuffer, RenderTarget, Operation, int, int, int, int, int, int, DrawParams *); -extern void RenderFinishRender (RenderTarget); +extern void RenderFinishRender (RenderTarget, pixman_region32_t *); extern int RenderTargetAge (RenderTarget); extern RenderFence RenderImportFdFence (int, Bool *); extern void RenderWaitFence (RenderFence); diff --git a/egl.c b/egl.c index 6cb4f00..7f8fc09 100644 --- a/egl.c +++ b/egl.c @@ -334,6 +334,7 @@ static PFNEGLCLIENTWAITSYNCKHRPROC IClientWaitSync; static PFNEGLGETSYNCATTRIBKHRPROC IGetSyncAttrib; static PFNEGLWAITSYNCKHRPROC IWaitSync; static PFNEGLDUPNATIVEFENCEFDANDROIDPROC IDupNativeFenceFD; +static PFNEGLSWAPBUFFERSWITHDAMAGEEXTPROC ISwapBuffersWithDamage; /* The EGL display handle. */ static EGLDisplay egl_display; @@ -527,6 +528,8 @@ EglInitFuncs (void) LoadProc (GetSyncAttrib, "KHR", "EGL_KHR_fence_sync"); LoadProc (WaitSync, "KHR", "EGL_KHR_wait_sync"); LoadProc (DupNativeFenceFD, "ANDROID", "EGL_ANDROID_native_fence_sync"); + LoadProc (SwapBuffersWithDamage, "EXT", + "EGL_EXT_swap_buffers_with_damage"); } static void @@ -1441,17 +1444,39 @@ Composite (RenderBuffer buffer, RenderTarget target, } static void -FinishRender (RenderTarget target) +FinishRender (RenderTarget target, pixman_region32_t *damage) { EglTarget *egl_target; + EGLint *rects; + int nboxes, i; + pixman_box32_t *boxes; egl_target = target.pointer; if (egl_target->flags & IsPixmap) glFinish (); - else + else if (!ISwapBuffersWithDamage || !damage) /* This should also do glFinish. */ eglSwapBuffers (egl_display, egl_target->surface); + else + { + /* Do a swap taking the buffer damage into account. First, + convert the damage into cartesian coordinates. */ + boxes = pixman_region32_rectangles (damage, &nboxes); + rects = alloca (nboxes * 4 * sizeof *damage); + + for (i = 0; i < nboxes; ++i) + { + rects[i * 4 + 0] = boxes[i].x1; + rects[i * 4 + 1] = egl_target->height - boxes[i].y2; + rects[i * 4 + 2] = boxes[i].x2 - boxes[i].x1; + rects[i * 4 + 3] = boxes[i].y2 - boxes[i].y1; + } + + /* Next, swap buffers with the damage. */ + ISwapBuffersWithDamage (egl_display, egl_target->surface, + rects, nboxes); + } } static int @@ -2324,15 +2349,16 @@ UpdateBuffer (RenderBuffer buffer, pixman_region32_t *damage, upload the contents. */ EnsureTexture (egl_buffer); else if (!damage) - { - /* Upload all the contents to the buffer's texture if the buffer - type requires manual updates. Buffers backed by EGLImages do - not need updates, since updates to the EGLImage are - automatically reflected in the texture. */ + /* Upload all the contents to the buffer's texture if the buffer + type requires manual updates. Buffers backed by EGLImages do + not appear to need updates, since updates to the EGLImage are + automatically reflected in the texture. - if (egl_buffer->u.type == ShmBuffer) - UpdateTexture (egl_buffer); - } + However, someone on #dri says calling + glEGLImageTargetTexture2DOES is still required and not doing so + may cause certain drivers to stop working in the future. So + still do it for buffers backed by EGLImages. */ + UpdateTexture (egl_buffer); else if (pixman_region32_not_empty (damage)) { switch (egl_buffer->u.type) @@ -2344,8 +2370,9 @@ UpdateBuffer (RenderBuffer buffer, pixman_region32_t *damage, params); break; - default: - /* These buffers need no updates. */ + case DmaBufBuffer: + /* See comment in !damage branch. */ + UpdateTexture (egl_buffer); break; } } diff --git a/explicit_synchronization.c b/explicit_synchronization.c index bc1d5b6..f800838 100644 --- a/explicit_synchronization.c +++ b/explicit_synchronization.c @@ -404,10 +404,10 @@ XLSyncRelease (SyncRelease *release) Bool error; int fd; - /* Optimization ideas. Every time a buffer is attached with a + /* Optimization ideas. Every time an shm buffer is attached with a release object, create new finish_fence for the buffer, that is - signalled whenever the contents are drawn. Then, just use that - fence here instead. */ + signalled whenever the contents are uploaded. Then, just use + that fence here instead. */ error = False; fd = RenderGetFinishFence (&error); diff --git a/renderer.c b/renderer.c index 7e466bf..00c7dc4 100644 --- a/renderer.c +++ b/renderer.c @@ -139,10 +139,10 @@ RenderComposite (RenderBuffer source, RenderTarget target, Operation op, } void -RenderFinishRender (RenderTarget target) +RenderFinishRender (RenderTarget target, pixman_region32_t *damage) { if (render_funcs.finish_render) - render_funcs.finish_render (target); + render_funcs.finish_render (target, damage); } int diff --git a/subcompositor.c b/subcompositor.c index 94e81f5..e457d1d 100644 --- a/subcompositor.c +++ b/subcompositor.c @@ -1634,8 +1634,7 @@ ViewWidth (View *view) view->dest_height can be fractional values. When that happens, we simply use the ceiling and rely on the renderer to DTRT with scaling. */ - return ceil (view->dest_width - * GetContentScale (view->scale)); + return ceil (view->dest_width); width = XLBufferWidth (view->buffer); @@ -1658,8 +1657,7 @@ ViewHeight (View *view) view->dest_height can be fractional values. When that happens, we simply use the ceiling and rely on the renderer to DTRT with scaling. */ - return ceil (view->dest_height - * GetContentScale (view->scale)); + return ceil (view->dest_height); height = XLBufferHeight (view->buffer); @@ -2504,7 +2502,12 @@ SubcompositorUpdate (Subcompositor *subcompositor) while (list != subcompositor->inferiors); /* Swap changes to display. */ - RenderFinishRender (subcompositor->target); + + if (IsGarbaged (subcompositor) || age < 0 || age >= 3) + RenderFinishRender (subcompositor->target, NULL); + else + /* Swap changes to display based on the update region. */ + RenderFinishRender (subcompositor->target, &update_region); complete: @@ -2699,7 +2702,7 @@ SubcompositorExpose (Subcompositor *subcompositor, XEvent *event) while (list != subcompositor->inferiors); /* Swap changes to display. */ - RenderFinishRender (subcompositor->target); + RenderFinishRender (subcompositor->target, NULL); } void diff --git a/surface.c b/surface.c index b4800b0..502c609 100644 --- a/surface.c +++ b/surface.c @@ -836,12 +836,20 @@ ApplyDamage (Surface *surface) pixman_region32_t temp; int scale; float x_factor, y_factor; + double src_width, src_height; + double dest_width, dest_height; scale = GetEffectiveScale (surface->current_state.buffer_scale); + /* If no surface was attached, just return. */ + if (!surface->current_state.buffer) + return; + /* N.B. that this must come after the scale is applied. */ - if (scale || surface->current_state.src_x != -1 + if (scale || (surface->current_state.src_x != -1 + && surface->current_state.src_x != 0.0 + && surface->current_state.src_y != 0.0) || surface->current_state.dest_width != -1) { pixman_region32_init (&temp); @@ -855,15 +863,33 @@ ApplyDamage (Surface *surface) /* If a viewport dest size is set, add that to the scale as well. */ - if (surface->current_state.src_width != -1) + if (surface->current_state.dest_width != -1) { - x_factor += (float) (surface->current_state.src_width - / surface->current_state.dest_width); - y_factor += (float) (surface->current_state.src_height - / surface->current_state.dest_height); + if (surface->current_state.src_width != -1) + { + src_width = surface->current_state.src_width; + src_height = surface->current_state.src_height; + } + else + { + src_width = XLBufferWidth (surface->current_state.buffer); + src_height = XLBufferHeight (surface->current_state.buffer); + + /* Now scale these buffer dimensions down by the buffer + scale, so they can be turned into surface + coordinates. */ + src_width /= surface->current_state.buffer_scale; + src_height /= surface->current_state.buffer_scale; + } + + dest_width = surface->current_state.dest_width; + dest_height = surface->current_state.dest_height; + + x_factor *= (float) (src_width / dest_width); + y_factor *= (float) (src_height / dest_height); } - if (x_factor != 1.0f && y_factor != 1.0f) + if (x_factor != 1.0f || y_factor != 1.0f) XLScaleRegion (&temp, &surface->current_state.damage, x_factor, y_factor); @@ -879,8 +905,7 @@ ApplyDamage (Surface *surface) pixman_region32_fini (&temp); } else - ViewDamage (surface->view, - &surface->current_state.damage); + ViewDamage (surface->view, &surface->current_state.damage); } static void diff --git a/xdg_surface.c b/xdg_surface.c index 27ea99d..82d5378 100644 --- a/xdg_surface.c +++ b/xdg_surface.c @@ -38,6 +38,8 @@ enum StateWaitingForAckCommit = (1 << 4), StateLateFrameAcked = (1 << 5), StateMaybeConfigure = (1 << 6), + StateDirtyFrameExtents = (1 << 7), + StateTemporaryBounds = (1 << 8), }; typedef struct _XdgRole XdgRole; @@ -631,6 +633,11 @@ Unfreeze (XdgRole *role) static Bool IsRoleMapped (XdgRole *role) { + if (!role->impl) + /* This can happen when destroying subsurfaces as part of client + cleanup. */ + return False; + return role->impl->funcs.is_window_mapped (&role->role, role->impl); } @@ -655,23 +662,36 @@ Commit (Surface *surface, Role *role) = xdg_role->pending_state.window_geometry_width; xdg_role->current_state.window_geometry_height = xdg_role->pending_state.window_geometry_height; + +#ifdef DEBUG_GEOMETRY_CALCULATION + fprintf (stderr, "Client set window geometry to: [%d %d %d %d]\n", + xdg_role->current_state.window_geometry_x, + xdg_role->current_state.window_geometry_y, + xdg_role->current_state.window_geometry_width, + xdg_role->current_state.window_geometry_height); +#endif + + /* Now, clear the "pending window geometry" flag. */ + xdg_role->state &= ~StatePendingWindowGeometry; + + /* Next, set the "dirty frame extents" flag; this is then used + to update the window geometry the next time the window is + resized. */ + xdg_role->state |= StateDirtyFrameExtents; } xdg_role->impl->funcs.commit (role, surface, xdg_role->impl); - if (xdg_role->state & StatePendingWindowGeometry) - { - if (xdg_role->impl->funcs.handle_geometry_change) - xdg_role->impl->funcs.handle_geometry_change (role, - xdg_role->impl); - - xdg_role->state &= ~StatePendingWindowGeometry; - } - /* This flag means no commit has happened after an ack_configure. */ - xdg_role->state &= ~StateWaitingForAckCommit; + if (!(xdg_role->state & StateWaitingForAckConfigure)) + { +#ifdef DEBUG_GEOMETRY_CALCULATION + fprintf (stderr, "Client aknowledged commit\n"); +#endif + xdg_role->state &= ~StateWaitingForAckCommit; + } /* If the window is unmapped, skip all of this code! Once the window is mapped again, the compositor will send _NET_FRAME_DRAWN @@ -1102,12 +1122,19 @@ NoteBounds (void *data, int min_x, int min_y, still in effect. */ return; + if (role->state & StateTemporaryBounds) + return; + /* Avoid resizing the window should its actual size not have changed. */ bounds_width = max_x - min_x + 1; bounds_height = max_y - min_y + 1; +#ifdef DEBUG_GEOMETRY_CALCULATION + fprintf (stderr, "Noticed bounds: %d %d\n", bounds_width, bounds_height); +#endif + if (role->bounds_width != bounds_width || role->bounds_height != bounds_height) { @@ -1121,6 +1148,18 @@ NoteBounds (void *data, int min_x, int min_y, bounds_width, bounds_height); + if (role->state & StateDirtyFrameExtents) + { + /* Only handle window geometry changes once a commit happens + and the window is really resized. */ + + if (role->impl->funcs.handle_geometry_change) + role->impl->funcs.handle_geometry_change (&role->role, + role->impl); + + role->state &= ~StateDirtyFrameExtents; + } + XResizeWindow (compositor.display, role->window, bounds_width, bounds_height); run_reconstrain_callbacks = True; @@ -1183,7 +1222,38 @@ ResizeForMap (XdgRole *role) SubcompositorBounds (role->subcompositor, &min_x, &min_y, &max_x, &max_y); + + /* At this point, we are probably still waiting for ack_commit; as a + result, NoteBounds will not really resize the window. */ NoteBounds (role, min_x, min_y, max_x, max_y); + +#ifdef DEBUG_GEOMETRY_CALCULATION + fprintf (stderr, "ResizeForMap: %d %d\n", + max_x - min_x + 1, max_y - min_y + 1); +#endif + + if (role->state & StateDirtyFrameExtents) + { + /* Only handle window geometry changes once a commit + happens. */ + + if (role->impl->funcs.handle_geometry_change) + role->impl->funcs.handle_geometry_change (&role->role, + role->impl); + + role->state &= ~StateDirtyFrameExtents; + } + + /* Resize the window pre-map. This should generate a + ConfigureNotify event once the resize completes. */ + XResizeWindow (compositor.display, role->window, + max_x - min_x + 1, max_y - min_y + 1); + + if (role->impl->funcs.note_window_resized) + role->impl->funcs.note_window_resized (&role->role, + role->impl, + max_x - min_x + 1, + max_y - min_y + 1); } static void @@ -1470,6 +1540,9 @@ XLXdgRoleSendConfigure (Role *role, uint32_t serial) xdg_role->state |= StateWaitingForAckConfigure; xdg_role->state |= StateWaitingForAckCommit; + /* See the comment under XLXdgRoleSetBoundsSize. */ + xdg_role->state &= ~StateTemporaryBounds; + /* We know know that the ConfigureNotify event following any _NET_WM_SYNC_REQUEST event was accepted, so clear the maybe configure flag. */ @@ -1573,6 +1646,17 @@ XLXdgRoleSetBoundsSize (Role *role, int bounds_width, int bounds_height) xdg_role = XdgRoleFromRole (role); xdg_role->bounds_width = bounds_width; xdg_role->bounds_height = bounds_height; + + /* Now, a temporary bounds_width and bounds_height has been + recorded. This means that if a configure event has not yet been + delivered, then any subsequent SubcompositorUpdate will cause + NoteBounds to resize back to the old width and height, confusing + the window manager and possibly causing it to maximize us. + + Set a flag that tells NoteBounds to abstain from resizing the + window. This flag is then cleared once a configure event is + delivered, or the next time the role is mapped. */ + xdg_role->state |= StateTemporaryBounds; } void @@ -1690,6 +1774,12 @@ XLXdgRoleResizeForMap (Role *role) XdgRole *xdg_role; xdg_role = XdgRoleFromRole (role); + + /* Clear the StateTemporaryBounds flag; it should not persist after + mapping, as a configure event is no longer guaranteed to be sent + if the toplevel is unmapped immediately after + XLXdgRoleSetBoundsSize. */ + xdg_role->state &= ~StateTemporaryBounds; ResizeForMap (xdg_role); } diff --git a/xdg_toplevel.c b/xdg_toplevel.c index 2582871..90bce59 100644 --- a/xdg_toplevel.c +++ b/xdg_toplevel.c @@ -432,7 +432,6 @@ NoteConfigureTime (Timer *timer, void *data, struct timespec time) toplevel = data; - /* If only the window state changed, call SendStates. */ if (!(toplevel->state & StatePendingConfigureSize)) SendStates (toplevel); @@ -1330,6 +1329,12 @@ HandleConfigureEvent (XdgToplevel *toplevel, XEvent *event) event->xconfigure.height)) WriteStates (toplevel); + /* Also set the bounds width and height to avoid resizing the + window. */ + XLXdgRoleSetBoundsSize (toplevel->role, + toplevel->width, + toplevel->height); + if (!MaybePostDelayedConfigure (toplevel, StatePendingConfigureSize)) { XLXdgRoleCalcNewWindowSize (toplevel->role, @@ -1346,12 +1351,6 @@ HandleConfigureEvent (XdgToplevel *toplevel, XEvent *event) toplevel->configure_width = toplevel->width; toplevel->configure_height = toplevel->height; - /* Also set the bounds width and height to avoid resizing - the window. */ - XLXdgRoleSetBoundsSize (toplevel->role, - toplevel->width, - toplevel->height); - RecordStateSize (toplevel); return True;