Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
198703002:
Add GL_TEXTURE_EXTERNAL_OES as supported texture target for CHROMIUM_map_image. (Closed)
|
Created:
6 years, 9 months ago by sohanjg Modified:
5 years, 1 month ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd GL_TEXTURE_EXTERNAL_OES as supported texture target for CHROMIUM_map_image.
This adds GL_TEXTURE_EXTERNAL_OES support for
SharedMemory backed GLImage.
We use an egl image for binding in GLImageShm, and
also update GLImageEGL to use an egl image for its
"release after use" fallback (required for gpu workaround)
BUG=322780
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259538
Patch Set 1 #
Total comments: 8
Patch Set 2 : Save/restore GL states + nits #
Total comments: 9
Patch Set 3 : reuse resource creation code + comments #
Total comments: 5
Patch Set 4 : comments + nits #
Total comments: 7
Patch Set 5 : inlining functions + comments #
Total comments: 3
Patch Set 6 : comments + nits #Patch Set 7 : address nvidia work-around patch in gl_image_egl for android_webview #
Total comments: 3
Patch Set 8 : nvidia workaround patch 2 #
Total comments: 3
Patch Set 9 : comments + nits #
Total comments: 3
Patch Set 10 : comments + nits #
Total comments: 1
Patch Set 11 : comments #
Total comments: 4
Patch Set 12 : creating eglimage and texture everytime during bindteximage + comments #
Total comments: 13
Patch Set 13 : comments + consitent use of eglimage create params #
Total comments: 4
Patch Set 14 : comments + nits #Patch Set 15 : prevent texture leak + comments #Patch Set 16 : mac build break #Patch Set 17 : mac build issue #Patch Set 18 : mac build break patch #Patch Set 19 : mac build break #
Messages
Total messages: 54 (1 generated)
https://codereview.chromium.org/198703002/diff/1/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/198703002/diff/1/ui/gl/gl_image_shm.cc#newcode98 ui/gl/gl_image_shm.cc:98: egl_imagekhr_ = EGL_NO_IMAGE_KHR; please initialize these in the ctor instead. https://codereview.chromium.org/198703002/diff/1/ui/gl/gl_image_shm.cc#newcod... ui/gl/gl_image_shm.cc:126: if (target == GL_TEXTURE_2D) { this should be "target != GL_TEXTURE_EXTERNAL_OES" as you'll break TEXTURE_RECTANGLE_ARB support otherwise https://codereview.chromium.org/198703002/diff/1/ui/gl/gl_image_shm.cc#newcod... ui/gl/gl_image_shm.cc:138: glGenTextures(1, &texture_id_); you're leaking textures and egl images by doing this every time. create the texture and egl image just once. https://codereview.chromium.org/198703002/diff/1/ui/gl/gl_image_shm.cc#newcod... ui/gl/gl_image_shm.cc:140: glBindTexture(GL_TEXTURE_2D, texture_id_); all the GL state you change in this function need to be restored before you return or you'll mess up the service. https://codereview.chromium.org/198703002/diff/1/ui/gl/gl_image_shm.cc#newcod... ui/gl/gl_image_shm.cc:154: shared_memory_->memory()); this is exactly the same as above. can you refactor the code so this doesn't have to be duplicated? https://codereview.chromium.org/198703002/diff/1/ui/gl/gl_image_shm.cc#newcod... ui/gl/gl_image_shm.cc:167: return false; shared_memory_->Unmap() is never called when you return early here. https://codereview.chromium.org/198703002/diff/1/ui/gl/gl_image_shm.h File ui/gl/gl_image_shm.h (right): https://codereview.chromium.org/198703002/diff/1/ui/gl/gl_image_shm.h#newcode35 ui/gl/gl_image_shm.h:35: GLuint texture_id_; nit: maybe external_texture_id_ or something like that to not have this confused with the texture id we're binding the image to https://codereview.chromium.org/198703002/diff/1/ui/gl/gl_image_shm.h#newcode36 ui/gl/gl_image_shm.h:36: EGLImageKHR egl_imagekhr_; nit: egl_image_
https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:134: glGenTextures(1, &external_texture_id_); Why did you make external_texture_id_ a member but still only use it locally? Can't you reuse the external_texture_id_ and the egl_image_ between each call to BindTexImage? That's why I suggested you make them members. https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:140: glGetIntegerv(GL_ACTIVE_TEXTURE, ¤t_active_texture_unit); Do you have to change active texture? https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:177: Destroy(); Destory is supposed to release all resources. Not just some. You can't use it here. https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.h File ui/gl/gl_image_shm.h (right): https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.h#new... ui/gl/gl_image_shm.h:35: GLuint external_texture_id_; nit: you use GLint for texture ids in the .cc file but GLuint here. Please make it consistent.
PTAL. Thanks https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:134: glGenTextures(1, &external_texture_id_); On 2014/03/14 14:06:22, reveman wrote: > Why did you make external_texture_id_ a member but still only use it locally? > Can't you reuse the external_texture_id_ and the egl_image_ between each call to > BindTexImage? That's why I suggested you make them members. Done. Have reused the creation code. But, they cant be created once and reused between BindTexImage calls. I release them at the end, so it shouldnt leak. https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:140: glGetIntegerv(GL_ACTIVE_TEXTURE, ¤t_active_texture_unit); On 2014/03/14 14:06:22, reveman wrote: > Do you have to change active texture? Done. Generally before bindTex we do an activeTex during creation of texture. But, here the old tex unit is also GL_TEXTURE0. https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:177: Destroy(); On 2014/03/14 14:06:22, reveman wrote: > Destory is supposed to release all resources. Not just some. You can't use it > here. Done. Added a new ReleaseEGLImageandTexture function to do it. https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.h File ui/gl/gl_image_shm.h (right): https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.h#new... ui/gl/gl_image_shm.h:35: GLuint external_texture_id_; On 2014/03/14 14:06:22, reveman wrote: > nit: you use GLint for texture ids in the .cc file but GLuint here. Please make > it consistent. this maybe tricky as in .cc we do a glGetIntegerv() to get the tex id, which will not accept an unsigned integer, and GenTex/BindTex will not accept a signed integer as tex id.
https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.h File ui/gl/gl_image_shm.h (right): https://codereview.chromium.org/198703002/diff/30001/ui/gl/gl_image_shm.h#new... ui/gl/gl_image_shm.h:35: GLuint external_texture_id_; On 2014/03/15 10:03:53, sohanjg wrote: > On 2014/03/14 14:06:22, reveman wrote: > > nit: you use GLint for texture ids in the .cc file but GLuint here. Please > make > > it consistent. > > this maybe tricky as in .cc we do a glGetIntegerv() to get the tex id, which > will not accept an unsigned integer, and GenTex/BindTex will not accept a signed > integer as tex id. Ok. Current code is good then. https://codereview.chromium.org/198703002/diff/70001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/198703002/diff/70001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:163: glGetIntegerv(GL_TEXTURE_BINDING_EXTERNAL_OES, ¤t_texture_id); EXTERNAL_OES binding is not the state you're changing. It's the TEXTURE_2D binding you're changing. https://codereview.chromium.org/198703002/diff/70001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:185: ReleaseEGLImageandTexture(); what's the point of having egl_image_ and external_texture_id_ members if the lifetime of them doesn't go beyond this function? You can reuse the egl image and the external texture id if you use glTexSubImage2D. If not already, please look at http://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_gl_image.txt to understand how this extension works.
https://codereview.chromium.org/198703002/diff/70001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/198703002/diff/70001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:9: #include "ui/gl/gl_surface_egl.h" why is gl_bindings.h not enough? https://codereview.chromium.org/198703002/diff/70001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:163: glGetIntegerv(GL_TEXTURE_BINDING_EXTERNAL_OES, ¤t_texture_id); Also, use ScopedTextureBinder here instead. https://codereview.chromium.org/198703002/diff/70001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:185: ReleaseEGLImageandTexture(); On 2014/03/15 14:08:45, reveman wrote: > what's the point of having egl_image_ and external_texture_id_ members if the > lifetime of them doesn't go beyond this function? > > You can reuse the egl image and the external texture id if you use > glTexSubImage2D. If not already, please look at > http://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_gl_image.txt to > understand how this extension works. Don't get hung up on reusing the egl image though. I'd like to keep this as clean and simple as possible. If calling TexImage2D and re-creating the egl image with each bind is cleaner, then do that. I'm pretty sure you still have to keep the egl image and texture around as members though. Binding the egl image to a texture and then destroying the egl image doesn't seem right.
PTAl. Thanks
https://codereview.chromium.org/198703002/diff/90001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/198703002/diff/90001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:68: external_texture_id_(0), nit: 0u https://codereview.chromium.org/198703002/diff/90001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:103: } I'd remove this utility function. Just makes the code harder to read IMO. There's at least no need for it to be a member function. https://codereview.chromium.org/198703002/diff/90001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:105: bool GLImageShm::CreateEGLImage() { I'm not sure this utility function is worth much either. If you really want it, please make it a non-member function with input/output parameters. https://codereview.chromium.org/198703002/diff/90001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:114: if (egl_image_ == EGL_NO_IMAGE_KHR) { To keep things simple, could this just be: DCHECK_NE(EGL_NO_IMAGE_KHR, egl_image_) << "Error creating EGLImage: " << error; https://codereview.chromium.org/198703002/diff/90001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:146: DCHECK(shared_memory_->memory()); move this DCHECK above so it doesn't have to be duplicated. https://codereview.chromium.org/198703002/diff/90001/ui/gl/gl_image_shm.h File ui/gl/gl_image_shm.h (right): https://codereview.chromium.org/198703002/diff/90001/ui/gl/gl_image_shm.h#new... ui/gl/gl_image_shm.h:19: void CreateTexture2D(); this is not implemented. https://codereview.chromium.org/198703002/diff/90001/ui/gl/gl_image_shm.h#new... ui/gl/gl_image_shm.h:38: GLuint external_texture_id_; Sorry, I know I asked you to call this external_texture_id but that was wrong. This is the texture 2d. Maybe name it egl_texture_id_ or texture_2d_id_ instead?
PTAL. Thanks.
looks good after fixing the following minor issues https://codereview.chromium.org/198703002/diff/110001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/198703002/diff/110001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:101: eglDestroyImageKHR(eglGetCurrentDisplay(), egl_image_); please reset egl_image_ if you want to do this in Destroy rather than dtor: if (egl_image_ != EGL_NO_IMAGE_KHR) { eglDestroyImageKHR(eglGetCurrentDisplay(), egl_image_); egl_image_ = EGL_NO_IMAGE_KHR; } https://codereview.chromium.org/198703002/diff/110001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:102: if (egl_texture_id_) and egl_texture_id_ = 0, the idea is that Destroy() can be called by the client followed by another call to Initialize. https://codereview.chromium.org/198703002/diff/110001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:161: EGLint error = eglGetError(); nit: avoid this temporary variable: "...EGLImage: " << eglGetError();
PTAL. Thanks.
lgtm Does it work correctly with android_webview (gl_image_egl.cc)? Does GLImageEGL::DidUseTexImage still work as intended? +piman for owner review +boliu as this affects android_webview
I think this is ok for webview but +sievers since I'm not actually that familiar with this. Webview does not use GLImageShm so I ignored changes there. The only other change will change the target of EGLImageTargetTexture2DOES from TEXTURE_2D to TEXTURE_EXTERNAL_OES, which afaict should just work. I tried this on a nexus 4 and nothing broke. I don't have other nexus devices handy, say to try on an nvidia or arm chipsets. But considering inline video and SurfaceTexture already uses TEXTURE_EXTERNAL_OES on android, it should be safe.
lgtm
reveman pointed out that TEXTURE_EXTERNAL_OES target does not work with the glTexImage2D call in GLImageEGL::DidUseTexImage. It's a workaround in android webview for nvidia chipset, so I'm trying to get a old nexus 7 to test this. Could you hold off landing for a bit?
Have uploaded a patch to avoid glTexImage2D for nvidia work-around. If you can please check this as well, in nvidia chipset. Thanks.
https://codereview.chromium.org/198703002/diff/150001/ui/gl/gl_image_egl.cc File ui/gl/gl_image_egl.cc (right): https://codereview.chromium.org/198703002/diff/150001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:26: native_buffer_ = buffer.native_buffer; are you sure buffer.native_buffer is a valid pointer after this function returns? Even if that happens to be the case, I'm not sure it's appropriate to hold on to it here. https://codereview.chromium.org/198703002/diff/150001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:119: glEGLImageTargetTexture2DOES(target_, egl_image); I don't understand why this would work. Binding the same native buffer again releases it? Please explain how this works.
Having some trouble bringing up a device. I will get back to this tomorrow GMT time. Sorry about that!
Finally got a device and managed to test this. A workaround for "release after use" is stil needed for GL_TEXTURE_EXTERNAL_OES target on nvidia. The current implementation in PS7 does not work. Would keeping a global dummy EGLImage (as opposed one dummy to per GLImageEGL) to unbind work? Also found out that webview isn't actually getting the correct target plumbed due to an unrelated issue, crbug.com/354382. https://codereview.chromium.org/198703002/diff/150001/ui/gl/gl_image_egl.cc File ui/gl/gl_image_egl.cc (right): https://codereview.chromium.org/198703002/diff/150001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:122: eglDestroyImageKHR(GLSurfaceEGL::GetHardwareDisplay(), egl_image); Currently this destroy fails.
https://codereview.chromium.org/198703002/diff/170001/ui/gl/gl_image_egl.cc File ui/gl/gl_image_egl.cc (right): https://codereview.chromium.org/198703002/diff/170001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:49: egl_image_for_unbind_ == EGL_NO_IMAGE_KHR) This is not right. You will call eglDestroyImageKHR with EGL_NO_IMAGE_KHR. Please remove this early return and instead make destruction conditional below instead. https://codereview.chromium.org/198703002/diff/170001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:123: if (!texture_id_) this can be DCHECK_EQ(0u, texture_id_) https://codereview.chromium.org/198703002/diff/170001/ui/gl/gl_image_egl.h File ui/gl/gl_image_egl.h (right): https://codereview.chromium.org/198703002/diff/170001/ui/gl/gl_image_egl.h#ne... ui/gl/gl_image_egl.h:40: GLuint texture_id_; nit: texture_id_for_unbind_ to be consistent
Some minor suggestions but current patch is good enough to check if it works. We might want to make egl_image_for_unbind_ global but let's make sure it works first. boliu@, can you give this patch a try on your old Nexus 7? https://codereview.chromium.org/198703002/diff/190001/ui/gl/gl_image_egl.cc File ui/gl/gl_image_egl.cc (right): https://codereview.chromium.org/198703002/diff/190001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:48: EGLBoolean success = EGL_FALSE; nit: move this inside the (egl_image_ != EGL_NO_IMAGE_KHR) scope where it's used https://codereview.chromium.org/198703002/diff/190001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:120: if (!egl_image_for_unbind_) { this should be (egl_image_for_unbind_ == EGL_NO_IMAGE_KHR) https://codereview.chromium.org/198703002/diff/190001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:143: } nit: I would add blank line here to separate the lazy create code a bit from the unbind.
I'll get to later today. But just a plug that it actually is possible to test this with open source code: http://www.chromium.org/developers/how-tos/build-instructions-android-webview It's just super involved: need to check out chromium + android, build all of android with chromium in it, then sacrifice a rooted nexus 7 for testing etc...
lgtm thanks
One tiny fix, then good to go for nvidia. Thanks! https://codereview.chromium.org/198703002/diff/210001/ui/gl/gl_image_egl.cc File ui/gl/gl_image_egl.cc (right): https://codereview.chromium.org/198703002/diff/210001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:135: EGLImageKHR egl_image_for_unbind_ = eglCreateImageKHR( Don't create a new local var here!
Updated. PTAL. Thanks.
https://codereview.chromium.org/198703002/diff/230001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/198703002/diff/230001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:156: if (!egl_image_) { should be egl_image == EGL_NO_IMAGE_KHR but are you not required to create a new egl image each time for this to work? https://codereview.chromium.org/198703002/diff/230001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:168: glBindTexture(GL_TEXTURE_2D, egl_texture_id_); it should not be necessary to have the egl_texture_id_ bound it this time. the scoped binder above is also already taking care of that. please remove this line and limit the lifetime of ScopedTextureBinder to when needed, which is only until after has been called glTexImage2D afaict.
https://codereview.chromium.org/198703002/diff/230001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/198703002/diff/230001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:101: eglDestroyImageKHR(eglGetCurrentDisplay(), egl_image_); GLSurfaceEGL::GetHardwareDisplay() instead https://codereview.chromium.org/198703002/diff/230001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:164: egl_attrib_list); Also, please change this to use GLSurfaceEGL::GetHardwareDisplay() and EGL_NO_CONTEXT instead to be consistent with gl_image_egl.cc
On 2014/03/21 13:39:47, reveman wrote: > https://codereview.chromium.org/198703002/diff/230001/ui/gl/gl_image_shm.cc > File ui/gl/gl_image_shm.cc (right): > > https://codereview.chromium.org/198703002/diff/230001/ui/gl/gl_image_shm.cc#n... > ui/gl/gl_image_shm.cc:101: eglDestroyImageKHR(eglGetCurrentDisplay(), > egl_image_); > GLSurfaceEGL::GetHardwareDisplay() instead > > https://codereview.chromium.org/198703002/diff/230001/ui/gl/gl_image_shm.cc#n... > ui/gl/gl_image_shm.cc:164: egl_attrib_list); > Also, please change this to use GLSurfaceEGL::GetHardwareDisplay() and > EGL_NO_CONTEXT instead to be consistent with gl_image_egl.cc
PTAL. Thanks.
https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_egl.cc File ui/gl/gl_image_egl.cc (right): https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:58: eglDestroyImageKHR(eglGetCurrentDisplay(), egl_image_for_unbind_); does this need to be GLSurfaceEGL::GetHardwareDisplay()? is it possible that a display is not current when Destory is called? For consistency, I prefer if you used GetHardwareDisplay() here and everywhere else where you currently use eglGetCurrentDisplay(). If you can't use GetHardwareDisplay(), please explain why. https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:124: ScopedTextureBinder texture_binder(GL_TEXTURE_2D, texture_id_for_unbind_); I assume this is only needed until glTexImage2D is called. Can you add a scope here to make that explicit? https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:136: eglGetCurrentDisplay(), GLSurfaceEGL::GetHardwareDisplay() if possible https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:137: eglGetCurrentContext(), EGL_NO_CONTEXT if possible? https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:101: eglDestroyImageKHR(eglGetCurrentDisplay(), egl_image_); GLSurfaceEGL::GetHardwareDisplay() https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:106: egl_texture_id_ = 0; nit: s/0/0u/ to match the assign in ctor https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:127: if (target != GL_TEXTURE_EXTERNAL_OES) { nit: reversing these if-clauses might improve readability a bit. so having this be "if (target == GL_TEXTURE_EXTERNAL_OES)" instead. your call though, I don't feel that strongly about it. https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:138: glGenTextures(1, &egl_texture_id_); Why did you change this? Do you really need to generate a new texture id everytime? https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:155: } thanks for making the intended scope of the binder explicit. please do the same in gl_image_egl.cc https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:157: EGLint egl_attrib_list[] = {EGL_GL_TEXTURE_LEVEL_KHR, 0, // mip-level. nit: maybe rename to "attrs" to be consistent with gl_image_egl.cc https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:160: eglCreateImageKHR(eglGetCurrentDisplay(), GLSurfaceEGL::GetHardwareDisplay() https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:161: eglGetCurrentContext(), and NO_CONTEXT if possible. if that doesn't work, please add a short comment explaining why eglGetCurrentContext() is needed here. https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:180: void GLImageShm::DidUseTexImage() { Destroy(); } Please don't use Destroy here. That's confusing and also not correct as Will/DidUseTexImage can be called multiple times after BindTexImage. Please delete the old egl_image_ in GLImageShm::BindTexImage instead.
On 2014/03/22 12:38:56, reveman wrote: > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_egl.cc > File ui/gl/gl_image_egl.cc (right): > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_egl.cc#n... > ui/gl/gl_image_egl.cc:58: eglDestroyImageKHR(eglGetCurrentDisplay(), > egl_image_for_unbind_); > does this need to be GLSurfaceEGL::GetHardwareDisplay()? is it possible that a > display is not current when Destory is called? For consistency, I prefer if you > used GetHardwareDisplay() here and everywhere else where you currently use > eglGetCurrentDisplay(). If you can't use GetHardwareDisplay(), please explain > why. > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_egl.cc#n... > ui/gl/gl_image_egl.cc:124: ScopedTextureBinder texture_binder(GL_TEXTURE_2D, > texture_id_for_unbind_); > I assume this is only needed until glTexImage2D is called. Can you add a scope > here to make that explicit? > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_egl.cc#n... > ui/gl/gl_image_egl.cc:136: eglGetCurrentDisplay(), > GLSurfaceEGL::GetHardwareDisplay() if possible > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_egl.cc#n... > ui/gl/gl_image_egl.cc:137: eglGetCurrentContext(), > EGL_NO_CONTEXT if possible? > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc > File ui/gl/gl_image_shm.cc (right): > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... > ui/gl/gl_image_shm.cc:101: eglDestroyImageKHR(eglGetCurrentDisplay(), > egl_image_); > GLSurfaceEGL::GetHardwareDisplay() > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... > ui/gl/gl_image_shm.cc:106: egl_texture_id_ = 0; > nit: s/0/0u/ to match the assign in ctor > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... > ui/gl/gl_image_shm.cc:127: if (target != GL_TEXTURE_EXTERNAL_OES) { > nit: reversing these if-clauses might improve readability a bit. so having this > be "if (target == GL_TEXTURE_EXTERNAL_OES)" instead. your call though, I don't > feel that strongly about it. > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... > ui/gl/gl_image_shm.cc:138: glGenTextures(1, &egl_texture_id_); > Why did you change this? Do you really need to generate a new texture id > everytime? > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... > ui/gl/gl_image_shm.cc:155: } > thanks for making the intended scope of the binder explicit. please do the same > in gl_image_egl.cc > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... > ui/gl/gl_image_shm.cc:157: EGLint egl_attrib_list[] = {EGL_GL_TEXTURE_LEVEL_KHR, > 0, // mip-level. > nit: maybe rename to "attrs" to be consistent with gl_image_egl.cc > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... > ui/gl/gl_image_shm.cc:160: eglCreateImageKHR(eglGetCurrentDisplay(), > GLSurfaceEGL::GetHardwareDisplay() > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... > ui/gl/gl_image_shm.cc:161: eglGetCurrentContext(), > and NO_CONTEXT if possible. if that doesn't work, please add a short comment > explaining why eglGetCurrentContext() is needed here. > > https://codereview.chromium.org/198703002/diff/260001/ui/gl/gl_image_shm.cc#n... > ui/gl/gl_image_shm.cc:180: void GLImageShm::DidUseTexImage() { Destroy(); } > Please don't use Destroy here. That's confusing and also not correct as > Will/DidUseTexImage can be called multiple times after BindTexImage. > > Please delete the old egl_image_ in GLImageShm::BindTexImage instead.
https://codereview.chromium.org/198703002/diff/300001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/198703002/diff/300001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:141: try adding this here: if (egl_image_ != EGL_NO_IMAGE_KHR) eglDestroyImageKHR(GLSurfaceEGL::GetHardwareDisplay(), egl_image_); you probably have to destroy the old image after binding a new if this doesn't work. https://codereview.chromium.org/198703002/diff/300001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:159: EGLint egl_attrib_list[] = {EGL_GL_TEXTURE_LEVEL_KHR, 0, // mip-level. nit: EGLint attrs[] = {EG... https://codereview.chromium.org/198703002/diff/300001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:170: glEGLImageTargetTexture2DOES(target, egl_image_); Add DCHECK_EQ(static_cast<GLenum>(GL_NO_ERROR), glGetError()); here to be consistent with gl_image_egl.cc https://codereview.chromium.org/198703002/diff/300001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:173: if (egl_image_ != EGL_NO_IMAGE_KHR) { remove this destruction code.
lgtm, thanks!
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/198703002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/198703002/520001
The CQ bit was unchecked by sohan.jyoti@samsung.com
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/198703002/520001
The CQ bit was unchecked by sohan.jyoti@samsung.com
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/198703002/520001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_compile_dbg
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/198703002/740001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/198703002/740001
Message was sent while issue was closed.
Change committed as 259538
Message was sent while issue was closed.
Description was changed from ========== Add GL_TEXTURE_EXTERNAL_OES as supported texture target for CHROMIUM_map_image. This adds GL_TEXTURE_EXTERNAL_OES support for SharedMemory backed GLImage. We use an egl image for binding in GLImageShm, and also update GLImageEGL to use an egl image for its "release after use" fallback (required for gpu workaround) BUG=322780 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259538 ========== to ========== Add GL_TEXTURE_EXTERNAL_OES as supported texture target for CHROMIUM_map_image. This adds GL_TEXTURE_EXTERNAL_OES support for SharedMemory backed GLImage. We use an egl image for binding in GLImageShm, and also update GLImageEGL to use an egl image for its "release after use" fallback (required for gpu workaround) BUG=322780 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259538 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
