683514 - Use GL_EXT_unpack_subimage when available on GLES

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Lord [:cwiiis]]

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]]

Attachment: Use GL_EXT_unpack_subimage when available

Comment 2

[Joe Drew (not getting mail)]

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]

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]]

(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]]

http://hg.mozilla.org/mozilla-central/rev/891e5dbae3ec

Comment 6

[Ali Juma [:ajuma]]

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)]

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]]

Weird, I see the crash too - will investigate.

Comment 9

[Chris Lord [:cwiiis]]

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]]

Attachment: 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

[(no longer active)]

Backout on central: http://hg.mozilla.org/mozilla-central/rev/4f3856dd4dd0

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7a8399ef7535

Comment 13

[Chris Lord [:cwiiis]]

Attachment: 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)]

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]]

Pushed to inbound with the suggested change (also made the change in the earlier usage above).

Comment 16

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/f037eaa1c8a5