Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
301793003:
During image destroy, delete textures only if we have a GL context. (Closed)
|
Created:
6 years, 6 months ago by sohanjg Modified:
5 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDuring image destroy, delete textures only if we have a GL context.
This adds Destroy function for ImageManager and invoke it from
command decoder destroy path with GL context info.
This is to ensure that when we destroy the image,
textures to which the image is bound to,
is deleted only if we have a GL context.
BUG=375507
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286811
Patch Set 1 #
Total comments: 5
Patch Set 2 : Destroy GLImage during ContextGroup destroy #
Total comments: 14
Patch Set 3 : change destroy flow for all GLImage types. #
Total comments: 17
Patch Set 4 : WIP - review comments addressed. #
Total comments: 11
Patch Set 5 : Move ImageManager destroy to GPUChannel + review comments #
Total comments: 10
Patch Set 6 : WIP - review comments addressed. #
Total comments: 6
Patch Set 7 : WIP - review comments addressed #
Total comments: 8
Patch Set 8 : WIP - Avoid optional image manager creation/destroy in context group #
Total comments: 5
Patch Set 9 : rebased to ToT #
Total comments: 11
Patch Set 10 : review comments addressed. #Patch Set 11 : prevent crash when accessing destroyed glimage while doing bind/releaseTexImage. #
Total comments: 3
Patch Set 12 : review comments addressed. #
Total comments: 22
Patch Set 13 : review comments addressed. #Patch Set 14 : build issues addressed. #
Total comments: 30
Patch Set 15 : review comments addressed. #
Total comments: 6
Patch Set 16 : review comment addressed. #
Total comments: 3
Patch Set 17 : review comment addressed. #
Total comments: 8
Patch Set 18 : review comments updated. #
Total comments: 5
Patch Set 19 : reverted to check images in cmd buffer. #
Total comments: 8
Patch Set 20 : Add early-outs for image add/remove. #
Total comments: 4
Patch Set 21 : style nits #Patch Set 22 : rebased to ToT #Patch Set 23 : resolve android clang dbg build issue. #Messages
Total messages: 85 (1 generated)
Can you PTAL. Thanks. https://codereview.chromium.org/301793003/diff/1/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/301793003/diff/1/ui/gl/gl_image_shm.cc#newcod... ui/gl/gl_image_shm.cc:121: } Wouldnt we end up leaking the texture if current_context is NULL ? I was wondering if we tackled this in ::BindTexImage, where we can use glTex, and create eglImage and texture for each bind (and delete the old ones).
https://codereview.chromium.org/301793003/diff/1/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/301793003/diff/1/ui/gl/gl_image_shm.cc#newcod... ui/gl/gl_image_shm.cc:121: } On 2014/05/28 10:16:00, sohanjg wrote: > Wouldnt we end up leaking the texture if current_context is NULL ? > I was wondering if we tackled this in ::BindTexImage, where we can use glTex, > and create eglImage and texture for each bind (and delete the old ones). I think we should rather pass through the flag from the decoder/context group. The IsCurrent() check is a bit flaky because we don't know if it's the correct context that is current (one that is in the correct share group). We could even explicitly destroy the textures with a current context, similar to what TextureManager does when the decoder or context group get destroyed.
https://codereview.chromium.org/301793003/diff/1/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/301793003/diff/1/ui/gl/gl_image_shm.cc#newcod... ui/gl/gl_image_shm.cc:121: } On 2014/05/28 18:55:32, sievers wrote: > On 2014/05/28 10:16:00, sohanjg wrote: > > Wouldnt we end up leaking the texture if current_context is NULL ? > > I was wondering if we tackled this in ::BindTexImage, where we can use glTex, > > and create eglImage and texture for each bind (and delete the old ones). > > I think we should rather pass through the flag from the decoder/context group. > The IsCurrent() check is a bit flaky because we don't know if it's the correct > context that is current (one that is in the correct share group). > > We could even explicitly destroy the textures with a current context, similar to > what TextureManager does when the decoder or context group get destroyed. I think the problem is that GLImages are ref counted and possibly used by multiple contexts. Maybe it would be fine to explicitly destroy images when destroying the context they were created with. The instance would still exist, binding or using it would just fail after it has been destroyed. We need to make sure all GLImage implementations do the right thing when used after being destroyed in this case though. Alternatively, we could just delete the texture when ::ReleaseTexImage is called.
https://codereview.chromium.org/301793003/diff/1/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/301793003/diff/1/ui/gl/gl_image_shm.cc#newcod... ui/gl/gl_image_shm.cc:121: } On 2014/05/28 20:23:33, reveman wrote: > On 2014/05/28 18:55:32, sievers wrote: > > On 2014/05/28 10:16:00, sohanjg wrote: > > > Wouldnt we end up leaking the texture if current_context is NULL ? > > > I was wondering if we tackled this in ::BindTexImage, where we can use > glTex, > > > and create eglImage and texture for each bind (and delete the old ones). > > > > I think we should rather pass through the flag from the decoder/context group. > > The IsCurrent() check is a bit flaky because we don't know if it's the correct > > context that is current (one that is in the correct share group). > > > > We could even explicitly destroy the textures with a current context, similar > to > > what TextureManager does when the decoder or context group get destroyed. > > I think the problem is that GLImages are ref counted and possibly used by > multiple contexts. Maybe it would be fine to explicitly destroy images when > destroying the context they were created with. The instance would still exist, > binding or using it would just fail after it has been destroyed. We need to make > sure all GLImage implementations do the right thing when used after being > destroyed in this case though. > > Alternatively, we could just delete the texture when ::ReleaseTexImage is > called. Should a gpu::gles2::Texture also call ReleaseTexImage() when it gets deleted? Or does it already somehow (doesn't look like it)? That would allow us to sort of piggy-back on the existing Texture refcounting (which is a bit more complicated because of the explicit destruction).
https://codereview.chromium.org/301793003/diff/1/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/301793003/diff/1/ui/gl/gl_image_shm.cc#newcod... ui/gl/gl_image_shm.cc:121: } On 2014/05/28 21:02:39, sievers wrote: > On 2014/05/28 20:23:33, reveman wrote: > > On 2014/05/28 18:55:32, sievers wrote: > > > On 2014/05/28 10:16:00, sohanjg wrote: > > > > Wouldnt we end up leaking the texture if current_context is NULL ? > > > > I was wondering if we tackled this in ::BindTexImage, where we can use > > glTex, > > > > and create eglImage and texture for each bind (and delete the old ones). > > > > > > I think we should rather pass through the flag from the decoder/context > group. > > > The IsCurrent() check is a bit flaky because we don't know if it's the > correct > > > context that is current (one that is in the correct share group). > > > > > > We could even explicitly destroy the textures with a current context, > similar > > to > > > what TextureManager does when the decoder or context group get destroyed. > > > > I think the problem is that GLImages are ref counted and possibly used by > > multiple contexts. Maybe it would be fine to explicitly destroy images when > > destroying the context they were created with. The instance would still exist, > > binding or using it would just fail after it has been destroyed. We need to > make > > sure all GLImage implementations do the right thing when used after being > > destroyed in this case though. > > > > Alternatively, we could just delete the texture when ::ReleaseTexImage is > > called. > > Should a gpu::gles2::Texture also call ReleaseTexImage() when it gets deleted? > Or does it already somehow (doesn't look like it)? I don't think it does but it seems like a good idea to do that so a GLImage implementation can assume that a BindTexImage call is always followed by a ReleaseTexImage. > > That would allow us to sort of piggy-back on the existing Texture refcounting > (which is a bit more complicated because of the explicit destruction).
Or simpler: If we add ImageManager::Destroy(bool have_context) (similar to TextureManager etc.) and call that from ContextGroup::Destroy(), and have that iterate through the images and call GLImage::Destroy(have_context), would that work?
On 2014/05/28 22:02:21, sievers wrote: > Or simpler: If we add ImageManager::Destroy(bool have_context) (similar to > TextureManager etc.) and call that from ContextGroup::Destroy(), and have that > iterate through the images and call GLImage::Destroy(have_context), would that > work? Sgtm.
On 2014/05/28 22:06:37, reveman wrote: > On 2014/05/28 22:02:21, sievers wrote: > > Or simpler: If we add ImageManager::Destroy(bool have_context) (similar to > > TextureManager etc.) and call that from ContextGroup::Destroy(), and have that > > iterate through the images and call GLImage::Destroy(have_context), would that > > work? > > Sgtm. This works well. As the bug is only for shm, i have not handled the context for other GLImage's except for Shared Memory, and passed default as false otherwise. Maybe for android_native_buffer backed GLImage, we can use the context (as a follow-up ?) Can you please take a look. thanks.
If you're changing the system for how GLImage instances are destroyed then you'll need to change it consistently across all implementations. https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/context_group.cc (right): https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/context_group.cc:328: image_manager_->Destroy(have_context); image_manager_.reset() for consistency https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/image_manager.cc:22: iter->second.get()->Destroy(have_context); should we drop the ref here too? https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/image_manager.h (right): https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/image_manager.h:28: virtual void Destroy(bool have_context); Why virtual? https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_android_n... File ui/gl/gl_image_android_native_buffer.cc (right): https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_android_n... ui/gl/gl_image_android_native_buffer.cc:21: Destroy(false); I don't think any GLImage dtor should be calling Destroy anymore. It's a bug if Destroy has not been called after a successful call to Initialize. Please add DCHECKs to verify that the work done by Destroy has happened at this point. ie. DCHECK_EQ(EGL_NO_IMAGE_KHR, egl_image_for_unbind_)... https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_android_n... ui/gl/gl_image_android_native_buffer.cc:39: glDeleteTextures(1, &texture_id_for_unbind_); This should only be called if have_context is true. https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_egl.cc File ui/gl/gl_image_egl.cc (right): https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_egl.cc#ne... ui/gl/gl_image_egl.cc:15: Destroy(false); No call to Destroy in dtor please. Instead verify that Destroy has been called. https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_glx.h File ui/gl/gl_image_glx.h (right): https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_glx.h#new... ui/gl/gl_image_glx.h:22: virtual void Destroy(bool have_context) OVERRIDE; I think this will fail to compile. You need to update the .cc file too. Please verify that Destroy has been called in the dtor instead of calling it from there. https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_io_surface.h File ui/gl/gl_image_io_surface.h (right): https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_io_surfac... ui/gl/gl_image_io_surface.h:22: virtual void Destroy(bool have_context) OVERRIDE {} I think this will fail to compile. You need to update the .cc file too. Please add an implementation of Destroy to the .cc file that calls io_surface_.reset() and verify that it has been called in the dtor. https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:9: #include "ui/gl/gl_context.h" do we need this? https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:83: GLImageShm::~GLImageShm() { Verify that Destroy has been called. https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:118: if (egl_texture_id_ && have_context) { I think you should set egl_texture_id_ to 0 even if have_context is false so you can verify that Destroy was called in the dtor. https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:122: #endif Please add shared_memory_.reset() to this function. Destroy should undo all the work that Initialize has done and we want to make sure BindTexImage fails predictably in a debug build if ever called after Destroy. https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_stub.cc File ui/gl/gl_image_stub.cc (right): https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_stub.cc#n... ui/gl/gl_image_stub.cc:12: Destroy(false); No call to Destroy in dtor please. https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_surface_t... File ui/gl/gl_image_surface_texture.cc (right): https://codereview.chromium.org/301793003/diff/20001/ui/gl/gl_image_surface_t... ui/gl/gl_image_surface_texture.cc:17: Destroy(false); No call to Destroy in dtor please. Instead verify that Destroy has been called.
On 2014/05/29 16:30:33, reveman wrote: > If you're changing the system for how GLImage instances are destroyed then > you'll need to change it consistently across all implementations. Ok. I understand. > > https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... > File gpu/command_buffer/service/context_group.cc (right): > > https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... > gpu/command_buffer/service/context_group.cc:328: > image_manager_->Destroy(have_context); > image_manager_.reset() for consistency image_manager_ needs to be made scoped_ptr instead of scoped ref ptr for this, and its Dtor needs to be made non-virtual, GPUChannel also uses the same ImageManager as scoped ref ptr, it will required change there too, and its destruction handling. Do you want me to do it as well ? > > https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... > File gpu/command_buffer/service/image_manager.cc (right): > > https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... > gpu/command_buffer/service/image_manager.cc:22: > iter->second.get()->Destroy(have_context); > should we drop the ref here too? > sorry, do you mean remove it from hash map as well ? Can you please take a look. Thanks.
On 2014/05/30 12:43:57, sohanjg wrote: > On 2014/05/29 16:30:33, reveman wrote: > > If you're changing the system for how GLImage instances are destroyed then > > you'll need to change it consistently across all implementations. > > Ok. I understand. > > > > > https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... > > File gpu/command_buffer/service/context_group.cc (right): > > > > > https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... > > gpu/command_buffer/service/context_group.cc:328: > > image_manager_->Destroy(have_context); > > image_manager_.reset() for consistency > > image_manager_ needs to be made scoped_ptr instead of scoped ref ptr for this, > and its Dtor needs to be made non-virtual, GPUChannel also uses the same > ImageManager as scoped ref ptr, it will required change there too, and its > destruction handling. > Do you want me to do it as well ? The plan is to make the image manager context specific and not shared at all. That will solve the problem but not possible until we've refactored some of the IPC. Maybe for now you could try removing ref counting from the image manager and move ownership to the context group. > > > > > > > https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... > > File gpu/command_buffer/service/image_manager.cc (right): > > > > > https://codereview.chromium.org/301793003/diff/20001/gpu/command_buffer/servi... > > gpu/command_buffer/service/image_manager.cc:22: > > iter->second.get()->Destroy(have_context); > > should we drop the ref here too? > > > sorry, do you mean remove it from hash map as well ? Yes, assuming the values in the hash are references to GLImages. Btw, please reply to the comments on the patchset instead of replying to the message and mark each comment that you've addressed as "Done". It's much easier for me to review the code when you do that as otherwise I have to go through the whole patch again to see what you've actually addressed.
https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_android_n... File ui/gl/gl_image_android_native_buffer.cc (right): https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_android_n... ui/gl/gl_image_android_native_buffer.cc:43: glDeleteTextures(1, &texture_id_for_unbind_); doesn't work as you set texture_id_for_unbind_ to 0 above https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_glx.cc#ne... ui/gl/gl_image_glx.cc:54: DCHECK_EQ(0u, pixmap_); this can fail. see why below. https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_glx.cc#ne... ui/gl/gl_image_glx.cc:103: pixmap_ = XCompositeNameWindowPixmap(display_, window_); For this new Destroy logic to work correctly, pixmap_ should not be set unless this function returns true. https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_io_surfac... File ui/gl/gl_image_io_surface.cc (right): https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_io_surfac... ui/gl/gl_image_io_surface.cc:33: DCHECK(io_surface_); This DCHECK seems inconsistent with other implementations of the Destroy function which seem to handle multiple calls to Destroy or Initialize not having been called fine. https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:122: glDeleteTextures(1, &egl_texture_id_); this doesn't do the right thing as you set it to 0 above. https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_stub.cc File ui/gl/gl_image_stub.cc (right): https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_stub.cc#n... ui/gl/gl_image_stub.cc:12: } I know that cl format now use this style but I don't like the inconsistency so please keep existing style (no line break after '{') for now until we reformat all GLImage code.
Please take a look. Thanks. https://codereview.chromium.org/301793003/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/context_group.cc (right): https://codereview.chromium.org/301793003/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/context_group.cc:330: if we try "removing ref counting from the image manager and move ownership to the context group", for being consistent here with .reset(), then GPUChannel will have issues while destruction, because imagemanager is ref-counted there too. How to go about it ? https://codereview.chromium.org/301793003/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/image_manager.cc:23: } If we clear the hash map here, we would need to ensure Unregister doesnt do the same, as Unregister will be invoked before Destroy and we would end up leaking and having problems. https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_android_n... File ui/gl/gl_image_android_native_buffer.cc (right): https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_android_n... ui/gl/gl_image_android_native_buffer.cc:43: glDeleteTextures(1, &texture_id_for_unbind_); On 2014/05/31 05:05:06, reveman wrote: > doesn't work as you set texture_id_for_unbind_ to 0 above Done. https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_glx.cc#ne... ui/gl/gl_image_glx.cc:54: DCHECK_EQ(0u, pixmap_); On 2014/05/31 05:05:06, reveman wrote: > this can fail. see why below. Done. https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_glx.cc#ne... ui/gl/gl_image_glx.cc:103: pixmap_ = XCompositeNameWindowPixmap(display_, window_); On 2014/05/31 05:05:06, reveman wrote: > For this new Destroy logic to work correctly, pixmap_ should not be set unless > this function returns true. Done. https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_io_surfac... File ui/gl/gl_image_io_surface.cc (right): https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_io_surfac... ui/gl/gl_image_io_surface.cc:33: DCHECK(io_surface_); On 2014/05/31 05:05:06, reveman wrote: > This DCHECK seems inconsistent with other implementations of the Destroy > function which seem to handle multiple calls to Destroy or Initialize not having > been called fine. Done. https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:122: glDeleteTextures(1, &egl_texture_id_); On 2014/05/31 05:05:06, reveman wrote: > this doesn't do the right thing as you set it to 0 above. Done. https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_stub.cc File ui/gl/gl_image_stub.cc (right): https://codereview.chromium.org/301793003/diff/60001/ui/gl/gl_image_stub.cc#n... ui/gl/gl_image_stub.cc:12: } On 2014/05/31 05:05:06, reveman wrote: > I know that cl format now use this style but I don't like the inconsistency so > please keep existing style (no line break after '{') for now until we reformat > all GLImage code. Done.
https://codereview.chromium.org/301793003/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/context_group.cc (right): https://codereview.chromium.org/301793003/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/context_group.cc:330: On 2014/06/02 13:59:42, sohanjg wrote: > if we try "removing ref counting from the image manager > and move ownership to the context group", for being consistent here with > .reset(), then GPUChannel will have issues while destruction, because > imagemanager is ref-counted there too. > How to go about it ? I think we just have to wait for Create/DeleteImage IPC to be replaced by Create/DestroyGpuMemoryBuffer IPC before we can do this properly. You can't call Destroy here but could move the call to tear-down of the GpuChannel if you want to land this before the above mentioned IPC changes. https://codereview.chromium.org/301793003/diff/60001/gpu/command_buffer/servi... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/image_manager.cc:23: } On 2014/06/02 13:59:42, sohanjg wrote: > If we clear the hash map here, we would need to ensure Unregister doesnt do the > same, as Unregister will be invoked before Destroy and we would end up leaking > and having problems. I'm not following. How can this cause a leak? https://codereview.chromium.org/301793003/diff/60001/gpu/command_buffer/servi... gpu/command_buffer/service/image_manager.cc:61: void ImageManager::RemoveImage(int32 service_id) { Don't you need to call Destroy here too? https://codereview.chromium.org/301793003/diff/80001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/301793003/diff/80001/ui/gl/gl_image_glx.cc#ne... ui/gl/gl_image_glx.cc:107: } This doesn't help in any way as XCompositeNameWindowPixmap is not going to fail. The problem is that you might fail below and |pixmap_| is then set to a valid value without the GLImage having been initialized so the DCHECK in Destroy will fail. https://codereview.chromium.org/301793003/diff/80001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/301793003/diff/80001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:82: GLImageShm::~GLImageShm() { Please add a DCHECK(!shared_memory_) here too. https://codereview.chromium.org/301793003/diff/80001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:84: DCHECK_EQ(0u, egl_texture_id_); These DCHECKs need to be ifdef-ed. https://codereview.chromium.org/301793003/diff/80001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:125: shared_memory_.reset(); nit: move this above the ifdef
https://codereview.chromium.org/301793003/diff/80001/gpu/command_buffer/servi... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/image_manager.cc:55: // This will be handled as part of ::Destroy. This is not right. If you're not removing and destroying the image here resources will be kept around forever or until the GPU process is killed. You're basically leaking GpuMemoryBuffers. You need to Destroy and remove the image here.
Can you PTAL. Thanks. https://codereview.chromium.org/301793003/diff/80001/gpu/command_buffer/servi... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/image_manager.cc:55: // This will be handled as part of ::Destroy. On 2014/06/02 14:59:16, reveman wrote: > This is not right. If you're not removing and destroying the image here > resources will be kept around forever or until the GPU process is killed. You're > basically leaking GpuMemoryBuffers. > > You need to Destroy and remove the image here. Done. https://codereview.chromium.org/301793003/diff/80001/gpu/command_buffer/servi... gpu/command_buffer/service/image_manager.cc:55: // This will be handled as part of ::Destroy. On 2014/06/02 14:59:16, reveman wrote: > This is not right. If you're not removing and destroying the image here > resources will be kept around forever or until the GPU process is killed. You're > basically leaking GpuMemoryBuffers. > > You need to Destroy and remove the image here. Done. https://codereview.chromium.org/301793003/diff/80001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/301793003/diff/80001/ui/gl/gl_image_glx.cc#ne... ui/gl/gl_image_glx.cc:107: } On 2014/06/02 14:34:47, reveman wrote: > This doesn't help in any way as XCompositeNameWindowPixmap is not going to fail. > > The problem is that you might fail below and |pixmap_| is then set to a valid > value without the GLImage having been initialized so the DCHECK in Destroy will > fail. Done. https://codereview.chromium.org/301793003/diff/80001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/301793003/diff/80001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:82: GLImageShm::~GLImageShm() { On 2014/06/02 14:34:47, reveman wrote: > Please add a DCHECK(!shared_memory_) here too. Done. https://codereview.chromium.org/301793003/diff/80001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:84: DCHECK_EQ(0u, egl_texture_id_); On 2014/06/02 14:34:47, reveman wrote: > These DCHECKs need to be ifdef-ed. Done. https://codereview.chromium.org/301793003/diff/80001/ui/gl/gl_image_shm.cc#ne... ui/gl/gl_image_shm.cc:125: shared_memory_.reset(); On 2014/06/02 14:34:47, reveman wrote: > nit: move this above the ifdef Done.
You also need to remove the optional creation of an ImageManager instance in ContextGroup or make sure Destroy is called in that case too. Probably easier to remove it as I don't think it's necessary. https://codereview.chromium.org/301793003/diff/100001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/301793003/diff/100001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:408: if (image_manager_ != NULL) { nit: "if (image_manager_)" instead and you can remove {} https://codereview.chromium.org/301793003/diff/100001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:55: LookupImage(id)->Destroy(true); Can LookupImage(id) return NULL? At least DCHECK if not. https://codereview.chromium.org/301793003/diff/100001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/301793003/diff/100001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:119: pixmap_ = 0u; You're leaking a pixmap here. https://codereview.chromium.org/301793003/diff/100001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:130: pixmap_ = 0u; You're leaking a pixmap here. https://codereview.chromium.org/301793003/diff/100001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/301793003/diff/100001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:116: shared_memory_.reset(); nit: please be consistent with blank lines around "#if def". ie. no blank line in dtor then no blank line here
I have kept the image manager instance in context group (and deleted it during destroy), seems it is required by cmd decoder for gpu control service. Please take a look. Thanks. https://codereview.chromium.org/301793003/diff/100001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/301793003/diff/100001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:408: if (image_manager_ != NULL) { On 2014/06/02 16:45:29, reveman wrote: > nit: "if (image_manager_)" instead and you can remove {} Done. https://codereview.chromium.org/301793003/diff/100001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:55: LookupImage(id)->Destroy(true); On 2014/06/02 16:45:29, reveman wrote: > Can LookupImage(id) return NULL? At least DCHECK if not. Done. https://codereview.chromium.org/301793003/diff/100001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/301793003/diff/100001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:119: pixmap_ = 0u; On 2014/06/02 16:45:29, reveman wrote: > You're leaking a pixmap here. Done. https://codereview.chromium.org/301793003/diff/100001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:130: pixmap_ = 0u; On 2014/06/02 16:45:29, reveman wrote: > You're leaking a pixmap here. Done. https://codereview.chromium.org/301793003/diff/100001/ui/gl/gl_image_shm.cc File ui/gl/gl_image_shm.cc (right): https://codereview.chromium.org/301793003/diff/100001/ui/gl/gl_image_shm.cc#n... ui/gl/gl_image_shm.cc:116: shared_memory_.reset(); On 2014/06/02 16:45:29, reveman wrote: > nit: please be consistent with blank lines around "#if def". ie. no blank line > in dtor then no blank line here Done.
https://codereview.chromium.org/301793003/diff/120001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/301793003/diff/120001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:409: image_manager_->Destroy(true); true? Do you always have a context current in this case? https://codereview.chromium.org/301793003/diff/120001/gpu/command_buffer/serv... File gpu/command_buffer/service/context_group.cc (right): https://codereview.chromium.org/301793003/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/context_group.cc:328: image_manager_->Destroy(have_context); This is not right. What if the image manager is used by another context group? https://codereview.chromium.org/301793003/diff/120001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:56: DCHECK(image); What's preventing a malicous renderer from using an invalid id and causing this to crash?
Can you please take a look. Thanks. https://codereview.chromium.org/301793003/diff/120001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/301793003/diff/120001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:409: image_manager_->Destroy(true); On 2014/06/03 13:57:39, reveman wrote: > true? Do you always have a context current in this case? Done. gpu channel was destroyed before cmd buffer and decoder, there the context was current, so i had set it as true. but to confirm i have queried GLContext::GetCurrent()->IsCurrent here. https://codereview.chromium.org/301793003/diff/120001/gpu/command_buffer/serv... File gpu/command_buffer/service/context_group.cc (right): https://codereview.chromium.org/301793003/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/context_group.cc:328: image_manager_->Destroy(have_context); On 2014/06/03 13:57:39, reveman wrote: > This is not right. What if the image manager is used by another context group? Done. https://codereview.chromium.org/301793003/diff/120001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:56: DCHECK(image); On 2014/06/03 13:57:39, reveman wrote: > What's preventing a malicous renderer from using an invalid id and causing this > to crash? Done.
Also still need to remove the optional creation of an ImageManager instance in ContextGroup. https://codereview.chromium.org/301793003/diff/140001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/301793003/diff/140001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:410: image_manager_->Destroy(have_context); I don't think this will work. What if the context that is current is not in the same group as one of the images that will be destroyed? https://codereview.chromium.org/301793003/diff/140001/gpu/command_buffer/serv... File gpu/command_buffer/service/context_group.cc (right): https://codereview.chromium.org/301793003/diff/140001/gpu/command_buffer/serv... gpu/command_buffer/service/context_group.cc:328: image_manager_->Destroy(have_context); How is this not causing images that belong to another context group to be destroyed? https://codereview.chromium.org/301793003/diff/140001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/140001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:61: DVLOG(0) << "Invalid GLImage corresponding to given ID"; nit: the GLImage is not invalid, the ID is https://codereview.chromium.org/301793003/diff/140001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:64: image->Destroy(true); nit: please make the use of blank lines consistent with RegisterGpuMemoryBuffer
Please take a look. Thanks. https://codereview.chromium.org/301793003/diff/140001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/301793003/diff/140001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:410: image_manager_->Destroy(have_context); On 2014/06/03 15:57:54, reveman wrote: > I don't think this will work. What if the context that is current is not in the > same group as one of the images that will be destroyed? I have tried checking the context from GpuCommandBufferStub. https://codereview.chromium.org/301793003/diff/140001/gpu/command_buffer/serv... File gpu/command_buffer/service/context_group.cc (right): https://codereview.chromium.org/301793003/diff/140001/gpu/command_buffer/serv... gpu/command_buffer/service/context_group.cc:328: image_manager_->Destroy(have_context); On 2014/06/03 15:57:54, reveman wrote: > How is this not causing images that belong to another context group to be > destroyed? Removed the destroy code and the optional creation part. https://codereview.chromium.org/301793003/diff/140001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/140001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:61: DVLOG(0) << "Invalid GLImage corresponding to given ID"; On 2014/06/03 15:57:54, reveman wrote: > nit: the GLImage is not invalid, the ID is Done. https://codereview.chromium.org/301793003/diff/140001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:64: image->Destroy(true); On 2014/06/03 15:57:54, reveman wrote: > nit: please make the use of blank lines consistent with RegisterGpuMemoryBuffer Done.
https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:411: lost_context = it.GetCurrentValue()->CheckContextLost(); This will set lost_context to whatever is returned from the CheckContextLost() call on the last stub. How is that supposed to help?
https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:411: lost_context = it.GetCurrentValue()->CheckContextLost(); On 2014/06/04 15:29:35, reveman wrote: > This will set lost_context to whatever is returned from the CheckContextLost() > call on the last stub. How is that supposed to help? Can you please suggest how we should get the have_context here? Decoder generally maintains the context, so i thot of querying the cmd buffer stub for it. Should we just pass true here ?
https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:411: lost_context = it.GetCurrentValue()->CheckContextLost(); On 2014/06/05 02:57:57, sohanjg wrote: > On 2014/06/04 15:29:35, reveman wrote: > > This will set lost_context to whatever is returned from the CheckContextLost() > > call on the last stub. How is that supposed to help? > > Can you please suggest how we should get the have_context here? Decoder > generally maintains the context, so i thot of querying the cmd buffer stub for > it. > Should we just pass true here ? When i checked, the stubmap size is 1, how about we check for decoder()->MakeCurrent on this stub (that is the value we passed to context group also). If we have multiple stubs, still we will have 1 image manager for the gpu channel,how can we handle then? or should we move image manager creation/handling to stub ?
https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:411: lost_context = it.GetCurrentValue()->CheckContextLost(); On 2014/06/05 06:33:57, sohanjg wrote: > On 2014/06/05 02:57:57, sohanjg wrote: > > On 2014/06/04 15:29:35, reveman wrote: > > > This will set lost_context to whatever is returned from the > CheckContextLost() > > > call on the last stub. How is that supposed to help? > > > > Can you please suggest how we should get the have_context here? Decoder > > generally maintains the context, so i thot of querying the cmd buffer stub for > > it. > > Should we just pass true here ? > > When i checked, the stubmap size is 1, how about we check for > decoder()->MakeCurrent on this stub (that is the value we passed to context > group also). > If we have multiple stubs, still we will have 1 image manager for the gpu > channel,how can we handle then? or should we move image manager > creation/handling to stub ? > > > We'll have one image manager per stub once we've Create/DeleteImage IPC refactor is done. This is why I suggested you wait with this until that is complete.
https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:411: lost_context = it.GetCurrentValue()->CheckContextLost(); On 2014/06/05 15:58:01, reveman wrote: > On 2014/06/05 06:33:57, sohanjg wrote: > > On 2014/06/05 02:57:57, sohanjg wrote: > > > On 2014/06/04 15:29:35, reveman wrote: > > > > This will set lost_context to whatever is returned from the > > CheckContextLost() > > > > call on the last stub. How is that supposed to help? > > > > > > Can you please suggest how we should get the have_context here? Decoder > > > generally maintains the context, so i thot of querying the cmd buffer stub > for > > > it. > > > Should we just pass true here ? > > > > When i checked, the stubmap size is 1, how about we check for > > decoder()->MakeCurrent on this stub (that is the value we passed to context > > group also). > > If we have multiple stubs, still we will have 1 image manager for the gpu > > channel,how can we handle then? or should we move image manager > > creation/handling to stub ? > > > > > > > > We'll have one image manager per stub once we've Create/DeleteImage IPC refactor > is done. This is why I suggested you wait with this until that is complete. I see. Is waiting for the refactor the only option then ? Or we can assume stub size as 1, and work with the decoder makecurrent value temporarily and mark a todo to revisit once the refactor lands ?
On 2014/06/06 02:30:03, sohanjg wrote: > https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... > File content/common/gpu/gpu_channel.cc (right): > > https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... > content/common/gpu/gpu_channel.cc:411: lost_context = > it.GetCurrentValue()->CheckContextLost(); > On 2014/06/05 15:58:01, reveman wrote: > > On 2014/06/05 06:33:57, sohanjg wrote: > > > On 2014/06/05 02:57:57, sohanjg wrote: > > > > On 2014/06/04 15:29:35, reveman wrote: > > > > > This will set lost_context to whatever is returned from the > > > CheckContextLost() > > > > > call on the last stub. How is that supposed to help? > > > > > > > > Can you please suggest how we should get the have_context here? Decoder > > > > generally maintains the context, so i thot of querying the cmd buffer stub > > for > > > > it. > > > > Should we just pass true here ? > > > > > > When i checked, the stubmap size is 1, how about we check for > > > decoder()->MakeCurrent on this stub (that is the value we passed to context > > > group also). > > > If we have multiple stubs, still we will have 1 image manager for the gpu > > > channel,how can we handle then? or should we move image manager > > > creation/handling to stub ? > > > > > > > > > > > > > We'll have one image manager per stub once we've Create/DeleteImage IPC > refactor > > is done. This is why I suggested you wait with this until that is complete. > > I see. Is waiting for the refactor the only option then ? Or we can assume stub > size as 1, and work with the decoder makecurrent value temporarily and mark a > todo to revisit once the refactor lands ? Think best option is to wait. Assuming that number of stubs are 1 is not an option. You can optionally track from where each registered image comes and only delete the set of images that belong to a specific context group. That seems like a lot of work for something that will be completely unnecessary once one or two refactor patches have landed though.
On 2014/06/06 05:29:05, reveman wrote: > On 2014/06/06 02:30:03, sohanjg wrote: > > > https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... > > File content/common/gpu/gpu_channel.cc (right): > > > > > https://codereview.chromium.org/301793003/diff/160001/content/common/gpu/gpu_... > > content/common/gpu/gpu_channel.cc:411: lost_context = > > it.GetCurrentValue()->CheckContextLost(); > > On 2014/06/05 15:58:01, reveman wrote: > > > On 2014/06/05 06:33:57, sohanjg wrote: > > > > On 2014/06/05 02:57:57, sohanjg wrote: > > > > > On 2014/06/04 15:29:35, reveman wrote: > > > > > > This will set lost_context to whatever is returned from the > > > > CheckContextLost() > > > > > > call on the last stub. How is that supposed to help? > > > > > > > > > > Can you please suggest how we should get the have_context here? Decoder > > > > > generally maintains the context, so i thot of querying the cmd buffer > stub > > > for > > > > > it. > > > > > Should we just pass true here ? > > > > > > > > When i checked, the stubmap size is 1, how about we check for > > > > decoder()->MakeCurrent on this stub (that is the value we passed to > context > > > > group also). > > > > If we have multiple stubs, still we will have 1 image manager for the gpu > > > > channel,how can we handle then? or should we move image manager > > > > creation/handling to stub ? > > > > > > > > > > > > > > > > > > We'll have one image manager per stub once we've Create/DeleteImage IPC > > refactor > > > is done. This is why I suggested you wait with this until that is complete. > > > > I see. Is waiting for the refactor the only option then ? Or we can assume > stub > > size as 1, and work with the decoder makecurrent value temporarily and mark a > > todo to revisit once the refactor lands ? > > Think best option is to wait. Assuming that number of stubs are 1 is not an > option. You can optionally track from where each registered image comes and only > delete the set of images that belong to a specific context group. That seems > like a lot of work for something that will be completely unnecessary once one or > two refactor patches have landed though. Should we revisit this? And work to land this w/o the refactors ?
I think you should be able to land this now after rebasing.
On 2014/07/18 16:15:19, reveman wrote: > I think you should be able to land this now after rebasing. Can you please take a look, if i am missing some thing here. Thanks.
https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:3496: if (image_manager()) { nit: image_manager_.get() for consistency https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:3497: image_manager()->Destroy(have_context); nit: image_manager_->Destroy() for consistency https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:30: images_.erase(service_id); when is GLImage::Destroy called for an image that is removed using a call to RemoveImage? https://codereview.chromium.org/301793003/diff/180001/ui/gl/gl_image_io_surfa... File ui/gl/gl_image_io_surface.cc (right): https://codereview.chromium.org/301793003/diff/180001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.cc:33: io_surface_.reset() missing ";" https://codereview.chromium.org/301793003/diff/180001/ui/gl/gl_image_stub.cc File ui/gl/gl_image_stub.cc (right): https://codereview.chromium.org/301793003/diff/180001/ui/gl/gl_image_stub.cc#... ui/gl/gl_image_stub.cc:12: } nit: cl format the full file or leave as "~GLImageStub() {}" for consistency
can you please take a look. do we want explicit gl image destroy when we remove image ? https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:3496: if (image_manager()) { On 2014/07/21 15:12:24, reveman wrote: > nit: image_manager_.get() for consistency Done. https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:3497: image_manager()->Destroy(have_context); On 2014/07/21 15:12:24, reveman wrote: > nit: image_manager_->Destroy() for consistency Done. https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:30: images_.erase(service_id); On 2014/07/21 15:12:24, reveman wrote: > when is GLImage::Destroy called for an image that is removed using a call to > RemoveImage? hmm..GLImage::Destroy, will be invoked when cmd buffer and cmd decoder is destroyed, cmd decoder will destroy image manager, holding all gl images. https://codereview.chromium.org/301793003/diff/180001/ui/gl/gl_image_io_surfa... File ui/gl/gl_image_io_surface.cc (right): https://codereview.chromium.org/301793003/diff/180001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.cc:33: io_surface_.reset() On 2014/07/21 15:12:24, reveman wrote: > missing ";" Done. https://codereview.chromium.org/301793003/diff/180001/ui/gl/gl_image_stub.cc File ui/gl/gl_image_stub.cc (right): https://codereview.chromium.org/301793003/diff/180001/ui/gl/gl_image_stub.cc#... ui/gl/gl_image_stub.cc:12: } On 2014/07/21 15:12:24, reveman wrote: > nit: cl format the full file or leave as "~GLImageStub() {}" for consistency Done.
https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:30: images_.erase(service_id); On 2014/07/21 15:39:51, sohanjg wrote: > On 2014/07/21 15:12:24, reveman wrote: > > when is GLImage::Destroy called for an image that is removed using a call to > > RemoveImage? > > hmm..GLImage::Destroy, will be invoked when cmd buffer and cmd decoder is > destroyed, cmd decoder will destroy image manager, holding all gl images. It's not going to destroy this image as you're removing it right here. I'm not sure calling Destroy here will work unless we make more changes. Here's the sequence of events that I'm worried about: 1. Renderer1 register GpuMemoryBuffer and creates GLImage 2. Renderer1 binds GLImage to texture 3. Renderer1 shares texture with Renderer2 4. Renderer1 unregister GLImage (::Destroy is called) 5. Renderer2 tries to use texture and GPU process crashes as we're trying to use a GLImage that has been destroyed This would be incorrect usage by Renderer1 but it can't cause the GPU process to crash. I think we need to make sure we handle these cases correctly and never crash.
On 2014/07/21 15:59:10, reveman wrote: > https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... > File gpu/command_buffer/service/image_manager.cc (right): > > https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... > gpu/command_buffer/service/image_manager.cc:30: images_.erase(service_id); > On 2014/07/21 15:39:51, sohanjg wrote: > > On 2014/07/21 15:12:24, reveman wrote: > > > when is GLImage::Destroy called for an image that is removed using a call to > > > RemoveImage? > > > > hmm..GLImage::Destroy, will be invoked when cmd buffer and cmd decoder is > > destroyed, cmd decoder will destroy image manager, holding all gl images. > > It's not going to destroy this image as you're removing it right here. I'm not > sure calling Destroy here will work unless we make more changes. Here's the > sequence of events that I'm worried about: > > 1. Renderer1 register GpuMemoryBuffer and creates GLImage > 2. Renderer1 binds GLImage to texture > 3. Renderer1 shares texture with Renderer2 > 4. Renderer1 unregister GLImage (::Destroy is called) > 5. Renderer2 tries to use texture and GPU process crashes as we're trying to use > a GLImage that has been destroyed > > This would be incorrect usage by Renderer1 but it can't cause the GPU process to > crash. I think we need to make sure we handle these cases correctly and never > crash. Yeah right, since we are removing from image map, destroy wont get it. Even if we call destroy, we dont have the context info. For, 4. Renderer1 unregister GLImage (::Destroy is called) , do you mean when RemoveImage is called from OnDestroyGpuMemoryBuffer ? Can we in that case somehow update mark the texture as invalid, if it has an associated glimage, which is destroyed ?
On 2014/07/23 14:00:33, sohanjg wrote: > On 2014/07/21 15:59:10, reveman wrote: > > > https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... > > File gpu/command_buffer/service/image_manager.cc (right): > > > > > https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... > > gpu/command_buffer/service/image_manager.cc:30: images_.erase(service_id); > > On 2014/07/21 15:39:51, sohanjg wrote: > > > On 2014/07/21 15:12:24, reveman wrote: > > > > when is GLImage::Destroy called for an image that is removed using a call > to > > > > RemoveImage? > > > > > > hmm..GLImage::Destroy, will be invoked when cmd buffer and cmd decoder is > > > destroyed, cmd decoder will destroy image manager, holding all gl images. > > > > It's not going to destroy this image as you're removing it right here. I'm not > > sure calling Destroy here will work unless we make more changes. Here's the > > sequence of events that I'm worried about: > > > > 1. Renderer1 register GpuMemoryBuffer and creates GLImage > > 2. Renderer1 binds GLImage to texture > > 3. Renderer1 shares texture with Renderer2 > > 4. Renderer1 unregister GLImage (::Destroy is called) > > 5. Renderer2 tries to use texture and GPU process crashes as we're trying to > use > > a GLImage that has been destroyed > > > > This would be incorrect usage by Renderer1 but it can't cause the GPU process > to > > crash. I think we need to make sure we handle these cases correctly and never > > crash. > > Yeah right, since we are removing from image map, destroy wont get it. Even if > we call destroy, we dont have the context info. > For, > 4. Renderer1 unregister GLImage (::Destroy is called) , do you mean when > RemoveImage is called from OnDestroyGpuMemoryBuffer ? > > Can we in that case somehow update mark the texture as invalid, if it has an > associated glimage, which is destroyed ? What changes do you propose for handling such scenarios ?
On 2014/07/23 14:28:22, sohanjg wrote: > On 2014/07/23 14:00:33, sohanjg wrote: > > On 2014/07/21 15:59:10, reveman wrote: > > > > > > https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... > > > File gpu/command_buffer/service/image_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/301793003/diff/180001/gpu/command_buffer/serv... > > > gpu/command_buffer/service/image_manager.cc:30: images_.erase(service_id); > > > On 2014/07/21 15:39:51, sohanjg wrote: > > > > On 2014/07/21 15:12:24, reveman wrote: > > > > > when is GLImage::Destroy called for an image that is removed using a > call > > to > > > > > RemoveImage? > > > > > > > > hmm..GLImage::Destroy, will be invoked when cmd buffer and cmd decoder is > > > > destroyed, cmd decoder will destroy image manager, holding all gl images. > > > > > > It's not going to destroy this image as you're removing it right here. I'm > not > > > sure calling Destroy here will work unless we make more changes. Here's the > > > sequence of events that I'm worried about: > > > > > > 1. Renderer1 register GpuMemoryBuffer and creates GLImage > > > 2. Renderer1 binds GLImage to texture > > > 3. Renderer1 shares texture with Renderer2 > > > 4. Renderer1 unregister GLImage (::Destroy is called) > > > 5. Renderer2 tries to use texture and GPU process crashes as we're trying to > > use > > > a GLImage that has been destroyed > > > > > > This would be incorrect usage by Renderer1 but it can't cause the GPU > process > > to > > > crash. I think we need to make sure we handle these cases correctly and > never > > > crash. > > > > Yeah right, since we are removing from image map, destroy wont get it. Even if > > we call destroy, we dont have the context info. > > For, > > 4. Renderer1 unregister GLImage (::Destroy is called) , do you mean when > > RemoveImage is called from OnDestroyGpuMemoryBuffer ? > > > > Can we in that case somehow update mark the texture as invalid, if it has an > > associated glimage, which is destroyed ? > > What changes do you propose for handling such scenarios ? Bind/ReleaseTexImage should fail but not cause a crash or DCHECK to fail if called after the image has been destroyed. I think that's easier to handle than trying to automatically detach an image from whatever textures it might be bound too at the time it's destroyed.
Can you please take a look. Thanks. https://codereview.chromium.org/301793003/diff/240001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/240001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:30: LookupImage(service_id)->Destroy(true); Will it be safe to assume we have context while destroying gpu mem buffers ? and, do we need validation for the service id and the image returned by look up ?
Can you verify that this bug is still an issue before you do more work here? I wonder if we avoid this issue simply be having made the image manager decoder specific. https://codereview.chromium.org/301793003/diff/240001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/240001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:30: LookupImage(service_id)->Destroy(true); On 2014/07/24 13:24:10, sohanjg wrote: > Will it be safe to assume we have context while destroying gpu mem buffers ? > and, do we need validation for the service id and the image returned by look up > ? It's safe to assume that you have a context here. I don't think it's safe to assume that service_id is valid. You'll have to fix that. Also, please avoid 2 service_id lookups here and instead erase using an iterator.
On 2014/07/24 13:51:55, reveman wrote: > Can you verify that this bug is still an issue before you do more work here? I > wonder if we avoid this issue simply be having made the image manager decoder > specific. > > https://codereview.chromium.org/301793003/diff/240001/gpu/command_buffer/serv... > File gpu/command_buffer/service/image_manager.cc (right): > > https://codereview.chromium.org/301793003/diff/240001/gpu/command_buffer/serv... > gpu/command_buffer/service/image_manager.cc:30: > LookupImage(service_id)->Destroy(true); > On 2014/07/24 13:24:10, sohanjg wrote: > > Will it be safe to assume we have context while destroying gpu mem buffers ? > > and, do we need validation for the service id and the image returned by look > up > > ? > > It's safe to assume that you have a context here. > > I don't think it's safe to assume that service_id is valid. You'll have to fix > that. Also, please avoid 2 service_id lookups here and instead erase using an > iterator. I think the scenario itself will be hard to get, when we would not use ST for zero-copy, and use shm instead. regarding the context, even if image manager is decoder specific, is it ensured we will have context during destruction? as we understood the crash was because of we were deleting texture without a context or current context.
On 2014/07/24 14:41:46, sohanjg wrote: > On 2014/07/24 13:51:55, reveman wrote: > > Can you verify that this bug is still an issue before you do more work here? I > > wonder if we avoid this issue simply be having made the image manager decoder > > specific. > > > > > https://codereview.chromium.org/301793003/diff/240001/gpu/command_buffer/serv... > > File gpu/command_buffer/service/image_manager.cc (right): > > > > > https://codereview.chromium.org/301793003/diff/240001/gpu/command_buffer/serv... > > gpu/command_buffer/service/image_manager.cc:30: > > LookupImage(service_id)->Destroy(true); > > On 2014/07/24 13:24:10, sohanjg wrote: > > > Will it be safe to assume we have context while destroying gpu mem buffers ? > > > and, do we need validation for the service id and the image returned by look > > up > > > ? > > > > It's safe to assume that you have a context here. > > > > I don't think it's safe to assume that service_id is valid. You'll have to fix > > that. Also, please avoid 2 service_id lookups here and instead erase using an > > iterator. > > I think the scenario itself will be hard to get, when we would not use ST for > zero-copy, and use shm instead. > regarding the context, even if image manager is decoder specific, is it ensured > we will have context during destruction? > as we understood the crash was because of we were deleting texture without a > context or current context. Right, thanks for reminding me. You're correct something has to be done about that for sure.
Please take a look, thanks. https://codereview.chromium.org/301793003/diff/240001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/240001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:30: LookupImage(service_id)->Destroy(true); On 2014/07/24 13:51:55, reveman wrote: > On 2014/07/24 13:24:10, sohanjg wrote: > > Will it be safe to assume we have context while destroying gpu mem buffers ? > > and, do we need validation for the service id and the image returned by look > up > > ? > > It's safe to assume that you have a context here. > > I don't think it's safe to assume that service_id is valid. You'll have to fix > that. Also, please avoid 2 service_id lookups here and instead erase using an > iterator. Done.
https://codereview.chromium.org/301793003/diff/260001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/260001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:27: images_[service_id] = image; This could drop a GLImage reference without calling Destroy if the client uses the same service_id twice. We need to check if service_id is already in use somehow. I think the best is to DCHECK(images_.find(service_id) == images_.end()) here and require the usage of ImageManager to ensure that the service_id is valid. https://codereview.chromium.org/301793003/diff/260001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:30: void ImageManager::RemoveImage(int32 service_id) { In light of the my comment below, this function should probably also DCHECK and require usage of ImageManager to ensure that service_id is valid. LookupImage can be used for that. https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_android_... File ui/gl/gl_image_android_native_buffer.cc (right): https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_android_... ui/gl/gl_image_android_native_buffer.cc:50: LOG(ERROR) << "No EGLImage to bind"; nit: "Uninitialized image cannot be bound to texture" might be a bit more descriptive and you can then also use that same message in all implementations. https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_android_... ui/gl/gl_image_android_native_buffer.cc:74: void GLImageAndroidNativeBuffer::WillUseTexImage() { this can also be called after the buffer has been destroyed https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_android_... ui/gl/gl_image_android_native_buffer.cc:82: void GLImageAndroidNativeBuffer::DidUseTexImage() { and this https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_egl.cc File ui/gl/gl_image_egl.cc (right): https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:50: } Let's leave this check up to derived classes for now. This can stay a DCHECK as before. https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:176: DVLOG(0) << "No GLXPixMap to bind"; "Uninitialized image cannot be bound to texture" https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:190: DVLOG(0) << "No GLXPixMap to release"; "Uninitialized image cannot be released from texture" https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_io_surfa... File ui/gl/gl_image_io_surface.cc (right): https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.cc:49: DCHECK(io_surface_); you need to update this too. https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_memory.cc File ui/gl/gl_image_memory.cc (right): https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_memory.c... ui/gl/gl_image_memory.cc:128: LOG(ERROR) << "No Image memory to bind"; "Uninitialized image cannot be bound to texture" https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_surface_... File ui/gl/gl_image_surface_texture.cc (right): https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_surface_... ui/gl/gl_image_surface_texture.cc:58: LOG(ERROR) << "No SurfaceTexture Image memory to bind"; "Uninitialized image cannot be bound to texture" and move it to the top of the function to be more consistent with other implementations.
Please take a look, thanks. https://codereview.chromium.org/301793003/diff/260001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/260001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:27: images_[service_id] = image; On 2014/07/24 16:22:03, reveman wrote: > This could drop a GLImage reference without calling Destroy if the client uses > the same service_id twice. We need to check if service_id is already in use > somehow. I think the best is to DCHECK(images_.find(service_id) == > images_.end()) here and require the usage of ImageManager to ensure that the > service_id is valid. Done. https://codereview.chromium.org/301793003/diff/260001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:30: void ImageManager::RemoveImage(int32 service_id) { On 2014/07/24 16:22:03, reveman wrote: > In light of the my comment below, this function should probably also DCHECK and > require usage of ImageManager to ensure that service_id is valid. LookupImage > can be used for that. Done. https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_android_... File ui/gl/gl_image_android_native_buffer.cc (right): https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_android_... ui/gl/gl_image_android_native_buffer.cc:50: LOG(ERROR) << "No EGLImage to bind"; On 2014/07/24 16:22:03, reveman wrote: > nit: "Uninitialized image cannot be bound to texture" might be a bit more > descriptive and you can then also use that same message in all implementations. Done. https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_android_... ui/gl/gl_image_android_native_buffer.cc:74: void GLImageAndroidNativeBuffer::WillUseTexImage() { On 2014/07/24 16:22:03, reveman wrote: > this can also be called after the buffer has been destroyed Done. https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_android_... ui/gl/gl_image_android_native_buffer.cc:82: void GLImageAndroidNativeBuffer::DidUseTexImage() { On 2014/07/24 16:22:03, reveman wrote: > and this we use a new eglimage for unbinding, not controlled by image manager, do we still need the check here ? https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_egl.cc File ui/gl/gl_image_egl.cc (right): https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_egl.cc#n... ui/gl/gl_image_egl.cc:50: } On 2014/07/24 16:22:03, reveman wrote: > Let's leave this check up to derived classes for now. This can stay a DCHECK as > before. Done. https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:176: DVLOG(0) << "No GLXPixMap to bind"; On 2014/07/24 16:22:03, reveman wrote: > "Uninitialized image cannot be bound to texture" Done. https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:190: DVLOG(0) << "No GLXPixMap to release"; On 2014/07/24 16:22:03, reveman wrote: > "Uninitialized image cannot be released from texture" Done. https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_io_surfa... File ui/gl/gl_image_io_surface.cc (right): https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.cc:49: DCHECK(io_surface_); On 2014/07/24 16:22:03, reveman wrote: > you need to update this too. Done. https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_memory.cc File ui/gl/gl_image_memory.cc (right): https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_memory.c... ui/gl/gl_image_memory.cc:128: LOG(ERROR) << "No Image memory to bind"; On 2014/07/24 16:22:03, reveman wrote: > "Uninitialized image cannot be bound to texture" Done. https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_surface_... File ui/gl/gl_image_surface_texture.cc (right): https://codereview.chromium.org/301793003/diff/260001/ui/gl/gl_image_surface_... ui/gl/gl_image_surface_texture.cc:58: LOG(ERROR) << "No SurfaceTexture Image memory to bind"; On 2014/07/24 16:22:03, reveman wrote: > "Uninitialized image cannot be bound to texture" and move it to the top of the > function to be more consistent with other implementations. Done.
https://codereview.chromium.org/301793003/diff/320001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/320001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:949: if (id <= 0) { I don't think this check should exist. Either we change the IPC to uint32 or fix so int32 is properly supported. I vote for the later as it looks like all we have to do is int32 instead uint32 as the key in the image manager hash_map. https://codereview.chromium.org/301793003/diff/320001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:953: image_manager->AddImage(image.get(), id); You need to check for collisions before this call. We can't add the image if one already exists with the same ID. https://codereview.chromium.org/301793003/diff/320001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:963: if (id <= 0) { this check shouldn't exist. https://codereview.chromium.org/301793003/diff/320001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:967: DCHECK(image_manager->LookupImage(id)); A DCHECK is not OK here. We need to log an error and early out to be sure that a compromised renderer can't cause the GPU process to crash or behave in undefined way. https://codereview.chromium.org/301793003/diff/320001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/320001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:27: // Ensure the service_id is not already in use. no need for this comment. https://codereview.chromium.org/301793003/diff/320001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:33: if (service_id <= 0) { this check should not be here. https://codereview.chromium.org/301793003/diff/320001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:38: if (iter == images_.end()) { replace this with DCHECK(iter != images_.end()); https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_android_... File ui/gl/gl_image_android_native_buffer.cc (right): https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_android_... ui/gl/gl_image_android_native_buffer.cc:52: } Hm, maybe all these should be DCHECKs after all. I can't think of a case where this can happen unless there's a bug in our code as BindTexImage() can only be called from the command buffer that owns the image. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_android_... ui/gl/gl_image_android_native_buffer.cc:75: if (egl_image_ == EGL_NO_IMAGE_KHR) { Let's make this DCHECK_NE(EGL_NO_IMAGE_KHR, egl_image_) too. It could be a problem if this was ever used outside android_webview but not sure we need to worry about that now. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_android_... ui/gl/gl_image_android_native_buffer.cc:86: if (egl_image_ == EGL_NO_IMAGE_KHR) { DCHECK_NE(EGL_NO_IMAGE_KHR, egl_image_) instead https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:178: } Let's keep this as before. Sorry. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:192: } Keep as before. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_io_surfa... File ui/gl/gl_image_io_surface.cc (right): https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.cc:42: } Let's keep this as before. Sorry. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_memory.cc File ui/gl/gl_image_memory.cc (right): https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_memory.c... ui/gl/gl_image_memory.cc:130: } Let's keep this as before. Sorry. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_surface_... File ui/gl/gl_image_surface_texture.cc (right): https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_surface_... ui/gl/gl_image_surface_texture.cc:45: } Let's keep this as before. Sorry.
Please take a look, thanks. https://codereview.chromium.org/301793003/diff/320001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/320001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:949: if (id <= 0) { On 2014/07/25 19:00:23, reveman wrote: > I don't think this check should exist. Either we change the IPC to uint32 or fix > so int32 is properly supported. I vote for the later as it looks like all we > have to do is int32 instead uint32 as the key in the image manager hash_map. Done. https://codereview.chromium.org/301793003/diff/320001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:953: image_manager->AddImage(image.get(), id); On 2014/07/25 19:00:23, reveman wrote: > You need to check for collisions before this call. We can't add the image if one > already exists with the same ID. Done. https://codereview.chromium.org/301793003/diff/320001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:963: if (id <= 0) { On 2014/07/25 19:00:23, reveman wrote: > this check shouldn't exist. Done. https://codereview.chromium.org/301793003/diff/320001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:967: DCHECK(image_manager->LookupImage(id)); On 2014/07/25 19:00:23, reveman wrote: > A DCHECK is not OK here. We need to log an error and early out to be sure that a > compromised renderer can't cause the GPU process to crash or behave in undefined > way. Done. https://codereview.chromium.org/301793003/diff/320001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/320001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:27: // Ensure the service_id is not already in use. On 2014/07/25 19:00:23, reveman wrote: > no need for this comment. Done. https://codereview.chromium.org/301793003/diff/320001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:33: if (service_id <= 0) { On 2014/07/25 19:00:23, reveman wrote: > this check should not be here. Done. https://codereview.chromium.org/301793003/diff/320001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:38: if (iter == images_.end()) { On 2014/07/25 19:00:23, reveman wrote: > replace this with DCHECK(iter != images_.end()); Done. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_android_... File ui/gl/gl_image_android_native_buffer.cc (right): https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_android_... ui/gl/gl_image_android_native_buffer.cc:52: } On 2014/07/25 19:00:23, reveman wrote: > Hm, maybe all these should be DCHECKs after all. I can't think of a case where > this can happen unless there's a bug in our code as BindTexImage() can only be > called from the command buffer that owns the image. Done. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_android_... ui/gl/gl_image_android_native_buffer.cc:75: if (egl_image_ == EGL_NO_IMAGE_KHR) { On 2014/07/25 19:00:23, reveman wrote: > Let's make this DCHECK_NE(EGL_NO_IMAGE_KHR, egl_image_) too. It could be a > problem if this was ever used outside android_webview but not sure we need to > worry about that now. Done. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_android_... ui/gl/gl_image_android_native_buffer.cc:86: if (egl_image_ == EGL_NO_IMAGE_KHR) { On 2014/07/25 19:00:23, reveman wrote: > DCHECK_NE(EGL_NO_IMAGE_KHR, egl_image_) instead Done. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:178: } On 2014/07/25 19:00:23, reveman wrote: > Let's keep this as before. Sorry. Done. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:192: } On 2014/07/25 19:00:23, reveman wrote: > Keep as before. Done. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_io_surfa... File ui/gl/gl_image_io_surface.cc (right): https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_io_surfa... ui/gl/gl_image_io_surface.cc:42: } On 2014/07/25 19:00:23, reveman wrote: > Let's keep this as before. Sorry. Done. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_memory.cc File ui/gl/gl_image_memory.cc (right): https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_memory.c... ui/gl/gl_image_memory.cc:130: } On 2014/07/25 19:00:23, reveman wrote: > Let's keep this as before. Sorry. Done. https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_surface_... File ui/gl/gl_image_surface_texture.cc (right): https://codereview.chromium.org/301793003/diff/320001/ui/gl/gl_image_surface_... ui/gl/gl_image_surface_texture.cc:45: } On 2014/07/25 19:00:23, reveman wrote: > Let's keep this as before. Sorry. Done.
lgtm with nits and please update the description https://codereview.chromium.org/301793003/diff/340001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/340001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:964: LOG(ERROR) << "Image doesnt exist."; nit: "Image with ID doesn't exist." https://codereview.chromium.org/301793003/diff/340001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/340001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:34: iter->second.get()->Destroy(true); Note: maybe it would make more sense to have the caller of RemoveImage call Destroy on the image. I'm not sure so let's leave it like this for now. https://codereview.chromium.org/301793003/diff/340001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/301793003/diff/340001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:188: return; nit: you don't have to change this. please keep it as before or maybe more consistent with dtor if: DCHECK_NE(0u, glx_pixmap_)
Thank you for your time on this :) https://codereview.chromium.org/301793003/diff/340001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/340001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:964: LOG(ERROR) << "Image doesnt exist."; On 2014/07/27 23:58:32, reveman wrote: > nit: "Image with ID doesn't exist." Done. https://codereview.chromium.org/301793003/diff/340001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/340001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:34: iter->second.get()->Destroy(true); On 2014/07/27 23:58:32, reveman wrote: > Note: maybe it would make more sense to have the caller of RemoveImage call > Destroy on the image. I'm not sure so let's leave it like this for now. Acknowledged. https://codereview.chromium.org/301793003/diff/340001/ui/gl/gl_image_glx.cc File ui/gl/gl_image_glx.cc (right): https://codereview.chromium.org/301793003/diff/340001/ui/gl/gl_image_glx.cc#n... ui/gl/gl_image_glx.cc:188: return; On 2014/07/27 23:58:32, reveman wrote: > nit: you don't have to change this. please keep it as before or maybe more > consistent with dtor if: DCHECK_NE(0u, glx_pixmap_) Done.
On 2014/07/28 10:15:58, sohanjg wrote: > Thank you for your time on this :) > > https://codereview.chromium.org/301793003/diff/340001/content/common/gpu/gpu_... > File content/common/gpu/gpu_command_buffer_stub.cc (right): > > https://codereview.chromium.org/301793003/diff/340001/content/common/gpu/gpu_... > content/common/gpu/gpu_command_buffer_stub.cc:964: LOG(ERROR) << "Image doesnt > exist."; > On 2014/07/27 23:58:32, reveman wrote: > > nit: "Image with ID doesn't exist." > > Done. > > https://codereview.chromium.org/301793003/diff/340001/gpu/command_buffer/serv... > File gpu/command_buffer/service/image_manager.cc (right): > > https://codereview.chromium.org/301793003/diff/340001/gpu/command_buffer/serv... > gpu/command_buffer/service/image_manager.cc:34: > iter->second.get()->Destroy(true); > On 2014/07/27 23:58:32, reveman wrote: > > Note: maybe it would make more sense to have the caller of RemoveImage call > > Destroy on the image. I'm not sure so let's leave it like this for now. > > Acknowledged. > > https://codereview.chromium.org/301793003/diff/340001/ui/gl/gl_image_glx.cc > File ui/gl/gl_image_glx.cc (right): > > https://codereview.chromium.org/301793003/diff/340001/ui/gl/gl_image_glx.cc#n... > ui/gl/gl_image_glx.cc:188: return; > On 2014/07/27 23:58:32, reveman wrote: > > nit: you don't have to change this. please keep it as before or maybe more > > consistent with dtor if: DCHECK_NE(0u, glx_pixmap_) > > Done. Should we cq this ?
gpu/ lgtm https://codereview.chromium.org/301793003/diff/360001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/360001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:964: LOG(ERROR) << "Image with ID doesn't exist."; nit: maybe the checks here and above fit better inside ImageManager? Then you could maybe avoid doing two lookups. I think hash_map::erase() returns the number of elements erased.
actually one thing: The ImageManager is currently referenced by ContextGroup. You should probably move the destruction from the decoder to ContextGroup::Destroy(have_context) similar to the other managers.
On 2014/07/28 17:41:14, sievers wrote: > actually one thing: > > The ImageManager is currently referenced by ContextGroup. > You should probably move the destruction from the decoder to > ContextGroup::Destroy(have_context) similar to the other managers. I think reveman@ has changed the ownership to decoder as art of his patch to remove Image IPCs's, https://codereview.chromium.org/331723003/
FYI, the context group doesn't have an ImageManager reference anymore. It's own by the decoder since r284097. https://codereview.chromium.org/301793003/diff/360001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/360001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:964: LOG(ERROR) << "Image with ID doesn't exist."; On 2014/07/28 17:38:25, sievers wrote: > nit: maybe the checks here and above fit better inside ImageManager? > Then you could maybe avoid doing two lookups. I think hash_map::erase() returns > the number of elements erased. I suggested we check this here and just DCHECK in the image manager as whether it's OK to crash or not depends on the command buffer. I'm fine doing it the other way though. AddImage just needs to be able to return false.
https://codereview.chromium.org/301793003/diff/360001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/360001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:964: LOG(ERROR) << "Image with ID doesn't exist."; On 2014/07/28 18:00:00, reveman wrote: > On 2014/07/28 17:38:25, sievers wrote: > > nit: maybe the checks here and above fit better inside ImageManager? > > Then you could maybe avoid doing two lookups. I think hash_map::erase() > returns > > the number of elements erased. > > I suggested we check this here and just DCHECK in the image manager as whether > it's OK to crash or not depends on the command buffer. I'm fine doing it the > other way though. AddImage just needs to be able to return false. sievers@ should we take the checks back to imagemanager or you are ok at this being here ?
On 2014/07/30 10:30:27, sohanjg wrote: > https://codereview.chromium.org/301793003/diff/360001/content/common/gpu/gpu_... > File content/common/gpu/gpu_command_buffer_stub.cc (right): > > https://codereview.chromium.org/301793003/diff/360001/content/common/gpu/gpu_... > content/common/gpu/gpu_command_buffer_stub.cc:964: LOG(ERROR) << "Image with ID > doesn't exist."; > On 2014/07/28 18:00:00, reveman wrote: > > On 2014/07/28 17:38:25, sievers wrote: > > > nit: maybe the checks here and above fit better inside ImageManager? > > > Then you could maybe avoid doing two lookups. I think hash_map::erase() > > returns > > > the number of elements erased. > > > > I suggested we check this here and just DCHECK in the image manager as whether > > it's OK to crash or not depends on the command buffer. I'm fine doing it the > > other way though. AddImage just needs to be able to return false. > > sievers@ should we take the checks back to imagemanager or you are ok at this > being here ? Maybe the way reveman suggested with AddImage() returning false?
On 2014/07/30 10:41:47, sievers wrote: > On 2014/07/30 10:30:27, sohanjg wrote: > > > https://codereview.chromium.org/301793003/diff/360001/content/common/gpu/gpu_... > > File content/common/gpu/gpu_command_buffer_stub.cc (right): > > > > > https://codereview.chromium.org/301793003/diff/360001/content/common/gpu/gpu_... > > content/common/gpu/gpu_command_buffer_stub.cc:964: LOG(ERROR) << "Image with > ID > > doesn't exist."; > > On 2014/07/28 18:00:00, reveman wrote: > > > On 2014/07/28 17:38:25, sievers wrote: > > > > nit: maybe the checks here and above fit better inside ImageManager? > > > > Then you could maybe avoid doing two lookups. I think hash_map::erase() > > > returns > > > > the number of elements erased. > > > > > > I suggested we check this here and just DCHECK in the image manager as > whether > > > it's OK to crash or not depends on the command buffer. I'm fine doing it the > > > other way though. AddImage just needs to be able to return false. > > > > sievers@ should we take the checks back to imagemanager or you are ok at this > > being here ? > > Maybe the way reveman suggested with AddImage() returning false? OK. Updated. Please take a look, thanks!
https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:32: images_[service_id] = image; ok now that you have moved the code, let's actually iterate only once :) std::pair<GLImageMap::iterator,bool> ret = images_.insert(service_id, image); if (!ret->second) { LOG(ERROR) << "Image already exists with same ID."; return false; } return true; https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:39: LOG(ERROR) << "Image with ID doesn't exist."; You can just do LOG_IF(ERROR, iter == images_.end()) and don't have to return a value form this method. https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.h (right): https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.h:28: bool RemoveImage(int32 service_id); I'd leave RemoveImage() as returning void
https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:32: images_[service_id] = image; On 2014/07/30 12:34:08, sievers wrote: > ok now that you have moved the code, let's actually iterate only once :) > > std::pair<GLImageMap::iterator,bool> ret = images_.insert(service_id, image); > > if (!ret->second) { > LOG(ERROR) << "Image already exists with same ID."; > return false; > } > > return true; Scrap that, my bad. This doesn't work, since it will only return |false| I think if both key and value match for the existing entry.
Please take a look, thanks. https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:32: images_[service_id] = image; On 2014/07/30 12:40:41, sievers wrote: > On 2014/07/30 12:34:08, sievers wrote: > > ok now that you have moved the code, let's actually iterate only once :) > > > > std::pair<GLImageMap::iterator,bool> ret = images_.insert(service_id, image); > > > > if (!ret->second) { > > LOG(ERROR) << "Image already exists with same ID."; > > return false; > > } > > > > return true; > > Scrap that, my bad. This doesn't work, since it will only return |false| I think > if both key and value match for the existing entry. Acknowledged. https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:39: LOG(ERROR) << "Image with ID doesn't exist."; On 2014/07/30 12:34:08, sievers wrote: > You can just do LOG_IF(ERROR, iter == images_.end()) and don't have to return a > value form this method. Done. https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.h (right): https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.h:28: bool RemoveImage(int32 service_id); On 2014/07/30 12:34:08, sievers wrote: > I'd leave RemoveImage() as returning void Done.
https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/380001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:32: images_[service_id] = image; On 2014/07/30 13:08:02, sohanjg wrote: > On 2014/07/30 12:40:41, sievers wrote: > > On 2014/07/30 12:34:08, sievers wrote: > > > ok now that you have moved the code, let's actually iterate only once :) > > > > > > std::pair<GLImageMap::iterator,bool> ret = images_.insert(service_id, > image); > > > > > > if (!ret->second) { > > > LOG(ERROR) << "Image already exists with same ID."; > > > return false; > > > } > > > > > > return true; > > > > Scrap that, my bad. This doesn't work, since it will only return |false| I > think > > if both key and value match for the existing entry. > > Acknowledged. Actually, I do think it works, and I just got confused by the MSDN example which is crafted in a way that's a bit unfortunate. You could try changing it but I'd add a unittest to gles2_cmd_decoder_unittest.cc to make sure it works: TEST_P(GLES2DecoderTest, AddImageWithExistingIdFails) { scoped_refptr<gfx::GLImage> old_image(new gfx::GLImageStub); scoped_refptr<gfx::GLImage> new_image(new gfx::GLImageStub); EXPECT_TRUE(GetImageManager()->AddImage(old_image.get(), 1)); EXPECT_FALSE(GetImageManager()->AddImage(new_image.get(), 1)); }
https://codereview.chromium.org/301793003/diff/400001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/400001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:950: return; I just remembered the second reason to why I prefer the check using LookupImage from here instead of in the tile manager. If this fails, we now have to create a GLImage instance and then destroy it. But if the check was done from here we could check that it's a valid ID before we create the GLImage. If you keep this as is, then you have to call Destroy on the image here when adding it fails. However, I would much prefer to not create the image in the first place if the ID is invalid. sievers@, what do you think? https://codereview.chromium.org/301793003/diff/400001/gpu/command_buffer/serv... File gpu/command_buffer/service/image_manager.cc (right): https://codereview.chromium.org/301793003/diff/400001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:38: LOG_IF(ERROR, iter == images_.end()); You need to return early if the service id doesn't exist. As of right now, a compromised renderer can cause the GPU process to crash or worse by sending an invalid ID with UnregisterGpuMemoryBuffer IPC. https://codereview.chromium.org/301793003/diff/400001/gpu/command_buffer/serv... gpu/command_buffer/service/image_manager.cc:39: iter->second.get()->Destroy(true); This is where the GPU process would crash or behave in an unknown way if ID was invalid.
https://codereview.chromium.org/301793003/diff/400001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/400001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:950: return; On 2014/07/30 14:11:12, reveman wrote: > I just remembered the second reason to why I prefer the check using LookupImage > from here instead of in the tile manager. If this fails, we now have to create a > GLImage instance and then destroy it. But if the check was done from here we > could check that it's a valid ID before we create the GLImage. > > If you keep this as is, then you have to call Destroy on the image here when > adding it fails. However, I would much prefer to not create the image in the > first place if the ID is invalid. sievers@, what do you think? Normally I'd say it makes more sense to optimize it for the expected case (one lookup instead of two) than the failure path. However, it's a hash map and the number of items will be comparably small anyhow (or we have other perf problems). So either is totally fine with me. As long as it behaves robustly (see your other comment), please land. I don't want to delay sohan further by nitpicking :) This patch is a good improvement.
Moved the image checks to command buffer. https://codereview.chromium.org/301793003/diff/400001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/400001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:950: return; On 2014/07/30 14:46:46, sievers wrote: > On 2014/07/30 14:11:12, reveman wrote: > > I just remembered the second reason to why I prefer the check using > LookupImage > > from here instead of in the tile manager. If this fails, we now have to create > a > > GLImage instance and then destroy it. But if the check was done from here we > > could check that it's a valid ID before we create the GLImage. > > > > If you keep this as is, then you have to call Destroy on the image here when > > adding it fails. However, I would much prefer to not create the image in the > > first place if the ID is invalid. sievers@, what do you think? > > Normally I'd say it makes more sense to optimize it for the expected case (one > lookup instead of two) than the failure path. However, it's a hash map and the > number of items will be comparably small anyhow (or we have other perf > problems). > So either is totally fine with me. As long as it behaves robustly (see your > other comment), please land. I don't want to delay sohan further by nitpicking > :) This patch is a good improvement. > thanks :)
lgtm https://codereview.chromium.org/301793003/diff/420001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/420001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:951: return; Now you can move 949-952 to before line 932.
https://codereview.chromium.org/301793003/diff/420001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/420001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:946: if (decoder_) { What if this check fails? That will cause the image to be deleted without Destroy being called. https://codereview.chromium.org/301793003/diff/420001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:952: } This check needs to happen before CreateImageForGpuMemoryBuffer, otherwise you're still missing a Destroy call. I think we should early out before CreateImageForGpuMemoryBuffer if decoder_ is NULL or the image ID already exists. Something like: if (!decoder_) return; gpu::gles2::ImageManager* image_manager = decoder_->GetImageManager(); if (image_manager->LookupImage(id)) { LOG(ERROR) << "Image already exists with same ID."; return; } scoped_refptr<gfx::GLImage> image = CreateImageForGpuMemoryBuffer... https://codereview.chromium.org/301793003/diff/420001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:960: if (decoder_) { depending on what you do in the function above you might want to change this to an early out for consistency: if (!decoder_) return;
https://codereview.chromium.org/301793003/diff/420001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/420001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:946: if (decoder_) { On 2014/07/30 15:16:04, reveman wrote: > What if this check fails? That will cause the image to be deleted without > Destroy being called. Acknowledged. https://codereview.chromium.org/301793003/diff/420001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:951: return; On 2014/07/30 15:11:27, sievers wrote: > Now you can move 949-952 to before line 932. Done. https://codereview.chromium.org/301793003/diff/420001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:952: } On 2014/07/30 15:16:06, reveman wrote: > This check needs to happen before CreateImageForGpuMemoryBuffer, otherwise > you're still missing a Destroy call. > > I think we should early out before CreateImageForGpuMemoryBuffer if decoder_ is > NULL or the image ID already exists. > > Something like: > > if (!decoder_) > return; > > gpu::gles2::ImageManager* image_manager = decoder_->GetImageManager(); > if (image_manager->LookupImage(id)) { > LOG(ERROR) << "Image already exists with same ID."; > return; > } > > scoped_refptr<gfx::GLImage> image = CreateImageForGpuMemoryBuffer... Done. https://codereview.chromium.org/301793003/diff/420001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:960: if (decoder_) { On 2014/07/30 15:16:05, reveman wrote: > depending on what you do in the function above you might want to change this to > an early out for consistency: > > if (!decoder_) > return; Done.
lgtm with minor styling nits https://codereview.chromium.org/301793003/diff/440001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/440001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:963: return; nit: blank line here for consistency https://codereview.chromium.org/301793003/diff/440001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:969: } nit: I would add a blank line here too
https://codereview.chromium.org/301793003/diff/440001/content/common/gpu/gpu_... File content/common/gpu/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/301793003/diff/440001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:963: return; On 2014/07/30 16:00:04, reveman wrote: > nit: blank line here for consistency Done. https://codereview.chromium.org/301793003/diff/440001/content/common/gpu/gpu_... content/common/gpu/gpu_command_buffer_stub.cc:969: } On 2014/07/30 16:00:04, reveman wrote: > nit: I would add a blank line here too Done.
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/301793003/460001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/35375) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/40406) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/35379) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/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/301793003/460001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/35392) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/40423) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/35403) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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/301793003/480001
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/301793003/500001
Message was sent while issue was closed.
Change committed as 286811
Message was sent while issue was closed.
Description was changed from ========== During image destroy, delete textures only if we have a GL context. This adds Destroy function for ImageManager and invoke it from command decoder destroy path with GL context info. This is to ensure that when we destroy the image, textures to which the image is bound to, is deleted only if we have a GL context. BUG=375507 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286811 ========== to ========== During image destroy, delete textures only if we have a GL context. This adds Destroy function for ImageManager and invoke it from command decoder destroy path with GL context info. This is to ensure that when we destroy the image, textures to which the image is bound to, is deleted only if we have a GL context. BUG=375507 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286811 ========== |
