Last Comment Bug 697990 - fTexSubImage2D row by row is too slow
: fTexSubImage2D row by row is too slow
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: ARM Android
: -- critical (vote)
: mozilla10
Assigned To: Ali Juma [:ajuma]
:
Mentors:
: 683411 698197 700458 (view as bug list)
Depends on:
Blocks: opengl-mobile
  Show dependency treegraph
 
Reported: 2011-10-28 08:19 PDT by Benoit Girard (:BenWa)
Modified: 2011-11-08 14:18 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Work around crashes on Tegra when using GL_UNPACK_ROW_LENGTH. (3.78 KB, patch)
2011-10-28 18:17 PDT, Ali Juma [:ajuma]
no flags Details | Diff | Splinter Review
Clean up GLES-specific workarounds for GL_UNPACK_ROW_LENGTH. (12.88 KB, patch)
2011-11-07 13:51 PST, Ali Juma [:ajuma]
jmuizelaar: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2011-10-28 08:19:46 PDT
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) 2011-10-28 08:20:45 PDT
We can't use* GL_UNPACK_ROW_LENGTH
Comment 2 Benoit Girard (:BenWa) 2011-10-28 15:00:29 PDT
*** Bug 683411 has been marked as a duplicate of this bug. ***
Comment 3 Ali Juma [:ajuma] 2011-10-28 18:07:30 PDT
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] 2011-10-28 18:17:12 PDT
Created attachment 570438 [details] [diff] [review]
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) 2011-10-28 19:15:19 PDT
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] 2011-10-29 08:19:43 PDT
(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] 2011-11-07 13:51:17 PST
Created attachment 572604 [details] [diff] [review]
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] 2011-11-07 14:35:08 PST
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 9 Ali Juma [:ajuma] 2011-11-08 07:13:00 PST
*** Bug 700458 has been marked as a duplicate of this bug. ***
Comment 10 Ali Juma [:ajuma] 2011-11-08 07:53:30 PST
https://hg.mozilla.org/mozilla-central/rev/53f4c8abf558
Comment 11 Ali Juma [:ajuma] 2011-11-08 14:18:50 PST
*** Bug 698197 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.