945586 - Split ReadTextureImage away from GLContext; some tweaks

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Move GLReadTexImageHelper out of GLContext

Also:
 - renamed to ReadTexImage to avoid confusion with the TextureImage business
 - removed some helper functions that were returning pointers into global arrays and, for one of them, modifying the contents of that global array...

Reviewer note: you want to check that I got the texCoordArray and vertexArray right...

Comment 1

[u480271]

(In reply to Benoit Jacob [:bjacob] from comment #0)
> Created attachment 8341523 [details] [diff] [review]
> Move GLReadTexImageHelper out of GLContext
> 
> Also:
>  - renamed to ReadTexImage to avoid confusion with the TextureImage business
>  - removed some helper functions that were returning pointers into global
> arrays and, for one of them, modifying the contents of that global array...
> 
> Reviewer note: you want to check that I got the texCoordArray and
> vertexArray right...

I wouldn't bother with making GLContext track ReadTexImageHelper. Currently it's only used in LayerScope which could manage the life time.

Not really a fan of having vertexArray and texCoordArray created inline, but that's just a personal preference. (It's really a large "do this, then do this" kind of function which looks messy, so ...)

Comment 2

[u480271]

Comment on attachment 8341523 [details] [diff] [review]
Move GLReadTexImageHelper out of GLContext

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

See my reply.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Regarding lifetime: I'm all for taking this completely out of GLContext. I'll look into anchoring it to LayerScope.

Regarding the inline arrays: I'll look for a better solution then. Conversely, I was afraid of a function modifying a global array and returning a pointer into it.

Let's see what Jeff thinks.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8341523 [details] [diff] [review]
Move GLReadTexImageHelper out of GLContext

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

You can ignore the [NYF] things. The move in this patch seems fine, but those big strings should sit in global scope and be properly const-decorated. I trust you to pull them out.

I'm a little surprised at the code in question though. That's a lot of debug-looking printf_stderr, and not a lot of asserts. I don't think using macros instead of gotos is a good tradeoff, either. This generally looks like it'll keep appearing to work, even if it hits errors, choosing to ignore them rather than doing anything more drastic.

::: gfx/gl/GLReadTexImageHelper.cpp
@@ +37,5 @@
> +        "varying vec2 vTexCoord;\n"
> +        "void main() { gl_Position = aVertex; vTexCoord = aTexCoord; }";
> +
> +    const char*
> +    readTextureImageFS[] = {

Just put these in the global scope, and const-decorate them properly. `const char* const foo[]`, I believe.

@@ +89,5 @@
> +    /* This might be overkill, but assure that we don't access out-of-bounds */
> +    MOZ_ASSERT((size_t) variant < ArrayLength(mPrograms));
> +    if (!mPrograms[variant]) {
> +        GLuint vs = mGL->fCreateShader(LOCAL_GL_VERTEX_SHADER);
> +        mGL->fShaderSource(vs, 1, (const GLchar**) &readTextureImageVS, NULL);

[NYF] s/NULL/nullptr/

@@ +106,5 @@
> +
> +        GLint success;
> +        mGL->fGetProgramiv(program, LOCAL_GL_LINK_STATUS, &success);
> +
> +        if (!success) {

[NotYourFault=NYF] This should assert.
This is also gross, because if we fail, we're going to keep trying every time, anyways, failing each time. It doesn't look like this failure is ever communicated, so it will be invisibly failing, which is everyone's favorite type, right after intermittent failures.

@@ +123,5 @@
> +
> +bool
> +GLReadTexImageHelper::DidGLErrorOccur(const char* str)
> +{
> +    GLenum error = mGL->fGetError();

This looks like it'll break WebGL's error reporting. It sounds like this will only be run on the compositor's context, but we should (later) add a mechanism so we can assert this.

@@ +143,5 @@
> +        mGL->fPixelStorei(LOCAL_GL_PACK_ALIGNMENT, 4);
> +
> +    mGL->fReadPixels(0, 0, aSize.width, aSize.height,
> +                     LOCAL_GL_RGBA, LOCAL_GL_UNSIGNED_BYTE,
> +                     aSurface->Data());

[NYF] This doesn't check if surface is a reasonable format, nor if the surface is even big enough.

@@ +153,5 @@
> +
> +    return result;
> +}
> +
> +#define CLEANUP_IF_GLERROR_OCCURRED(x)                                      \

This...surprises me. This shouldn't have been hard to avoid.

@@ +185,5 @@
> +    nsRefPtr<gfxImageSurface> isurf;
> +    isurf = new gfxImageSurface(aSize, gfxImageFormatARGB32);
> +    if (!isurf || isurf->CairoStatus()) {
> +        isurf = nullptr;
> +        return isurf.forget();

[NYF] Just returning null here would have been fine.

@@ +249,5 @@
> +        /* Setup vertex and fragment shader */
> +        layers::ShaderProgramType shaderProgram = (ShaderProgramType) aShaderProgram;
> +        GLuint program = TextureImageProgramFor(aTextureTarget, shaderProgram);
> +        if (!program) {
> +            printf_stderr("failed to compile program for texture target %u and"

[NYF] Printing enums should be "0x%x" or "0x%04x". Also, this should assert.

@@ +274,5 @@
> +        vertexArray[4*4] = {
> +            -1.0f, -1.0f, 0.0f, 1.0f,
> +            1.0f, -1.0f, 0.0f, 1.0f,
> +            -1.0f,  1.0f, 0.0f, 1.0f,
> +            1.0f,  1.0f, 0.0f, 1.0f

[NYF] Why are these vec4s?

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

You are right, this code shouldn't have received r+ in the first place. I'll make a separate patch fixing it.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Move GLReadTexImageHelper out of GLContext (for landing)

Updated, moved the strings back to global scope as requested

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix a few things in GLReadTexImageHelper

This fixes some of the issues (not all) that you noted.

Also fixes the issue that this was taking the address of file-scope strings and storing it into a pointer, making it difficult for the compiler to know that these could be resolved at compile time. http://glandium.org/blog/?p=2361

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8346562 [details] [diff] [review]
Fix a few things in GLReadTexImageHelper

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

::: gfx/gl/GLReadTexImageHelper.cpp
@@ +101,5 @@
>      /* This might be overkill, but assure that we don't access out-of-bounds */
>      MOZ_ASSERT((size_t) variant < ArrayLength(mPrograms));
>      if (!mPrograms[variant]) {
>          GLuint vs = mGL->fCreateShader(LOCAL_GL_VERTEX_SHADER);
> +        const GLchar* vsSourcePtr = &readTextureImageVS[0];

I really don't like the `ptr = &arr[0]` meme. Why not just cast it?

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c85b53ac74fe
http://hg.mozilla.org/integration/mozilla-inbound/rev/6e250e7e2884

(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> > +        const GLchar* vsSourcePtr = &readTextureImageVS[0];
> 
> I really don't like the `ptr = &arr[0]` meme. Why not just cast it?

I didn't find a better way to do the array-to-pointer casting there. Suggestions welcome!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c85b53ac74fe
https://hg.mozilla.org/mozilla-central/rev/6e250e7e2884