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 | 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 | 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 | 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 (out sick) 2011-09-08 10:28:22 PDT
Backout on central: http://hg.mozilla.org/mozilla-central/rev/4f3856dd4dd0
Comment 12 :Ehsan Akhgari (out sick) 2011-09-09 08:37:40 PDT
http://hg.mozilla.org/mozilla-central/rev/7a8399ef7535
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).
Comment 16 :Ehsan Akhgari (out sick) 2011-09-15 07:31:55 PDT
https://hg.mozilla.org/mozilla-central/rev/f037eaa1c8a5

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