Last Comment Bug 721467 - Add a codepath to only use glTexImage2D instead of glTexSubImage2D when texture uploading in GLContext
: Add a codepath to only use glTexImage2D instead of glTexSubImage2D when textu...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: George Wright (:gw280) (:gwright)
:
Mentors:
Depends on: 722167
Blocks: 619615
  Show dependency treegraph
 
Reported: 2012-01-26 11:35 PST by George Wright (:gw280) (:gwright)
Modified: 2012-02-04 02:34 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use glTexImage2D (14.55 KB, patch)
2012-01-26 12:27 PST, George Wright (:gw280) (:gwright)
joe: review+
bgirard: review+
Details | Diff | Splinter Review
updated patch (19.69 KB, patch)
2012-01-26 13:27 PST, George Wright (:gw280) (:gwright)
joe: review+
Details | Diff | Splinter Review
updated patch removing the glTexImage2D general case optimisation (11.20 KB, patch)
2012-01-30 12:27 PST, George Wright (:gw280) (:gwright)
joe: review+
Details | Diff | Splinter Review
final patch (6.79 KB, patch)
2012-01-30 15:32 PST, George Wright (:gw280) (:gwright)
no flags Details | Diff | Splinter Review

Description George Wright (:gw280) (:gwright) 2012-01-26 11:35:27 PST
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) (:gwright) 2012-01-26 12:27:05 PST
Created attachment 591894 [details] [diff] [review]
use glTexImage2D
Comment 2 Joe Drew (not getting mail) 2012-01-26 12:54:10 PST
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) 2012-01-26 13:00:02 PST
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) (:gwright) 2012-01-26 13:27:08 PST
Created attachment 591921 [details] [diff] [review]
updated patch
Comment 5 Joe Drew (not getting mail) 2012-01-26 13:35:42 PST
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) 2012-01-27 12:06:42 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9699edcbcedd
Comment 7 Joe Drew (not getting mail) 2012-01-28 19:12:27 PST
https://hg.mozilla.org/mozilla-central/rev/9699edcbcedd
Comment 8 Joe Drew (not getting mail) 2012-01-29 16:42:54 PST
Backed out for causing bug 722167: https://hg.mozilla.org/mozilla-central/rev/d10da858631a
Comment 9 George Wright (:gw280) (:gwright) 2012-01-30 12:27:26 PST
Created attachment 592822 [details] [diff] [review]
updated patch removing the glTexImage2D general case optimisation

This removes the general optimisation that was causing bug 722167
Comment 10 George Wright (:gw280) (:gwright) 2012-01-30 13:35:49 PST
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) 2012-01-30 15:00:09 PST
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) (:gwright) 2012-01-30 15:32:39 PST
Created attachment 592893 [details] [diff] [review]
final patch

Final patch for committing
Comment 14 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-02-04 02:34:52 PST
https://hg.mozilla.org/mozilla-central/rev/eb06e8a06ff3

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