From 9d1abea931aaacf2354cc49b3d9a484de2693e93 Mon Sep 17 00:00:00 2001 From: Chia-I Wu Date: Thu, 23 Aug 2018 13:44:43 -0700 Subject: surfaceflinger: reorder width and height in RenderArea ctor Height is before width only in a crazy world. Bug: 113041375 Test: take screenshot, rotate screen, screencap Change-Id: Ia10b26dbba9a6a91abb5dae9fbe20bf17cd3e78f --- services/surfaceflinger/SurfaceFlinger.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'services/surfaceflinger/SurfaceFlinger.cpp') diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 37d014686e..d4f384f934 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -4915,7 +4915,7 @@ status_t SurfaceFlinger::captureScreen(const sp& displayToken, } } - DisplayRenderArea renderArea(display, sourceCrop, reqHeight, reqWidth, rotation); + DisplayRenderArea renderArea(display, sourceCrop, reqWidth, reqHeight, rotation); auto traverseLayers = std::bind(std::mem_fn(&SurfaceFlinger::traverseLayersInDisplay), this, display, minLayerZ, maxLayerZ, std::placeholders::_1); @@ -4931,7 +4931,7 @@ status_t SurfaceFlinger::captureLayers(const sp& layerHandleBinder, public: LayerRenderArea(SurfaceFlinger* flinger, const sp& layer, const Rect crop, int32_t reqWidth, int32_t reqHeight, bool childrenOnly) - : RenderArea(reqHeight, reqWidth, CaptureFill::CLEAR), + : RenderArea(reqWidth, reqHeight, CaptureFill::CLEAR), mLayer(layer), mCrop(crop), mFlinger(flinger), -- cgit v1.2.3-59-g8ed1b From c80dcbb574106f3ff126d6b77cbcd12463089301 Mon Sep 17 00:00:00 2001 From: Chia-I Wu Date: Fri, 24 Aug 2018 15:34:02 -0700 Subject: surfaceflinger: remove ISurfaceComposer.h from RenderArea RenderArea can be made more generic by removing ISurfaceComposer.h dependency. The caller also prefers to work with ui::Transform::orientation_flags than ISurfaceComposer::Rotation (we want to move updateDimensions to the caller). Bug: 113041375 Test: take screenshot, rotate screen, screencap Change-Id: I16e1392d5c92c2f423f98307e867918415404d26 --- services/surfaceflinger/DisplayDevice.h | 5 ++--- services/surfaceflinger/RenderArea.cpp | 21 --------------------- services/surfaceflinger/RenderArea.h | 12 +++++++----- services/surfaceflinger/SurfaceFlinger.cpp | 19 ++++++++++++++++++- 4 files changed, 27 insertions(+), 30 deletions(-) (limited to 'services/surfaceflinger/SurfaceFlinger.cpp') diff --git a/services/surfaceflinger/DisplayDevice.h b/services/surfaceflinger/DisplayDevice.h index 928f1cee76..42909880c7 100644 --- a/services/surfaceflinger/DisplayDevice.h +++ b/services/surfaceflinger/DisplayDevice.h @@ -25,7 +25,6 @@ #include #include -#include #include #include #include @@ -334,11 +333,11 @@ private: class DisplayRenderArea : public RenderArea { public: DisplayRenderArea(const sp device, - ISurfaceComposer::Rotation rotation = ISurfaceComposer::eRotateNone) + ui::Transform::orientation_flags rotation = ui::Transform::ROT_0) : DisplayRenderArea(device, device->getBounds(), device->getWidth(), device->getHeight(), rotation) {} DisplayRenderArea(const sp device, Rect sourceCrop, uint32_t reqWidth, - uint32_t reqHeight, ISurfaceComposer::Rotation rotation) + uint32_t reqHeight, ui::Transform::orientation_flags rotation) : RenderArea(reqWidth, reqHeight, CaptureFill::OPAQUE, rotation), mDevice(device), mSourceCrop(sourceCrop) {} diff --git a/services/surfaceflinger/RenderArea.cpp b/services/surfaceflinger/RenderArea.cpp index 918bbd3a47..0a7a546f66 100644 --- a/services/surfaceflinger/RenderArea.cpp +++ b/services/surfaceflinger/RenderArea.cpp @@ -4,27 +4,6 @@ namespace android { -ui::Transform::orientation_flags fromRotation(ISurfaceComposer::Rotation rotation) { - switch (rotation) { - case ISurfaceComposer::eRotateNone: - return ui::Transform::ROT_0; - case ISurfaceComposer::eRotate90: - return ui::Transform::ROT_90; - case ISurfaceComposer::eRotate180: - return ui::Transform::ROT_180; - case ISurfaceComposer::eRotate270: - return ui::Transform::ROT_270; - } - ALOGE("Invalid rotation passed to captureScreen(): %d\n", rotation); - return ui::Transform::ROT_0; -} - -RenderArea::RenderArea(uint32_t reqWidth, uint32_t reqHeight, CaptureFill captureFill, - ISurfaceComposer::Rotation rotation) - : mReqWidth(reqWidth), mReqHeight(reqHeight), mCaptureFill(captureFill) { - mRotationFlags = fromRotation(rotation); -} - float RenderArea::getCaptureFillValue(CaptureFill captureFill) { switch(captureFill) { case CaptureFill::CLEAR: diff --git a/services/surfaceflinger/RenderArea.h b/services/surfaceflinger/RenderArea.h index 61abaec3a2..c12ff45e41 100644 --- a/services/surfaceflinger/RenderArea.h +++ b/services/surfaceflinger/RenderArea.h @@ -1,7 +1,5 @@ #pragma once -#include -#include #include #include @@ -21,7 +19,11 @@ public: static float getCaptureFillValue(CaptureFill captureFill); RenderArea(uint32_t reqWidth, uint32_t reqHeight, CaptureFill captureFill, - ISurfaceComposer::Rotation rotation = ISurfaceComposer::eRotateNone); + ui::Transform::orientation_flags rotation = ui::Transform::ROT_0) + : mReqWidth(reqWidth), + mReqHeight(reqHeight), + mCaptureFill(captureFill), + mRotationFlags(rotation) {} virtual ~RenderArea() = default; @@ -73,8 +75,8 @@ public: private: uint32_t mReqWidth; uint32_t mReqHeight; - ui::Transform::orientation_flags mRotationFlags; - CaptureFill mCaptureFill; + const CaptureFill mCaptureFill; + const ui::Transform::orientation_flags mRotationFlags; }; } // namespace android diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index d4f384f934..2dc55c637f 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -143,6 +143,21 @@ bool isWideColorMode(const ColorMode colorMode) { return false; } +ui::Transform::orientation_flags fromSurfaceComposerRotation(ISurfaceComposer::Rotation rotation) { + switch (rotation) { + case ISurfaceComposer::eRotateNone: + return ui::Transform::ROT_0; + case ISurfaceComposer::eRotate90: + return ui::Transform::ROT_90; + case ISurfaceComposer::eRotate180: + return ui::Transform::ROT_180; + case ISurfaceComposer::eRotate270: + return ui::Transform::ROT_270; + } + ALOGE("Invalid rotation passed to captureScreen(): %d\n", rotation); + return ui::Transform::ROT_0; +} + #pragma clang diagnostic pop class ConditionalLock { @@ -4902,6 +4917,8 @@ status_t SurfaceFlinger::captureScreen(const sp& displayToken, if (!displayToken) return BAD_VALUE; + auto renderAreaRotation = fromSurfaceComposerRotation(rotation); + const auto display = getDisplayDeviceLocked(displayToken); if (!display) return BAD_VALUE; @@ -4915,7 +4932,7 @@ status_t SurfaceFlinger::captureScreen(const sp& displayToken, } } - DisplayRenderArea renderArea(display, sourceCrop, reqWidth, reqHeight, rotation); + DisplayRenderArea renderArea(display, sourceCrop, reqWidth, reqHeight, renderAreaRotation); auto traverseLayers = std::bind(std::mem_fn(&SurfaceFlinger::traverseLayersInDisplay), this, display, minLayerZ, maxLayerZ, std::placeholders::_1); -- cgit v1.2.3-59-g8ed1b From 20261cb526480d8a98f4c393ac0f28cf0a250892 Mon Sep 17 00:00:00 2001 From: Chia-I Wu Date: Thu, 23 Aug 2018 12:55:44 -0700 Subject: surfaceflinger: fix race conditions in captureScreen The display was looked up without holding the state lock. It was also used (including indirectly from updateDimensions) without holding the lock either. Inline updateDimensions in captureScreen with the state lock held. Stop calling updateDimensions in captureLayers. Bug: 113041375 Test: take screenshot, rotate screen, screencap Change-Id: I8b361847c44373ce08930d906ec4a995efd1c21b --- services/surfaceflinger/RenderArea.cpp | 34 ------------------ services/surfaceflinger/RenderArea.h | 6 ++-- services/surfaceflinger/SurfaceFlinger.cpp | 56 ++++++++++++++++++++++++------ 3 files changed, 47 insertions(+), 49 deletions(-) (limited to 'services/surfaceflinger/SurfaceFlinger.cpp') diff --git a/services/surfaceflinger/RenderArea.cpp b/services/surfaceflinger/RenderArea.cpp index 0a7a546f66..93759e8ad7 100644 --- a/services/surfaceflinger/RenderArea.cpp +++ b/services/surfaceflinger/RenderArea.cpp @@ -1,7 +1,5 @@ #include "RenderArea.h" -#include - namespace android { float RenderArea::getCaptureFillValue(CaptureFill captureFill) { @@ -13,37 +11,5 @@ float RenderArea::getCaptureFillValue(CaptureFill captureFill) { return 1.0f; } } -/* - * Checks that the requested width and height are valid and updates them to the render area - * dimensions if they are set to 0 - */ -status_t RenderArea::updateDimensions(int displayRotation) { - // get screen geometry - - uint32_t width = getWidth(); - uint32_t height = getHeight(); - - if (mRotationFlags & ui::Transform::ROT_90) { - std::swap(width, height); - } - - if (displayRotation & DisplayState::eOrientationSwapMask) { - std::swap(width, height); - } - - if ((mReqWidth > width) || (mReqHeight > height)) { - ALOGE("size mismatch (%d, %d) > (%d, %d)", mReqWidth, mReqHeight, width, height); - return BAD_VALUE; - } - - if (mReqWidth == 0) { - mReqWidth = width; - } - if (mReqHeight == 0) { - mReqHeight = height; - } - - return NO_ERROR; -} } // namespace android diff --git a/services/surfaceflinger/RenderArea.h b/services/surfaceflinger/RenderArea.h index c12ff45e41..3c11e73199 100644 --- a/services/surfaceflinger/RenderArea.h +++ b/services/surfaceflinger/RenderArea.h @@ -70,11 +70,9 @@ public: // covered by any rendered layer should be filled with this color. CaptureFill getCaptureFill() const { return mCaptureFill; }; - status_t updateDimensions(int displayRotation); - private: - uint32_t mReqWidth; - uint32_t mReqHeight; + const uint32_t mReqWidth; + const uint32_t mReqHeight; const CaptureFill mCaptureFill; const ui::Transform::orientation_flags mRotationFlags; }; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 2dc55c637f..5acfe59c4e 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -4919,16 +4919,44 @@ status_t SurfaceFlinger::captureScreen(const sp& displayToken, auto renderAreaRotation = fromSurfaceComposerRotation(rotation); - const auto display = getDisplayDeviceLocked(displayToken); - if (!display) return BAD_VALUE; + sp display; + { + Mutex::Autolock _l(mStateLock); + + display = getDisplayDeviceLocked(displayToken); + if (!display) return BAD_VALUE; + + const Rect& dispScissor = display->getScissor(); + if (!dispScissor.isEmpty()) { + sourceCrop.set(dispScissor); + // adb shell screencap will default reqWidth and reqHeight to zeros. + if (reqWidth == 0 || reqHeight == 0) { + reqWidth = uint32_t(display->getViewport().width()); + reqHeight = uint32_t(display->getViewport().height()); + } + } + + // get screen geometry + uint32_t width = display->getWidth(); + uint32_t height = display->getHeight(); + + if (renderAreaRotation & ui::Transform::ROT_90) { + std::swap(width, height); + } - const Rect& dispScissor = display->getScissor(); - if (!dispScissor.isEmpty()) { - sourceCrop.set(dispScissor); - // adb shell screencap will default reqWidth and reqHeight to zeros. - if (reqWidth == 0 || reqHeight == 0) { - reqWidth = uint32_t(display->getViewport().width()); - reqHeight = uint32_t(display->getViewport().height()); + if (mPrimaryDisplayOrientation & DisplayState::eOrientationSwapMask) { + std::swap(width, height); + } + + if ((reqWidth > width) || (reqHeight > height)) { + ALOGE("size mismatch (%d, %d) > (%d, %d)", reqWidth, reqHeight, width, height); + } else { + if (reqWidth == 0) { + reqWidth = width; + } + if (reqHeight == 0) { + reqHeight = height; + } } } @@ -5040,6 +5068,14 @@ status_t SurfaceFlinger::captureLayers(const sp& layerHandleBinder, int32_t reqWidth = crop.width() * frameScale; int32_t reqHeight = crop.height() * frameScale; + // really small crop or frameScale + if (reqWidth <= 0) { + reqWidth = 1; + } + if (reqHeight <= 0) { + reqHeight = 1; + } + LayerRenderArea renderArea(this, parent, crop, reqWidth, reqHeight, childrenOnly); auto traverseLayers = [parent, childrenOnly](const LayerVector::Visitor& visitor) { @@ -5061,8 +5097,6 @@ status_t SurfaceFlinger::captureScreenCommon(RenderArea& renderArea, bool useIdentityTransform) { ATRACE_CALL(); - renderArea.updateDimensions(mPrimaryDisplayOrientation); - const uint32_t usage = GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_OFTEN | GRALLOC_USAGE_HW_RENDER | GRALLOC_USAGE_HW_TEXTURE; *outBuffer = new GraphicBuffer(renderArea.getReqWidth(), renderArea.getReqHeight(), -- cgit v1.2.3-59-g8ed1b