Last Comment Bug 660090 - TexturesImageEGL on non-maemo does not call glTexImage2D on creation
: TexturesImageEGL on non-maemo does not call glTexImage2D on creation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: mozilla8
Assigned To: Joe Drew (not getting mail)
:
Mentors:
Depends on:
Blocks: 661002 opengl-mobile
  Show dependency treegraph
 
Reported: 2011-05-26 14:51 PDT by Joe Drew (not getting mail)
Modified: 2011-07-28 08:16 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
always allocate textures in the construtor (1.43 KB, patch)
2011-05-26 14:51 PDT, Joe Drew (not getting mail)
florian.haenel: review+
Details | Diff | Splinter Review
always allocate textures in the construtor v2 (1.44 KB, patch)
2011-05-30 12:58 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
part 2: track creation/allocation/valid status of textures (7.89 KB, patch)
2011-05-30 14:27 PDT, Joe Drew (not getting mail)
romaxa: review+
Details | Diff | Splinter Review
Previous patch updated part1, v3 (1.45 KB, patch)
2011-05-30 14:48 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
part 1: allocate textures in the constructor (2.35 KB, patch)
2011-05-30 16:08 PDT, Joe Drew (not getting mail)
florian.haenel: review+
romaxa: review+
Details | Diff | Splinter Review
part 2: track creation/allocation/valid status of textures, v2 (7.86 KB, patch)
2011-05-30 16:12 PDT, Joe Drew (not getting mail)
florian.haenel: review+
romaxa: review+
Details | Diff | Splinter Review
part 2, rebased (7.56 KB, patch)
2011-06-28 16:12 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
part 1: allocate textures in the constructor + review comment (2.85 KB, patch)
2011-07-26 13:53 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
part 2: track creation/allocation/valid status of textures, v2 + review (8.60 KB, patch)
2011-07-26 13:54 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review

Description Joe Drew (not getting mail) 2011-05-26 14:51:26 PDT
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.
Comment 1 Oleg Romashin (:romaxa) 2011-05-26 15:16:00 PDT
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
Comment 2 Florian Hänel [:heeen] 2011-05-27 06:49:40 PDT
Comment on attachment 535484 [details] [diff] [review]
always allocate textures in the construtor

I support this change.
Comment 3 Oleg Romashin (:romaxa) 2011-05-30 12:43:45 PDT
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
Comment 4 Oleg Romashin (:romaxa) 2011-05-30 12:48:09 PDT
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
Comment 5 Oleg Romashin (:romaxa) 2011-05-30 12:58:43 PDT
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
Comment 6 Joe Drew (not getting mail) 2011-05-30 14:24:34 PDT
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.
Comment 7 Joe Drew (not getting mail) 2011-05-30 14:27:04 PDT
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.
Comment 8 Oleg Romashin (:romaxa) 2011-05-30 14:46:30 PDT
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;
Comment 9 Oleg Romashin (:romaxa) 2011-05-30 14:48:49 PDT
Created attachment 536177 [details] [diff] [review]
Previous patch updated part1, v3
Comment 10 Joe Drew (not getting mail) 2011-05-30 16:08:08 PDT
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.
Comment 11 Joe Drew (not getting mail) 2011-05-30 16:12:38 PDT
Created attachment 536192 [details] [diff] [review]
part 2: track creation/allocation/valid status of textures, v2

Rebased for new part 1.
Comment 12 Oleg Romashin (:romaxa) 2011-05-30 17:02:19 PDT
Comment on attachment 536190 [details] [diff] [review]
part 1: allocate textures in the constructor

this on is ok
Comment 13 Chris Lord [:cwiiis] 2011-06-28 16:12:33 PDT
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.
Comment 14 Benoit Girard (:BenWa) 2011-07-25 13:11:26 PDT
Review ping heeen. Let us know if you can't get to review this so we can find another reviewer.
Comment 15 Matt Woodrow (:mattwoodrow) 2011-07-25 15:17:53 PDT
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?
Comment 16 Matt Woodrow (:mattwoodrow) 2011-07-25 15:18:54 PDT
I believe this patch is also causing (in part) bug 661002, see my comments there.
Comment 17 Florian Hänel [:heeen] 2011-07-26 07:04:04 PDT
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
Comment 18 Florian Hänel [:heeen] 2011-07-26 07:05:59 PDT
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.
Comment 19 Benoit Girard (:BenWa) 2011-07-26 13:25:01 PDT
(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?
Comment 20 Benoit Girard (:BenWa) 2011-07-26 13:53:55 PDT
Created attachment 548571 [details] [diff] [review]
part 1: allocate textures in the constructor + review comment
Comment 21 Benoit Girard (:BenWa) 2011-07-26 13:54:32 PDT
Created attachment 548573 [details] [diff] [review]
part 2: track creation/allocation/valid status of textures, v2 + review
Comment 22 Benoit Girard (:BenWa) 2011-07-26 13:56:35 PDT
(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.
Comment 23 Benoit Girard (:BenWa) 2011-07-27 11:47:02 PDT
http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2148365fbb7c
Comment 24 :Ehsan Akhgari (away Aug 1-5) 2011-07-28 08:16:27 PDT
http://hg.mozilla.org/mozilla-central/rev/b5c330319edd
Comment 25 :Ehsan Akhgari (away Aug 1-5) 2011-07-28 08:16:50 PDT
Also: http://hg.mozilla.org/mozilla-central/rev/2148365fbb7c

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