Closed
Bug 1077301
Opened 10 years ago
Closed 10 years ago
Cleanup gralloc textures code.
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: nical, Assigned: nical)
References
Details
Attachments
(2 files, 14 obsolete files)
83.57 KB,
patch
|
Details | Diff | Splinter Review | |
912 bytes,
patch
|
aosmond
:
review+
|
Details | Diff | Splinter Review |
Our gralloc code has progressively evolved into becoming complex and hard to understand. Now is a good time to simplify it and factor the solutions to the different problems we have solved in the past two years into something simpler that takes these problems into account. The solution Sotaro and I discussed is consists into splitting the TextureHost and TextureSource implementation for the gralloc backend so that the analogy "TextureHost/TextureSource" maps directly to "GraphicBuffer/GLTextureHandle". We are already close to this except that currently TextureSource is strictly oned by TextureHost and to reach our goal we need to relax this constraint a bit. The first step is to replace the method TextureSource* TextureHost::GetTextureSources() by bool TextureHost::BindTextureSource(TextureSourceRef& aTextureSource) which is more flexible (gives the TextureHost the oportunity to attach to an already existing TextureSource if present, and lets us remove helper classes such as CompositableBackendSpecificData which contribute to the complexity of the current code. An additional method TextureHost::UnbindTexture() is added to let notify the TextureHost when the current TextureSource it is bound to, is being bound to another TextureHost. For non-gralloc textures, the implementation of BindTextureSources can simply remain something like: aTextureSource = this->GetTextureSources(); For gralloc, however, we can centralise most of the tricky logic in this function and have the opportunity to do the various optimisations we want to do. More detailed explanation in this pad: https://etherpad.mozilla.org/texturesource This is purely a maintainability issue. The goal of this bug is that after this we make roughly the same gralloc/gl calls that we currently do but with simpler code, and be able to build optimizations on top of this as followups.
Assignee | ||
Comment 1•10 years ago
|
||
Prototype put together in the train, needs cleanups and testing, seems to work on linux, but doesn't implement the important part (gralloc stuff).
Assignee | ||
Comment 2•10 years ago
|
||
Also made in the train, sort of works but not really (I see things on the screen but get a lot of black flashes), but gives an idea of what the code should be like.
Assignee | ||
Comment 3•10 years ago
|
||
The problem was caused by a silly mistake. This patch seems to work (still not implemented for tiling though) on the flame (with my very limited testing).
Attachment #8499415 -
Attachment is obsolete: true
Updated•10 years ago
|
OS: Linux → Gonk (Firefox OS)
Assignee | ||
Comment 4•10 years ago
|
||
I had to write a bunch of code so that the LayerScope code paths also use the new api, and also had to copy-paste ContentHostBase::Composite into ContentHostIncremental::Composite so that the specifics of incremental content host get out of the way (this is okay because incremental content host should be removed soon-ish). As a result the patch is a bit bigger than I hoped but the changes are fairly mechanical. What's important is that this introduces the BindTextureSource method which, if not overridden, defaults to calling GetTextureSources internally.
Assignee: nobody → nical.bugzilla
Attachment #8499410 -
Attachment is obsolete: true
Attachment #8501355 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 5•10 years ago
|
||
The interesting part: all of the gralloc code goes into one function and the function is actually quite simple in comparison with our current gralloc logic. This has worked well for me on the flame so far but I haven't run the test suite on it yet. Tiling doesn't use this code yet but all of the other layer types do.
Attachment #8499421 -
Attachment is obsolete: true
Attachment #8501359 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 6•10 years ago
|
||
This is the reward: removing the code that is made obsolete by the previous two patches.
Attachment #8501361 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8501359 [details] [diff] [review] 2) Implement BindTextureSource for gralloc Turning review into feedback request because there are some issues.
Attachment #8501359 -
Flags: review?(sotaro.ikeda.g) → feedback?(sotaro.ikeda.g)
Assignee | ||
Updated•10 years ago
|
Attachment #8501361 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 8•10 years ago
|
||
So I think that I am close but there is something I am missing. A) If I have the TextureSource keep the EGLImage, it means that in the common case there is one EGLImage+gl handle pair per compositable, and successions of GraphicBuffers get attached to them one frame after the other. B) If I store the EGLImage in the TextureHost and keep the gl handle in the TextureSource, in the common case each GraphicBuffer has its own EGLImage and the pairs GraphicBuffer+EGLImage get attached to the gl texture handle that is used by the compositable. I first tried A) and there are some rendering issues, then I tried B) and almost nothing works. Interestingly the way we currently do tiling looks more like B so there must be a little somthing I missed. - in the case of A) 1st frame glGenTextures glActiveTexture glBindTexture glTexParameteri glTexParameteri gl->EGLImageTargetTexture2D; 2nd frame, flipping back and front buffers glActiveTexture glBindTexture // with the previous gl handle gl->EGLImageTargetTexture2D; 3rd frame, flipping back and front buffers glActiveTexture glBindTexture // with the previous gl handle gl->EGLImageTargetTexture2D; etc... when destroying the layer glDeleteTextures destroy the egl image
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8501355 -
Attachment is obsolete: true
Attachment #8501359 -
Attachment is obsolete: true
Attachment #8501355 -
Flags: review?(sotaro.ikeda.g)
Attachment #8501359 -
Flags: feedback?(sotaro.ikeda.g)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8501361 -
Attachment is obsolete: true
Comment 12•10 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #8) > So I think that I am close but there is something I am missing. > > A) If I have the TextureSource keep the EGLImage, it means that in the > common case there is one EGLImage+gl handle pair per compositable, and > successions of GraphicBuffers get attached to them one frame after the other. > B) If I store the EGLImage in the TextureHost and keep the gl handle in the > TextureSource, in the common case each GraphicBuffer has its own EGLImage > and the pairs GraphicBuffer+EGLImage get attached to the gl texture handle > that is used by the compositable. > > I first tried A) and there are some rendering issues, then I tried B) and > almost nothing works. Interestingly the way we currently do tiling looks > more like B so there must be a little somthing I missed. Current gralloc implementation works as B). It's implementation mimics anroid's BufferQueue implementation. I think that it is important to prevent unexpected problems. In andorid, gralloc buffer and EGLImage are handle as a pair. EGL Image is created when gralloc buffer is bound to OpneGL texture, and the EGLImage is deleted when gralloc buffer is released or the BufferQueue is detached from a OpenGL context.
Comment 13•10 years ago
|
||
From the patches, I am not sure about the following. - how one OpenGL texture is shared among TextureHosts that are managed under one CompositableHost. - how TextureHost is going to own OpenGL texture when it detect that is owned by multiple ImageHosts.
Comment 14•10 years ago
|
||
UseTextureHost() calls UnbindTextureSource() at first. It seems to break current "OpenGL binding replacement by new Texture Host".
Comment 15•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #12) > > In andorid, gralloc buffer and EGLImage are handle as a pair. EGL Image is > created when gralloc buffer is bound to OpneGL texture, and the EGLImage is > deleted when gralloc buffer is released or the BufferQueue is detached from > a OpenGL context. EGLImage is bound to OpenGL texture, when the corresponding gralloc buffer is updated and rendered.
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #14) > UseTextureHost() calls UnbindTextureSource() at first. It seems to break > current "OpenGL binding replacement by new Texture Host". UnbindTextureSource currently only clears the mEGLTextureSource reference in GrallocTextureHost, so that the latter knows that it will need to rebind next time it is rendered. It doesn't actually do any unlocking. Unlocking will happen either when another TextureHost is bound to the TextureSource, or when the TextureSource's ref count goes to zero and the gl texture is deleted.
Assignee | ||
Comment 17•10 years ago
|
||
\o/ The reason B) didn't work is that I stupidly forgot to initialize a variable in a constructor. Patches coming soon!
Comment 18•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #13) > From the patches, I am not sure about the following. > - how one OpenGL texture is shared among TextureHosts that are managed > under one CompositableHost. > - how TextureHost is going to own OpenGL texture when it detect that is > owned by multiple ImageHosts. From the patches, it seems to be handled by GrallocTextureHostOGL::BindTextureSource().
Assignee | ||
Comment 19•10 years ago
|
||
This version works great, and is optimal as far as I understand how gralloc works (we only ever bind when we need to). The nice part of going for solution B is that we already had GLTextureSource so I didn't even need to create a new TextureSource class. I ended up folding the patches because of the way fixes ended up mixed around, let me now if you would like me to split the new code away from the removal of the old code for the review. Sotaro, I added some comments in the code that try to answer your questions. As you guessed, all of the interesting logic is in BindTextureSource
Attachment #8502548 -
Attachment is obsolete: true
Attachment #8502549 -
Attachment is obsolete: true
Attachment #8503162 -
Flags: feedback?(sotaro.ikeda.g)
Assignee | ||
Comment 20•10 years ago
|
||
With the hwcomposer part fixed.
Attachment #8503162 -
Attachment is obsolete: true
Attachment #8503162 -
Flags: feedback?(sotaro.ikeda.g)
Attachment #8503169 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Updated•10 years ago
|
Attachment #8502546 -
Flags: review?(sotaro.ikeda.g)
Comment 21•10 years ago
|
||
I have two comment to the current patches. - In current gecko, CompositableHost::UseTextureHost() replace OpenGL texture's EGLImage binding. But the patch seems not update the binding. Then, gralloc buffers could return to client side without unbinding it. It breaks how adnroid's gralloc buffer works and it cause to hamachi device genlock failures. - BindTextureSource() could be called several times for same TextureSource and TextureHost pairs. If it is already bound, it should skip the binding. calling fEGLImageTargetTexture2D() is relatively heavy.
Updated•10 years ago
|
Attachment #8502546 -
Flags: review?(sotaro.ikeda.g)
Updated•10 years ago
|
Attachment #8503169 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #21) > I have two comment to the current patches. > > - In current gecko, CompositableHost::UseTextureHost() replace OpenGL > texture's EGLImage binding. > But the patch seems not update the binding. > Then, gralloc buffers could return to client side without unbinding it. > It breaks how adnroid's gralloc buffer works and it cause to hamachi > device genlock failures. It's the BindTextureSource of TextureHost N+1 that unlocks TextureHost N, or when the TextureSource is destroyed. If TextureHost N returns to the client side before Texture N+1 is used for compositing you are in trouble anyway, because the GPU is still using TextureHost N. If you forcefully unbind TextureHost N without binding TextureHost N+1, then you are also in trouble because slow things happen in the driver. Moving the binding from UseTextureHost to BindTextureSource (which happens during composition) should not make a difference because, as far as understand, the effect of fEGLImageTargetTexture2D on genlock is not synchronous, and genlock only unlocks TextureHost N when TextureHost N+1 has been composited. So I think that this patch is still correct for genlock (unless I misunderstood your comment). > > - BindTextureSource() could be called several times for same TextureSource > and TextureHost pairs. > If it is already bound, it should skip the binding. > calling fEGLImageTargetTexture2D() is relatively heavy. If BindTextureSource is called twice in a frame, it will skip the binding the second time, because it sets the GrallocTextureHostOGL::mGLTextureSource member the first time, and then we early return at the beginning of BindTextureSource the second time because mGLTextureSource is not null.
Comment 23•10 years ago
|
||
Actually genlock failure happens on hamachi device. It say that a part of the code does not work as expected.
Comment 24•10 years ago
|
||
In current gecko code, "binding/unbinding to OpenGL texture" and "OpenGL texture handling" is more explicit. By the patch, some important property becomes implicit. It seems to make more debugging difficult when the problem happens like the current patch's genlock problem.
Comment 25•10 years ago
|
||
The patches is bitrotted in latest master.
Comment 26•10 years ago
|
||
BindTextureSource seems to be called only around composing, but not around layer transaction. There is a case that a new swapped TextureHost is not used for composition, because of layer state. In this case, BindTextureSource seems not called as expected.
Comment 27•10 years ago
|
||
>
> >
> > - BindTextureSource() could be called several times for same TextureSource
> > and TextureHost pairs.
> > If it is already bound, it should skip the binding.
> > calling fEGLImageTargetTexture2D() is relatively heavy.
>
> If BindTextureSource is called twice in a frame, it will skip the binding
> the second time, because it sets the GrallocTextureHostOGL::mGLTextureSource
> member the first time, and then we early return at the beginning of
> BindTextureSource the second time because mGLTextureSource is not null.
I confirmed the above part. Thanks.
Comment 28•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #24) > In current gecko code, "binding/unbinding to OpenGL texture" and "OpenGL > texture handling" is more explicit. By the patch, some important property > becomes implicit. It seems to make more debugging difficult when the problem > happens like the current patch's genlock problem. Sorry, it depends on point of view.
Assignee | ||
Comment 29•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #28) > Sorry, it depends on point of view. No problem, I do think the new system is simpler and therefore will be easier to debug, but I may of course have gotten some stuff wrong, we'll find out and fix the mistakes if any.
Assignee | ||
Comment 30•10 years ago
|
||
I am rebasing the patches and i'll try on hamachi today
Assignee | ||
Comment 31•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #26) > BindTextureSource seems to be called only around composing, but not around > layer transaction. There is a case that a new swapped TextureHost is not > used for composition, because of layer state. In this case, > BindTextureSource seems not called as expected. Ah interesting! when is that happening, when the layer is outside the screen? When it's hidden? I guess we could move the BindTextureSource call into UseTextureHost instead of during compositing, but it I need to check how that interacts with lock/unlock
Comment 32•10 years ago
|
||
I faced it in the past, therefore current GrallocTextureHost update OpenGL texture binding during layer transaction. ContainerLayerComposite seems to skip non-shown layer at ContainerPrepare(). http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContainerLayerComposite.cpp#183
Assignee | ||
Comment 33•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #32) > I faced it in the past, therefore current GrallocTextureHost update OpenGL > texture binding during layer transaction. ContainerLayerComposite seems to > skip non-shown layer at ContainerPrepare(). > http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ > ContainerLayerComposite.cpp#183 Ok, we could add a method like "TextureHost::PrepareTextureSource(CompositableTextureSourceRef&)" which is called in UseTextureHost in the transaction, and move the code from GrallocTextureHost::BindTextureSource to there. I'd need to still have something for render time because of the D3D11 backend. How does this sound ?
Comment 34•10 years ago
|
||
That sounds good, gralloc also need another method for this, we need to defer WaitAcquireFenceSyncComplete() call as long as possible.
Assignee | ||
Comment 35•10 years ago
|
||
rebased patch.
Attachment #8502546 -
Attachment is obsolete: true
Attachment #8505526 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 36•10 years ago
|
||
rebased, and added the PrepareTextureSource method. Sotaro you were right about the genlock issue when a layer is updated but not composited. This patch fixes the problem and I don't see any genlock failure anymore with hamachi (when trying the camera, some canvas games and scrollong around the web).
Attachment #8503169 -
Attachment is obsolete: true
Assignee | ||
Comment 37•10 years ago
|
||
Comment on attachment 8505529 [details] [diff] [review] 2) gralloc texture I left the printfs in the patch so that if you want to apply it locally you can see what's happening and in what order in the logcat, (disregard them for the review).
Attachment #8505529 -
Flags: review?(sotaro.ikeda.g)
Updated•10 years ago
|
Attachment #8505526 -
Flags: review?(sotaro.ikeda.g) → review+
Comment 38•10 years ago
|
||
Comment on attachment 8505529 [details] [diff] [review] 2) gralloc texture Review of attachment 8505529 [details] [diff] [review]: ----------------------------------------------------------------- Good! I also confirmed that genlock failure did not happen on hamachi. And transPerfMod2 in Bug 1015984 could have similar performance to current master.
Attachment #8505529 -
Flags: review?(sotaro.ikeda.g) → review+
Assignee | ||
Comment 39•10 years ago
|
||
try push https://tbpl.mozilla.org/?tree=Try&rev=99e3f96dca28
Assignee | ||
Comment 40•10 years ago
|
||
Attachment #8505526 -
Attachment is obsolete: true
Attachment #8505529 -
Attachment is obsolete: true
Assignee | ||
Comment 41•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d16adf321576
Comment 42•10 years ago
|
||
Backed out for B2G ICS bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2216810527 https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3058280&repo=mozilla-inbound
Assignee | ||
Comment 43•10 years ago
|
||
fixed and relanded https://hg.mozilla.org/integration/mozilla-inbound/rev/197317c196cf
Comment 44•10 years ago
|
||
I'm getting rendering problems with this patch. Regression checking led me here. Mozilla/5.0 (Windows NT 6.3; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 GOOD ---- 20141017045355 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d96887e4712 BAD --- 20141017053755 https://hg.mozilla.org/integration/mozilla-inbound/rev/197317c196cf STR: 1. Have this patch on. 2. Go to http://www.weather.com/weather/map/interactive/07746:4?interactiveMapLayer=radar&baseMap=r&zoom=7 3. Move the pointer around and watch the screen or portions of it turn black. Also, if you hide the menu toolbar and then press the Alt key to show it you will also get the 'blackness'.
Comment 45•10 years ago
|
||
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1085038
Depends on: 1084118
Given that we already have two windows regressions filed and this hasn't even merged to central yet, I tend to think this should probably get backed out before it merges to central. Does that make sense to you?
Flags: needinfo?(nical.bugzilla)
(My suspicion is that it broke component alpha on unaccelerated Windows, or something like that.)
I decided to go for backing this out now since otherwise there's a danger it'll get merged to central and in a nightly before you see the above: https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb2a8673c20 (Relanding is easy, and better safe than sorry, I think.)
Assignee | ||
Comment 49•10 years ago
|
||
(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) (away/busy Oct. 24-31) from comment #47) > (My suspicion is that it broke component alpha on unaccelerated Windows, or > something like that.) Component alpha was indeed broken because of a copy-paste mistake. fixed try push https://tbpl.mozilla.org/?tree=Try&rev=fb0d72b9e67b
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Comment 50•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff4c7dded93
https://hg.mozilla.org/mozilla-central/rev/5ff4c7dded93
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 52•10 years ago
|
||
nical, are you going to keep the fix landed? It affects to bug 998019 land.
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Comment 53•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #52) > nical, are you going to keep the fix landed? It affects to bug 998019 land. Let's keep it landed.
Flags: needinfo?(nical.bugzilla)
Comment 54•10 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #53) > (In reply to Sotaro Ikeda [:sotaro] from comment #52) > > nical, are you going to keep the fix landed? It affects to bug 998019 land. > > Let's keep it landed. Thanks, I just want to confirm it.
Comment 55•10 years ago
|
||
Backed out for the same WinXP reftest near perma-fail it was backed out for last time. https://hg.mozilla.org/integration/mozilla-inbound/rev/ca03ec269799
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla36 → ---
Assignee | ||
Comment 56•10 years ago
|
||
Got it. There was another issue related to component alpha where if a layer went from being component alpha to not not being component alpha, a pointer wasn't cleared and the layer be considered as component alpha by the rendering code. It also explains why the problem was intermittent: the bug depended on what was rendered before rather than on what was being rendered at the time of the test. try push with half a gazillion of xp reftest retriggers: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f340788a936e
Assignee | ||
Comment 57•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/703cf7b92df4
Comment 58•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/703cf7b92df4
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 59•10 years ago
|
||
This is causing the regression in camera described in bug 1088808. From the regression window, I backed out only the above change and it is working as expected now. From the about:memory reports, it looks like the kgsl-memory/b2g/egl_image allocation keeps going up every time we take a picture.
Updated•10 years ago
|
Blocks: gfx-target-2.2
Comment 60•10 years ago
|
||
nical: Based on my earlier comments there appearing to be a egl image leak, I tested this patch with camera and the problem seems to be resolved. Of course, I'm not sure if there are other side effects since my GL knowledge is a bit weak :). At least this should help point you in the right area if it isn't enough on its own.
Attachment #8517478 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 61•10 years ago
|
||
Comment on attachment 8517478 [details] [diff] [review] Fix EGL image leak, v1 Review of attachment 8517478 [details] [diff] [review]: ----------------------------------------------------------------- Ouch, that's something I forgot when I moved the EGLImage form TextureSource to TextureHost. Good catch.
Attachment #8517478 -
Flags: review?(nical.bugzilla) → review+
Comment 62•10 years ago
|
||
Adding proper header to patch file for submit.
Attachment #8517478 -
Attachment is obsolete: true
Attachment #8517513 -
Flags: review+
Comment 63•10 years ago
|
||
try for rollup p2: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0502c4958170
Updated•10 years ago
|
Status: REOPENED → ASSIGNED
Comment 64•10 years ago
|
||
checkin-needed for attachment 8517513 [details] [diff] [review]
Keywords: checkin-needed
Comment 65•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/66be6ec6d514
Keywords: checkin-needed
Comment 67•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/66be6ec6d514
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•