Last Comment Bug 683514 - Use GL_EXT_unpack_subimage when available on GLES
: Use GL_EXT_unpack_subimage when available on GLES
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla9
Assigned To: Chris Lord [:cwiiis]
:
:
Mentors:
Depends on: 684716
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-31 07:56 PDT by Chris Lord [:cwiiis]
Modified: 2011-09-15 07:31 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use GL_EXT_unpack_subimage when available (2.43 KB, patch)
2011-08-31 08:02 PDT, Chris Lord [:cwiiis]
joe: review+
Details | Diff | Splinter Review
Use GL_EXT_unpack_subimage when available (3.00 KB, patch)
2011-09-08 08:45 PDT, Chris Lord [:cwiiis]
joe: review+
Details | Diff | Splinter Review
Use GL_EXT_unpack_subimage when available (part 2) (1.38 KB, patch)
2011-09-12 21:31 PDT, Chris Lord [:cwiiis]
joe: review+
Details | Diff | Splinter Review

Description Chris Lord [:cwiiis] 2011-08-31 07:56:42 PDT
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.
Comment 1 Chris Lord [:cwiiis] 2011-08-31 08:02:03 PDT
Created attachment 557174 [details] [diff] [review]
Use GL_EXT_unpack_subimage when available
Comment 2 Joe Drew (not getting mail) 2011-08-31 09:04:42 PDT
Comment on attachment 557174 [details] [diff] [review]
Use GL_EXT_unpack_subimage when available

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

lovely!
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-31 11:10:22 PDT
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?
Comment 4 Chris Lord [:cwiiis] 2011-08-31 12:28:08 PDT
(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).
Comment 5 Ed Morley [:emorley] 2011-09-01 01:46:55 PDT
http://hg.mozilla.org/mozilla-central/rev/891e5dbae3ec
Comment 6 Ali Juma [:ajuma] 2011-09-07 12:19:39 PDT
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.
Comment 7 Benoit Girard (:BenWa) 2011-09-07 14:42:21 PDT
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.
Comment 8 Chris Lord [:cwiiis] 2011-09-08 02:28:55 PDT
Weird, I see the crash too - will investigate.
Comment 9 Chris Lord [:cwiiis] 2011-09-08 06:48:20 PDT
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.
Comment 10 Chris Lord [:cwiiis] 2011-09-08 08:45:25 PDT
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.
Comment 11 :Ehsan Akhgari 2011-09-08 10:28:22 PDT
Backout on central: http://hg.mozilla.org/mozilla-central/rev/4f3856dd4dd0
Comment 13 Chris Lord [:cwiiis] 2011-09-12 21:31:21 PDT
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.
Comment 14 Joe Drew (not getting mail) 2011-09-14 10:42:56 PDT
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.
Comment 15 Chris Lord [:cwiiis] 2011-09-14 15:40:27 PDT
Pushed to inbound with the suggested change (also made the change in the earlier usage above).

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