libgui: disallow NULL Fence pointers
This change eliminates the uses of a NULL sp<Fence> indicating that no waiting
is required. Instead we use a non-NULL but invalid Fence object for which the
wait methods will return immediately.
Bug: 7892871
Change-Id: I5360aebe3090422ef6920d56c99fc4eedc642e48
diff --git a/libs/gui/BufferItemConsumer.cpp b/libs/gui/BufferItemConsumer.cpp
index 5079883..885b4e4 100644
--- a/libs/gui/BufferItemConsumer.cpp
+++ b/libs/gui/BufferItemConsumer.cpp
@@ -62,7 +62,7 @@
return err;
}
- if (waitForFence && item->mFence.get()) {
+ if (waitForFence) {
err = item->mFence->waitForever(1000, "BufferItemConsumer::acquireBuffer");
if (err != OK) {
BI_LOGE("Failed to wait for fence of acquired buffer: %s (%d)",
diff --git a/libs/gui/BufferQueue.cpp b/libs/gui/BufferQueue.cpp
index 4be655b..d93c067 100644
--- a/libs/gui/BufferQueue.cpp
+++ b/libs/gui/BufferQueue.cpp
@@ -372,8 +372,6 @@
h = mDefaultHeight;
}
- // buffer is now in DEQUEUED (but can also be current at the same time,
- // if we're in synchronous mode)
mSlots[buf].mBufferState = BufferSlot::DEQUEUED;
const sp<GraphicBuffer>& buffer(mSlots[buf].mGraphicBuffer);
@@ -387,7 +385,7 @@
mSlots[buf].mGraphicBuffer = NULL;
mSlots[buf].mRequestBufferCalled = false;
mSlots[buf].mEglFence = EGL_NO_SYNC_KHR;
- mSlots[buf].mFence.clear();
+ mSlots[buf].mFence = Fence::NO_FENCE;
mSlots[buf].mEglDisplay = EGL_NO_DISPLAY;
returnFlags |= IGraphicBufferProducer::BUFFER_NEEDS_REALLOCATION;
@@ -397,7 +395,7 @@
eglFence = mSlots[buf].mEglFence;
outFence = mSlots[buf].mFence;
mSlots[buf].mEglFence = EGL_NO_SYNC_KHR;
- mSlots[buf].mFence.clear();
+ mSlots[buf].mFence = Fence::NO_FENCE;
} // end lock scope
if (returnFlags & IGraphicBufferProducer::BUFFER_NEEDS_REALLOCATION) {
@@ -423,7 +421,6 @@
}
}
-
if (eglFence != EGL_NO_SYNC_KHR) {
EGLint result = eglClientWaitSyncKHR(dpy, eglFence, 0, 1000000000);
// If something goes wrong, log the error, but return the buffer without
@@ -488,6 +485,11 @@
input.deflate(×tamp, &crop, &scalingMode, &transform, &fence);
+ if (fence == NULL) {
+ ST_LOGE("queueBuffer: fence is NULL");
+ return BAD_VALUE;
+ }
+
ST_LOGV("queueBuffer: slot=%d time=%#llx crop=[%d,%d,%d,%d] tr=%#x "
"scale=%s",
buf, timestamp, crop.left, crop.top, crop.right, crop.bottom,
@@ -607,6 +609,9 @@
ST_LOGE("cancelBuffer: slot %d is not owned by the client (state=%d)",
buf, mSlots[buf].mBufferState);
return;
+ } else if (fence == NULL) {
+ ST_LOGE("cancelBuffer: fence is NULL");
+ return;
}
mSlots[buf].mBufferState = BufferSlot::FREE;
mSlots[buf].mFrameNumber = 0;
@@ -785,7 +790,7 @@
eglDestroySyncKHR(mSlots[slot].mEglDisplay, mSlots[slot].mEglFence);
mSlots[slot].mEglFence = EGL_NO_SYNC_KHR;
}
- mSlots[slot].mFence.clear();
+ mSlots[slot].mFence = Fence::NO_FENCE;
}
void BufferQueue::freeAllBuffersLocked() {
@@ -843,7 +848,7 @@
mSlots[buf].mAcquireCalled = true;
mSlots[buf].mNeedsCleanupOnRelease = false;
mSlots[buf].mBufferState = BufferSlot::ACQUIRED;
- mSlots[buf].mFence.clear();
+ mSlots[buf].mFence = Fence::NO_FENCE;
mQueue.erase(front);
mDequeueCondition.broadcast();
@@ -863,8 +868,8 @@
Mutex::Autolock _l(mMutex);
- if (buf == INVALID_BUFFER_SLOT) {
- return -EINVAL;
+ if (buf == INVALID_BUFFER_SLOT || fence == NULL) {
+ return BAD_VALUE;
}
mSlots[buf].mEglDisplay = display;
diff --git a/libs/gui/ConsumerBase.cpp b/libs/gui/ConsumerBase.cpp
index 8f391aa..fb6ba7d 100644
--- a/libs/gui/ConsumerBase.cpp
+++ b/libs/gui/ConsumerBase.cpp
@@ -84,7 +84,7 @@
void ConsumerBase::freeBufferLocked(int slotIndex) {
CB_LOGV("freeBufferLocked: slotIndex=%d", slotIndex);
mSlots[slotIndex].mGraphicBuffer = 0;
- mSlots[slotIndex].mFence = 0;
+ mSlots[slotIndex].mFence = Fence::NO_FENCE;
}
// Used for refactoring, should not be in final interface
@@ -228,7 +228,7 @@
freeBufferLocked(slot);
}
- mSlots[slot].mFence.clear();
+ mSlots[slot].mFence = Fence::NO_FENCE;
return err;
}
diff --git a/libs/gui/GLConsumer.cpp b/libs/gui/GLConsumer.cpp
index ba23f9e..09831fb 100644
--- a/libs/gui/GLConsumer.cpp
+++ b/libs/gui/GLConsumer.cpp
@@ -117,6 +117,7 @@
GLenum texTarget, bool useFenceSync, const sp<BufferQueue> &bufferQueue) :
ConsumerBase(bufferQueue == 0 ? new BufferQueue(allowSynchronousMode) : bufferQueue),
mCurrentTransform(0),
+ mCurrentFence(Fence::NO_FENCE),
mCurrentTimestamp(0),
mFilteringEnabled(true),
mTexName(tex),
@@ -823,7 +824,7 @@
return INVALID_OPERATION;
}
- if (mCurrentFence != NULL) {
+ if (mCurrentFence->isValid()) {
if (useWaitSync) {
// Create an EGLSyncKHR from the current fence.
int fenceFd = mCurrentFence->dup();
diff --git a/libs/gui/IGraphicBufferProducer.cpp b/libs/gui/IGraphicBufferProducer.cpp
index c949817..54860d7 100644
--- a/libs/gui/IGraphicBufferProducer.cpp
+++ b/libs/gui/IGraphicBufferProducer.cpp
@@ -94,9 +94,11 @@
return result;
}
*buf = reply.readInt32();
- fence.clear();
- bool hasFence = reply.readInt32();
- if (hasFence) {
+ bool fenceWasWritten = reply.readInt32();
+ if (fenceWasWritten) {
+ // If the fence was written by the callee, then overwrite the
+ // caller's fence here. If it wasn't written then don't touch the
+ // caller's fence.
fence = new Fence();
reply.read(*fence.get());
}
@@ -121,13 +123,9 @@
virtual void cancelBuffer(int buf, sp<Fence> fence) {
Parcel data, reply;
- bool hasFence = fence.get() && fence->isValid();
data.writeInterfaceToken(IGraphicBufferProducer::getInterfaceDescriptor());
data.writeInt32(buf);
- data.writeInt32(hasFence);
- if (hasFence) {
- data.write(*fence.get());
- }
+ data.write(*fence.get());
remote()->transact(CANCEL_BUFFER, data, &reply);
}
@@ -218,10 +216,9 @@
int buf;
sp<Fence> fence;
int result = dequeueBuffer(&buf, fence, w, h, format, usage);
- bool hasFence = fence.get() && fence->isValid();
reply->writeInt32(buf);
- reply->writeInt32(hasFence);
- if (hasFence) {
+ reply->writeInt32(fence != NULL);
+ if (fence != NULL) {
reply->write(*fence.get());
}
reply->writeInt32(result);
@@ -241,12 +238,8 @@
case CANCEL_BUFFER: {
CHECK_INTERFACE(IGraphicBufferProducer, data, reply);
int buf = data.readInt32();
- sp<Fence> fence;
- bool hasFence = data.readInt32();
- if (hasFence) {
- fence = new Fence();
- data.read(*fence.get());
- }
+ sp<Fence> fence = new Fence();
+ data.read(*fence.get());
cancelBuffer(buf, fence);
return NO_ERROR;
} break;
@@ -289,10 +282,6 @@
// ----------------------------------------------------------------------------
-static bool isValid(const sp<Fence>& fence) {
- return fence.get() && fence->isValid();
-}
-
IGraphicBufferProducer::QueueBufferInput::QueueBufferInput(const Parcel& parcel) {
parcel.read(*this);
}
@@ -303,29 +292,24 @@
+ sizeof(crop)
+ sizeof(scalingMode)
+ sizeof(transform)
- + sizeof(bool)
- + (isValid(fence) ? fence->getFlattenedSize() : 0);
+ + fence->getFlattenedSize();
}
size_t IGraphicBufferProducer::QueueBufferInput::getFdCount() const
{
- return isValid(fence) ? fence->getFdCount() : 0;
+ return fence->getFdCount();
}
status_t IGraphicBufferProducer::QueueBufferInput::flatten(void* buffer, size_t size,
int fds[], size_t count) const
{
status_t err = NO_ERROR;
- bool haveFence = isValid(fence);
char* p = (char*)buffer;
memcpy(p, ×tamp, sizeof(timestamp)); p += sizeof(timestamp);
memcpy(p, &crop, sizeof(crop)); p += sizeof(crop);
memcpy(p, &scalingMode, sizeof(scalingMode)); p += sizeof(scalingMode);
memcpy(p, &transform, sizeof(transform)); p += sizeof(transform);
- memcpy(p, &haveFence, sizeof(haveFence)); p += sizeof(haveFence);
- if (haveFence) {
- err = fence->flatten(p, size - (p - (char*)buffer), fds, count);
- }
+ err = fence->flatten(p, size - (p - (char*)buffer), fds, count);
return err;
}
@@ -333,17 +317,13 @@
size_t size, int fds[], size_t count)
{
status_t err = NO_ERROR;
- bool haveFence;
const char* p = (const char*)buffer;
memcpy(×tamp, p, sizeof(timestamp)); p += sizeof(timestamp);
memcpy(&crop, p, sizeof(crop)); p += sizeof(crop);
memcpy(&scalingMode, p, sizeof(scalingMode)); p += sizeof(scalingMode);
memcpy(&transform, p, sizeof(transform)); p += sizeof(transform);
- memcpy(&haveFence, p, sizeof(haveFence)); p += sizeof(haveFence);
- if (haveFence) {
- fence = new Fence();
- err = fence->unflatten(p, size - (p - (const char*)buffer), fds, count);
- }
+ fence = new Fence();
+ err = fence->unflatten(p, size - (p - (const char*)buffer), fds, count);
return err;
}
diff --git a/libs/gui/SurfaceTextureClient.cpp b/libs/gui/SurfaceTextureClient.cpp
index c015b81..5ed2e38 100644
--- a/libs/gui/SurfaceTextureClient.cpp
+++ b/libs/gui/SurfaceTextureClient.cpp
@@ -216,7 +216,7 @@
}
}
- if (fence.get()) {
+ if (fence->isValid()) {
*fenceFd = fence->dup();
if (*fenceFd == -1) {
ALOGE("dequeueBuffer: error duping fence: %d", errno);
@@ -241,7 +241,7 @@
if (i < 0) {
return i;
}
- sp<Fence> fence(fenceFd >= 0 ? new Fence(fenceFd) : NULL);
+ sp<Fence> fence(fenceFd >= 0 ? new Fence(fenceFd) : Fence::NO_FENCE);
mSurfaceTexture->cancelBuffer(i, fence);
return OK;
}
@@ -287,7 +287,7 @@
Rect crop;
mCrop.intersect(Rect(buffer->width, buffer->height), &crop);
- sp<Fence> fence(fenceFd >= 0 ? new Fence(fenceFd) : NULL);
+ sp<Fence> fence(fenceFd >= 0 ? new Fence(fenceFd) : Fence::NO_FENCE);
IGraphicBufferProducer::QueueBufferOutput output;
IGraphicBufferProducer::QueueBufferInput input(timestamp, crop, mScalingMode,
mTransform, fence);
diff --git a/libs/gui/tests/BufferQueue_test.cpp b/libs/gui/tests/BufferQueue_test.cpp
index 12cbfb0..93f8faf 100644
--- a/libs/gui/tests/BufferQueue_test.cpp
+++ b/libs/gui/tests/BufferQueue_test.cpp
@@ -71,7 +71,7 @@
sp<Fence> fence;
sp<GraphicBuffer> buf;
IGraphicBufferProducer::QueueBufferInput qbi(0, Rect(0, 0, 1, 1),
- NATIVE_WINDOW_SCALING_MODE_FREEZE, 0, fence);
+ NATIVE_WINDOW_SCALING_MODE_FREEZE, 0, Fence::NO_FENCE);
BufferQueue::BufferItem item;
for (int i = 0; i < 2; i++) {
diff --git a/libs/gui/tests/SurfaceTexture_test.cpp b/libs/gui/tests/SurfaceTexture_test.cpp
index b6020ca..9062f84 100644
--- a/libs/gui/tests/SurfaceTexture_test.cpp
+++ b/libs/gui/tests/SurfaceTexture_test.cpp
@@ -202,7 +202,6 @@
while ((err = glGetError()) != GL_NO_ERROR) {
msg += String8::format(", %#x", err);
}
- fprintf(stderr, "pixel check failure: %s\n", msg.string());
return ::testing::AssertionFailure(
::testing::Message(msg.string()));
}
@@ -228,7 +227,6 @@
msg += String8::format("a(%d isn't %d)", pixel[3], a);
}
if (!msg.isEmpty()) {
- fprintf(stderr, "pixel check failure: %s\n", msg.string());
return ::testing::AssertionFailure(
::testing::Message(msg.string()));
} else {
diff --git a/libs/ui/Fence.cpp b/libs/ui/Fence.cpp
index a01ac29..b9e0f00 100644
--- a/libs/ui/Fence.cpp
+++ b/libs/ui/Fence.cpp
@@ -29,7 +29,7 @@
namespace android {
-const sp<Fence> Fence::NO_FENCE = sp<Fence>();
+const sp<Fence> Fence::NO_FENCE = sp<Fence>(new Fence);
Fence::Fence() :
mFenceFd(-1) {
@@ -71,7 +71,19 @@
sp<Fence> Fence::merge(const String8& name, const sp<Fence>& f1,
const sp<Fence>& f2) {
ATRACE_CALL();
- int result = sync_merge(name.string(), f1->mFenceFd, f2->mFenceFd);
+ int result;
+ // Merge the two fences. In the case where one of the fences is not a
+ // valid fence (e.g. NO_FENCE) we merge the one valid fence with itself so
+ // that a new fence with the given name is created.
+ if (f1->isValid() && f2->isValid()) {
+ result = sync_merge(name.string(), f1->mFenceFd, f2->mFenceFd);
+ } else if (f1->isValid()) {
+ result = sync_merge(name.string(), f1->mFenceFd, f1->mFenceFd);
+ } else if (f2->isValid()) {
+ result = sync_merge(name.string(), f2->mFenceFd, f2->mFenceFd);
+ } else {
+ return NO_FENCE;
+ }
if (result == -1) {
status_t err = -errno;
ALOGE("merge: sync_merge(\"%s\", %d, %d) returned an error: %s (%d)",
@@ -83,9 +95,6 @@
}
int Fence::dup() const {
- if (mFenceFd == -1) {
- return -1;
- }
return ::dup(mFenceFd);
}
@@ -121,22 +130,24 @@
}
size_t Fence::getFdCount() const {
- return 1;
+ return isValid() ? 1 : 0;
}
status_t Fence::flatten(void* buffer, size_t size, int fds[],
size_t count) const {
- if (size != 0 || count != 1) {
+ if (size != getFlattenedSize() || count != getFdCount()) {
return BAD_VALUE;
}
- fds[0] = mFenceFd;
+ if (isValid()) {
+ fds[0] = mFenceFd;
+ }
return NO_ERROR;
}
status_t Fence::unflatten(void const* buffer, size_t size, int fds[],
size_t count) {
- if (size != 0 || count != 1) {
+ if (size != 0 || (count != 0 && count != 1)) {
return BAD_VALUE;
}
if (mFenceFd != -1) {
@@ -144,7 +155,10 @@
return INVALID_OPERATION;
}
- mFenceFd = fds[0];
+ if (count == 1) {
+ mFenceFd = fds[0];
+ }
+
return NO_ERROR;
}
diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
index ead158e..7a24d4c 100644
--- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp
+++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
@@ -539,7 +539,7 @@
}
int acquireFenceFd = -1;
- if (acquireFence != NULL) {
+ if (acquireFence->isValid()) {
acquireFenceFd = acquireFence->dup();
}
@@ -659,7 +659,7 @@
for (size_t i=0 ; i<mNumDisplays ; i++) {
DisplayData& disp(mDisplayData[i]);
disp.lastDisplayFence = disp.lastRetireFence;
- disp.lastRetireFence = NULL;
+ disp.lastRetireFence = Fence::NO_FENCE;
if (disp.list) {
if (disp.list->retireFenceFd != -1) {
disp.lastRetireFence = new Fence(disp.list->retireFenceFd);
@@ -725,9 +725,7 @@
if (mHwc && hwcHasApiVersion(mHwc, HWC_DEVICE_API_VERSION_1_1)) {
return setFramebufferTarget(id, acquireFence, buffer);
} else {
- if (acquireFence != NULL) {
- acquireFence->waitForever(1000, "HWComposer::fbPost");
- }
+ acquireFence->waitForever(1000, "HWComposer::fbPost");
return mFbDev->post(mFbDev, buffer->handle);
}
}
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 99af857..1401154 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -298,7 +298,7 @@
if (layer.getCompositionType() == HWC_OVERLAY) {
sp<Fence> fence = mSurfaceFlingerConsumer->getCurrentFence();
- if (fence.get()) {
+ if (fence->isValid()) {
fenceFd = fence->dup();
if (fenceFd == -1) {
ALOGW("failed to dup layer fence, skipping sync: %d", errno);
diff --git a/services/surfaceflinger/SurfaceFlingerConsumer.cpp b/services/surfaceflinger/SurfaceFlingerConsumer.cpp
index dc9089e..e427072 100644
--- a/services/surfaceflinger/SurfaceFlingerConsumer.cpp
+++ b/services/surfaceflinger/SurfaceFlingerConsumer.cpp
@@ -15,6 +15,7 @@
*/
#define ATRACE_TAG ATRACE_TAG_GRAPHICS
+//#define LOG_NDEBUG 0
#include "SurfaceFlingerConsumer.h"