From aa61419b69daed4794c593f0718b3330ee2ec8dc Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Thu, 6 Jun 2019 13:28:34 -0700 Subject: [SurfaceFlinger] correct present time for negative phase offsets DispSync::expectedPresentTime returns the expected presentation time for the current frame, but when we're in negative offsets we are targetting the following frame instead. Bug: 133241520 Bug: 134589085 Test: systrace when flinging through news Change-Id: I7cc05a0b9e8e9b5c3e8d0c4b1d59b0a7dabd43d4 --- services/surfaceflinger/BufferQueueLayer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'services/surfaceflinger/BufferQueueLayer.cpp') diff --git a/services/surfaceflinger/BufferQueueLayer.cpp b/services/surfaceflinger/BufferQueueLayer.cpp index bd0b55f688..57f1008e85 100644 --- a/services/surfaceflinger/BufferQueueLayer.cpp +++ b/services/surfaceflinger/BufferQueueLayer.cpp @@ -143,7 +143,7 @@ bool BufferQueueLayer::framePresentTimeIsCurrent() const { } Mutex::Autolock lock(mQueueItemLock); - return mQueueItems[0].mTimestamp <= mFlinger->mScheduler->expectedPresentTime(); + return mQueueItems[0].mTimestamp <= mFlinger->getExpectedPresentTime(); } nsecs_t BufferQueueLayer::getDesiredPresentTime() { -- cgit v1.2.3-59-g8ed1b From 8fe1102f1feec1aa8b9a00a22a3cab13166fff0d Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Wed, 12 Jun 2019 17:11:12 -0700 Subject: SurfaceFlinger: get present time from SF and not from Scheduler SF hold the most accurate expected present time as it also knows whether we are operating at negative offset and which vsync we are targeting. Bug: 133241520 Bug: 134589085 Test: systrace when scrolling Change-Id: I934df3a8bf807b0e52555765a6861f252b69c0d1 --- services/surfaceflinger/BufferQueueLayer.cpp | 4 ++-- services/surfaceflinger/Scheduler/Scheduler.cpp | 2 +- services/surfaceflinger/Scheduler/Scheduler.h | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) (limited to 'services/surfaceflinger/BufferQueueLayer.cpp') diff --git a/services/surfaceflinger/BufferQueueLayer.cpp b/services/surfaceflinger/BufferQueueLayer.cpp index 57f1008e85..d6853661a5 100644 --- a/services/surfaceflinger/BufferQueueLayer.cpp +++ b/services/surfaceflinger/BufferQueueLayer.cpp @@ -201,7 +201,7 @@ uint64_t BufferQueueLayer::getFrameNumber() const { uint64_t frameNumber = mQueueItems[0].mFrameNumber; // The head of the queue will be dropped if there are signaled and timely frames behind it - nsecs_t expectedPresentTime = mFlinger->mScheduler->expectedPresentTime(); + nsecs_t expectedPresentTime = mFlinger->getExpectedPresentTime(); if (isRemovedFromCurrentState()) { expectedPresentTime = 0; @@ -279,7 +279,7 @@ status_t BufferQueueLayer::updateTexImage(bool& recomputeVisibleRegions, nsecs_t getProducerStickyTransform() != 0, mName.string(), mOverrideScalingMode, getTransformToDisplayInverse(), mFreezeGeometryUpdates); - nsecs_t expectedPresentTime = mFlinger->mScheduler->expectedPresentTime(); + nsecs_t expectedPresentTime = mFlinger->getExpectedPresentTime(); if (isRemovedFromCurrentState()) { expectedPresentTime = 0; diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index e2a348e146..1d899dfbad 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -309,7 +309,7 @@ void Scheduler::setIgnorePresentFences(bool ignore) { mPrimaryDispSync->setIgnorePresentFences(ignore); } -nsecs_t Scheduler::expectedPresentTime() { +nsecs_t Scheduler::getDispSyncExpectedPresentTime() { return mPrimaryDispSync->expectedPresentTime(); } diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h index f6cc87bb89..a32bd4142d 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.h +++ b/services/surfaceflinger/Scheduler/Scheduler.h @@ -149,7 +149,7 @@ public: void addResyncSample(const nsecs_t timestamp, bool* periodFlushed); void addPresentFence(const std::shared_ptr& fenceTime); void setIgnorePresentFences(bool ignore); - nsecs_t expectedPresentTime(); + nsecs_t getDispSyncExpectedPresentTime(); // Registers the layer in the scheduler, and returns the handle for future references. std::unique_ptr registerLayer(std::string const& name, int windowType); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index dadb3fed05..cd4f69a9db 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -1646,7 +1646,7 @@ bool SurfaceFlinger::previousFrameMissed() NO_THREAD_SAFETY_ANALYSIS { nsecs_t SurfaceFlinger::getExpectedPresentTime() NO_THREAD_SAFETY_ANALYSIS { DisplayStatInfo stats; mScheduler->getDisplayStatInfo(&stats); - const nsecs_t presentTime = mScheduler->expectedPresentTime(); + const nsecs_t presentTime = mScheduler->getDispSyncExpectedPresentTime(); // Inflate the expected present time if we're targetting the next vsync. const nsecs_t correctedTime = mVsyncModulator.getOffsets().sf < mPhaseOffsets->getOffsetThresholdForNextVsync() @@ -3653,7 +3653,7 @@ bool SurfaceFlinger::containsAnyInvalidClientState(const Vector& bool SurfaceFlinger::transactionIsReadyToBeApplied(int64_t desiredPresentTime, const Vector& states) { - nsecs_t expectedPresentTime = mScheduler->expectedPresentTime(); + nsecs_t expectedPresentTime = getExpectedPresentTime(); // Do not present if the desiredPresentTime has not passed unless it is more than one second // in the future. We ignore timestamps more than 1 second in the future for stability reasons. if (desiredPresentTime >= 0 && desiredPresentTime >= expectedPresentTime && -- cgit v1.2.3-59-g8ed1b From 44685cb3f0e53c0f0b5f260891b9dd78d25410d4 Mon Sep 17 00:00:00 2001 From: Steven Thomas Date: Tue, 23 Jul 2019 16:19:31 -0700 Subject: Merge damage region when dropping an app buffer When we decide to drop a buffer from a buffer queue, make sure to merge the damage region from the dropped buffer into the current damage region. Otherwise we'll pass the wrong damage region to hardware composer and get broken rendering. Bug: 136158117 Test: Wrote a new test in Transaction_test.cpp to repro the problem and confirm the fix works. Change-Id: Icdc61e1be3297450f2869c496dad1453fb6dca6d --- services/surfaceflinger/BufferLayerConsumer.cpp | 9 +++ services/surfaceflinger/BufferLayerConsumer.h | 3 + services/surfaceflinger/BufferQueueLayer.cpp | 2 + services/surfaceflinger/tests/Transaction_test.cpp | 94 ++++++++++++++++++++++ 4 files changed, 108 insertions(+) (limited to 'services/surfaceflinger/BufferQueueLayer.cpp') diff --git a/services/surfaceflinger/BufferLayerConsumer.cpp b/services/surfaceflinger/BufferLayerConsumer.cpp index 6709fb4b48..bd9bd81e2a 100644 --- a/services/surfaceflinger/BufferLayerConsumer.cpp +++ b/services/surfaceflinger/BufferLayerConsumer.cpp @@ -369,6 +369,15 @@ const Region& BufferLayerConsumer::getSurfaceDamage() const { return mCurrentSurfaceDamage; } +void BufferLayerConsumer::mergeSurfaceDamage(const Region& damage) { + if (damage.bounds() == Rect::INVALID_RECT || + mCurrentSurfaceDamage.bounds() == Rect::INVALID_RECT) { + mCurrentSurfaceDamage = Region::INVALID_REGION; + } else { + mCurrentSurfaceDamage |= damage; + } +} + int BufferLayerConsumer::getCurrentApi() const { Mutex::Autolock lock(mMutex); return mCurrentApi; diff --git a/services/surfaceflinger/BufferLayerConsumer.h b/services/surfaceflinger/BufferLayerConsumer.h index e3f6100c35..901556a53f 100644 --- a/services/surfaceflinger/BufferLayerConsumer.h +++ b/services/surfaceflinger/BufferLayerConsumer.h @@ -140,6 +140,9 @@ public: // must be called from SF main thread const Region& getSurfaceDamage() const; + // Merge the given damage region into the current damage region value. + void mergeSurfaceDamage(const Region& damage); + // getCurrentApi retrieves the API which queues the current buffer. int getCurrentApi() const; diff --git a/services/surfaceflinger/BufferQueueLayer.cpp b/services/surfaceflinger/BufferQueueLayer.cpp index d6853661a5..f35a4fd497 100644 --- a/services/surfaceflinger/BufferQueueLayer.cpp +++ b/services/surfaceflinger/BufferQueueLayer.cpp @@ -316,6 +316,7 @@ status_t BufferQueueLayer::updateTexImage(bool& recomputeVisibleRegions, nsecs_t // and return early if (queuedBuffer) { Mutex::Autolock lock(mQueueItemLock); + mConsumer->mergeSurfaceDamage(mQueueItems[0].mSurfaceDamage); mFlinger->mTimeStats->removeTimeRecord(layerID, mQueueItems[0].mFrameNumber); mQueueItems.removeAt(0); mQueuedFrames--; @@ -351,6 +352,7 @@ status_t BufferQueueLayer::updateTexImage(bool& recomputeVisibleRegions, nsecs_t // Remove any stale buffers that have been dropped during // updateTexImage while (mQueueItems[0].mFrameNumber != currentFrameNumber) { + mConsumer->mergeSurfaceDamage(mQueueItems[0].mSurfaceDamage); mFlinger->mTimeStats->removeTimeRecord(layerID, mQueueItems[0].mFrameNumber); mQueueItems.removeAt(0); mQueuedFrames--; diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp index d5f65348d8..c93e15ef96 100644 --- a/services/surfaceflinger/tests/Transaction_test.cpp +++ b/services/surfaceflinger/tests/Transaction_test.cpp @@ -28,6 +28,7 @@ #include #include +#include #include #include #include @@ -6059,4 +6060,97 @@ TEST_F(RelativeZTest, LayerRemovedOffscreenRelativeParent) { } } +// This test ensures that when we drop an app buffer in SurfaceFlinger, we merge +// the dropped buffer's damage region into the next buffer's damage region. If +// we don't do this, we'll report an incorrect damage region to hardware +// composer, resulting in broken rendering. This test checks the BufferQueue +// case. +// +// Unfortunately, we don't currently have a way to inspect the damage region +// SurfaceFlinger sends to hardware composer from a test, so this test requires +// the dev to manually watch the device's screen during the test to spot broken +// rendering. Because the results can't be automatically verified, this test is +// marked disabled. +TEST_F(LayerTransactionTest, DISABLED_BufferQueueLayerMergeDamageRegionWhenDroppingBuffers) { + const int width = mDisplayWidth; + const int height = mDisplayHeight; + sp layer; + ASSERT_NO_FATAL_FAILURE(layer = createLayer("test", width, height)); + const auto producer = layer->getIGraphicBufferProducer(); + const sp dummyListener(new DummyProducerListener); + IGraphicBufferProducer::QueueBufferOutput queueBufferOutput; + ASSERT_EQ(OK, + producer->connect(dummyListener, NATIVE_WINDOW_API_CPU, true, &queueBufferOutput)); + + std::map> slotMap; + auto slotToBuffer = [&](int slot, sp* buf) { + ASSERT_NE(nullptr, buf); + const auto iter = slotMap.find(slot); + ASSERT_NE(slotMap.end(), iter); + *buf = iter->second; + }; + + auto dequeue = [&](int* outSlot) { + ASSERT_NE(nullptr, outSlot); + *outSlot = -1; + int slot; + sp fence; + uint64_t age; + FrameEventHistoryDelta timestamps; + const status_t dequeueResult = + producer->dequeueBuffer(&slot, &fence, width, height, PIXEL_FORMAT_RGBA_8888, + GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_OFTEN, + &age, ×tamps); + if (dequeueResult == IGraphicBufferProducer::BUFFER_NEEDS_REALLOCATION) { + sp newBuf; + ASSERT_EQ(OK, producer->requestBuffer(slot, &newBuf)); + ASSERT_NE(nullptr, newBuf.get()); + slotMap[slot] = newBuf; + } else { + ASSERT_EQ(OK, dequeueResult); + } + *outSlot = slot; + }; + + auto queue = [&](int slot, const Region& damage, nsecs_t displayTime) { + IGraphicBufferProducer::QueueBufferInput input( + /*timestamp=*/displayTime, /*isAutoTimestamp=*/false, HAL_DATASPACE_UNKNOWN, + /*crop=*/Rect::EMPTY_RECT, NATIVE_WINDOW_SCALING_MODE_SCALE_TO_WINDOW, + /*transform=*/0, Fence::NO_FENCE); + input.setSurfaceDamage(damage); + IGraphicBufferProducer::QueueBufferOutput output; + ASSERT_EQ(OK, producer->queueBuffer(slot, input, &output)); + }; + + auto fillAndPostBuffers = [&](const Color& color) { + int slot1; + ASSERT_NO_FATAL_FAILURE(dequeue(&slot1)); + int slot2; + ASSERT_NO_FATAL_FAILURE(dequeue(&slot2)); + + sp buf1; + ASSERT_NO_FATAL_FAILURE(slotToBuffer(slot1, &buf1)); + sp buf2; + ASSERT_NO_FATAL_FAILURE(slotToBuffer(slot2, &buf2)); + fillGraphicBufferColor(buf1, Rect(width, height), color); + fillGraphicBufferColor(buf2, Rect(width, height), color); + + const auto displayTime = systemTime() + milliseconds_to_nanoseconds(100); + ASSERT_NO_FATAL_FAILURE(queue(slot1, Region::INVALID_REGION, displayTime)); + ASSERT_NO_FATAL_FAILURE( + queue(slot2, Region(Rect(width / 3, height / 3, 2 * width / 3, 2 * height / 3)), + displayTime)); + }; + + const auto startTime = systemTime(); + const std::array colors = {Color::RED, Color::GREEN, Color::BLUE}; + int colorIndex = 0; + while (nanoseconds_to_seconds(systemTime() - startTime) < 10) { + ASSERT_NO_FATAL_FAILURE(fillAndPostBuffers(colors[colorIndex++ % colors.size()])); + std::this_thread::sleep_for(1s); + } + + ASSERT_EQ(OK, producer->disconnect(NATIVE_WINDOW_API_CPU)); +} + } // namespace android -- cgit v1.2.3-59-g8ed1b