Closed Bug 697990 Opened 8 years ago Closed 8 years ago

fTexSubImage2D row by row is too slow


(Core :: Graphics, defect, critical)

Not set





(Reporter: BenWa, Assigned: ajuma)




(1 file, 1 obsolete file)

We can GL_UNPACK_ROW_LENGTH when rowLength > mMaxTextureSize because we get driver crashes even if the documentation for GL_UNPACK_ROW_LENGTH doesn't state this. To overcome this we upload the image row by row using TexSubImage2D. This ruins performance when we hit this case.

ajuma is investigating work a around.
Duplicate of this bug: 683411
It turns out that we even get crashes (on Tegra devices) when using GL_UNPACK_ROW_LENGTH with rowLength <= mMaxTextureSize. The problem seems to be a buggy implementation of fTexSubImage2D when using GL_UNPACK_ROW_LENGTH. Specifically, when we set GL_UNPACK_ROW_LENGTH to be rowLength and then attempt to upload an area of width w and height h, the driver seems to be attempting to read h*rowLength pixels from the source buffer, when it should really only be reading (h-1)*rowLength+w pixels. This can lead to the driver reading past the end of the source buffer.

Here's an example to make this discussion more concrete. Say we have a source buffer for a 100x100 surface, and we want to upload the right half of this buffer. We set GL_UNPACK_ROW_LENGTH to 100, and then call fTexSubImage2D with width 50, height 100, and a pointer to the 51st pixel in row 1 of our source buffer. If the driver attempts to read 100*100 pixels starting at the given pointer, it's going to read 50 pixels past the end of the buffer.

If we modify the fTexSubImage2D call to upload one less row, we no longer crash here (even when rowLength > mMaxTextureSize), since the extra pixels read by the driver are still within our source buffer.
OS: Mac OS X → Android
Hardware: x86 → ARM
This lets us use GL_UNPACK_ROW_LENGTH (avoiding row-by-row fTexSubImage2D) while working around the driver bug. We split the upload into two calls to fTexSubImage2D: we upload the last row on its own (without using GL_UNPACK_ROW_LENGTH) and then we upload the remaining rows (using GL_UNPACK_ROW_LENGTH).
Attachment #570438 - Flags: review?(jmuizelaar)
Patch looks good to me. Thanks for getting it done on a friday night!

Should we file a follow up bug to implement the memcpy solution for devices where the UNPACK ext isn't available such as the PowerVR?
(In reply to Benoit Girard (:BenWa) from comment #5)
> Should we file a follow up bug to implement the memcpy solution for devices
> where the UNPACK ext isn't available such as the PowerVR?

Great idea, filed Bug 698197.
This puts the logic for the workaround into a separate function called TexSubImage2DWithUnpackSubimageGLES. I've also folded in the patch from Bug 698197 (which deals with the situation where GL_UNPACK_ROW_LENGTH isn't available) and put that logic into its own function called TexSubImage2DWithoutUnpackSubimage.
Attachment #570438 - Attachment is obsolete: true
Attachment #570438 - Flags: review?(jmuizelaar)
Attachment #572604 - Flags: review?(jmuizelaar)
Attachment #572604 - Flags: review?(jmuizelaar) → review+
Comment on attachment 572604 [details] [diff] [review]
Clean up GLES-specific workarounds for GL_UNPACK_ROW_LENGTH.

Maybe TexSubImage2DWithMemcpy might be a better name than TexSubImage2DWithoutUnpackSubimage
Duplicate of this bug: 700458
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Duplicate of this bug: 698197
You need to log in before you can comment on or make changes to this bug.