Repository navigation
gpu: use 1-byte pixel row alignment for texture upload and read back - #500
Merged
Merged
Conversation
Image buffers handed to the GLES path are tightly packed (each row is width * bytes-per-pixel), but create_texture() uploaded the YCbCr 420, 422, 444 and RGB888 formats with the default GL_UNPACK_ALIGNMENT of 4. For widths whose rows are not a multiple of 4 bytes, the driver then pads every row and reads past the end of the caller's buffer, and the texture content is skewed. Set GL_UNPACK_ALIGNMENT and GL_PACK_ALIGNMENT to 1 once when the context is created, so every upload and read back matches the buffer layout, and drop the per-call toggles the 400 format used. Add OpenGLContextTest.TextureUploadOfTightlyPackedRows, which uploads each format at widths 1 to 10 and checks the texture reads back exactly the source bytes.
There was a problem hiding this comment.
Code Review
This pull request configures the OpenGL context globally to use 1-byte row alignment for unpacking and packing during context initialization, and removes local alignment toggles in create_texture and read_texture. It also adds a new unit test TextureUploadOfTightlyPackedRows to verify texture upload and readback of tightly packed rows for various formats and widths. I have no feedback to provide as there are no review comments.
DichenZhang1
approved these changes
Oct 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The image buffers that
uhdr_opengl_ctxtuploads withglTexImage2D()are tightly packed: each row isw * bytes per pixellong. OpenGL ES reads them with the defaultGL_UNPACK_ALIGNMENTof 4, which assumes every row starts on a 4-byte boundary. For the 8-bit YCbCr formats (uploaded asGL_R8) and forRGB888, a width whose row size is not a multiple of 4 makes the driver read past the end of the buffer and shears the texture. Only8bppYCbCr400already set the alignment to 1 around its upload and read back.With GPU acceleration enabled this is reached by decoding, for example, a 4:4:4 base JPEG whose width is not a multiple of 4:
applyGainMapGLES()uploads the contiguous YUV planes and the driver reads beyond the allocation.This PR sets
GL_UNPACK_ALIGNMENTandGL_PACK_ALIGNMENTto 1 once, right after the library's own EGL context is made current ininit_opengl_ctxt(). Every texture upload and read back in the library goes through that context, so all formats now use packed rows and no call site has to remember to set it; the per-call toggles for8bppYCbCr400are removed. Formats whose rows are always a multiple of 4 bytes are unaffected.Test:
OpenGLContextTest.TextureUploadOfTightlyPackedRowsuploads 400, 420, 422, 444 and RGB888 buffers at widths 1 to 10, reads the textures back and checks they hold exactly the source bytes. Built with-DUHDR_ENABLE_GLES=1and AddressSanitizer on Mesa llvmpipe, the original code reports a heap-buffer-overflow increate_texture()on this test; with the change it passes, along with the rest of the unit tests.Side note, not changed here: under GLES the existing
EditorHelperTestcases for12bppYCbCr420passplanes[0]of an image whose Y, U and V planes are separate allocations tocreate_texture(), which expects the three planes in one buffer, so those cases read past the Y plane with or without this change.