Skip to content

Commit 3e52318

Browse files
i-nazarov-ukraineAngle LUCI CQ
authored andcommitted
Vulkan: Ensure always using resolved Window Surface size
`WindowSurfaceVk::getWidth/Height()` methods return cached, previously resolved Surface size. Using these methods while current Window Surface size is unresolved may return stale values, causing undesired behavior. Appropriate ASSERTs were added to these methods to prevent such usage. Added ASSERTs revealed few places with incorrect usage: - In `Context::makeCurrent()` to set initial viewport or for capture. - In `IsPartialBlit()` and `ValidateReadPixelsBase()` validations. - In `SerializeFramebufferAttachment()` during capture. Rest of the code was thoroughly checked if it is possible to call `WindowSurfaceVk::getWidth/Height()` when size is unresolved. All other places always call these methods after framebuffer state synchronization, which acquires swapchain images and resolves the surface size. Added `ensureSizeResolved()` method that is called during validation and in the `SerializeFramebufferAttachment()` method. It is possible to use existing `Framebuffer::syncState()` method as alternative, but this solution was discarded since it may potentially interfere with `State::syncDirtyObjects()` method. The `Surface::getUserSize()` replaces old methods as optimization, to prevent calling relatively expensive method twice from `Context::makeCurrent()` to get width and height of the `drawSurface`. Test: angle_trace_tests --gtest_filter=EGLSurfaceTest.ResizeBeforeMakeCurrent/* Test: angle_trace_tests --gtest_filter=EGLSurfaceTest.ResizeBeforeMakeCurrentPostSizeQuery/* Test: angle_trace_tests --gtest_filter=EGLSurfaceTest.ResizeAndReadPixelsRobustANGLE/* Test: angle_trace_tests --gtest_filter=EGLSurfaceTest.ResizeAndBlitFramebufferANGLE/* Bug: angleproject:397848903 Change-Id: I082e13d0b8db5fd7d08ff25b102df1f283e1256d Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/6792928 Commit-Queue: Igor Nazarov <[email protected]> Reviewed-by: Cody Northrop <[email protected]> Reviewed-by: Shahbaz Youssefi <[email protected]>
1 parent 11120f4 commit 3e52318

19 files changed

+537
-168
lines changed

src/libANGLE/Context.cpp

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1047,6 +1047,15 @@ egl::Error Context::makeCurrent(egl::Display *display,
10471047
{
10481048
mDisplay = display;
10491049

1050+
EGLint width = 0;
1051+
EGLint height = 0;
1052+
1053+
angle::FrameCaptureShared *frameCaptureShared = getShareGroup()->getFrameCaptureShared();
1054+
if ((frameCaptureShared->enabled() || !mHasBeenCurrent) && drawSurface != nullptr)
1055+
{
1056+
ANGLE_TRY(drawSurface->getUserSize(display, &width, &height));
1057+
}
1058+
10501059
if (!mHasBeenCurrent)
10511060
{
10521061
initializeDefaultResources();
@@ -1055,14 +1064,6 @@ egl::Error Context::makeCurrent(egl::Display *display,
10551064
initVersionStrings();
10561065
initExtensionStrings();
10571066

1058-
int width = 0;
1059-
int height = 0;
1060-
if (drawSurface != nullptr)
1061-
{
1062-
width = drawSurface->getWidth();
1063-
height = drawSurface->getHeight();
1064-
}
1065-
10661067
ContextPrivateViewport(getMutablePrivateState(), getMutablePrivateStateCache(), 0, 0, width,
10671068
height);
10681069
ContextPrivateScissor(getMutablePrivateState(), getMutablePrivateStateCache(), 0, 0, width,
@@ -1073,7 +1074,7 @@ egl::Error Context::makeCurrent(egl::Display *display,
10731074

10741075
ANGLE_TRY(unsetDefaultFramebuffer());
10751076

1076-
getShareGroup()->getFrameCaptureShared()->onMakeCurrent(this, drawSurface);
1077+
frameCaptureShared->onMakeCurrent(this, drawSurface, width, height);
10771078

10781079
// TODO(jmadill): Rework this when we support ContextImpl
10791080
mState.setAllDirtyBits();

src/libANGLE/FramebufferAttachment.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,8 @@ class FramebufferAttachment final
120120
bool isRenderToTexture() const;
121121
GLsizei getRenderToTextureSamples() const;
122122

123+
// Explicitly resolves attachment size to use before state synchronization (e.g. validation).
124+
angle::Result ensureSizeResolved(const Context *context) const;
123125
// The size of the underlying resource the attachment points to. The 'depth' value will
124126
// correspond to a 3D texture depth or the layer count of a 2D array texture. For Surfaces and
125127
// Renderbuffers, it will always be 1.
@@ -224,6 +226,7 @@ class FramebufferAttachmentObject : public angle::Subject, public angle::Observe
224226
FramebufferAttachmentObject();
225227
~FramebufferAttachmentObject() override;
226228

229+
virtual angle::Result ensureSizeResolved(const Context *context) const = 0;
227230
virtual bool isAttachmentSpecified(const ImageIndex &imageIndex) const = 0;
228231
virtual Extents getAttachmentSize(const ImageIndex &imageIndex) const = 0;
229232
virtual Format getAttachmentFormat(GLenum binding, const ImageIndex &imageIndex) const = 0;
@@ -268,6 +271,12 @@ inline const ImageIndex &FramebufferAttachment::getTextureImageIndex() const
268271
return mTarget.textureIndex();
269272
}
270273

274+
inline angle::Result FramebufferAttachment::ensureSizeResolved(const Context *context) const
275+
{
276+
ASSERT(mResource);
277+
return mResource->ensureSizeResolved(context);
278+
}
279+
271280
inline bool FramebufferAttachment::isSpecified() const
272281
{
273282
ASSERT(mResource);

src/libANGLE/Image.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,11 @@ void ImageSibling::setSourceEGLImageInitState(gl::InitState initState) const
129129
mTargetOf->setInitState(initState);
130130
}
131131

132+
angle::Result ImageSibling::ensureSizeResolved(const gl::Context *context) const
133+
{
134+
return angle::Result::Continue;
135+
}
136+
132137
bool ImageSibling::isAttachmentSpecified(const gl::ImageIndex &imageIndex) const
133138
{
134139
return !getAttachmentSize(imageIndex).empty();

src/libANGLE/Image.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ class ImageSibling : public gl::FramebufferAttachmentObject
4949
gl::InitState sourceEGLImageInitState() const;
5050
void setSourceEGLImageInitState(gl::InitState initState) const;
5151

52+
angle::Result ensureSizeResolved(const gl::Context *context) const override;
5253
bool isAttachmentSpecified(const gl::ImageIndex &imageIndex) const override;
5354
bool isRenderable(const gl::Context *context,
5455
GLenum binding,

src/libANGLE/Surface.cpp

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -543,29 +543,24 @@ EGLint Surface::getHeight() const
543543
return mFixedSize ? static_cast<EGLint>(mFixedHeight) : mImplementation->getHeight();
544544
}
545545

546-
egl::Error Surface::getUserWidth(const egl::Display *display, EGLint *value) const
546+
egl::Error Surface::getUserSize(const egl::Display *display, EGLint *width, EGLint *height) const
547547
{
548+
ASSERT(width != nullptr || height != nullptr);
548549
if (mFixedSize)
549550
{
550-
*value = static_cast<EGLint>(mFixedWidth);
551-
return NoError();
552-
}
553-
else
554-
{
555-
return mImplementation->getUserWidth(display, value);
556-
}
557-
}
558-
559-
egl::Error Surface::getUserHeight(const egl::Display *display, EGLint *value) const
560-
{
561-
if (mFixedSize)
562-
{
563-
*value = static_cast<EGLint>(mFixedHeight);
551+
if (width != nullptr)
552+
{
553+
*width = static_cast<EGLint>(mFixedWidth);
554+
}
555+
if (height != nullptr)
556+
{
557+
*height = static_cast<EGLint>(mFixedHeight);
558+
}
564559
return NoError();
565560
}
566561
else
567562
{
568-
return mImplementation->getUserHeight(display, value);
563+
return mImplementation->getUserSize(display, width, height);
569564
}
570565
}
571566

@@ -617,6 +612,11 @@ Error Surface::releaseTexImageFromTexture(const gl::Context *context)
617612
return releaseRef(context->getDisplay());
618613
}
619614

615+
angle::Result Surface::ensureSizeResolved(const gl::Context *context) const
616+
{
617+
return mImplementation->ensureSizeResolved(context);
618+
}
619+
620620
bool Surface::isAttachmentSpecified(const gl::ImageIndex & /*imageIndex*/) const
621621
{
622622
// Surface is always specified even if it has 0 sizes.

src/libANGLE/Surface.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -113,11 +113,11 @@ class Surface : public LabeledObject, public gl::FramebufferAttachmentObject
113113
const Config *getConfig() const;
114114

115115
// width and height can change with client window resizing
116+
// Size must be resolved before the call either during state synchronization or explicitly.
116117
EGLint getWidth() const;
117118
EGLint getHeight() const;
118-
// Sizes that Surface will have after render target is first accessed (e.g. after draw).
119-
egl::Error getUserWidth(const egl::Display *display, EGLint *value) const;
120-
egl::Error getUserHeight(const egl::Display *display, EGLint *value) const;
119+
// Unresolved Surface size until render target is first accessed (e.g. after draw).
120+
egl::Error getUserSize(const egl::Display *display, EGLint *width, EGLint *height) const;
121121
EGLint getPixelAspectRatio() const;
122122
EGLenum getRenderBuffer() const;
123123
EGLenum getRequestedRenderBuffer() const;
@@ -161,6 +161,8 @@ class Surface : public LabeledObject, public gl::FramebufferAttachmentObject
161161
EGLint isFixedSize() const;
162162

163163
// FramebufferAttachmentObject implementation
164+
// Explicitly resolves surface size to use before state synchronization (e.g. validation).
165+
angle::Result ensureSizeResolved(const gl::Context *context) const override;
164166
bool isAttachmentSpecified(const gl::ImageIndex &imageIndex) const override;
165167
gl::Extents getAttachmentSize(const gl::ImageIndex &imageIndex) const override;
166168
gl::Format getAttachmentFormat(GLenum binding, const gl::ImageIndex &imageIndex) const override;

src/libANGLE/capture/FrameCapture.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8405,7 +8405,10 @@ void FrameCaptureShared::onDestroyContext(const gl::Context *context)
84058405
}
84068406
}
84078407

8408-
void FrameCaptureShared::onMakeCurrent(const gl::Context *context, const egl::Surface *drawSurface)
8408+
void FrameCaptureShared::onMakeCurrent(const gl::Context *context,
8409+
const egl::Surface *drawSurface,
8410+
EGLint surfaceWidth,
8411+
EGLint surfaceHeight)
84098412
{
84108413
if (!drawSurface)
84118414
{
@@ -8414,7 +8417,7 @@ void FrameCaptureShared::onMakeCurrent(const gl::Context *context, const egl::Su
84148417

84158418
// Track the width, height and color space of the draw surface as provided to makeCurrent
84168419
SurfaceParams &params = mDrawSurfaceParams[context->id()];
8417-
params.extents = gl::Extents(drawSurface->getWidth(), drawSurface->getHeight(), 1);
8420+
params.extents = gl::Extents(surfaceWidth, surfaceHeight, 1);
84188421
params.colorSpace = egl::FromEGLenum<egl::ColorSpace>(drawSurface->getGLColorspace());
84198422
}
84208423

src/libANGLE/capture/FrameCapture.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -749,7 +749,10 @@ class FrameCaptureShared final : angle::NonCopyable
749749
void onEndFrame(gl::Context *context);
750750
void onDestroyContext(const gl::Context *context);
751751
bool onEndCLCapture();
752-
void onMakeCurrent(const gl::Context *context, const egl::Surface *drawSurface);
752+
void onMakeCurrent(const gl::Context *context,
753+
const egl::Surface *drawSurface,
754+
EGLint surfaceWidth,
755+
EGLint surfaceHeight);
753756
bool enabled() const { return mEnabled; }
754757

755758
bool isCapturing() const;

src/libANGLE/capture/FrameCapture_mock.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,10 @@ FrameCapture::~FrameCapture() {}
4343
FrameCaptureShared::FrameCaptureShared() : mEnabled(false) {}
4444
FrameCaptureShared::~FrameCaptureShared() {}
4545
void FrameCaptureShared::onEndFrame(gl::Context *context) {}
46-
void FrameCaptureShared::onMakeCurrent(const gl::Context *context, const egl::Surface *drawSurface)
46+
void FrameCaptureShared::onMakeCurrent(const gl::Context *context,
47+
const egl::Surface *drawSurface,
48+
EGLint surfaceWidth,
49+
EGLint surfaceHeight)
4750
{}
4851
void FrameCaptureShared::onDestroyContext(const gl::Context *context) {}
4952
void *FrameCaptureShared::maybeGetShadowMemoryPointer(gl::Buffer *buffer,

src/libANGLE/capture/serialize.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,9 @@ Result SerializeFramebufferAttachment(const gl::Context *context,
291291
json->addScalar("ViewIndex", framebufferAttachment.getBaseViewIndex());
292292
json->addScalar("Samples", framebufferAttachment.getRenderToTextureSamples());
293293

294+
// Need to resolve the size before getting it below.
295+
ANGLE_TRY(framebufferAttachment.ensureSizeResolved(context));
296+
294297
{
295298
GroupScope extentsGroup(json, "Extents");
296299
SerializeExtents(json, framebufferAttachment.getSize());

0 commit comments

Comments
 (0)