The default bug view has changed. See this FAQ.

fTexSubImage2D row by row is too slow

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: BenWa, Assigned: ajuma)

Tracking

unspecified
mozilla10
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
We can't use* GL_UNPACK_ROW_LENGTH
(Reporter)

Updated

6 years ago
Duplicate of this bug: 683411
(Assignee)

Comment 3

6 years ago
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.
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → ARM
(Assignee)

Comment 4

6 years ago
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).
Attachment #570438 - Flags: review?(jmuizelaar)
(Reporter)

Comment 5

6 years ago
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?
(Assignee)

Comment 6

6 years ago
(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.
(Assignee)

Comment 7

6 years ago
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.
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
(Assignee)

Updated

6 years ago
Duplicate of this bug: 700458
(Assignee)

Comment 10

6 years ago
https://hg.mozilla.org/mozilla-central/rev/53f4c8abf558
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(Assignee)

Updated

6 years ago
Duplicate of this bug: 698197
You need to log in before you can comment on or make changes to this bug.