721467 - Add a codepath to only use glTexImage2D instead of glTexSubImage2D when texture uploading in GLContext

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[George Wright (:gw280) (needinfo me!)]

Some drivers don't do glTexSubImage2D very well. We should have a code path that avoids using that function in GLContext

Comment 1

[George Wright (:gw280) (needinfo me!)]

Attachment: use glTexImage2D

Comment 2

[Joe Drew (not getting mail)]

Comment on attachment 591894 [details] [diff] [review]
use glTexImage2D

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

LGTM. Make a couple of changes and you should be good to go!

::: gfx/gl/GLContext.cpp
@@ +2076,5 @@
> +        if (iterRect->x == 0 &&
> +            iterRect->y == 0 &&
> +            iterRect->width == aTextureSize.width &&
> +            iterRect->height == aTextureSize.height)
> +            useTexSubImage2D = false;

Add {}

::: gfx/gl/GLContext.h
@@ +1214,5 @@
>                                               GLuint& aTexture,
>                                               bool aOverwrite = false,
>                                               const nsIntPoint& aSrcPoint = nsIntPoint(0, 0),
> +                                             bool aPixelBuffer = false,
> +                                             const nsIntSize& aTextureSize = nsIntSize(0, 0));

I don't think it makes sense for aTextureSize to have a default value. It should just be added to the earlier, non-default parameters.

Also, document the parameter.

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 591894 [details] [diff] [review]
use glTexImage2D

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

Excellent patch!

::: gfx/gl/GLContext.cpp
@@ +400,5 @@
>              }
>          }
> +
> +        glRendererString = (const char *)fGetString(LOCAL_GL_RENDERER);
> +        const char *rendererMatchStrings[RendererOther] = {

The name implies string equality. It should be renamed to something like 'DoesSubStringMatch'

@@ +2075,5 @@
> +        // the entire texture
> +        if (iterRect->x == 0 &&
> +            iterRect->y == 0 &&
> +            iterRect->width == aTextureSize.width &&
> +            iterRect->height == aTextureSize.height)

I think we should do >=. I know with the call site this wont happen but let's make this function more generic and let it grows the texture if it needs to.

Comment 4

[George Wright (:gw280) (needinfo me!)]

Attachment: updated patch

Comment 5

[Joe Drew (not getting mail)]

Comment on attachment 591921 [details] [diff] [review]
updated patch

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

I checked the UploadSurfaceToTexture call sites, and the size arguments all look correct.

Comment 6

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9699edcbcedd

Comment 7

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/9699edcbcedd

Comment 8

[Joe Drew (not getting mail)]

Backed out for causing bug 722167: https://hg.mozilla.org/mozilla-central/rev/d10da858631a

Comment 9

[George Wright (:gw280) (needinfo me!)]

Attachment: updated patch removing the glTexImage2D general case optimisation

This removes the general optimisation that was causing bug 722167

Comment 10

[George Wright (:gw280) (needinfo me!)]

https://tbpl.mozilla.org/?tree=Try&rev=d338c6ebc498

New patched has been pushed to try. I've run a local build on my OS X box and confirmed it no longer causes gfx corruption.

Comment 11

[Joe Drew (not getting mail)]

Comment on attachment 592822 [details] [diff] [review]
updated patch removing the glTexImage2D general case optimisation

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

::: gfx/gl/GLContext.cpp
@@ +712,5 @@
>      // determine the region the client will need to repaint
> +    if (!mGLContext->CanUploadSubTextures()) {
> +        aRegion = nsIntRect(nsIntPoint(0, 0), mSize);
> +    } else {
> +        GetUpdateRegion(aRegion);

Reverse these two so we don't have a negation

@@ +2012,5 @@
>          // to the start of the data block.
>          if (!aPixelBuffer) {
>                data = imageSurface->Data();
>          }
> +        

Spaces on this line.

@@ +2079,5 @@
>                       "Must be uploading to the origin when we don't have an existing texture");
>  
> +        bool useTexSubImage2D = true;
> +
> +        nsIntRect bounds = aDstRegion.GetBounds();

You can remove these two lines.

Comment 12

[George Wright (:gw280) (needinfo me!)]

Attachment: final patch

Final patch for committing

Comment 13

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb06e8a06ff3

Comment 14

[Marco Bonardo [:mak]]

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