Mesa (master): gallium/osmesa: Fix flushing and Y-flipping of the depth buffer.
GitLab Mirror
gitlab-mirror at kemper.freedesktop.org
Sat Dec 5 00:20:43 UTC 2020
Module: Mesa
Branch: master
Commit: c5c1aa7c75c05927017325829cb3f354654d0b73
URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=c5c1aa7c75c05927017325829cb3f354654d0b73
Author: Eric Anholt <eric at anholt.net>
Date: Tue Nov 10 16:51:36 2020 -0800
gallium/osmesa: Fix flushing and Y-flipping of the depth buffer.
We were returning a pointer to use-after-free the depth buffer, not
updating it in after future rendering, and also not y flipping it. A
little refactor to mostly reuse the color buffer's path makes it easy to
do it all right.
Adds a unit test to check for these bugs.
Closes: #885
Reviewed-by: Adam Jackson <ajax at redhat.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/7886>
---
src/gallium/frontends/osmesa/osmesa.c | 111 ++++++++++++++++-------------
src/gallium/targets/osmesa/test-render.cpp | 53 ++++++++++++++
2 files changed, 115 insertions(+), 49 deletions(-)
diff --git a/src/gallium/frontends/osmesa/osmesa.c b/src/gallium/frontends/osmesa/osmesa.c
index 42cc8256027..eae487c5c03 100644
--- a/src/gallium/frontends/osmesa/osmesa.c
+++ b/src/gallium/frontends/osmesa/osmesa.c
@@ -101,6 +101,13 @@ struct osmesa_context
struct osmesa_buffer *current_buffer;
+ /* Storage for depth/stencil, if the user has requested access. The backing
+ * driver always has its own storage for the actual depth/stencil, which we
+ * have to transfer in and out.
+ */
+ void *zs;
+ unsigned zs_stride;
+
enum pipe_format depth_stencil_format, accum_format;
GLenum format; /*< User-specified context format */
@@ -176,6 +183,43 @@ get_st_manager(void)
return stmgr;
}
+/* Reads the color or depth buffer from the backing context to either the user storage
+ * (color buffer) or our temporary (z/s)
+ */
+static void
+osmesa_read_buffer(OSMesaContext osmesa, struct pipe_resource *res, void *dst,
+ int dst_stride, bool y_up)
+{
+ struct pipe_context *pipe = osmesa->stctx->pipe;
+
+ struct pipe_box box;
+ u_box_2d(0, 0, res->width0, res->height0, &box);
+
+ struct pipe_transfer *transfer = NULL;
+ ubyte *src = pipe->transfer_map(pipe, res, 0, PIPE_MAP_READ, &box,
+ &transfer);
+
+ /*
+ * Copy the color buffer from the resource to the user's buffer.
+ */
+
+ if (y_up) {
+ /* need to flip image upside down */
+ dst = (ubyte *)dst + (res->height0 - 1) * dst_stride;
+ dst_stride = -dst_stride;
+ }
+
+ unsigned bpp = util_format_get_blocksize(res->format);
+ for (unsigned y = 0; y < res->height0; y++)
+ {
+ memcpy(dst, src, bpp * res->width0);
+ dst = (ubyte *)dst + dst_stride;
+ src += transfer->stride;
+ }
+
+ pipe->transfer_unmap(pipe, transfer);
+}
+
/**
* Given an OSMESA_x format and a GL_y type, return the best
@@ -316,13 +360,8 @@ osmesa_st_framebuffer_flush_front(struct st_context_iface *stctx,
{
OSMesaContext osmesa = OSMesaGetCurrentContext();
struct osmesa_buffer *osbuffer = stfbi_to_osbuffer(stfbi);
- struct pipe_context *pipe = stctx->pipe;
struct pipe_resource *res = osbuffer->textures[statt];
- struct pipe_transfer *transfer = NULL;
- struct pipe_box box;
- void *map;
- ubyte *src, *dst;
- unsigned y, bytes, bpp;
+ unsigned bpp;
int dst_stride;
if (osmesa->pp) {
@@ -347,37 +386,21 @@ osmesa_st_framebuffer_flush_front(struct st_context_iface *stctx,
pp_run(osmesa->pp, res, res, zsbuf);
}
- u_box_2d(0, 0, res->width0, res->height0, &box);
-
- map = pipe->transfer_map(pipe, res, 0, PIPE_MAP_READ, &box,
- &transfer);
-
- /*
- * Copy the color buffer from the resource to the user's buffer.
- */
+ /* Snapshot the color buffer to the user's buffer. */
bpp = util_format_get_blocksize(osbuffer->visual.color_format);
- src = map;
- dst = osbuffer->map;
if (osmesa->user_row_length)
dst_stride = bpp * osmesa->user_row_length;
else
dst_stride = bpp * osbuffer->width;
- bytes = bpp * res->width0;
- if (osmesa->y_up) {
- /* need to flip image upside down */
- dst = dst + (res->height0 - 1) * dst_stride;
- dst_stride = -dst_stride;
- }
+ osmesa_read_buffer(osmesa, res, osbuffer->map, dst_stride, osmesa->y_up);
- for (y = 0; y < res->height0; y++) {
- memcpy(dst, src, bytes);
- dst += dst_stride;
- src += transfer->stride;
+ /* If the user has requested the Z/S buffer, then snapshot that one too. */
+ if (osmesa->zs) {
+ osmesa_read_buffer(osmesa, osbuffer->textures[ST_ATTACHMENT_DEPTH_STENCIL],
+ osmesa->zs, osmesa->zs_stride, true);
}
- pipe->transfer_unmap(pipe, transfer);
-
return true;
}
@@ -730,6 +753,7 @@ OSMesaDestroyContext(OSMesaContext osmesa)
if (osmesa) {
pp_free(osmesa->pp);
osmesa->stctx->destroy(osmesa->stctx);
+ free(osmesa->zs);
FREE(osmesa);
}
}
@@ -914,33 +938,22 @@ OSMesaGetDepthBuffer(OSMesaContext c, GLint *width, GLint *height,
GLint *bytesPerValue, void **buffer)
{
struct osmesa_buffer *osbuffer = c->current_buffer;
- struct pipe_context *pipe = c->stctx->pipe;
struct pipe_resource *res = osbuffer->textures[ST_ATTACHMENT_DEPTH_STENCIL];
- struct pipe_transfer *transfer = NULL;
- struct pipe_box box;
-
- /*
- * Note: we can't really implement this function with gallium as
- * we did for swrast. We can't just map the resource and leave it
- * mapped (and there's no OSMesaUnmapDepthBuffer() function) so
- * we unmap the buffer here and return a 'stale' pointer. This should
- * actually be OK in most cases where the caller of this function
- * immediately uses the pointer.
- */
-
- u_box_2d(0, 0, res->width0, res->height0, &box);
-
- *buffer = pipe->transfer_map(pipe, res, 0, PIPE_MAP_READ, &box,
- &transfer);
- if (!*buffer) {
- return GL_FALSE;
- }
*width = res->width0;
*height = res->height0;
*bytesPerValue = util_format_get_blocksize(res->format);
- pipe->transfer_unmap(pipe, transfer);
+ if (!c->zs) {
+ c->zs_stride = *width * *bytesPerValue;
+ c->zs = calloc(c->zs_stride, *height);
+ if (!c->zs)
+ return GL_FALSE;
+
+ osmesa_read_buffer(c, res, c->zs, c->zs_stride, true);
+ }
+
+ *buffer = c->zs;
return GL_TRUE;
}
diff --git a/src/gallium/targets/osmesa/test-render.cpp b/src/gallium/targets/osmesa/test-render.cpp
index 2e5ff962d8e..1a720f0cd0e 100644
--- a/src/gallium/targets/osmesa/test-render.cpp
+++ b/src/gallium/targets/osmesa/test-render.cpp
@@ -162,3 +162,56 @@ INSTANTIATE_TEST_CASE_P(
),
name_params
);
+
+TEST(OSMesaRenderTest, depth)
+{
+ std::unique_ptr<osmesa_context, decltype(&OSMesaDestroyContext)> ctx{
+ OSMesaCreateContextExt(OSMESA_RGB_565, 24, 8, 0, NULL), &OSMesaDestroyContext};
+ ASSERT_TRUE(ctx);
+
+ int w = 3, h = 2;
+ uint8_t pixels[4096 * h * 2] = {0}; /* different cpp from our depth! */
+ auto ret = OSMesaMakeCurrent(ctx.get(), &pixels, GL_UNSIGNED_SHORT_5_6_5, w, h);
+ ASSERT_EQ(ret, GL_TRUE);
+
+ /* Expand the row length for the color buffer so we can see that it doesn't affect depth. */
+ OSMesaPixelStore(OSMESA_ROW_LENGTH, 4096);
+
+ uint32_t *depth;
+ GLint dw, dh, depth_cpp;
+ ASSERT_EQ(true, OSMesaGetDepthBuffer(ctx.get(), &dw, &dh, &depth_cpp, (void **)&depth));
+
+ ASSERT_EQ(dw, w);
+ ASSERT_EQ(dh, h);
+ ASSERT_EQ(depth_cpp, 4);
+
+ glClearDepth(1.0);
+ glClear(GL_DEPTH_BUFFER_BIT);
+ glFinish();
+ EXPECT_EQ(depth[w * 0 + 0], 0x00ffffff);
+ EXPECT_EQ(depth[w * 0 + 1], 0x00ffffff);
+ EXPECT_EQ(depth[w * 1 + 0], 0x00ffffff);
+ EXPECT_EQ(depth[w * 1 + 1], 0x00ffffff);
+
+ /* Scissor to the top half and clear */
+ glEnable(GL_SCISSOR_TEST);
+ glScissor(0, 1, 2, 1);
+ glClearDepth(0.0);
+ glClear(GL_DEPTH_BUFFER_BIT);
+ glFinish();
+ EXPECT_EQ(depth[w * 0 + 0], 0x00ffffff);
+ EXPECT_EQ(depth[w * 0 + 1], 0x00ffffff);
+ EXPECT_EQ(depth[w * 1 + 0], 0x00000000);
+ EXPECT_EQ(depth[w * 1 + 1], 0x00000000);
+
+ /* Y_UP didn't affect depth buffer orientation in classic osmesa. */
+ OSMesaPixelStore(OSMESA_Y_UP, false);
+ glScissor(0, 1, 1, 1);
+ glClearDepth(1.0);
+ glClear(GL_DEPTH_BUFFER_BIT);
+ glFinish();
+ EXPECT_EQ(depth[w * 0 + 0], 0x00ffffff);
+ EXPECT_EQ(depth[w * 0 + 1], 0x00ffffff);
+ EXPECT_EQ(depth[w * 1 + 0], 0x00ffffff);
+ EXPECT_EQ(depth[w * 1 + 1], 0x00000000);
+}
More information about the mesa-commit
mailing list