TexturesImageEGL on non-maemo does not call glTexImage2D on creation

RESOLVED FIXED in mozilla8

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Joe Drew (not getting mail))

Tracking

(Blocks: 1 bug)

Trunk
mozilla8
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 535484 [details] [diff] [review]
always allocate textures in the construtor

Currently, textures are only allocated (by using the backing surface path) on Maemo. This means that when we try to bind a framebuffer to the texture for blitting, we get an incomplete framebuffer error.

Instead of specifically calling CreateBackingSurface from the backing surface path in the constructor, we can call Resize and have the texture allocated properly in both cases.
Attachment #535484 - Flags: review?(florian.haenel)
(Assignee)

Updated

6 years ago
Attachment #535484 - Flags: review?(romaxa)
(Assignee)

Updated

6 years ago
Assignee: nobody → joe
OS: Linux → Android
Hardware: x86_64 → All
Version: unspecified → Trunk
Comment on attachment 535484 [details] [diff] [review]
always allocate textures in the construtor

Ok, Resize(aSize) is checking also for aSize == mSize && mCreated, and mCreated will be false in ctor, so it should be working fine
Attachment #535484 - Flags: review?(romaxa) → review+
Comment on attachment 535484 [details] [diff] [review]
always allocate textures in the construtor

I support this change.
Attachment #535484 - Flags: review?(florian.haenel) → review+
Keywords: checkin-needed
Comment on attachment 535484 [details] [diff] [review]
always allocate textures in the construtor

hmm.. I have tested this patch on device, and found problems with RGB/BRG color swap while scrolling awesomebar url list... need to check where is the problem
Attachment #535484 - Flags: review+ → review-
Keywords: checkin-needed
Comment on attachment 535484 [details] [diff] [review]
always allocate textures in the construtor

Ok, this patch will break first CreateBacking surface call, because ::Resize checking 
if (mBackingSurface) {
            CreateBackingSurface(gfxIntSize(aSize.width, aSize.height));
} else

and in ctor first time we don't have mBackingSurface yet
Created attachment 536149 [details] [diff] [review]
always allocate textures in the construtor v2

What we should do is to keep CreateBackingSurface where it was, but set mCreated=true in CreateBackingSurface, so Resize on maemo will not call CreateBackingSurface second time because aSize == mSize and mCreate == true
Attachment #535484 - Attachment is obsolete: true
Attachment #536149 - Flags: review?
(Assignee)

Comment 6

6 years ago
Comment on attachment 535484 [details] [diff] [review]
always allocate textures in the construtor

Instead of using romaxa's patch, let's fix this problem once and for all by properly tracking the status of the texture. Patch upcoming.
Attachment #535484 - Attachment is obsolete: false
Attachment #535484 - Flags: review- → review?(romaxa)
(Assignee)

Comment 7

6 years ago
Created attachment 536173 [details] [diff] [review]
part 2: track creation/allocation/valid status of textures

This is mostly a port of the status tracking code from BasicTextureImage. Unfortunately TextureImageEGL isn't derived from BasicTextureImage.
Attachment #536149 - Attachment is obsolete: true
Attachment #536149 - Flags: review?
Attachment #536173 - Flags: review?(romaxa)
Attachment #536173 - Flags: review?(florian.haenel)
Comment on attachment 536173 [details] [diff] [review]
part 2: track creation/allocation/valid status of textures

Ok, this looks good to me, and we need also previous patch with small modification
-- mCreate = PR_TRUE
++ mTextureState = Allocated;
Attachment #536173 - Flags: review?(romaxa) → review+
Created attachment 536177 [details] [diff] [review]
Previous patch updated part1, v3
Attachment #536177 - Flags: review?(joe)
(Assignee)

Comment 10

6 years ago
Created attachment 536190 [details] [diff] [review]
part 1: allocate textures in the constructor

OK, I finally understand what the problem romaxa was working around is, and this should fix it. This uses gUseBackingSurface as the condition for CreateBackingSurface() in Resize(), and will fall back to glTexImage2D if that fails.
Attachment #535484 - Attachment is obsolete: true
Attachment #536177 - Attachment is obsolete: true
Attachment #535484 - Flags: review?(romaxa)
Attachment #536177 - Flags: review?(joe)
Attachment #536190 - Flags: review?(romaxa)
Attachment #536190 - Flags: review?(florian.haenel)
(Assignee)

Updated

6 years ago
Attachment #536190 - Attachment is patch: true
Attachment #536190 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Comment 11

6 years ago
Created attachment 536192 [details] [diff] [review]
part 2: track creation/allocation/valid status of textures, v2

Rebased for new part 1.
Attachment #536173 - Attachment is obsolete: true
Attachment #536173 - Flags: review?(florian.haenel)
Attachment #536192 - Flags: review?(romaxa)
Attachment #536192 - Flags: review?(florian.haenel)
Comment on attachment 536190 [details] [diff] [review]
part 1: allocate textures in the constructor

this on is ok
Attachment #536190 - Flags: review?(romaxa) → review+
Attachment #536192 - Flags: review?(romaxa) → review+
Blocks: 661002
Created attachment 542653 [details] [diff] [review]
part 2, rebased

The first patch is tiny, but the second is a bit bigger and doesn't apply anymore - so to save someone else some work, here's it rebased on today's mozilla-central.
Review ping heeen. Let us know if you can't get to review this so we can find another reviewer.
Whiteboard: review-needed
Comment on attachment 536192 [details] [diff] [review]
part 2: track creation/allocation/valid status of textures, v2

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

::: gfx/thebes/GLContextProviderEGL.cpp
@@ +1512,5 @@
> +    {
> +      Created, // Texture created, but has not had glTexImage called to initialize it.
> +      Allocated,  // Texture memory exists, but contents are invalid.
> +      Valid  // Texture fully ready to use.
> +    };

Isn't this already defined in GLContext.h - TextureImage?
I believe this patch is also causing (in part) bug 661002, see my comments there.
Comment on attachment 536190 [details] [diff] [review]
part 1: allocate textures in the constructor

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

I'd like to see some comments here to clear up what's supposed to happen, eg.

::: gfx/thebes/GLContextProviderEGL.cpp
@@ +1073,5 @@
>              }
>              // We currently always use BGRA type textures
>              mShaderType = BGRALayerProgramType;
>          }
> +

// we resize here so we should have a valid buffer after creation

@@ +1295,5 @@
>          if (mSize == aSize && mCreated)
>              return;
>  
>          mGLContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture);
> +

// Try to generate a backing surface first if we have the ability

@@ +1301,4 @@
>              CreateBackingSurface(gfxIntSize(aSize.width, aSize.height));
> +        }
> +
> +        if (!mBackingSurface) {

// if we don't have backing surfaces or failed to obtain one, use the GL Texture failsafe
Attachment #536190 - Flags: review?(florian.haenel) → review+
Comment on attachment 536192 [details] [diff] [review]
part 2: track creation/allocation/valid status of textures, v2

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

r+ with nits fixed

::: gfx/thebes/GLContextProviderEGL.cpp
@@ +1513,5 @@
> +      Created, // Texture created, but has not had glTexImage called to initialize it.
> +      Allocated,  // Texture memory exists, but contents are invalid.
> +      Valid  // Texture fully ready to use.
> +    };
> +

yep, looks like it already exists there, so we should just drop it here.
Attachment #536192 - Flags: review?(florian.haenel) → review+
(In reply to comment #13)
> Created attachment 542653 [details] [diff] [review] [review]
> part 2, rebased
> 
> The first patch is tiny, but the second is a bit bigger and doesn't apply
> anymore - so to save someone else some work, here's it rebased on today's
> mozilla-central.

This patch is more then just a rebase - it introduced changes in gfx/thebes/GLContext.*. Is this intentional?
Created attachment 548571 [details] [diff] [review]
part 1: allocate textures in the constructor + review comment
Created attachment 548573 [details] [diff] [review]
part 2: track creation/allocation/valid status of textures, v2 + review
(In reply to comment #16)
> I believe this patch is also causing (in part) bug 661002, see my comments
> there.

I say we land this change. This will make solving the bug easier if the change dependent has landed.
http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2148365fbb7c
Whiteboard: review-needed → [inbound]
http://hg.mozilla.org/mozilla-central/rev/b5c330319edd
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Also: http://hg.mozilla.org/mozilla-central/rev/2148365fbb7c
You need to log in before you can comment on or make changes to this bug.