Skip to content

Commit f2d8a82

Browse files
author
Melody Hsu
committed
Stabilize libgui RegionSamplingTests
libgui RegionSamplingTests fail on physical devices due to incorrect sampled luma values. The incorrect region was being sampled due to other active listeners that were not cleared. Modify test API to get all existing active listeners, cache them and clear the list to create a blank test slate, and re-add the cached listeners to restore state after a test is done. Bug: b/365708032 Test: atest RegionSamplingTest in libgui_test, libgui_test Flag: EXEMPT, test fix Change-Id: I70eb3bae070174b7db87ee16d8343bbc1e81bad0
1 parent 35f9fab commit f2d8a82

10 files changed

Lines changed: 188 additions & 3 deletions

libs/gui/SurfaceComposerClient.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3007,13 +3007,34 @@ status_t SurfaceComposerClient::addRegionSamplingListener(
30073007
return statusTFromBinderStatus(status);
30083008
}
30093009

3010+
status_t SurfaceComposerClient::addRegionSamplingListenerWithStopLayerId(
3011+
const Rect& samplingArea, const int32_t stopLayerId,
3012+
const sp<IRegionSamplingListener>& listener) {
3013+
gui::ARect rect;
3014+
rect.left = samplingArea.left;
3015+
rect.top = samplingArea.top;
3016+
rect.right = samplingArea.right;
3017+
rect.bottom = samplingArea.bottom;
3018+
binder::Status status =
3019+
ComposerServiceAIDL::getComposerService()
3020+
->addRegionSamplingListenerWithStopLayerId(rect, stopLayerId, listener);
3021+
return statusTFromBinderStatus(status);
3022+
}
3023+
30103024
status_t SurfaceComposerClient::removeRegionSamplingListener(
30113025
const sp<IRegionSamplingListener>& listener) {
30123026
binder::Status status =
30133027
ComposerServiceAIDL::getComposerService()->removeRegionSamplingListener(listener);
30143028
return statusTFromBinderStatus(status);
30153029
}
30163030

3031+
status_t SurfaceComposerClient::getRegionSamplingListeners(
3032+
std::vector<gui::RegionSamplingDescriptor>* listeners) {
3033+
binder::Status status =
3034+
ComposerServiceAIDL::getComposerService()->getRegionSamplingListeners(listeners);
3035+
return statusTFromBinderStatus(status);
3036+
}
3037+
30173038
status_t SurfaceComposerClient::addFpsListener(int32_t taskId,
30183039
const sp<gui::IFpsListener>& listener) {
30193040
binder::Status status =

libs/gui/aidl/android/gui/ISurfaceComposer.aidl

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ import android.gui.IJankListener;
4747
import android.gui.LayerCaptureArgs;
4848
import android.gui.OverlayProperties;
4949
import android.gui.PullAtomData;
50+
import android.gui.RegionSamplingDescriptor;
5051
import android.gui.ScreenCaptureResults;
5152
import android.gui.ARect;
5253
import android.gui.SchedulingPolicy;
@@ -350,19 +351,42 @@ interface ISurfaceComposer {
350351
* The sampling area is bounded by both samplingArea and the given stopLayerHandle
351352
* (i.e., only layers behind the stop layer will be captured and sampled).
352353
*
353-
* Multiple listeners may be provided so long as they have independent listeners.
354-
* If multiple listeners are provided, the effective sampling region for each listener will
355-
* be bounded by whichever stop layer has a lower Z value.
354+
* Multiple listeners for the same sampling region may be provided so long as they have
355+
* independent IRegionSamplingListener objects. If multiple listeners are provided, the
356+
* effective sampling region for each listener will be bounded by whichever stop layer has
357+
* a lower Z-value.
356358
*
357359
* Requires the same permissions as captureLayers and captureScreen.
358360
*/
359361
void addRegionSamplingListener(in ARect samplingArea, @nullable IBinder stopLayerHandle, IRegionSamplingListener listener);
360362

363+
/**
364+
* Registers a listener by stopLayerId to stream median luma updates from SurfaceFlinger.
365+
*
366+
* The sampling area is bounded by both samplingArea and the given stopLayerId
367+
* (i.e., only layers behind the stop layer will be captured and sampled).
368+
*
369+
* Multiple listeners for the same sampling region may be provided so long as they have
370+
* independent IRegionSamplingListener objects. If multiple listeners are provided, the
371+
* effective sampling region for each listener will be bounded by whichever stop layer has
372+
* a lower Z-value.
373+
*
374+
* Requires the ACCESS_SURFACE_FLINGER permission.
375+
*/
376+
void addRegionSamplingListenerWithStopLayerId(in ARect samplingArea, int stopLayerId, IRegionSamplingListener listener);
377+
361378
/**
362379
* Removes a listener that was streaming median luma updates from SurfaceFlinger.
363380
*/
364381
void removeRegionSamplingListener(IRegionSamplingListener listener);
365382

383+
/**
384+
* Gets all listeners that are streaming median luma updates from SurfaceFlinger.
385+
*
386+
* Requires the ACCESS_SURFACE_FLINGER permission.
387+
*/
388+
List<RegionSamplingDescriptor> getRegionSamplingListeners();
389+
366390
/**
367391
* Registers a listener that streams fps updates from SurfaceFlinger.
368392
*
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/*
2+
* Copyright 2025 The Android Open Source Project
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package android.gui;
18+
19+
import android.gui.ARect;
20+
import android.gui.IRegionSamplingListener;
21+
22+
/** @hide */
23+
parcelable RegionSamplingDescriptor {
24+
// Sampling area region
25+
ARect area;
26+
27+
// All layers under this layer ID will be sampled from
28+
int stopLayerId;
29+
30+
// Listener receiving median luma notifications
31+
IRegionSamplingListener listener;
32+
}

libs/gui/include/gui/SurfaceComposerClient.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545

4646
#include <android/gui/BnJankListener.h>
4747
#include <android/gui/ISurfaceComposerClient.h>
48+
#include <android/gui/RegionSamplingDescriptor.h>
4849

4950
#include <gui/BufferReleaseChannel.h>
5051
#include <gui/CpuConsumer.h>
@@ -835,7 +836,11 @@ class SurfaceComposerClient : public RefBase
835836
static status_t addRegionSamplingListener(const Rect& samplingArea,
836837
const sp<IBinder>& stopLayerHandle,
837838
const sp<IRegionSamplingListener>& listener);
839+
static status_t addRegionSamplingListenerWithStopLayerId(
840+
const Rect& samplingArea, const int32_t stopLayerId,
841+
const sp<IRegionSamplingListener>& listener);
838842
static status_t removeRegionSamplingListener(const sp<IRegionSamplingListener>& listener);
843+
static status_t getRegionSamplingListeners(std::vector<gui::RegionSamplingDescriptor>*);
839844
static status_t addFpsListener(int32_t taskId, const sp<gui::IFpsListener>& listener);
840845
static status_t removeFpsListener(const sp<gui::IFpsListener>& listener);
841846
static status_t addTunnelModeEnabledListener(

libs/gui/tests/RegionSampling_test.cpp

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,23 @@ struct RegionSamplingTest : ::testing::Test {
212212
.setPosition(mTopLayer, 0, 0)
213213
.show(mBackgroundLayer)
214214
.apply();
215+
216+
// Cache any existing listeners that could impact luma sampling test results
217+
sp<gui::ISurfaceComposer> composer = ComposerServiceAIDL::getComposerService();
218+
composer->getRegionSamplingListeners(&mExistingListeners);
219+
for (const auto& descriptor : mExistingListeners) {
220+
composer->removeRegionSamplingListener(descriptor.listener);
221+
}
222+
}
223+
224+
void TearDown() override {
225+
// Restore device state by re-adding listeners that were present prior to the test run
226+
sp<gui::ISurfaceComposer> composer = ComposerServiceAIDL::getComposerService();
227+
for (const auto& descriptor : mExistingListeners) {
228+
composer->addRegionSamplingListenerWithStopLayerId(descriptor.area,
229+
descriptor.stopLayerId,
230+
descriptor.listener);
231+
}
215232
}
216233

217234
void fill_render(uint32_t rgba_value) {
@@ -234,6 +251,7 @@ struct RegionSamplingTest : ::testing::Test {
234251
sp<SurfaceControl> mBackgroundLayer;
235252
sp<SurfaceControl> mContentLayer;
236253
sp<SurfaceControl> mTopLayer;
254+
std::vector<gui::RegionSamplingDescriptor> mExistingListeners;
237255

238256
uint32_t const rgba_green = 0xFF00FF00;
239257
float const luma_green = 0.7152;

libs/gui/tests/Surface_test.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -858,11 +858,22 @@ class FakeSurfaceComposerAIDL : public gui::ISurfaceComposer {
858858
return binder::Status::ok();
859859
}
860860

861+
binder::Status addRegionSamplingListenerWithStopLayerId(
862+
const gui::ARect& /*samplingArea*/, const int32_t /*stopLayerId*/,
863+
const sp<gui::IRegionSamplingListener>& /*listener*/) override {
864+
return binder::Status::ok();
865+
}
866+
861867
binder::Status removeRegionSamplingListener(
862868
const sp<gui::IRegionSamplingListener>& /*listener*/) override {
863869
return binder::Status::ok();
864870
}
865871

872+
binder::Status getRegionSamplingListeners(
873+
std::vector<gui::RegionSamplingDescriptor>*) override {
874+
return binder::Status::ok();
875+
}
876+
866877
binder::Status addFpsListener(int32_t /*taskId*/,
867878
const sp<gui::IFpsListener>& /*listener*/) override {
868879
return binder::Status::ok();

services/surfaceflinger/RegionSamplingThread.cpp

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,25 @@ void RegionSamplingThread::removeListener(const sp<IRegionSamplingListener>& lis
141141
mDescriptors.erase(wp<IBinder>(IInterface::asBinder(listener)));
142142
}
143143

144+
const std::vector<gui::RegionSamplingDescriptor> RegionSamplingThread::getListeners() {
145+
std::lock_guard lock(mSamplingMutex);
146+
std::vector<gui::RegionSamplingDescriptor> listeners;
147+
for (const auto& [listener, descriptor] : mDescriptors) {
148+
gui::ARect guiRect;
149+
guiRect.left = descriptor.area.left;
150+
guiRect.top = descriptor.area.top;
151+
guiRect.right = descriptor.area.right;
152+
guiRect.bottom = descriptor.area.bottom;
153+
154+
gui::RegionSamplingDescriptor guiDescriptor;
155+
guiDescriptor.area = guiRect;
156+
guiDescriptor.stopLayerId = descriptor.stopLayerId;
157+
guiDescriptor.listener = descriptor.listener;
158+
listeners.emplace_back(guiDescriptor);
159+
}
160+
return listeners;
161+
}
162+
144163
void RegionSamplingThread::checkForStaleLuma() {
145164
std::lock_guard lock(mThreadControlMutex);
146165

services/surfaceflinger/RegionSamplingThread.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
#include <android-base/thread_annotations.h>
2020
#include <android/gui/IRegionSamplingListener.h>
21+
#include <android/gui/RegionSamplingDescriptor.h>
2122
#include <binder/IBinder.h>
2223
#include <renderengine/ExternalTexture.h>
2324
#include <ui/GraphicBuffer.h>
@@ -77,6 +78,8 @@ class RegionSamplingThread : public IBinder::DeathRecipient {
7778
const sp<IRegionSamplingListener>& listener);
7879
// Remove the listener to stop receiving median luma notifications.
7980
void removeListener(const sp<IRegionSamplingListener>& listener);
81+
// Gets all listeners that are receiving median luma notifications.
82+
const std::vector<gui::RegionSamplingDescriptor> getListeners();
8083

8184
// Notifies sampling engine that composition is done and new content is
8285
// available, and the deadline for the sampling work on the main thread to

services/surfaceflinger/SurfaceFlinger.cpp

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2056,6 +2056,18 @@ status_t SurfaceFlinger::addRegionSamplingListener(const Rect& samplingArea,
20562056
return NO_ERROR;
20572057
}
20582058

2059+
status_t SurfaceFlinger::addRegionSamplingListenerWithStopLayerId(
2060+
const Rect& samplingArea, const int32_t stopLayerId,
2061+
const sp<IRegionSamplingListener>& listener) {
2062+
if (!listener || samplingArea == Rect::INVALID_RECT || samplingArea.isEmpty()) {
2063+
return BAD_VALUE;
2064+
}
2065+
2066+
mRegionSamplingThread->addListener(samplingArea,
2067+
stopLayerId ? stopLayerId : UNASSIGNED_LAYER_ID, listener);
2068+
return NO_ERROR;
2069+
}
2070+
20592071
status_t SurfaceFlinger::removeRegionSamplingListener(const sp<IRegionSamplingListener>& listener) {
20602072
if (!listener) {
20612073
return BAD_VALUE;
@@ -2064,6 +2076,12 @@ status_t SurfaceFlinger::removeRegionSamplingListener(const sp<IRegionSamplingLi
20642076
return NO_ERROR;
20652077
}
20662078

2079+
status_t SurfaceFlinger::getRegionSamplingListeners(
2080+
std::vector<gui::RegionSamplingDescriptor>* listeners) const {
2081+
*listeners = mRegionSamplingThread->getListeners();
2082+
return NO_ERROR;
2083+
}
2084+
20672085
status_t SurfaceFlinger::addFpsListener(int32_t taskId, const sp<gui::IFpsListener>& listener) {
20682086
if (!listener) {
20692087
return BAD_VALUE;
@@ -9359,6 +9377,22 @@ binder::Status SurfaceComposerAIDL::addRegionSamplingListener(
93599377
return binderStatusFromStatusT(status);
93609378
}
93619379

9380+
binder::Status SurfaceComposerAIDL::addRegionSamplingListenerWithStopLayerId(
9381+
const gui::ARect& samplingArea, const int32_t stopLayerId,
9382+
const sp<gui::IRegionSamplingListener>& listener) {
9383+
status_t status = checkAccessPermission();
9384+
if (status != OK) {
9385+
return binderStatusFromStatusT(status);
9386+
}
9387+
android::Rect rect;
9388+
rect.left = samplingArea.left;
9389+
rect.top = samplingArea.top;
9390+
rect.right = samplingArea.right;
9391+
rect.bottom = samplingArea.bottom;
9392+
status = mFlinger->addRegionSamplingListenerWithStopLayerId(rect, stopLayerId, listener);
9393+
return binderStatusFromStatusT(status);
9394+
}
9395+
93629396
binder::Status SurfaceComposerAIDL::removeRegionSamplingListener(
93639397
const sp<gui::IRegionSamplingListener>& listener) {
93649398
status_t status = checkReadFrameBufferPermission();
@@ -9368,6 +9402,15 @@ binder::Status SurfaceComposerAIDL::removeRegionSamplingListener(
93689402
return binderStatusFromStatusT(status);
93699403
}
93709404

9405+
binder::Status SurfaceComposerAIDL::getRegionSamplingListeners(
9406+
std::vector<gui::RegionSamplingDescriptor>* listeners) {
9407+
status_t status = checkAccessPermission();
9408+
if (status == OK) {
9409+
status = mFlinger->getRegionSamplingListeners(listeners);
9410+
}
9411+
return binderStatusFromStatusT(status);
9412+
}
9413+
93719414
binder::Status SurfaceComposerAIDL::addFpsListener(int32_t taskId,
93729415
const sp<gui::IFpsListener>& listener) {
93739416
status_t status = checkReadFrameBufferPermission();

services/surfaceflinger/SurfaceFlinger.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -602,7 +602,12 @@ class SurfaceFlinger : public BnSurfaceComposer,
602602
status_t isWideColorDisplay(const sp<IBinder>& displayToken, bool* outIsWideColorDisplay) const;
603603
status_t addRegionSamplingListener(const Rect& samplingArea, const sp<IBinder>& stopLayerHandle,
604604
const sp<IRegionSamplingListener>& listener);
605+
status_t addRegionSamplingListenerWithStopLayerId(const Rect& samplingArea,
606+
const int32_t stopLayerId,
607+
const sp<IRegionSamplingListener>& listener);
605608
status_t removeRegionSamplingListener(const sp<IRegionSamplingListener>& listener);
609+
status_t getRegionSamplingListeners(
610+
std::vector<gui::RegionSamplingDescriptor>* listeners) const;
606611
status_t addFpsListener(int32_t taskId, const sp<gui::IFpsListener>& listener);
607612
status_t removeFpsListener(const sp<gui::IFpsListener>& listener);
608613
status_t addTunnelModeEnabledListener(const sp<gui::ITunnelModeEnabledListener>& listener);
@@ -1648,8 +1653,12 @@ class SurfaceComposerAIDL : public gui::BnSurfaceComposer {
16481653
binder::Status addRegionSamplingListener(
16491654
const gui::ARect& samplingArea, const sp<IBinder>& stopLayerHandle,
16501655
const sp<gui::IRegionSamplingListener>& listener) override;
1656+
binder::Status addRegionSamplingListenerWithStopLayerId(
1657+
const gui::ARect& samplingArea, const int32_t stopLayerId,
1658+
const sp<gui::IRegionSamplingListener>& listener) override;
16511659
binder::Status removeRegionSamplingListener(
16521660
const sp<gui::IRegionSamplingListener>& listener) override;
1661+
binder::Status getRegionSamplingListeners(std::vector<gui::RegionSamplingDescriptor>*) override;
16531662
binder::Status addFpsListener(int32_t taskId, const sp<gui::IFpsListener>& listener) override;
16541663
binder::Status removeFpsListener(const sp<gui::IFpsListener>& listener) override;
16551664
binder::Status addTunnelModeEnabledListener(

0 commit comments

Comments
 (0)