Closed Bug 660090 Opened 9 years ago Closed 9 years ago

TexturesImageEGL on non-maemo does not call glTexImage2D on creation

Categories

(Core :: Graphics, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: joe, Assigned: joe)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

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)
Attachment #535484 - Flags: review?(romaxa)
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+
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-
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
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?
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)
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+
Attached patch Previous patch updated part1, v3 (obsolete) — Splinter Review
Attachment #536177 - Flags: review?(joe)
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)
Attachment #536190 - Attachment is patch: true
Attachment #536190 - Attachment mime type: text/x-patch → text/plain
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+
Attached patch part 2, rebasedSplinter Review
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?
(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://hg.mozilla.org/mozilla-central/rev/b5c330319edd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.