Commit 8e851ee7 authored by Vlad Zahorodnii's avatar Vlad Zahorodnii
Browse files

Emit the committed() signal after the state is applied

Currently, the committed signal is emitted after the client has called
wl_surface.commit. However, this breaks with synchronized subsurfaces.

Notably, Firefox splits a web page in a bunch of smaller layers, which
can be backed by wl_subsurface objects.

All the subsurfaces are in the sync mode. If a layer needs to be
repainted, Firefox will commit the corresponding subsurface with a frame
callback.

Since the committed signal is emitted when the wl_surface.commit request
is invoked, kwin will schedule a new frame immediately. Meaning, that it
is quite likely that firefox will have old contents.

The right thing to do would be to schedule a frame when all the ancestors
of the layer subsurface have been committed.

This change re-jitters the commit logic so the committed signal is
emitted when a new state is applied to the surface. It also slightly
cleans up how SubSurfaceInterface::parentCommit() is called.

It will be nice to cleanup the commit logic further by calling the
surface role's commit hook unconditionally, i.e. not check whether it's
a subsurface. But doing so may result in infinite recursions. How to
clean up that is still TBD.

(cherry picked from commit 37890904)
parent 9602863c
Pipeline #70621 passed with stage
in 8 minutes
......@@ -174,34 +174,17 @@ void SubSurfaceInterfacePrivate::subsurface_set_desync(Resource *)
}
mode = SubSurfaceInterface::Mode::Desynchronized;
if (!q->isSynchronized()) {
synchronizedCommit();
auto surfacePrivate = SurfaceInterfacePrivate::get(surface);
surfacePrivate->commitFromCache();
}
Q_EMIT q->modeChanged(SubSurfaceInterface::Mode::Desynchronized);
}
void SubSurfaceInterfacePrivate::commit()
{
SurfaceInterfacePrivate *surfacePrivate = SurfaceInterfacePrivate::get(surface);
if (q->isSynchronized()) {
commitToCache();
} else {
if (hasCacheState) {
commitToCache();
commitFromCache();
} else {
surfacePrivate->swapStates(&surfacePrivate->pending, &surfacePrivate->current, true);
}
const QList<SubSurfaceInterface *> children = surfacePrivate->current.children;
for (SubSurfaceInterface *subsurface : children) {
SubSurfaceInterfacePrivate *subsurfacePrivate = SubSurfaceInterfacePrivate::get(subsurface);
subsurfacePrivate->parentCommit();
}
}
}
void SubSurfaceInterfacePrivate::parentCommit(bool synchronized)
void SubSurfaceInterfacePrivate::parentCommit()
{
if (hasPendingPosition) {
hasPendingPosition = false;
......@@ -209,37 +192,12 @@ void SubSurfaceInterfacePrivate::parentCommit(bool synchronized)
Q_EMIT q->positionChanged(position);
}
if (synchronized || mode == SubSurfaceInterface::Mode::Synchronized) {
synchronizedCommit();
}
}
void SubSurfaceInterfacePrivate::synchronizedCommit()
{
const SurfaceInterfacePrivate *surfacePrivate = SurfaceInterfacePrivate::get(surface);
commitFromCache();
const QList<SubSurfaceInterface *> children = surfacePrivate->current.children;
for (SubSurfaceInterface *subsurface : children) {
SubSurfaceInterfacePrivate *subsurfacePrivate = SubSurfaceInterfacePrivate::get(subsurface);
subsurfacePrivate->parentCommit(true);
if (mode == SubSurfaceInterface::Mode::Synchronized) {
auto surfacePrivate = SurfaceInterfacePrivate::get(surface);
surfacePrivate->commitFromCache();
}
}
void SubSurfaceInterfacePrivate::commitToCache()
{
SurfaceInterfacePrivate *surfacePrivate = SurfaceInterfacePrivate::get(surface);
surfacePrivate->swapStates(&surfacePrivate->pending, &surfacePrivate->cached, false);
hasCacheState = true;
}
void SubSurfaceInterfacePrivate::commitFromCache()
{
SurfaceInterfacePrivate *surfacePrivate = SurfaceInterfacePrivate::get(surface);
surfacePrivate->swapStates(&surfacePrivate->cached, &surfacePrivate->current, true);
hasCacheState = false;
}
SubSurfaceInterface::SubSurfaceInterface(SurfaceInterface *surface, SurfaceInterface *parent,
wl_resource *resource)
: d(new SubSurfaceInterfacePrivate(this, surface, parent, resource))
......
......@@ -41,12 +41,7 @@ public:
SurfaceInterface *parent, ::wl_resource *resource);
void commit() override;
void parentCommit(bool synchronized = false);
void synchronizedCommit();
void commitToCache();
void commitFromCache();
void parentCommit();
SubSurfaceInterface *q;
QPoint position = QPoint(0, 0);
......@@ -54,7 +49,6 @@ public:
SubSurfaceInterface::Mode mode = SubSurfaceInterface::Mode::Synchronized;
QPointer<SurfaceInterface> surface;
QPointer<SurfaceInterface> parent;
bool hasCacheState = false;
bool hasPendingPosition = false;
protected:
......
......@@ -662,26 +662,51 @@ void SurfaceInterfacePrivate::swapStates(State *source, State *target, bool emit
if (childrenChanged) {
Q_EMIT q->childSubSurfacesChanged();
}
// The position of a sub-surface is applied when its parent is committed.
const QList<SubSurfaceInterface *> children = current.children;
for (SubSurfaceInterface *subsurface : children) {
auto subsurfacePrivate = SubSurfaceInterfacePrivate::get(subsurface);
subsurfacePrivate->parentCommit();
}
if (role) {
role->commit();
}
Q_EMIT q->committed();
}
void SurfaceInterfacePrivate::commit()
{
if (!subSurface) {
if (subSurface) {
commitSubSurface();
} else {
swapStates(&pending, &current, true);
}
}
// The position of a sub-surface is applied when its parent is committed.
const QList<SubSurfaceInterface *> children = current.children;
for (SubSurfaceInterface *subsurface : children) {
auto subsurfacePrivate = SubSurfaceInterfacePrivate::get(subsurface);
subsurfacePrivate->parentCommit();
void SurfaceInterfacePrivate::commitSubSurface()
{
if (subSurface->isSynchronized()) {
commitToCache();
} else {
if (hasCacheState) {
commitToCache();
commitFromCache();
} else {
swapStates(&pending, &current, true);
}
}
}
if (role) {
role->commit();
}
void SurfaceInterfacePrivate::commitToCache()
{
swapStates(&pending, &cached, false);
hasCacheState = true;
}
Q_EMIT q->committed();
void SurfaceInterfacePrivate::commitFromCache()
{
swapStates(&cached, &current, true);
hasCacheState = false;
}
QRegion SurfaceInterface::damage() const
......
......@@ -87,7 +87,11 @@ public:
void installPointerConstraint(ConfinedPointerV1Interface *confinement);
void installIdleInhibitor(IdleInhibitorV1Interface *inhibitor);
void commitToCache();
void commitFromCache();
void commit();
void commitSubSurface();
QMatrix4x4 buildSurfaceToBufferMatrix(const State *state);
void swapStates(State *source, State *target, bool emitChanged);
......@@ -103,6 +107,7 @@ public:
QMatrix4x4 bufferToSurfaceMatrix;
QSize bufferSize;
QRegion inputRegion;
bool hasCacheState = false;
// workaround for https://bugreports.qt.io/browse/QTBUG-52192
// A subsurface needs to be considered mapped even if it doesn't have a buffer attached
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment