Closed Bug 1005908 Opened 10 years ago Closed 10 years ago

binding textures sometimes takes a long time

Categories

(Core :: Graphics: Layers, defect, P1)

Product:
Core
Component:
Graphics: Layers
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32
Project Flags:
blocking-b2g 1.4+
Tracking Flags:
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: kats, Assigned: sotaro)

References

Details

(Keywords: perf, Whiteboard: [c=handeye p= s= u=1.4])

Attachments

(4 files, 9 obsolete files)

Add a pseudostack label
(deleted), patch
BenWa
: review+
Details | Diff | Splinter Review
temporary patch - Add some logs for debugging
(deleted), patch
Details | Diff | Splinter Review
logcat log with the tempray logout patch
(deleted), text/plain
Details
patch - Avoid unnecessary fEGLImageTargetTexture2D'ing
(deleted), patch
sotaro
: review+
Details | Diff | Splinter Review
Attached patch Add a pseudostack label (deleted) — — Splinter Review 
I have a local patch queue that enables low-precision buffer and progressive painting on B2G (see bug 993473 and dependencies). With this patch queue applied I find that scrolling in the contacts app with the medium reference workload is quite janky (for some reason, it is particularly noticeable after you scroll down past the "A" contacts). Profiling seems to indicate that often we have composites that are >20ms, and these are a result of long times spent in GrallocTextureSourceOGL::BindTexture.

Here is a sample profile created with the attached patch that shows 11.9% of compositor time (overall) being spent in this function, most of it in some android syscall, looks like.

http://people.mozilla.org/~bgirard/cleopatra/#report=7693dd6883f6e179f5fb2574a4703422c2d5f54f
Quick thought: I wonder if this is actually indicative of contention for gralloc buffers and the bind is taking a long time because it's actually waiting on a lock?

If that was the case, it'd indicate some bad recycling behaviour though (possibly because the tile cache size would need to be adjusted upwards?)
Adding more profile markers locally indicates that the time is spent in the call to fEGLImageTargetTexture2D at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/GrallocTextureHost.cpp?rev=e8afdde798c3#141
(In reply to Chris Lord [:cwiiis] from comment #1)
> Quick thought: I wonder if this is actually indicative of contention for
> gralloc buffers and the bind is taking a long time because it's actually
> waiting on a lock?
> 
> If that was the case, it'd indicate some bad recycling behaviour though
> (possibly because the tile cache size would need to be adjusted upwards?)

Is there some way to verify this, and some heuristic to find the optimal tile cache size?
texture pool uses std::stack, it might affect to this.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClientPool.h#107
I bumped up the sMaxTextureClients to 100 (and then 150) and that made no appreciable difference. I also changed mTextureClients to be a queue and that also didn't help.
Changing mTextureClientsDeferred to a std:queue also didn't help.

It seems odd to me that the scrolling is smooth while I'm scrolling through the top of the contacts list (the "A" part of the alphabet) but then it gets janky later. It is similarly smooth when scrolling at the very bottom of the list (around W, Y or Z). I know that while scrolling in that area the low-res displayport isn't really moving so maybe when that starts moving and we start painting that it's causing contention of some sort?
Another possibility is related to vsync and number of framebuffers. Is it possible to increase num frame buffers? I am not sure that this change is possible on ICS gonk. On JB gonk, it can be changed like Bug 962152 Comment 45.

> #define NUM_FRAME_BUFFERS  2

http://androidxref.com/4.0.4/xref/frameworks/base/include/ui/FramebufferNativeWindow.h#33
Temporary contention of gralloc buffer usage could also cause the problem. But I am not sure about how it could happen in tiling case.
It would be worth sticking in printfs in the texture cache recycling functions, to monitor when new tiles get allocated/returned/reused - ideally, during scrolling, we should have reasonably stable layer sizes.

If we're reusing lots of tiles during scrolling, it's possible we're hitting the over-production case and waiting on gralloc locks - There's a check when returning tiles to the cache for a function ImplementsLocking - if you replace that with false (so the check always fails), does that make any appreciable difference to the jank? If it makes it better, it would indicate contention due to over-production (which is pretty unlikely except on very powerful devices...)
Blocks: 897996
No longer blocks: 993473
Summary: With low-precision buffer and progressive painting enabled, binding textures sometimes takes a long time → With low-precision buffer enabled, binding textures sometimes takes a long time
(In reply to Chris Lord [:cwiiis] from comment #9)
> There's a check when
> returning tiles to the cache for a function ImplementsLocking - if you
> replace that with false (so the check always fails)

Replacing the call with false makes the if condition always be true, because the result of the call is negated. And doing that (i.e. reporting clients lost instead of returning them to the pool) doesn't help.
I added tons of logging and something that I'm noticing is that the instances where fEGLImageTargetTexture2D takes a long time are often the second call in a row to fEGLImageTargetTexture2D on the same GrallocTextureSourceOGL instance. What seems to be happening is that at [1] the screenRects and regionRects look something like this:
  < (x=0, y=0, w=320, h=50); (x=315, y=50, w=5, h=406); >
and so the loop below it executes twice. On both iterations it calls fEGLImageTargetTexture2D on the same GrallocTextureSourceOGL and the second one often takes a long time to complete.

Does this provide any useful information? It seems to happen far too often to be a coincidence.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContentHost.cpp?rev=d66c2280ba15#168
Can we add an optimization to not redo the eglImageTargetTexture2D call if the right EGLImage is already bound to the right place?  That certainly sounds like it could cause that slowdown -- that second one might be forcing a flush in order to be allowed to "re-use" the EGLImage.
If we use same OpenGL texture for same GrallocTextureHost, it seems possible.
(In reply to Sotaro Ikeda [:sotaro] from comment #13)
> If we use same OpenGL texture for same GrallocTextureHost, it seems possible.

Except tiling layer, this relationship seems almost exist by using CompositableBackendSpecificData.
Attached patch Hacky caching in fEGLImageTargetTexture2D (obsolete) (deleted) — — Splinter Review 
I hacked up this patch just to see if it helps. It does, sort of. I still see jank while scrolling and the profiler still reports frames taking longer than 16ms to composite, but now the culprit seems to be CompositorOGL::EndFrame. It's not clear to me from visual inspection if this is overall better or not - it seems like it might be, but not by much.
Also I'm really out of my depth in this code so if anybody can suggest more concrete code changes or provide patches for me to try please do so.
Comment 15 suggests that we still have locking issues but not those occur in eglSwapBuffers which is much better so we definitely want this fix. Step by step.
Just a thought before we jump to more conclusions - maybe the displayports are just too big once progressive and low-precision have been enabled? Have you checked to see the difference in sizes with disabled/enabled and what that means in terms of increased number of tiles? It could be the displayport size just needs to be adjusted slightly.

Another thing to check out is whether the code that determines when the low precision buffer is updated is working correctly - it shouldn't get rendered at all unless the viewport approaches the edge of the displayport (at least, that's the logic in Android). Similarly, we have code (in TiledContentHost) that is meant to only draw tiles that are visible on screen - we should double-check that this is working too.
Attached patch Hacky simplifying of region (obsolete) (deleted) — — Splinter Review 
Another workaround that results in the same effect as attachment 8417624 [details] [diff] [review]. When low-res tiling is enabled, one of the layers ends up having a visible region that has two rects in it, and squashing that into one rect avoids the double-call to bind the texture.
Comment on attachment 8417624 [details] [diff] [review]
Hacky caching in fEGLImageTargetTexture2D

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

What's a good way to do this sort of caching? See comment 11 for some more context.
Attachment #8417624 - Flags: feedback?(nical.bugzilla)
Attachment #8417400 - Attachment description: 15-tiling-39e47ec → Add a pseudostack label
Attachment #8417400 - Flags: review?(bgirard)
Comment on attachment 8417624 [details] [diff] [review]
Hacky caching in fEGLImageTargetTexture2D

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

Actually it looks jgilbert and/or bjacob have more of a history with this file. Redirecting f?
Attachment #8417624 - Flags: feedback?(nical.bugzilla) → feedback?(bjacob)
Attachment #8417400 - Flags: review?(bgirard) → review+
For the record, I'm now convinced that the stall in fEGLImageTargetTexture2D and the stall in SwapBuffers are separate issues, both triggered by enabling low-res tiling. Fixing the fEGLImageTargetTexture2D stall doesn't really make the SwapBuffers thing any worse, but we'll need to figure out why that is happening as well.
Comment on attachment 8417624 [details] [diff] [review]
Hacky caching in fEGLImageTargetTexture2D

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

::: gfx/gl/GLContext.h
@@ +2162,5 @@
> +        if (sTarget == target && sImage == image) {
> +            return;
> +        }
> +        sTarget = target;
> +        sImage = image;

By making this a static local variable in this method, you're making this global to the entire process --- across all instances of GLContext. "static" is unaffected by this being a class method.

You really want this to be local to each GLContext instance instead i.e. regular member data in class GLContext.

I would feedback+ the corresponding patch doing it that way.
Attachment #8417624 - Flags: feedback?(bjacob) → feedback-
And to get a r+ you'll need to work out the details of when to properly invalidate this caching (e.g. when the image is destroyed).
Moving the stall to EndFrame/SwapBuffers is a good thing -- it means we're pipelining the issuing of drawing commands properly, and just need to figure out why we're stalling when getting them on the screen.  We should be able to let the GPU keep working on it while we prepare the next frame, but that's a separate problem to solve.

EGLImageTargetTexture2D is very heavyweight.  I wish there was a way for us to always allocate a GL image ID and leave it permanently bound to each EGLImage we might use.  That really *should* work, but we're using the binding operation as a hacky lock/unlock, even though it feels like that shouldn't be necessary.
(In reply to Benoit Jacob [:bjacob] from comment #24)
> And to get a r+ you'll need to work out the details of when to properly
> invalidate this caching (e.g. when the image is destroyed).

I looked into this, and it seems nontrivial to do. If we consider the example at [1], an EGLImage is created on line 91, mapped into a texture at line 109, and destroyed at line 111. This means that at line 111, we would need to clear the cached values on the "prodGL" GLContext. I see only two ways to do this:

1) at the call site for fDestroyImage. However this doesn't scale well as any code that creates and destroys EGLImage instances will need to track all the possible GLContext instances that image was mapped in, and clear the cache on all of them

2) set up a generic listener system, so that all GLContextEGL instances are registered with the sEGLLibrary on creation, and any time sEGLLibrary.fDestroyImage is called, we notify all the GLContext instances to clear any caches involving that image. This seems like a rather heavy approach to go with.

As per our IRC discussion, I'd love to hear about suggestions on a better approach if you have one.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/gl/SharedSurfaceGralloc.cpp?rev=865619dc14ca#109
Flags: needinfo?(bjacob)
Attached patch (WIP) Add a cache to fEGLImageTargetTexture2D (obsolete) (deleted) — — Splinter Review 
This is the WIP I have so far, that I think catches all the conditions to clear the cache *except for* the EGLImage destruction. This is already getting a little messier than I would like.
Attachment #8417624 - Attachment is obsolete: true
For the reasons you outlined in comment 26 and comment 27, getting this right by generic interception at the GL API level is bound to be a sizeable, somewhat intrusive patch. It would require us to store metadata around EGLImages, or something like that.

Maybe, then, we could solve only a simpler problem. Do you need this optimization for all fEGLImageTargetTexture2D calls we ever make, or only for the particular ones where you've run into this performance issue? I guess the latter. In that case, that should be a much simpler problem to solve, because we control that code making these calls to the GL.

More specifically: if I understand correctly the above comments, the problematic fEGLImageTargetTexture2D calls are some of those in GrallocTextureHost.cpp. There, we are in control of the GL textures and the EGLImages, we control their lifetimes etc. So you could write a much simpler patch, local to this code.

Note that Sotaro is actually a better specialist than me of both EGLImage/gralloc binding in general, and this code in particular ;-)
Flags: needinfo?(bjacob)
Based on bjacob's comments above I tried this approach which seems to work.
Attachment #8418096 - Attachment is obsolete: true
Attachment #8419403 - Attachment is obsolete: true
Attachment #8420102 - Flags: review?(sotaro.ikeda.g)
Btw, here's a profile taken with the above patch applied. Any thoughts on how I can figure out why the SwapBuffers is stalling?

http://people.mozilla.org/~bgirard/cleopatra/#report=a93afd07e55a6dc191eac3ec2f395d751b054fe1
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #30)
> Btw, here's a profile taken with the above patch applied. Any thoughts on
> how I can figure out why the SwapBuffers is stalling?
> 
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=a93afd07e55a6dc191eac3ec2f395d751b054fe1

Which device are you testing? If it is on flame device, it seems better if HWComposer is enabled. If it is not it is better to enable it by applyig Bug 1006152.
On ICS, it could be stalled by waiting vsync in kernel side.
If it is JB and HWComposer is enabled, FramebufferSurface allocates only 2 buffers, lacking vsync support like Bug 987523 easily causes stall somewhere. As a temporary workaround, you can temporarily increase the number of FramebufferSurface's buffer to 3 by changing NUM_FRAMEBUFFER_SURFACE_BUFFERS.
(In reply to Sotaro Ikeda [:sotaro] from comment #31)
> Which device are you testing? If it is on flame device, it seems better if
> HWComposer is enabled. If it is not it is better to enable it by applyig Bug
> 1006152.

This is on a Hamachi.
For the record, I've been using Hamachi because I don't have a Flame and we (APZ and homescreen 2.0 folks) decided that we would use the Hamachi as our reference device for this work. However if we really don't care about Hamachi for 2.0 I can stop working on this for and pick up a Flame from the office next week to see if it has the same problem.
(In reply to Sotaro Ikeda [:sotaro] from comment #31)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #30)
> > Btw, here's a profile taken with the above patch applied. Any thoughts on
> > how I can figure out why the SwapBuffers is stalling?
> > 
> > http://people.mozilla.org/~bgirard/cleopatra/
> > #report=a93afd07e55a6dc191eac3ec2f395d751b054fe1

Not sure exactly what is meant by "SwapBuffers is stalling". I do see on this profile that CompositorOGL::EndFrame, which is the caller of SwapBuffers, does take a long time. That is actually desirable. For one thing, it's where we wait for vsync. Most of the time under CompositorOGL::EndFrame is spend under __futex_syscall3 and __ioctl i.e. waiting on some kernel-driven event. My guess is that that's the waiting for vsync i.e. the system is working as intended.

What would be abnormal is if we spend a lot of CPU time in the earlier, actual drawing calls made by the compositor (DrawQuad*). But if we count the samples spent in DrawQuad* in this profile, we see that it's only a small minority. That's because graphics operations (GL calls) are asynchronous, so actual work occurs *after* the drawing function that we call has returned. So at least that part seems fine :)
... or other things besides DrawQuad, such as the GrallocTextureSourceOGL::BindTexture function that does the fEGLImageTargetTexture2D call... but none of that seems prominent in this profile.
(In reply to Benoit Jacob [:bjacob] from comment #35)
> Not sure exactly what is meant by "SwapBuffers is stalling". I do see on
> this profile that CompositorOGL::EndFrame, which is the caller of
> SwapBuffers, does take a long time. That is actually desirable. For one
> thing, it's where we wait for vsync. Most of the time under
> CompositorOGL::EndFrame is spend under __futex_syscall3 and __ioctl i.e.
> waiting on some kernel-driven event. My guess is that that's the waiting for
> vsync i.e. the system is working as intended.

Ok, taking a step back then, the problem is that I still see jank while scrolling, and we're having composites that are taking > 16ms. These long composites seem to be spending a lot of time in SwapBuffers(). So I guess it's possible that our pre-SwapBuffers work is taking too long and we're missing the vsync, causing us to miss frames. I'd like to find out why this is happening.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #34)
> For the record, I've been using Hamachi because I don't have a Flame and we
> (APZ and homescreen 2.0 folks) decided that we would use the Hamachi as our
> reference device for this work. However if we really don't care about
> Hamachi for 2.0 I can stop working on this for and pick up a Flame from the
> office next week to see if it has the same problem.

ICS device seems still important to check if lock/unlock calls are correct. In JB, we do not have a genlock. Good thing about it is we do not care about the genlock failure. But rendering could corrupt without error detection on JB/KK device.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #37)
> Ok, taking a step back then, the problem is that I still see jank while
> scrolling, and we're having composites that are taking > 16ms. These long
> composites seem to be spending a lot of time in SwapBuffers(). So I guess
> it's possible that our pre-SwapBuffers work is taking too long and we're
> missing the vsync, causing us to miss frames. I'd like to find out why this
> is happening.

I added some code to time the call to SwapBuffers in CompositorOGL::EndFrame and print it out. As mentioned in comment 6, I see minimal jank while scrolling through the "A" section of the contacts, but I do see jank while scrolling in the "B" section, so I logged all the swapbuffer times for one fling in each section and ran the numbers:

Scrolling in "A" (minimal visual jank):
Samples: 151
Average:       6.20
Stddev:       4.13
Max: 41.4734
Min: 3.71833

Scrolling in "B" (significant visual jank):
Samples: 175
Average:       7.07
Stddev:       4.85
Max: 43.8199
Min: 3.07833

The averages are in milliseconds, and the two sets of numbers seem pretty close. So maybe it's not SwapBuffers that's the culprit at all here, but it just looks that way from the profile. The stats for "all SwapBuffers that took longer than 16ms" are also quite similar.
Comment on attachment 8420102 [details] [diff] [review]
Add a RepeatedTextureBindHint to avoid unnecessary fEGLImageTargetTexture2D'ing

This patch could not reduce to call gl()->fEGLImageTargetTexture2D(). Tiling layer does not use CompositableBackendData. The related code is the following.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/CompositableHost.cpp#207

Therefore, GrallocTextureSourceOGL under tiling layer uses temporary texutre. For each GrallocTextureSourceOGL::Lock() call, GrallocTextureHost get new open gl texture and calling fEGLImageTargetTexture2D().

I feel that without changing the way of the OpengGL texture recycling, we can not remove to call fEGLImageTargetTexture2D().
I used the patch to confirm how attachment 8420102 [details] [diff] [review] works.
By applying attachment 8420178 [details] [diff] [review], got logcat during Setting app's scrolling.
For tiling layer, we need one opengl texture for one tile on b2g. If the tile is not updated, we do not need to call fEGLImageTargetTexture2D() between composition.
(In reply to Sotaro Ikeda [:sotaro] from comment #40)
> Comment on attachment 8420102 [details] [diff] [review]
> Add a RepeatedTextureBindHint to avoid unnecessary
> fEGLImageTargetTexture2D'ing
> 
> This patch could not reduce to call gl()->fEGLImageTargetTexture2D(). Tiling
> layer does not use CompositableBackendData. The related code is the
> following.

It's not the tiling layer that's causing the problem. There is a non-tiled layer that is being split into a complex region as a result of the low-res buffer (in)validation code. That non-tiled layers is the one causing the problem.

If you want to reproduce the problem exactly as I'm seeing it, you need to apply at minimum:
- the 4 patches on bug 1001438 (really you just need the first three for b2g)
- the patch on bug 897996 to enable low-res painting

Then scrolling down in the contacts app on Hamachi you'll see the repeated calls to fEGLImageTargetTexture2D. I imagine that this will happen in other scenarios as well on non-tiled layers that get split into complex regions and are backed by a GrallocTextureSourceOGL, because they will also go through the same code path (described in comment 11).

> I feel that without changing the way of the OpengGL texture recycling, we
> can not remove to call fEGLImageTargetTexture2D().

If you have a specific change in mind I'd be happy to try to implement it. Just bear in mind I'm still learning how all this works so I might need some guidance.
This comment is not related to tile layer. If CompositableBackendData is present, it seems better to move calling fEGLImageTargetTexture2D() and some binding code to CompositableBackendData. CompositableBackendData owns open gl texture. It could know if we can skip fEGLImageTargetTexture2D().
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #44)
> (In reply to Sotaro Ikeda [:sotaro] from comment #40)
> > Comment on attachment 8420102 [details] [diff] [review]
> > Add a RepeatedTextureBindHint to avoid unnecessary
> > fEGLImageTargetTexture2D'ing
> > 
> > This patch could not reduce to call gl()->fEGLImageTargetTexture2D(). Tiling
> > layer does not use CompositableBackendData. The related code is the
> > following.
> 
> It's not the tiling layer that's causing the problem. There is a non-tiled
> layer that is being split into a complex region as a result of the low-res
> buffer (in)validation code. That non-tiled layers is the one causing the
> problem.

If it is not about the tiled layer, I like Comment 45 approach.
(In reply to Sotaro Ikeda [:sotaro] from comment #45)
> This comment is not related to tile layer. If CompositableBackendData is
> present, it seems better to move calling fEGLImageTargetTexture2D() and some
> binding code to CompositableBackendData. CompositableBackendData owns open
> gl texture. It could know if we can skip fEGLImageTargetTexture2D().

Implementation approach is like the following code(SurfaceTexture::updateTexImage()).

http://androidxref.com/4.0.4/xref/frameworks/base/libs/gui/SurfaceTexture.cpp#749
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #44)
> (In reply to Sotaro Ikeda [:sotaro] from comment #40)
> > Comment on attachment 8420102 [details] [diff] [review]
> > Add a RepeatedTextureBindHint to avoid unnecessary
> > fEGLImageTargetTexture2D'ing
> > 
> > This patch could not reduce to call gl()->fEGLImageTargetTexture2D(). Tiling
> > layer does not use CompositableBackendData. The related code is the
> > following.
> 
> It's not the tiling layer that's causing the problem. There is a non-tiled
> layer that is being split into a complex region as a result of the low-res
> buffer (in)validation code. That non-tiled layers is the one causing the
> problem.

Sorry, I misunderstood about the cause of the problem.
No worries. I'll try and implement it as you suggested. Thanks!
> 
> > I feel that without changing the way of the OpengGL texture recycling, we
> > can not remove to call fEGLImageTargetTexture2D().
> 
> If you have a specific change in mind I'd be happy to try to implement it.
> Just bear in mind I'm still learning how all this works so I might need some
> guidance.

I just have some vague possible ideas, not so clear yet. It is like the following.

- Extend the temporary open gl texutre usage's lifetime beyond each composition.
  + Current implementation got new open gl texure for each GrallocTextureSourceOGL::Lock() call.
  + One idea is hold the texture in CompositableBackendData.
    Other idea is hold the texture in GrallocTextureSourceOGL.
- In using CompositableBackendData case, implement CompositableBackendSpecificData derived class.
  Set the derived class to GrallocTextureHost.

The tricky part is that TileHost seems to be re-created for each rendering update.
And when the gralloc buffer's ownership is returned to the client, GrallocTextureHost have to unbound open gl texture.
Comment 50 is about tiling case.
(In reply to Sotaro Ikeda [:sotaro] from comment #50)
> The tricky part is that TileHost seems to be re-created for each rendering
> update.
> And when the gralloc buffer's ownership is returned to the client,
> GrallocTextureHost have to unbound open gl texture.

In normal CompositableHost cases except tiling, new gralloc buffer replaces the old gralloc buffer's binding.
Attachment #8420102 - Flags: review?(sotaro.ikeda.g)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #39)

Just to note, the reason I'm still seeing jank after applying the patch above is because *dun dun dunnnn* we're scrolling synchronously! I probably should have realized this earlier, but the layers dump shows that the ContainerLayerComposite that has the APZC attached has no children once I scroll into the "B" section of the contact list. The scroll layer basically turns into a scroll info layer. I'll have to look at the display list to figure out why. I'll file a separate bug to follow that up and dig in.
Attached patch Avoid unnecessary fEGLImageTargetTexture2D'ing (obsolete) (deleted) — — Splinter Review 
Is this more what you had in mind?
Attachment #8420102 - Attachment is obsolete: true
Attachment #8420324 - Flags: review?(sotaro.ikeda.g)
Filed bug 1008412 to track the thing in comment 53.
Comment on attachment 8420324 [details] [diff] [review]
Avoid unnecessary fEGLImageTargetTexture2D'ing

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

I have the following comments to the patch.

- There seems a possiblity that CompositableDataGonkOGL stores an already deleted EGLImage.
- CompositableDataGonkOGL seems not handle OpenGL texture recreation use case.
    + Current implementation might not have such use case.
    + But as a implementation it seems better to handle it.
- The following use case seems not handled.
    + One TextureHost is used by Multiple CompositableHost. 
    + On media framework via ImageLayer, this could happen.

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +429,5 @@
>    return mTexture;
>  }
>  
>  void
> +GrallocTextureSourceOGL::SetGLTextureImage(bool aMayBeRedundant)

I like redundancy check that does not need "aMayBeRedundant".

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +176,5 @@
>  
> +bool
> +CompositableDataGonkOGL::IsEGLImageAlreadyBound(EGLImage aImage)
> +{
> +  return mBoundEGLImage == aImage;

This check seems not handle CompositableDataGonkOGL's open gl texture deleted use case.
Attachment #8420324 - Flags: review?(sotaro.ikeda.g) → review-
Keywords: perf
Priority: -- → P2
Whiteboard: [c=handeye p= s= u=]
Attached patch Avoid unnecessary fEGLImageTargetTexture2D'ing (obsolete) (deleted) — — Splinter Review 
(In reply to Sotaro Ikeda [:sotaro] from comment #56)
> - There seems a possiblity that CompositableDataGonkOGL stores an already
> deleted EGLImage.

Good point, I added code to null out the stored EGLImage pointer in CompositableDataGonkOGL if the GrallocTextureSource deletes its copy.

> - CompositableDataGonkOGL seems not handle OpenGL texture recreation use
> case.
>     + Current implementation might not have such use case.
>     + But as a implementation it seems better to handle it.

Also added code to clear the image in DeleteTextureIfPresent(). I assume this is what you meant, but if not please clarify.

> - The following use case seems not handled.
>     + One TextureHost is used by Multiple CompositableHost. 
>     + On media framework via ImageLayer, this could happen.

I don't see why this is a problem. If there are multiple CompositableHost instances using the same GrallocTextureSourceOGL instance, then there will be only a single CompositableDataGonkOGL instance also. This should be a pretty simple case that is already handled, because there are no new ways in which the cache could be invalidated.

> >  void
> > +GrallocTextureSourceOGL::SetGLTextureImage(bool aMayBeRedundant)
> 
> I like redundancy check that does not need "aMayBeRedundant".

Do you want me to get rid of the parameter entirely? It seemed to me that there are some calls to SetGLTextureImage where we want to force the fEGLImageTargetTexture2D call regardless, such as the one in SetCompositableBackendSpecificData, which is why I left that parameter in. If you think we should just always do the redundancy check and that will be safe then I can take out the aMayBeRedundant parameter.

> > +bool
> > +CompositableDataGonkOGL::IsEGLImageAlreadyBound(EGLImage aImage)
> > +{
> > +  return mBoundEGLImage == aImage;
> 
> This check seems not handle CompositableDataGonkOGL's open gl texture
> deleted use case.

Should be handled in the new patch, as mBoundEGLImage is nulled out when the GL texture is deleted.
Attachment #8420324 - Attachment is obsolete: true
Attachment #8420948 - Flags: review?(sotaro.ikeda.g)
Also with bug 1008412 fixed, I don't even see this problem anymore. With that bug fixed the scenario I described in comment 11 doesn't happen, as the regions just have a single rect. I think this is still a bug that we should fix but it's not blocking low-res painting anymore.
No longer blocks: 897996
> 
> > - The following use case seems not handled.
> >     + One TextureHost is used by Multiple CompositableHost. 
> >     + On media framework via ImageLayer, this could happen.
> 
> I don't see why this is a problem. If there are multiple CompositableHost
> instances using the same GrallocTextureSourceOGL instance, then there will
> be only a single CompositableDataGonkOGL instance also. This should be a
> pretty simple case that is already handled, because there are no new ways in
> which the cache could be invalidated.

If there are multiple CompositableHost instances using the same GrallocTextureSourceOGL instance, there are multiple CompositableDataGonkOGL instances.

CompositableDataGonkOGL exists per CompositableHost. ImageHost::Composite() calls the following before drawing to change a CompositableDataGonkOGL instance.

> mFrontBuffer->SetCompositableBackendSpecificData(GetCompositableBackendSpecificData());

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ImageHost.cpp#82
> > >  void
> > > +GrallocTextureSourceOGL::SetGLTextureImage(bool aMayBeRedundant)
> > 
> > I like redundancy check that does not need "aMayBeRedundant".
> 
> Do you want me to get rid of the parameter entirely? It seemed to me that
> there are some calls to SetGLTextureImage where we want to force the
> fEGLImageTargetTexture2D call regardless, such as the one in
> SetCompositableBackendSpecificData, which is why I left that parameter in.
> If you think we should just always do the redundancy check and that will be
> safe then I can take out the aMayBeRedundant parameter.

My concern is that "aMayBeRedundant" does not help to detect a situation in Comment 59. And if we handle Comment 59 correctly, "aMayBeRedundant" seems not necessary.
(In reply to Sotaro Ikeda [:sotaro] from comment #59)
> If there are multiple CompositableHost instances using the same
> GrallocTextureSourceOGL instance, there are multiple CompositableDataGonkOGL
> instances.
> 
> CompositableDataGonkOGL exists per CompositableHost. ImageHost::Composite()
> calls the following before drawing to change a CompositableDataGonkOGL
> instance.

Ah I see. It seems a bit odd that it's architected this way because if I understand correctly the same image will get bound to multiple different textures.

> 
> > mFrontBuffer->SetCompositableBackendSpecificData(GetCompositableBackendSpecificData());
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ImageHost.
> cpp#82

(In reply to Sotaro Ikeda [:sotaro] from comment #60)
> My concern is that "aMayBeRedundant" does not help to detect a situation in
> Comment 59. And if we handle Comment 59 correctly, "aMayBeRedundant" seems
> not necessary.

I don't follow. I think the "aMayBeRedundant" does actually detect the situation in comment 59. My changes to SetCompositableBackendSpecificData pass "aMayBeRedundant" as false, and so the call to fEGLImageTargetTexture2D will be forced. There should not be any cases where a necessary call to fEGLImageTargetTexture2D is skipped, and there shouldn't be any cases where the cache state is rendered invalid. Can you explain the exact steps that you think are problematic? I agree that if we handle the SetCompositableBackendSpecificData case in some other way then we should be able to get rid of aMayBeRedundant, but I don't know what other way to handle it.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #61)
> > 
> > CompositableDataGonkOGL exists per CompositableHost. ImageHost::Composite()
> > calls the following before drawing to change a CompositableDataGonkOGL
> > instance.
> 
> Ah I see. It seems a bit odd that it's architected this way because if I
> understand correctly the same image will get bound to multiple different
> textures.

Yeah, I know. Current implementation does not handle Comment 59 correctly. But at this time there is no other good way to handle this situation. If we want to unbind gralloc buffer from texture, we have only 2 choices.
- [1] Bind new gralloc buffer
- [2] delete texture.

[2]"delete texture" costs a lot, Therefore, by using CompositableDataGonkOGL [1] is implemented. Though, [1] have a problem in the situation in Comment 59.
Comment on attachment 8420948 [details] [diff] [review]
Avoid unnecessary fEGLImageTargetTexture2D'ing

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

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +160,5 @@
>    gl()->fBindTexture(textureTarget, mTexture);
>    if (!mEGLImage) {
>      mEGLImage = EGLImageCreateFromNativeBuffer(gl(), mGraphicBuffer->getNativeBuffer());
>    }
> +  SetGLTextureImage(false);

This fEGLImageTargetTexture2D() is for mCompositableBackendData not present case. This path is not used when mCompositableBackendData is present. This call is for tiling use case. Tiling does not use mCompositableBackendData now.
Comment 63 is for GrallocTextureSourceOGL::Lock() change.
Ok, for that one I can go back to calling fEGLImageTargetTexture2D directly. Although I don't see the harm in calling SetGLTextureImage(false) because it will take the code path where mCompositableBackendData is null anyway, which is effectively the same thing.
I just do not understand well about the necessity of aMayBeRedundant. If we unify SetBoundEGLImage() and calling fEGLImageTargetTexture2D(), IsEGLImageAlreadyBound() can be used to detect whether calling fEGLImageTargetTexture2D() is necessary. How do you think about it?
(In reply to Sotaro Ikeda [:sotaro] from comment #66)
> I just do not understand well about the necessity of aMayBeRedundant. If we
> unify SetBoundEGLImage() and calling fEGLImageTargetTexture2D(),
> IsEGLImageAlreadyBound() can be used to detect whether calling
> fEGLImageTargetTexture2D() is necessary. How do you think about it?

Also binding texture.
(In reply to Sotaro Ikeda [:sotaro] from comment #50)
> > 
> > > I feel that without changing the way of the OpengGL texture recycling, we
> > > can not remove to call fEGLImageTargetTexture2D().
> > 
> > If you have a specific change in mind I'd be happy to try to implement it.
> > Just bear in mind I'm still learning how all this works so I might need some
> > guidance.
> 
> I just have some vague possible ideas, not so clear yet. It is like the
> following.
> 
> - Extend the temporary open gl texutre usage's lifetime beyond each
> composition.
>   + Current implementation got new open gl texure for each
> GrallocTextureSourceOGL::Lock() call.
>   + One idea is hold the texture in CompositableBackendData.
>     Other idea is hold the texture in GrallocTextureSourceOGL.
> - In using CompositableBackendData case, implement
> CompositableBackendSpecificData derived class.
>   Set the derived class to GrallocTextureHost.
> 
> The tricky part is that TileHost seems to be re-created for each rendering
> update.
> And when the gralloc buffer's ownership is returned to the client,
> GrallocTextureHost have to unbound open gl texture.

The above is about tiling layer, tiling layer uses recycling callback. The recycling callback might be used to detect "the gralloc buffer's ownership is returned to the client".
Attached patch Avoid unnecessary fEGLImageTargetTexture2D'ing (obsolete) (deleted) — — Splinter Review 

        Attachment #8420948 -
        Attachment is obsolete: true

        Attachment #8420948 -
        Flags: review?(sotaro.ikeda.g)

        Attachment #8421337 -
        Flags: review?(sotaro.ikeda.g)

    
    

    
  
I locally confirmed how attachment 8421337 [details] [diff] [review] works on master hamachi. It caused a lot of genlock failure happens in the following STR.

[1] Install youtube app from Marketplace.
[2] Start youtube app
[3] Select youtube video
[4] Playback youtube video.
[5] During youtube playback push home button.
     youtube app is set to backgournd
[6] Tap youtube app icon.
     youtube app comes to forgorunnd.

repeat [4][5][6] until video playback update stop(audio only). In the logcat, genlock failures are printed.

By applying attachment 8421337 [details] [diff] [review], genlock failures becomes significant. But the a few genlock failure happens by the above STR since b2g v1.4.

    
    

    
  
Based on Comment 71, nominate to b2g "v1.4+". This genlock failure could affect to video playback and camera preview performance.
blocking-b2g: --- → 1.4?

    
    

    
  
I seems to understand how to fix Comment 71.

    
    

    
  
Comment on attachment 8421337 [details] [diff] [review]
Avoid unnecessary fEGLImageTargetTexture2D'ing

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

The patch becomes better! But have a problem on hamachi device. By comment is based on the confirmation on the device.

mCompositableBackendData wil be used in GrallocTextureSourceOGL::DeallocateDeviceData().
In GrallocTextureSourceOGL::SetCompositableBackendSpecificData), Updating mCompositableBackendData need to be after DeallocateDeviceData() call.

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +188,5 @@
> +void
> +CompositableDataGonkOGL::ClearBoundEGLImage(EGLImage aImage)
> +{
> +  if (mBoundEGLImage == aImage) {
> +    mBoundEGLImage = EGL_NO_IMAGE;

Need to delete OpenGL tecture to unbind gralloc buffer.
Even when EGLImage is deleted, if OpenGL texture is present, the gralloc buffer is still bound to the OpenGL texture.

Another way of unbinding is use dummy gralloc buffer. In this bug, just removing OpenGL texture is easier.

        Attachment #8421337 -
        Flags: review?(sotaro.ikeda.g) → review-

    
    

    
  
(In reply to Sotaro Ikeda [:sotaro] from comment #73)
> I seems to understand how to fix Comment 71.

Please feel free to steal this bug, Sotaro. It is no longer blocking my work on low-res and progressive painting so I probably won't be spending much more time on this.

    
    

    
  
Okey, I take a bug.

    
  
Assignee: nobody → sotaro.ikeda.g

    
  
Priority: P2 → --

    
  
Summary: With low-precision buffer enabled, binding textures sometimes takes a long time → binding textures sometimes takes a long time

    
    

    
  
QA Wanted - how many times do you need to repeat steps called out in comment 71 before this reproduces on 1.4? Also, does this reproduce on 1.3?
Keywords: qawanted

    
    

    
  
(In reply to Jason Smith [:jsmith] from comment #77)
> QA Wanted - how many times do you need to repeat steps called out in comment
> 71 before this reproduces on 1.4? Also, does this reproduce on 1.3?

To confirm the genlock failure, need to get logcat. Significant problem as in comment 71 happens only when applying attachment 8421337 [details] [diff] [review].

    
    

    
  


  
Fix genlock failure.

        Attachment #8421337 -
        Attachment is obsolete: true

    
    

    
  


  
Fix nits.

        Attachment #8422150 -
        Attachment is obsolete: true

    
  

        Attachment #8422152 -
        Flags: review?(nical.bugzilla)

    
    

    
  
(In reply to Jason Smith [:jsmith] from comment #77)
> QA Wanted - how many times do you need to repeat steps called out in comment
> 71 before this reproduces on 1.4? Also, does this reproduce on 1.3?

If the ROM does not have no modification, the following genlock failure log appear when the youtube app go to background.

>05-13 21:42:21.909   141   586 E libgenlock: perform_lock_unlock_operation: GENLOCK_IOC_DREADLOCK failed (lockType0x1, err=Connection timed out fd=55)
>05-13 21:42:21.909   141   586 E omx_vdec: Failed to acquire genlock, ret = 1

    
    

    
  
Comment on attachment 8422152 [details] [diff] [review]
patch - Avoid unnecessary fEGLImageTargetTexture2D'ing

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

r=me with SetGLTextureImage renamed into BindEGLImage.

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +435,5 @@
>    return mTexture;
>  }
>  
>  void
> +GrallocTextureSourceOGL::SetGLTextureImage()

to me, GLTextureImage means http://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLTextureImage.h

Let's find another name for this. How about BindEGLImage just like the BackendSpecificData?
BindEGLImage in BackendSpecificData is also doing the same thing (calling fEGLImageTargetTexture2D, with the extra optimization of avoiding the call when possible), so there seem to be no reason to name these two methods differently.

        Attachment #8422152 -
        Flags: review?(nical.bugzilla) → review+

    
    

    
  
:mikeh, in the past, you are talking about genlock failure that happened on camera. This bug seems to fix the problem.
Flags: needinfo?(mhabicher)

    
    

    
  
(In reply to Nicolas Silva [:nical] from comment #82)
> Comment on attachment 8422152 [details] [diff] [review]
> patch - Avoid unnecessary fEGLImageTargetTexture2D'ing
> 
> Review of attachment 8422152 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with SetGLTextureImage renamed into BindEGLImage.

I will update the name to BindEGLImage.

    
    

    
  


  
Apply the comment. Carry "r=nical".

        Attachment #8422152 -
        Attachment is obsolete: true

        Attachment #8422533 -
        Flags: review+

    
  
Severity: normal → blocker
Status: NEW → ASSIGNED
blocking-b2g: 1.4? → 1.4+
Priority: -- → P1
Whiteboard: [c=handeye p= s= u=] → [c=handeye p= s= u=1.4]
Keywords: qawanted

    
  
Blocks: 1010966

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/6e20f63f33a6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32

    
  
Flags: needinfo?(mhabicher)

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: