The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla13

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gw280, Assigned: gw280)

Tracking

unspecified
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Updated

5 years ago
Blocks: 619615
Created attachment 591894 [details] [diff] [review]
use glTexImage2D
Attachment #591894 - Flags: review?(bgirard)
(Assignee)

Updated

5 years ago
Attachment #591894 - Flags: review?(bgirard) → review?(joe)
(Assignee)

Updated

5 years ago
Attachment #591894 - Flags: review?(bgirard)
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.
Attachment #591894 - Flags: review?(joe) → review+
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.
Attachment #591894 - Flags: review?(bgirard) → review+
Created attachment 591921 [details] [diff] [review]
updated patch
Attachment #591894 - Attachment is obsolete: true
Attachment #591921 - Flags: review?(joe)
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.
Attachment #591921 - Flags: review?(joe) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9699edcbcedd
Assignee: nobody → gwright
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/9699edcbcedd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 722167
Backed out for causing bug 722167: https://hg.mozilla.org/mozilla-central/rev/d10da858631a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 592822 [details] [diff] [review]
updated patch removing the glTexImage2D general case optimisation

This removes the general optimisation that was causing bug 722167
Attachment #591921 - Attachment is obsolete: true
Attachment #592822 - Flags: review?(joe)
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 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.
Attachment #592822 - Flags: review?(joe) → review+
Created attachment 592893 [details] [diff] [review]
final patch

Final patch for committing
Attachment #592822 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb06e8a06ff3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eb06e8a06ff3
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla12 → mozilla13
You need to log in before you can comment on or make changes to this bug.