Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1657003003:
Use eglBindAPI to choose the correct API (Closed)
|
Created:
4 years, 10 months ago by Sami Väisänen Modified:
4 years, 10 months ago CC:
chromium-reviews, kalyank, ozone-reviews_chromium.org, boliu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse eglBindAPI to choose the correct (EGL_ES) API
If the calling thread has chosen another EGL API before
the call to eglCreateContext can fail surprisingly,
so a call to eglBindAPI is needed before eglCreateContext.
BUG=skia:2992
Committed: https://crrev.com/8430468e13d9b71231c0febf277b56ebdfcb5301
Cr-Commit-Position: refs/heads/master@{#376454}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fix whitespace #Messages
Total messages: 32 (12 generated)
Description was changed from ========== Use eglBindAPI to choose the correct API Use eglBindAPI to choose the correct (EGL_ES) API If the calling thread has chosen another EGL API before the call to eglCreateContext can fail surprisingly, so a call to eglBindAPI is needed before eglCreateContext. BUG=skia:2992 ========== to ========== Use eglBindAPI to choose the correct (EGL_ES) API If the calling thread has chosen another EGL API before the call to eglCreateContext can fail surprisingly, so a call to eglBindAPI is needed before eglCreateContext. BUG=skia:2992 ==========
svaisanen@nvidia.com changed reviewers: + kbr@chromium.org
svaisanen@nvidia.com changed reviewers: + kkinnunen@nvidia.com
kbr@chromium.org changed reviewers: + aelias@chromium.org, sievers@chromium.org
I have no idea what the potential risk of this change is to Chrome's Android port. I would like to defer review to sievers@ and aelias@.
https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc File ui/gl/gl_context_egl.cc (right): https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc#new... ui/gl/gl_context_egl.cc:73: if (!eglBindAPI(EGL_OPENGL_ES_API)) { Can you do this in InitializeStaticGLBindingsEGL() instead?
On 2016/02/09 22:09:34, sievers wrote: > https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc > File ui/gl/gl_context_egl.cc (right): > > https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc#new... > ui/gl/gl_context_egl.cc:73: if (!eglBindAPI(EGL_OPENGL_ES_API)) { > Can you do this in InitializeStaticGLBindingsEGL() instead? It seems that the InitializeStaticGLBindingsEGL is a one off kind of thing. If I'm correct then that would not be sufficient. Initially the problem was encountered during Skia/nanobench testing. Nanobench creates a rendering context native to the platform through EGL and tries to create either a GL context or a GL ES context using eglBindAPI. However when a command buffer context is requested an GL ES context is always wanted, but the prior creation of the native platform context might have changed the EGL API binding for the calling thread.
On 2016/02/11 11:28:09, Sami Väisänen wrote: > On 2016/02/09 22:09:34, sievers wrote: > > https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc > > File ui/gl/gl_context_egl.cc (right): > > > > > https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc#new... > > ui/gl/gl_context_egl.cc:73: if (!eglBindAPI(EGL_OPENGL_ES_API)) { > > Can you do this in InitializeStaticGLBindingsEGL() instead? > > It seems that the InitializeStaticGLBindingsEGL is a one off kind of thing. > If I'm correct then that would not be sufficient. > > Initially the problem was encountered during Skia/nanobench testing. Nanobench > creates a rendering context native to the platform through EGL and tries to > create > either a GL context or a GL ES context using eglBindAPI. > > However when a command buffer context is requested an GL ES context is always > wanted, > but the prior creation of the native platform context might have changed the EGL > API > binding for the calling thread. Ah, I see. Then there's maybe a slightly better place we can find to do it. Since it sets the API for the thread, doing it for each GLContext being created is a bit arbitrary. And in the production use case we'd have a dedicated GPU thread where the API cannot change. How about either in the test suite or when the Chrome GL stack is being set up? Does it go through the command buffer? Or does it use Chrome's GL bindings to the driver directly?
On 2016/02/11 17:55:28, sievers wrote: > On 2016/02/11 11:28:09, Sami Väisänen wrote: > > On 2016/02/09 22:09:34, sievers wrote: > > > https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc > > > File ui/gl/gl_context_egl.cc (right): > > > > > > > > > https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc#new... > > > ui/gl/gl_context_egl.cc:73: if (!eglBindAPI(EGL_OPENGL_ES_API)) { > > > Can you do this in InitializeStaticGLBindingsEGL() instead? > > > > It seems that the InitializeStaticGLBindingsEGL is a one off kind of thing. > > If I'm correct then that would not be sufficient. > > > > Initially the problem was encountered during Skia/nanobench testing. Nanobench > > creates a rendering context native to the platform through EGL and tries to > > create > > either a GL context or a GL ES context using eglBindAPI. > > > > However when a command buffer context is requested an GL ES context is always > > wanted, > > but the prior creation of the native platform context might have changed the > EGL > > API > > binding for the calling thread. > > Ah, I see. Then there's maybe a slightly better place we can find to do it. > Since it sets the API for the thread, doing it for each GLContext being created > is a bit arbitrary. > > And in the production use case we'd have a dedicated GPU thread where the API > cannot change. > How about either in the test suite or when the Chrome GL stack is being set up? > > Does it go through the command buffer? Or does it use Chrome's GL bindings to > the driver directly? Sorry for late reply. I think it makes sense to have the eglBindAPI call exactly next to the function(s) it affects. In this case eglCreateContext to increase the locality of code and make it explicit instead of relying some implicit state being correct. In the initial problem the adjunct components are far apart but affect each others behaviour through implicit state stored in the thread. GL contexts in Skia only use Chrome GL bindings when running in command buffer configuration. So in Skia/nanobench the testing code can create multiple different contexts. For example a "non command-buffer native gl context" and a "command buffer gles context". It will use eglBindAPI in the former case and this then confuses things way deep inside chromium when the latter type of a context is being created.
On 2016/02/16 08:58:04, Sami Väisänen wrote: > On 2016/02/11 17:55:28, sievers wrote: > > On 2016/02/11 11:28:09, Sami Väisänen wrote: > > > On 2016/02/09 22:09:34, sievers wrote: > > > > https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc > > > > File ui/gl/gl_context_egl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc#new... > > > > ui/gl/gl_context_egl.cc:73: if (!eglBindAPI(EGL_OPENGL_ES_API)) { > > > > Can you do this in InitializeStaticGLBindingsEGL() instead? > > > > > > It seems that the InitializeStaticGLBindingsEGL is a one off kind of thing. > > > If I'm correct then that would not be sufficient. > > > > > > Initially the problem was encountered during Skia/nanobench testing. > Nanobench > > > creates a rendering context native to the platform through EGL and tries to > > > create > > > either a GL context or a GL ES context using eglBindAPI. > > > > > > However when a command buffer context is requested an GL ES context is > always > > > wanted, > > > but the prior creation of the native platform context might have changed the > > EGL > > > API > > > binding for the calling thread. > > > > Ah, I see. Then there's maybe a slightly better place we can find to do it. > > Since it sets the API for the thread, doing it for each GLContext being > created > > is a bit arbitrary. > > > > And in the production use case we'd have a dedicated GPU thread where the API > > cannot change. > > How about either in the test suite or when the Chrome GL stack is being set > up? > > > > Does it go through the command buffer? Or does it use Chrome's GL bindings to > > the driver directly? > > Sorry for late reply. > > I think it makes sense to have the eglBindAPI call exactly next to the > function(s) > it affects. In this case eglCreateContext to increase the locality of code and > make it > explicit instead of relying some implicit state being correct. > That would suggest putting it next to MakeCurrent() then which seems a bit heavy though. (You never know what this causes in different drivers.) Not trying to be nit-picky here, but it just seems hard for someone in the future that is touching, moving, or refactoring this code to reason about why this call is here. > In the initial problem the adjunct components are far apart but affect each > others behaviour > through implicit state stored in the thread. > > GL contexts in Skia only use Chrome GL bindings when running in command buffer > configuration. > > So in Skia/nanobench the testing code can create multiple different contexts. > For example > a "non command-buffer native gl context" and a "command buffer gles context". It > will use > eglBindAPI in the former case and this then confuses things way deep inside > chromium > when the latter type of a context is being created. If it already calls eglBindAPI() for the former, can it just do the same for the latter?
On 2016/02/16 18:30:40, sievers wrote: > On 2016/02/16 08:58:04, Sami Väisänen wrote: > > On 2016/02/11 17:55:28, sievers wrote: > > > On 2016/02/11 11:28:09, Sami Väisänen wrote: > > > > On 2016/02/09 22:09:34, sievers wrote: > > > > > > https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc > > > > > File ui/gl/gl_context_egl.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc#new... > > > > > ui/gl/gl_context_egl.cc:73: if (!eglBindAPI(EGL_OPENGL_ES_API)) { > > > > > Can you do this in InitializeStaticGLBindingsEGL() instead? > > > > > > > > It seems that the InitializeStaticGLBindingsEGL is a one off kind of > thing. > > > > If I'm correct then that would not be sufficient. > > > > > > > > Initially the problem was encountered during Skia/nanobench testing. > > Nanobench > > > > creates a rendering context native to the platform through EGL and tries > to > > > > create > > > > either a GL context or a GL ES context using eglBindAPI. > > > > > > > > However when a command buffer context is requested an GL ES context is > > always > > > > wanted, > > > > but the prior creation of the native platform context might have changed > the > > > EGL > > > > API > > > > binding for the calling thread. > > > > > > Ah, I see. Then there's maybe a slightly better place we can find to do it. > > > Since it sets the API for the thread, doing it for each GLContext being > > created > > > is a bit arbitrary. > > > > > > And in the production use case we'd have a dedicated GPU thread where the > API > > > cannot change. > > > How about either in the test suite or when the Chrome GL stack is being set > > up? > > > > > > Does it go through the command buffer? Or does it use Chrome's GL bindings > to > > > the driver directly? > > > > Sorry for late reply. > > > > I think it makes sense to have the eglBindAPI call exactly next to the > > function(s) > > it affects. In this case eglCreateContext to increase the locality of code and > > make it > > explicit instead of relying some implicit state being correct. > > > > That would suggest putting it next to MakeCurrent() then which seems a bit heavy > though. (You never know what this causes in different drivers.) > Not trying to be nit-picky here, but it just seems hard for someone in the > future that is touching, moving, or refactoring this code to reason about why > this call is here. > It does indeed. How often is MakeCurrent called though? > > > In the initial problem the adjunct components are far apart but affect each > > others behaviour > > through implicit state stored in the thread. > > > > GL contexts in Skia only use Chrome GL bindings when running in command buffer > > configuration. > > > > So in Skia/nanobench the testing code can create multiple different contexts. > > For example > > a "non command-buffer native gl context" and a "command buffer gles context". > It > > will use > > eglBindAPI in the former case and this then confuses things way deep inside > > chromium > > when the latter type of a context is being created. > > If it already calls eglBindAPI() for the former, can it just do the same for the > latter? I guess logically if the EGL implementation in command buffer would provide a eglBindAPI then SkCommandBufferGLContext could call that appropriately.
BTW: if adding the eglBindAPI is problematic, just say so. I think we can make Skia aware of the problem and not fix it for command_buffer_gles2
sievers@chromium.org changed reviewers: + bsalomon@chromium.org - aelias@chromium.org
On 2016/02/17 10:55:50, Sami Väisänen wrote: > On 2016/02/16 18:30:40, sievers wrote: > > On 2016/02/16 08:58:04, Sami Väisänen wrote: > > > On 2016/02/11 17:55:28, sievers wrote: > > > > On 2016/02/11 11:28:09, Sami Väisänen wrote: > > > > > On 2016/02/09 22:09:34, sievers wrote: > > > > > > > > https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc > > > > > > File ui/gl/gl_context_egl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1657003003/diff/1/ui/gl/gl_context_egl.cc#new... > > > > > > ui/gl/gl_context_egl.cc:73: if (!eglBindAPI(EGL_OPENGL_ES_API)) { > > > > > > Can you do this in InitializeStaticGLBindingsEGL() instead? > > > > > > > > > > It seems that the InitializeStaticGLBindingsEGL is a one off kind of > > thing. > > > > > If I'm correct then that would not be sufficient. > > > > > > > > > > Initially the problem was encountered during Skia/nanobench testing. > > > Nanobench > > > > > creates a rendering context native to the platform through EGL and tries > > to > > > > > create > > > > > either a GL context or a GL ES context using eglBindAPI. > > > > > > > > > > However when a command buffer context is requested an GL ES context is > > > always > > > > > wanted, > > > > > but the prior creation of the native platform context might have changed > > the > > > > EGL > > > > > API > > > > > binding for the calling thread. > > > > > > > > Ah, I see. Then there's maybe a slightly better place we can find to do > it. > > > > Since it sets the API for the thread, doing it for each GLContext being > > > created > > > > is a bit arbitrary. > > > > > > > > And in the production use case we'd have a dedicated GPU thread where the > > API > > > > cannot change. > > > > How about either in the test suite or when the Chrome GL stack is being > set > > > up? > > > > > > > > Does it go through the command buffer? Or does it use Chrome's GL bindings > > to > > > > the driver directly? > > > > > > Sorry for late reply. > > > > > > I think it makes sense to have the eglBindAPI call exactly next to the > > > function(s) > > > it affects. In this case eglCreateContext to increase the locality of code > and > > > make it > > > explicit instead of relying some implicit state being correct. > > > > > > > That would suggest putting it next to MakeCurrent() then which seems a bit > heavy > > though. (You never know what this causes in different drivers.) > > Not trying to be nit-picky here, but it just seems hard for someone in the > > future that is touching, moving, or refactoring this code to reason about why > > this call is here. > > > It does indeed. How often is MakeCurrent called though? > > > > > > In the initial problem the adjunct components are far apart but affect each > > > others behaviour > > > through implicit state stored in the thread. > > > > > > GL contexts in Skia only use Chrome GL bindings when running in command > buffer > > > configuration. > > > > > > So in Skia/nanobench the testing code can create multiple different > contexts. > > > For example > > > a "non command-buffer native gl context" and a "command buffer gles > context". > > It > > > will use > > > eglBindAPI in the former case and this then confuses things way deep inside > > > chromium > > > when the latter type of a context is being created. > > > > If it already calls eglBindAPI() for the former, can it just do the same for > the > > latter? > > I guess logically if the EGL implementation in command buffer would provide a > eglBindAPI then SkCommandBufferGLContext could call that appropriately. +bsalomon to help find the best place to restore the client API I guess I don't understand why the nanobench suite can't call eglBindAPI() a second time if it already calls it. But maybe if I knew that code, I'd understand better why it makes sense to call it here instead :)
On 2016/02/17 23:22:13, sievers wrote: > +bsalomon to help find the best place to restore the client API > > I guess I don't understand why the nanobench suite can't call eglBindAPI() a > second time if it already calls it. > But maybe if I knew that code, I'd understand better why it makes sense to call > it here instead :) So I try to clarify in high level: App has 3 different platform-specific GL bindings: agl, bgl, cgl App calls aglBindAPI(OpenGL); // AGL supports both, so try OpenGL acontext = aglCreateContext() // BGL supports ES only, no need to bind. bcontext = bglCreateContext() // CGL supports ES only, no need to bind. ccontext = cglCreateContext() It logically does not make sense that the "aglBindAPI()" affects the behavior of "bglCreateContext()" or "cglCreateContext()". The problem arises when "cgl" is implemented with "agl". This implementation detail should be encapsulated inside the library and not leak outside. Now the implementation of "cgl" should ensure that every time a "cgl" function implementation uses "agl" function, the client might have changed "agl" state. In concrete terms, for command_buffer_gles2 purposes: When command buffer invokes functions eglCreateContext, eglGetCurrentContext, eglGetCurrentDisplay, eglGetCurrentSurface, eglMakeCurrent, eglWaitClient, and eglWaitNative, it should first bind GL ES. Now we do understand that this testing-only feature causes a bit of a inconvenience, as Chrome itself does not strictly need this. And as I was saying, if this is a major hurdle, we can change Skia to understand that the command buffer EGL API leaks as an abstraction and instead call eglBindAPI before calling into command buffer. It will just look funny, and thus our initial preference would be to fix the command buffer.
On 2016/02/18 06:34:56, Kimmo Kinnunen wrote: > we can change Skia to understand that the > command buffer EGL API leaks as an abstraction and instead call eglBindAPI > before calling into command buffer. It will just look funny, And naturally we acknowledge that even fixing eglBindAPI things are not perfect. The abstraction leaks because the current context is sometimes shared, sometimes not. However, this is already accounted for in the structure of most GL-using apps, so the funniness of it is not as apparent every day.
bsalomon@google.com changed reviewers: + bsalomon@google.com
Hopefully this is already clear but the problem here is that Skia is testing multiple OpenGL implementations in a single run of a program. That can include native Android OpenGL using egl to create the context and also the command buffer shared library built from Chromium. In Skia these appear as independent interfaces. In reality using the native eglBindAPI call affects the command buffer. As Kimmo says we could work around this in Skia's test framework. However, it does seem logical that the Chromium code that makes egl calls that rely on the bound API should ensure the correct API is bound.
On 2016/02/18 14:30:17, bsalomon wrote: > Hopefully this is already clear but the problem here is that Skia is testing > multiple OpenGL implementations in a single run of a program. That can include > native Android OpenGL using egl to create the context and also the command > buffer shared library built from Chromium. In Skia these appear as independent > interfaces. In reality using the native eglBindAPI call affects the command > buffer. As Kimmo says we could work around this in Skia's test framework. > However, it does seem logical that the Chromium code that makes egl calls that > rely on the bound API should ensure the correct API is bound. Also you'd imagine that if the state is currently already EGL_OPENGL_ES_API the driver is smart enough to reduce this to a "no op" basically. However if for some reason it isn't (like in this case) then eglBindAPI would do the *right* thing.
ok fair enough, lgtm.
The CQ bit was checked by svaisanen@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1657003003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1657003003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by svaisanen@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1657003003/#ps20001 (title: "Fix whitespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1657003003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1657003003/20001
Message was sent while issue was closed.
Description was changed from ========== Use eglBindAPI to choose the correct (EGL_ES) API If the calling thread has chosen another EGL API before the call to eglCreateContext can fail surprisingly, so a call to eglBindAPI is needed before eglCreateContext. BUG=skia:2992 ========== to ========== Use eglBindAPI to choose the correct (EGL_ES) API If the calling thread has chosen another EGL API before the call to eglCreateContext can fail surprisingly, so a call to eglBindAPI is needed before eglCreateContext. BUG=skia:2992 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Use eglBindAPI to choose the correct (EGL_ES) API If the calling thread has chosen another EGL API before the call to eglCreateContext can fail surprisingly, so a call to eglBindAPI is needed before eglCreateContext. BUG=skia:2992 ========== to ========== Use eglBindAPI to choose the correct (EGL_ES) API If the calling thread has chosen another EGL API before the call to eglCreateContext can fail surprisingly, so a call to eglBindAPI is needed before eglCreateContext. BUG=skia:2992 Committed: https://crrev.com/8430468e13d9b71231c0febf277b56ebdfcb5301 Cr-Commit-Position: refs/heads/master@{#376454} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8430468e13d9b71231c0febf277b56ebdfcb5301 Cr-Commit-Position: refs/heads/master@{#376454} |
