From 631f358d348ea5e7813ca01f86fc9f2a6536add6 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 25 May 2010 17:51:34 -0700 Subject: [PATCH] fix [2712278] The preview buffer left some black borders in left and bottom edges we were incorrectly flagging push_buffer surfaces as invalid Change-Id: I4dfd4ffbbe8a71f7e23e835db8d71966416c29bb --- .../surfaceflinger/SharedBufferStack.h | 1 - include/surfaceflinger/Surface.h | 3 +- .../surfaceflinger/SurfaceComposerClient.h | 23 ++- libs/surfaceflinger/LayerBase.cpp | 2 +- .../SharedBufferStack.cpp | 6 - libs/surfaceflinger_client/Surface.cpp | 140 +++++++++--------- .../SurfaceComposerClient.cpp | 58 ++++---- 7 files changed, 115 insertions(+), 118 deletions(-) diff --git a/include/private/surfaceflinger/SharedBufferStack.h b/include/private/surfaceflinger/SharedBufferStack.h index ea8391d6a..b859e78b7 100644 --- a/include/private/surfaceflinger/SharedBufferStack.h +++ b/include/private/surfaceflinger/SharedBufferStack.h @@ -159,7 +159,6 @@ public: SharedBufferBase(SharedClient* sharedClient, int surface, int32_t identity); ~SharedBufferBase(); - uint32_t getIdentity(); status_t getStatus() const; size_t getFrontBuffer() const; String8 dump(char const* prefix) const; diff --git a/include/surfaceflinger/Surface.h b/include/surfaceflinger/Surface.h index 77e4a61ea..e561fb92f 100644 --- a/include/surfaceflinger/Surface.h +++ b/include/surfaceflinger/Surface.h @@ -228,7 +228,7 @@ private: */ void init(); status_t validate() const; - sp getClient() const; + status_t initCheck() const; sp getISurface() const; inline const GraphicBufferMapper& getBufferMapper() const { return mBufferMapper; } @@ -271,6 +271,7 @@ private: uint32_t mFlags; GraphicBufferMapper& mBufferMapper; SharedBufferClient* mSharedBufferClient; + status_t mInitCheck; // protected by mSurfaceLock Rect mSwapRectangle; diff --git a/include/surfaceflinger/SurfaceComposerClient.h b/include/surfaceflinger/SurfaceComposerClient.h index 9d0f0cbeb..839603824 100644 --- a/include/surfaceflinger/SurfaceComposerClient.h +++ b/include/surfaceflinger/SurfaceComposerClient.h @@ -123,13 +123,6 @@ public: status_t linkToComposerDeath(const sp& recipient, void* cookie = NULL, uint32_t flags = 0); -private: - friend class Surface; - friend class SurfaceControl; - - SurfaceComposerClient(const sp& sm, - const sp& conn); - status_t hide(SurfaceID id); status_t show(SurfaceID id, int32_t layer = -1); status_t freeze(SurfaceID id); @@ -142,17 +135,21 @@ private: status_t setMatrix(SurfaceID id, float dsdx, float dtdx, float dsdy, float dtdy); status_t setPosition(SurfaceID id, int32_t x, int32_t y); status_t setSize(SurfaceID id, uint32_t w, uint32_t h); - void signalServer(); - status_t destroySurface(SurfaceID sid); - void _init(const sp& sm, + SharedClient* getSharedClient() const; + +private: + SurfaceComposerClient(const sp& sm, + const sp& conn); + + void init(const sp& sm, const sp& conn); - inline layer_state_t* _get_state_l(SurfaceID id); - layer_state_t* _lockLayerState(SurfaceID id); - inline void _unlockLayerState(); + inline layer_state_t* get_state_l(SurfaceID id); + layer_state_t* lockLayerState(SurfaceID id); + inline void unlockLayerState(); mutable Mutex mLock; layer_state_t* mPrebuiltLayerState; diff --git a/libs/surfaceflinger/LayerBase.cpp b/libs/surfaceflinger/LayerBase.cpp index 51673ffc3..090404ecf 100644 --- a/libs/surfaceflinger/LayerBase.cpp +++ b/libs/surfaceflinger/LayerBase.cpp @@ -500,7 +500,7 @@ void LayerBase::dump(String8& result, char* buffer, size_t SIZE) const // --------------------------------------------------------------------------- -int32_t LayerBaseClient::sIdentity = 0; +int32_t LayerBaseClient::sIdentity = 1; LayerBaseClient::LayerBaseClient(SurfaceFlinger* flinger, DisplayID display, const sp& client, int32_t i) diff --git a/libs/surfaceflinger_client/SharedBufferStack.cpp b/libs/surfaceflinger_client/SharedBufferStack.cpp index b45225984..2577dc00e 100644 --- a/libs/surfaceflinger_client/SharedBufferStack.cpp +++ b/libs/surfaceflinger_client/SharedBufferStack.cpp @@ -155,12 +155,6 @@ SharedBufferBase::~SharedBufferBase() { } -uint32_t SharedBufferBase::getIdentity() -{ - SharedBufferStack& stack( *mSharedStack ); - return stack.identity; -} - status_t SharedBufferBase::getStatus() const { SharedBufferStack& stack( *mSharedStack ); diff --git a/libs/surfaceflinger_client/Surface.cpp b/libs/surfaceflinger_client/Surface.cpp index 71504fa79..534a54e6a 100644 --- a/libs/surfaceflinger_client/Surface.cpp +++ b/libs/surfaceflinger_client/Surface.cpp @@ -151,75 +151,75 @@ bool SurfaceControl::isSameSurface( } status_t SurfaceControl::setLayer(int32_t layer) { - const sp& client(mClient); status_t err = validate(); if (err < 0) return err; + const sp& client(mClient); return client->setLayer(mToken, layer); } status_t SurfaceControl::setPosition(int32_t x, int32_t y) { - const sp& client(mClient); status_t err = validate(); if (err < 0) return err; + const sp& client(mClient); return client->setPosition(mToken, x, y); } status_t SurfaceControl::setSize(uint32_t w, uint32_t h) { - const sp& client(mClient); status_t err = validate(); if (err < 0) return err; + const sp& client(mClient); return client->setSize(mToken, w, h); } status_t SurfaceControl::hide() { - const sp& client(mClient); status_t err = validate(); if (err < 0) return err; + const sp& client(mClient); return client->hide(mToken); } status_t SurfaceControl::show(int32_t layer) { - const sp& client(mClient); status_t err = validate(); if (err < 0) return err; + const sp& client(mClient); return client->show(mToken, layer); } status_t SurfaceControl::freeze() { - const sp& client(mClient); status_t err = validate(); if (err < 0) return err; + const sp& client(mClient); return client->freeze(mToken); } status_t SurfaceControl::unfreeze() { - const sp& client(mClient); status_t err = validate(); if (err < 0) return err; + const sp& client(mClient); return client->unfreeze(mToken); } status_t SurfaceControl::setFlags(uint32_t flags, uint32_t mask) { - const sp& client(mClient); status_t err = validate(); if (err < 0) return err; + const sp& client(mClient); return client->setFlags(mToken, flags, mask); } status_t SurfaceControl::setTransparentRegionHint(const Region& transparent) { - const sp& client(mClient); status_t err = validate(); if (err < 0) return err; + const sp& client(mClient); return client->setTransparentRegionHint(mToken, transparent); } status_t SurfaceControl::setAlpha(float alpha) { - const sp& client(mClient); status_t err = validate(); if (err < 0) return err; + const sp& client(mClient); return client->setAlpha(mToken, alpha); } status_t SurfaceControl::setMatrix(float dsdx, float dtdx, float dsdy, float dtdy) { - const sp& client(mClient); status_t err = validate(); if (err < 0) return err; + const sp& client(mClient); return client->setMatrix(mToken, dsdx, dtdx, dsdy, dtdy); } status_t SurfaceControl::setFreezeTint(uint32_t tint) { - const sp& client(mClient); status_t err = validate(); if (err < 0) return err; + const sp& client(mClient); return client->setFreezeTint(mToken, tint); } @@ -230,23 +230,6 @@ status_t SurfaceControl::validate() const mToken, mIdentity, mClient.get()); return NO_INIT; } - SharedClient const* cblk = mClient->mControl; - if (cblk == 0) { - LOGE("cblk is null (surface id=%d, identity=%u)", mToken, mIdentity); - return NO_INIT; - } - status_t err = cblk->validate(mToken); - if (err != NO_ERROR) { - LOGE("surface (id=%d, identity=%u) is invalid, err=%d (%s)", - mToken, mIdentity, err, strerror(-err)); - return err; - } - uint32_t identity = cblk->getIdentity(mToken); - if (mIdentity != identity) { - LOGE("using an invalid surface id=%d, identity=%u should be %d", - mToken, mIdentity, identity); - return NO_INIT; - } return NO_ERROR; } @@ -300,16 +283,15 @@ Surface::Surface(const sp& surface) mToken(surface->mToken), mIdentity(surface->mIdentity), mFormat(surface->mFormat), mFlags(surface->mFlags), mBufferMapper(GraphicBufferMapper::get()), mSharedBufferClient(NULL), + mInitCheck(NO_INIT), mWidth(surface->mWidth), mHeight(surface->mHeight) { - mSharedBufferClient = new SharedBufferClient( - mClient->mControl, mToken, 2, mIdentity); - init(); } Surface::Surface(const Parcel& parcel) - : mBufferMapper(GraphicBufferMapper::get()), mSharedBufferClient(NULL) + : mBufferMapper(GraphicBufferMapper::get()), + mSharedBufferClient(NULL), mInitCheck(NO_INIT) { sp clientBinder = parcel.readStrongBinder(); mSurface = interface_cast(parcel.readStrongBinder()); @@ -323,9 +305,6 @@ Surface::Surface(const Parcel& parcel) // FIXME: what does that mean if clientBinder is NULL here? if (clientBinder != NULL) { mClient = SurfaceComposerClient::clientForConnection(clientBinder); - - mSharedBufferClient = new SharedBufferClient( - mClient->mControl, mToken, 2, mIdentity); } init(); @@ -339,7 +318,7 @@ void Surface::init() android_native_window_t::queueBuffer = queueBuffer; android_native_window_t::query = query; android_native_window_t::perform = perform; - mSwapRectangle.makeInvalid(); + DisplayInfo dinfo; SurfaceComposerClient::getDisplayInfo(0, &dinfo); const_cast(android_native_window_t::xdpi) = dinfo.xdpi; @@ -348,11 +327,19 @@ void Surface::init() const_cast(android_native_window_t::minSwapInterval) = 1; const_cast(android_native_window_t::maxSwapInterval) = 1; const_cast(android_native_window_t::flags) = 0; - // be default we request a hardware surface + mConnected = 0; + mSwapRectangle.makeInvalid(); // two buffers by default mBuffers.setCapacity(2); mBuffers.insertAt(0, 2); + + if (mClient != 0) { + mSharedBufferClient = new SharedBufferClient( + mClient->getSharedClient(), mToken, 2, mIdentity); + } + + mInitCheck = initCheck(); } Surface::~Surface() @@ -375,56 +362,73 @@ Surface::~Surface() IPCThreadState::self()->flushCommands(); } -sp Surface::getClient() const { - return mClient; -} - -sp Surface::getISurface() const { - return mSurface; -} - -bool Surface::isValid() { - return mToken>=0 && mClient!=0; -} - -status_t Surface::validate() const +status_t Surface::initCheck() const { - sp client(getClient()); if (mToken<0 || mClient==0) { - LOGE("invalid token (%d, identity=%u) or client (%p)", - mToken, mIdentity, client.get()); return NO_INIT; } - SharedClient const* cblk = mClient->mControl; + SharedClient const* cblk = mClient->getSharedClient(); if (cblk == 0) { LOGE("cblk is null (surface id=%d, identity=%u)", mToken, mIdentity); return NO_INIT; } + return NO_ERROR; +} + +bool Surface::isValid() { + return mInitCheck == NO_ERROR; +} + +status_t Surface::validate() const +{ + // check that we initialized ourself properly + if (mInitCheck != NO_ERROR) { + LOGE("invalid token (%d, identity=%u) or client (%p)", + mToken, mIdentity, mClient.get()); + return mInitCheck; + } + + // verify the identity of this surface + SharedClient const* cblk = mClient->getSharedClient(); + + uint32_t identity = cblk->getIdentity(mToken); + + // this is a bit of a (temporary) special case, identity==0 means that + // no operation are allowed from the client (eg: dequeue/queue), this + // is used with PUSH_BUFFER surfaces for instance + if (identity == 0) { + LOGE("[Surface] invalid operation (identity=%u)", mIdentity); + return INVALID_OPERATION; + } + + if (mIdentity != identity) { + LOGE("[Surface] using an invalid surface id=%d, " + "identity=%u should be %d", + mToken, mIdentity, identity); + return NO_INIT; + } + + // check the surface didn't become invalid status_t err = cblk->validate(mToken); if (err != NO_ERROR) { LOGE("surface (id=%d, identity=%u) is invalid, err=%d (%s)", mToken, mIdentity, err, strerror(-err)); return err; } - uint32_t identity = cblk->getIdentity(mToken); - if (mIdentity != identity) { - LOGE("using an invalid surface id=%d, identity=%u should be %d", - mToken, mIdentity, identity); - return NO_INIT; - } + return NO_ERROR; } - -bool Surface::isSameSurface( - const sp& lhs, const sp& rhs) -{ +bool Surface::isSameSurface(const sp& lhs, const sp& rhs) { if (lhs == 0 || rhs == 0) return false; - return lhs->mSurface->asBinder() == rhs->mSurface->asBinder(); } +sp Surface::getISurface() const { + return mSurface; +} + // ---------------------------------------------------------------------------- int Surface::setSwapInterval(android_native_window_t* window, int interval) { @@ -485,7 +489,6 @@ bool Surface::needNewBuffer(int bufIdx, int Surface::dequeueBuffer(android_native_buffer_t** buffer) { - sp client(getClient()); status_t err = validate(); if (err != NO_ERROR) return err; @@ -534,7 +537,6 @@ int Surface::dequeueBuffer(android_native_buffer_t** buffer) int Surface::lockBuffer(android_native_buffer_t* buffer) { - sp client(getClient()); status_t err = validate(); if (err != NO_ERROR) return err; @@ -547,7 +549,6 @@ int Surface::lockBuffer(android_native_buffer_t* buffer) int Surface::queueBuffer(android_native_buffer_t* buffer) { - sp client(getClient()); status_t err = validate(); if (err != NO_ERROR) return err; @@ -564,6 +565,7 @@ int Surface::queueBuffer(android_native_buffer_t* buffer) if (err == NO_ERROR) { // FIXME: can we avoid this IPC if we know there is one pending? + const sp& client(mClient); client->signalServer(); } return err; diff --git a/libs/surfaceflinger_client/SurfaceComposerClient.cpp b/libs/surfaceflinger_client/SurfaceComposerClient.cpp index 9ac73d231..96ed5661b 100644 --- a/libs/surfaceflinger_client/SurfaceComposerClient.cpp +++ b/libs/surfaceflinger_client/SurfaceComposerClient.cpp @@ -123,11 +123,11 @@ SurfaceComposerClient::SurfaceComposerClient() { sp sm(getComposerService()); if (sm == 0) { - _init(0, 0); + init(0, 0); return; } - _init(sm, sm->createConnection()); + init(sm, sm->createConnection()); if (mClient != 0) { Mutex::Autolock _l(gLock); @@ -139,9 +139,14 @@ SurfaceComposerClient::SurfaceComposerClient() SurfaceComposerClient::SurfaceComposerClient( const sp& sm, const sp& conn) { - _init(sm, interface_cast(conn)); + init(sm, interface_cast(conn)); } +SurfaceComposerClient::~SurfaceComposerClient() +{ + VERBOSE("Destroying client %p, conn %p", this, mClient.get()); + dispose(); +} status_t SurfaceComposerClient::linkToComposerDeath( const sp& recipient, @@ -151,7 +156,7 @@ status_t SurfaceComposerClient::linkToComposerDeath( return sm->asBinder()->linkToDeath(recipient, cookie, flags); } -void SurfaceComposerClient::_init( +void SurfaceComposerClient::init( const sp& sm, const sp& conn) { VERBOSE("Creating client %p, conn %p", this, conn.get()); @@ -172,10 +177,9 @@ void SurfaceComposerClient::_init( mControl = static_cast(mControlMemory->getBase()); } -SurfaceComposerClient::~SurfaceComposerClient() +SharedClient* SurfaceComposerClient::getSharedClient() const { - VERBOSE("Destroying client %p, conn %p", this, mClient.get()); - dispose(); + return mControl; } status_t SurfaceComposerClient::initCheck() const @@ -488,7 +492,7 @@ status_t SurfaceComposerClient::closeTransaction() return NO_ERROR; } -layer_state_t* SurfaceComposerClient::_get_state_l(SurfaceID index) +layer_state_t* SurfaceComposerClient::get_state_l(SurfaceID index) { // API usage error, do nothing. if (mTransactionOpen<=0) { @@ -508,49 +512,49 @@ layer_state_t* SurfaceComposerClient::_get_state_l(SurfaceID index) return mStates.editArray() + i; } -layer_state_t* SurfaceComposerClient::_lockLayerState(SurfaceID id) +layer_state_t* SurfaceComposerClient::lockLayerState(SurfaceID id) { layer_state_t* s; mLock.lock(); - s = _get_state_l(id); + s = get_state_l(id); if (!s) mLock.unlock(); return s; } -void SurfaceComposerClient::_unlockLayerState() +void SurfaceComposerClient::unlockLayerState() { mLock.unlock(); } status_t SurfaceComposerClient::setPosition(SurfaceID id, int32_t x, int32_t y) { - layer_state_t* s = _lockLayerState(id); + layer_state_t* s = lockLayerState(id); if (!s) return BAD_INDEX; s->what |= ISurfaceComposer::ePositionChanged; s->x = x; s->y = y; - _unlockLayerState(); + unlockLayerState(); return NO_ERROR; } status_t SurfaceComposerClient::setSize(SurfaceID id, uint32_t w, uint32_t h) { - layer_state_t* s = _lockLayerState(id); + layer_state_t* s = lockLayerState(id); if (!s) return BAD_INDEX; s->what |= ISurfaceComposer::eSizeChanged; s->w = w; s->h = h; - _unlockLayerState(); + unlockLayerState(); return NO_ERROR; } status_t SurfaceComposerClient::setLayer(SurfaceID id, int32_t z) { - layer_state_t* s = _lockLayerState(id); + layer_state_t* s = lockLayerState(id); if (!s) return BAD_INDEX; s->what |= ISurfaceComposer::eLayerChanged; s->z = z; - _unlockLayerState(); + unlockLayerState(); return NO_ERROR; } @@ -579,34 +583,34 @@ status_t SurfaceComposerClient::unfreeze(SurfaceID id) status_t SurfaceComposerClient::setFlags(SurfaceID id, uint32_t flags, uint32_t mask) { - layer_state_t* s = _lockLayerState(id); + layer_state_t* s = lockLayerState(id); if (!s) return BAD_INDEX; s->what |= ISurfaceComposer::eVisibilityChanged; s->flags &= ~mask; s->flags |= (flags & mask); s->mask |= mask; - _unlockLayerState(); + unlockLayerState(); return NO_ERROR; } status_t SurfaceComposerClient::setTransparentRegionHint( SurfaceID id, const Region& transparentRegion) { - layer_state_t* s = _lockLayerState(id); + layer_state_t* s = lockLayerState(id); if (!s) return BAD_INDEX; s->what |= ISurfaceComposer::eTransparentRegionChanged; s->transparentRegion = transparentRegion; - _unlockLayerState(); + unlockLayerState(); return NO_ERROR; } status_t SurfaceComposerClient::setAlpha(SurfaceID id, float alpha) { - layer_state_t* s = _lockLayerState(id); + layer_state_t* s = lockLayerState(id); if (!s) return BAD_INDEX; s->what |= ISurfaceComposer::eAlphaChanged; s->alpha = alpha; - _unlockLayerState(); + unlockLayerState(); return NO_ERROR; } @@ -615,7 +619,7 @@ status_t SurfaceComposerClient::setMatrix( float dsdx, float dtdx, float dsdy, float dtdy ) { - layer_state_t* s = _lockLayerState(id); + layer_state_t* s = lockLayerState(id); if (!s) return BAD_INDEX; s->what |= ISurfaceComposer::eMatrixChanged; layer_state_t::matrix22_t matrix; @@ -624,17 +628,17 @@ status_t SurfaceComposerClient::setMatrix( matrix.dsdy = dsdy; matrix.dtdy = dtdy; s->matrix = matrix; - _unlockLayerState(); + unlockLayerState(); return NO_ERROR; } status_t SurfaceComposerClient::setFreezeTint(SurfaceID id, uint32_t tint) { - layer_state_t* s = _lockLayerState(id); + layer_state_t* s = lockLayerState(id); if (!s) return BAD_INDEX; s->what |= ISurfaceComposer::eFreezeTintChanged; s->tint = tint; - _unlockLayerState(); + unlockLayerState(); return NO_ERROR; }