697990 - fTexSubImage2D row by row is too slow

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

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.

Comment 1

[Benoit Girard (:BenWa)]

We can't use* GL_UNPACK_ROW_LENGTH

Comment 3

[Ali Juma [:ajuma]]

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.

Comment 4

[Ali Juma [:ajuma]]

Attachment: Work around crashes on Tegra when using GL_UNPACK_ROW_LENGTH.

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).

Comment 5

[Benoit Girard (:BenWa)]

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?

Comment 6

[Ali Juma [:ajuma]]

(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.

Comment 7

[Ali Juma [:ajuma]]

Attachment: Clean up GLES-specific workarounds for GL_UNPACK_ROW_LENGTH.

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.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

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

Comment 10

[Ali Juma [:ajuma]]

https://hg.mozilla.org/mozilla-central/rev/53f4c8abf558