The default bug view has changed. See this FAQ.

Use GL_EXT_unpack_subimage when available on GLES

RESOLVED FIXED in mozilla9

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

Trunk
mozilla9
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
There's an extension, GL_EXT_unpack_subimage, that restores support for the GL_UNPACK_ROW_LENGTH variable, amongst others: http://www.khronos.org/registry/gles/extensions/EXT/GL_EXT_unpack_subimage.txt

This is available on Tegra 2 Android devices, and really helps for uploads from a sub-region of a buffer when the width of that region isn't equal to the stride (e.g. horizontal scrolling).

Patch incoming to enable use of it.
(Assignee)

Comment 1

6 years ago
Created attachment 557174 [details] [diff] [review]
Use GL_EXT_unpack_subimage when available
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Attachment #557174 - Flags: review?(joe)
Comment on attachment 557174 [details] [diff] [review]
Use GL_EXT_unpack_subimage when available

Review of attachment 557174 [details] [diff] [review]:
-----------------------------------------------------------------

lovely!
Attachment #557174 - Flags: review?(joe) → review+
Comment on attachment 557174 [details] [diff] [review]
Use GL_EXT_unpack_subimage when available

Drive-by:

>-#ifndef USE_GLES2
>-    fPixelStorei(LOCAL_GL_UNPACK_ROW_LENGTH, 0);
>-#endif
>+    if (useUnpackRowLength)
>+        fPixelStorei(LOCAL_GL_UNPACK_ROW_LENGTH, 0);
>     fPixelStorei(LOCAL_GL_UNPACK_ALIGNMENT, 4);
> }
> 

Is this change going to cause us to cause fPixelStorei() twice when useUnpackRowLength?
(Assignee)

Comment 4

6 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> Comment on attachment 557174 [details] [diff] [review]
> Use GL_EXT_unpack_subimage when available
> 
> Drive-by:
> 
> >-#ifndef USE_GLES2
> >-    fPixelStorei(LOCAL_GL_UNPACK_ROW_LENGTH, 0);
> >-#endif
> >+    if (useUnpackRowLength)
> >+        fPixelStorei(LOCAL_GL_UNPACK_ROW_LENGTH, 0);
> >     fPixelStorei(LOCAL_GL_UNPACK_ALIGNMENT, 4);
> > }
> > 
> 
> Is this change going to cause us to cause fPixelStorei() twice when
> useUnpackRowLength?

That's what we did before; once to set it, and again to unset it. There are other areas we do texture uploads outside of this code (though I think we should consider consolidating in the future).
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/891e5dbae3ec
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9

Updated

6 years ago
Depends on: 684716

Comment 6

6 years ago
This is consistently causing a crash when fully zoomed-in in landscape orientation on Tegra 2 devices (Bug 684716). According to the stack from bp-39e67446-7962-4fa4-bac4-917872110907, we're crashing during the call to fPixelStorei(LOCAL_GL_UNPACK_ROW_LENGTH, 0). 

Perhaps the implementation of the GL_EXT_unpack_subimage extension is still buggy on Tegra 2? 

While we get this sorted out, I suggest that we back out this patch.
Backout:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4f3856dd4dd0

You're welcome to reland this once the crash is fixed. Let us know if you want us to test a fix on our hardware.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

6 years ago
Weird, I see the crash too - will investigate.
(Assignee)

Comment 9

6 years ago
Found the bug - it crashes when we set UNPACK_ROW_LENGTH to greater than the max texture size. Unsurprising really. I'll add a check.

This would affect desktop too, except we wouldn't have a surface that large as we don't use tiles on desktop yet.
(Assignee)

Comment 10

6 years ago
Created attachment 559173 [details] [diff] [review]
Use GL_EXT_unpack_subimage when available

Note, that regardless of this patch, I still see crashes when zooming (on 76664:183fb86c9ac4), but this version of the patch doesn't add/remove any crashes.
Attachment #557174 - Attachment is obsolete: true
Attachment #559173 - Flags: review?(joe)
Backout on central: http://hg.mozilla.org/mozilla-central/rev/4f3856dd4dd0
Attachment #559173 - Flags: review?(joe) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/7a8399ef7535
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
(Assignee)

Comment 13

6 years ago
Created attachment 559929 [details] [diff] [review]
Use GL_EXT_unpack_subimage when available (part 2)

Idiot me, didn't notice the use of the exact same construct a few lines below the one I augmented - So here's the final part of the patch that actually applies in the more common/useful case.
Attachment #559929 - Flags: review?(joe)
(Assignee)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 559929 [details] [diff] [review]
Use GL_EXT_unpack_subimage when available (part 2)

Review of attachment 559929 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/GLContext.cpp
@@ +1926,5 @@
> +      useUnpackRowLength = false;
> +
> +    if (useUnpackRowLength)
> +        fPixelStorei(LOCAL_GL_UNPACK_ROW_LENGTH, rowLength);
> +    else if (stride != width * pixelsize) {

Probably it's better to add {} around the fPixelStorei.
Attachment #559929 - Flags: review?(joe) → review+
(Assignee)

Comment 15

6 years ago
Pushed to inbound with the suggested change (also made the change in the earlier usage above).
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/f037eaa1c8a5
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.