Commit c81271a3 authored by Martin Flöser's avatar Martin Flöser
Browse files

Fix shared memory access in BufferInterface

The BufferInterface used nested calls of wl_shm_buffer_begin_access
and allowed multiple different BufferInterfaces to be in a block
between wl_shm_buffer_begin_access and wl_shm_buffer_end_access.
But this is not allowed: nesting is only possible if it accesses
the same buffer!

This resulted in an abort when we accessed two BufferInterfaces
at the same time. The added test case aborts in the previous
version.

The fix now changes the semantic of the method. The BufferInterface
doesn't hold the QImage any more, but creates a new one every time
::data is invoked. The QImage is created with a custom cleanup
function which calls to wl_shm_buffer_end_access. It ensures that
we don't call into wl_shm_buffer_begin_access as long as there is
a valid QImage for any other BufferInterface still around and
instead returns a null QImage.

Thus a user of a BufferInterface::data should destroy the returned
QImage as soon as it's no longer needed or create a deep copy if
it needs to be kept around.
parent 6c406df7
......@@ -47,6 +47,7 @@ private Q_SLOTS:
void testDamage();
void testFrameCallback();
void testAttachBuffer();
void testMultipleSurfaces();
void testOpaque();
void testInput();
void testDestroy();
......@@ -347,6 +348,100 @@ void TestWaylandSurface::testAttachBuffer()
buffer->unref();
}
void TestWaylandSurface::testMultipleSurfaces()
{
using namespace KWayland::Client;
using namespace KWayland::Server;
// here we need a shm pool
m_display->createShm();
Registry registry;
QSignalSpy shmSpy(&registry, SIGNAL(shmAnnounced(quint32,quint32)));
registry.create(m_connection->display());
QVERIFY(registry.isValid());
registry.setup();
QVERIFY(shmSpy.wait());
ShmPool pool1;
ShmPool pool2;
pool1.setup(registry.bindShm(shmSpy.first().first().value<quint32>(), shmSpy.first().last().value<quint32>()));
pool2.setup(registry.bindShm(shmSpy.first().first().value<quint32>(), shmSpy.first().last().value<quint32>()));
QVERIFY(pool1.isValid());
QVERIFY(pool2.isValid());
// create the surfaces
QSignalSpy serverSurfaceCreated(m_compositorInterface, SIGNAL(surfaceCreated(KWayland::Server::SurfaceInterface*)));
QVERIFY(serverSurfaceCreated.isValid());
QScopedPointer<Surface> s1(m_compositor->createSurface());
QVERIFY(serverSurfaceCreated.wait());
SurfaceInterface *serverSurface1 = serverSurfaceCreated.first().first().value<KWayland::Server::SurfaceInterface*>();
QVERIFY(serverSurface1);
//second surface
QScopedPointer<Surface> s2(m_compositor->createSurface());
QVERIFY(serverSurfaceCreated.wait());
SurfaceInterface *serverSurface2 = serverSurfaceCreated.last().first().value<KWayland::Server::SurfaceInterface*>();
QVERIFY(serverSurface2);
QVERIFY(serverSurface1->surface() != serverSurface2->surface());
// create two images
QImage black(24, 24, QImage::Format_RGB32);
black.fill(Qt::black);
QImage red(24, 24, QImage::Format_ARGB32);
red.fill(QColor(255, 0, 0, 128));
auto blackBuffer = pool1.createBuffer(black);
auto redBuffer = pool2.createBuffer(red);
s1->attachBuffer(blackBuffer);
s1->damage(QRect(0, 0, 24, 24));
s1->commit(Surface::CommitFlag::None);
QSignalSpy damageSpy1(serverSurface1, SIGNAL(damaged(QRegion)));
QVERIFY(damageSpy1.isValid());
QVERIFY(damageSpy1.wait());
// now the ServerSurface should have the black image attached as a buffer
BufferInterface *buffer1 = serverSurface1->buffer();
QVERIFY(buffer1);
QImage buffer1Data = buffer1->data();
QCOMPARE(buffer1Data, black);
// accessing the same buffer is OK
QImage buffer1Data2 = buffer1->data();
QCOMPARE(buffer1Data2, buffer1Data);
buffer1Data = QImage();
QVERIFY(buffer1Data.isNull());
buffer1Data2 = QImage();
QVERIFY(buffer1Data2.isNull());
// attach a buffer for the other surface
s2->attachBuffer(redBuffer);
s2->damage(QRect(0, 0, 24, 24));
s2->commit(Surface::CommitFlag::None);
QSignalSpy damageSpy2(serverSurface2, SIGNAL(damaged(QRegion)));
QVERIFY(damageSpy2.isValid());
QVERIFY(damageSpy2.wait());
BufferInterface *buffer2 = serverSurface2->buffer();
QVERIFY(buffer2);
QImage buffer2Data = buffer2->data();
QCOMPARE(buffer2Data, red);
// while buffer2 is accessed we cannot access buffer1
buffer1Data = buffer1->data();
QVERIFY(buffer1Data.isNull());
// a deep copy can be kept around
QImage deepCopy = buffer2Data.copy();
QCOMPARE(deepCopy, red);
buffer2Data = QImage();
QVERIFY(buffer2Data.isNull());
QCOMPARE(deepCopy, red);
// now that buffer2Data is destroyed we can access buffer1 again
buffer1Data = buffer1->data();
QVERIFY(!buffer1Data.isNull());
QCOMPARE(buffer1Data, black);
}
void TestWaylandSurface::testOpaque()
{
using namespace KWayland::Client;
......
......@@ -33,24 +33,27 @@ public:
Private(BufferInterface *q, wl_resource *resource, SurfaceInterface *parent);
~Private();
QImage::Format format() const;
void createImage();
void releaseImage();
QImage createImage();
wl_resource *buffer;
wl_shm_buffer *shmBuffer;
SurfaceInterface *surface;
int refCount;
QImage image;
private:
static void destroyListenerCallback(wl_listener *listener, void *data);
static Private *cast(wl_resource *r);
static void imageBufferCleanupHandler(void *info);
static QList<Private*> s_buffers;
static Private *s_accessedBuffer;
static int s_accessCounter;
BufferInterface *q;
wl_listener listener;
};
QList<BufferInterface::Private*> BufferInterface::Private::s_buffers;
BufferInterface::Private *BufferInterface::Private::s_accessedBuffer = nullptr;
int BufferInterface::Private::s_accessCounter = 0;
BufferInterface::Private *BufferInterface::Private::cast(wl_resource *r)
{
......@@ -61,6 +64,18 @@ BufferInterface::Private *BufferInterface::Private::cast(wl_resource *r)
return *it;
}
void BufferInterface::Private::imageBufferCleanupHandler(void *info)
{
Private *p = reinterpret_cast<Private*>(info);
Q_ASSERT(p == s_accessedBuffer);
Q_ASSERT(s_accessCounter > 0);
s_accessCounter--;
if (s_accessCounter == 0) {
s_accessedBuffer = nullptr;
}
wl_shm_buffer_end_access(p->shmBuffer);
}
BufferInterface::Private::Private(BufferInterface *q, wl_resource *resource, SurfaceInterface *parent)
: buffer(resource)
, shmBuffer(wl_shm_buffer_get(resource))
......@@ -88,17 +103,6 @@ BufferInterface::BufferInterface(wl_resource *resource, SurfaceInterface *parent
BufferInterface::~BufferInterface()
{
Q_ASSERT(d->refCount == 0);
d->releaseImage();
}
void BufferInterface::Private::releaseImage()
{
if (image.isNull()) {
return;
}
// first destroy it
image = QImage();
wl_shm_buffer_end_access(shmBuffer);
}
void BufferInterface::Private::destroyListenerCallback(wl_listener *listener, void *data)
......@@ -120,7 +124,6 @@ void BufferInterface::unref()
Q_ASSERT(d->refCount > 0);
d->refCount--;
if (d->refCount == 0) {
d->releaseImage();
if (d->buffer) {
wl_buffer_send_release(d->buffer);
}
......@@ -142,27 +145,30 @@ QImage::Format BufferInterface::Private::format() const
QImage BufferInterface::data()
{
if (d->image.isNull()) {
d->createImage();
}
return d->image;
return std::move(d->createImage());
}
void BufferInterface::Private::createImage()
QImage BufferInterface::Private::createImage()
{
if (!shmBuffer) {
return;
return QImage();
}
if (s_accessedBuffer != nullptr && s_accessedBuffer != this) {
return QImage();
}
const QImage::Format imageFormat = format();
if (imageFormat == QImage::Format_Invalid) {
return;
return QImage();
}
s_accessedBuffer = this;
s_accessCounter++;
wl_shm_buffer_begin_access(shmBuffer);
image = QImage((const uchar*)wl_shm_buffer_get_data(shmBuffer),
wl_shm_buffer_get_width(shmBuffer),
wl_shm_buffer_get_height(shmBuffer),
wl_shm_buffer_get_stride(shmBuffer),
imageFormat);
return std::move(QImage((const uchar*)wl_shm_buffer_get_data(shmBuffer),
wl_shm_buffer_get_width(shmBuffer),
wl_shm_buffer_get_height(shmBuffer),
wl_shm_buffer_get_stride(shmBuffer),
imageFormat,
&imageBufferCleanupHandler, this));
}
bool BufferInterface::isReferenced() const
......
......@@ -47,6 +47,26 @@ public:
SurfaceInterface *surface() const;
wl_shm_buffer *shmBuffer();
/**
* Creates a QImage for the shared memory buffer.
*
* If the BufferInterface does not reference a shared memory buffer a null QImage is returned.
*
* The QImage shares the memory with the buffer and this constraints how the returned
* QImage can be used and when this method can be invoked.
*
* It is not safe to have two shared memory QImages for different BufferInterfaces at
* the same time. This method ensures that this does not happen and returns a null
* QImage if a different BufferInterface's data is still mapped to a QImage. Please note
* that this also applies to all implicitly data shared copies.
*
* In case it is needed to keep a copy, a deep copy has to be performed by using QImage::copy.
*
* As the underlying shared memory buffer is owned by a different client it is not safe to
* write to the returned QImage. The image is a read-only buffer. If there is need to modify
* the image, perform a deep copy.
*
**/
QImage data();
Q_SIGNALS:
......
Supports Markdown
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