Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11428140:
gpu: Add async pixel transfer interface, stub and tests. (Closed)
|
Created:
8 years ago by epenner Modified:
8 years ago Reviewers:
greggman, nduca, reveman, aelias_OOO_until_Jul13, apatrick_chromium, apatrick, no sievers, epennerAtGoogle CC:
chromium-reviews, apatrick_chromium Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Descriptiongpu: Add async pixel transfers.
This adds a generic async pixel transfer interface, a
stub/fallback implementation and mocks/tests.
NOTRY=true
BUG=161337
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173430
Patch Set 1 #
Total comments: 46
Patch Set 2 : Address feedback. Added asyncTexImage. #
Total comments: 21
Patch Set 3 : Added memory dup/mmap #
Total comments: 53
Patch Set 4 : Rebase. Fix lint. #
Total comments: 44
Patch Set 5 : Address feedback (still looking into FBO/float error feedback) #Patch Set 6 : Address remaining feedback (FBO and float types) #Patch Set 7 : Split stub into .h/.cc. Minor other changes. Rebase. #Patch Set 8 : Fix build. #Patch Set 9 : Fix Asan bot. #
Total comments: 1
Patch Set 10 : Address feedback. Rebase. #Patch Set 11 : Fix bots. #
Total comments: 16
Patch Set 12 : Address new feedback. #Patch Set 13 : Move BindFinishedTransfers to MakeCurrent. Rebase again. #
Total comments: 8
Patch Set 14 : Address Feedback. #Messages
Total messages: 39 (0 generated)
PTAL. I'm android sheriff so it's a good time to collect feedback. It's working great now with a lot of random EGL/chrome issues solved. But there is still some design issues to resolve. Feedback on the TODOs and description are much appreciated!
I mixture of hopefully valuable feedback and annoying nits. I know it's a work in progress so feel free to disregard the nits for now. Regarding ensuring that the client cannot destroy the shared memory while the async thread is still using it, which is vital, you could dup the shared memory handle and have the async thread close it when it is done. The OS will effectively reference count. You might also be able to remap the shared memory copy-on-write so that future modifications of the shared memory by the client are not visible to the async thread. I don't know if Android can do that but windows can and I'm about 90% POSIX can. piman would be a good person to ask. On Windows, see DuplicateHandle, MapViewOfFile on MSDN. https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gles2_cmd_decoder.cc:9418: // TODO(epenner): We should really do this level clear as part of the It might be a bit tricky to synchronize that. Maybe it's okay to do it here if it is known to be rare. Let's see what gman thinks. https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/qu... File gpu/command_buffer/service/query_manager.cc (right): https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/qu... gpu/command_buffer/service/query_manager.cc:169: , public base::SupportsWeakPtr<AsyncPixelTransfersCompletedQuery> { style nit: comma wants to be on line above. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate.h (right): https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate.h:5: #ifndef UI_GL_ASYNC_TASK_DELEGATE_H_ At first I was like, this definitely shouldn't be in this directory. I'm not sure now. It doesn't have any dependencies on gpu/, which is great. I don't think it needs a new target. I will ponder. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate.h:16: : public base::RefCounted<AsyncPixelTransferState> { RefCountedThreadSafe will do atomic increment / decrement for ref counting and I think you might want that. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:27: nit: delete blank line. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:28: bool check_errors(const char* file, int line) { nit: CheckErrors https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:49: , egl_image_(0) {} nit: comma to line above. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:56: void no_op() {} I think you sent with the free function in the anonymous namespace. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:118: return make_scoped_refptr((AsyncPixelTransferState*) nit: no c-style casts. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:150: void no_op() {} nit: no indentation. NoOp. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:177: AsyncTransferStateAndroid* state =(AsyncTransferStateAndroid*)transfer_state; nit: no c-style casts. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:190: EGLClientBuffer egl_buffer = (EGLClientBuffer) state->texture_id_; nit: c-style casts. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:227: thread_surface_ = new gfx::PbufferGLSurfaceEGL(software, gfx::Size(1,1)); Neither GLSurface or GLContext implementations are thread-safe in general. I don't object to changing that. They would need to derive from RefCountedThreadSafe. There are, unfortunatley, a bunch of globals. In particular there is a glocal current context pointer that GLContext::MakeCurrent updates. That would need to become TLS or something. Alternatively, and this is not necessarily a good idea, you could just call into EGL directly on the async upload thread and not worry about the thread safety of these classes. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:232: bool is_current = thread_context_->MakeCurrent(thread_surface_); This will modify the current context pointer, shared with the main thread.
Thanks! I'll fix the nits along with the next patch. https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gles2_cmd_decoder.cc:9418: // TODO(epenner): We should really do this level clear as part of the On 2012/12/03 21:23:59, apatrick wrote: > It might be a bit tricky to synchronize that. Maybe it's okay to do it here if > it is known to be rare. Let's see what gman thinks. Hmm.. You are right. If we keep the clear here, then everything is safe. Since the cost can easily be avoided, best to just keep it here. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate.h (right): https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate.h:5: #ifndef UI_GL_ASYNC_TASK_DELEGATE_H_ On 2012/12/03 21:23:59, apatrick wrote: > At first I was like, this definitely shouldn't be in this directory. I'm not > sure now. It doesn't have any dependencies on gpu/, which is great. I don't > think it needs a new target. I will ponder. I initially had this code in gpu/ but ran into problems with #including EGL headers. This file needs the android specific headers and something is different about this build target. I can dig into it if there is a better location to put this. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate.h:16: : public base::RefCounted<AsyncPixelTransferState> { On 2012/12/03 21:23:59, apatrick wrote: > RefCountedThreadSafe will do atomic increment / decrement for ref counting and I > think you might want that. Currently lifetime for this object is controlled entirely on the main thread. In one case a reply holds a reference to insure it isn't deleted before a task is complete, but since the reply executes on the main thread I think it's safe. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:56: void no_op() {} On 2012/12/03 21:23:59, apatrick wrote: > I think you sent with the free function in the anonymous namespace. I use this one too, solely such that a reply holds a reference to this object. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:227: thread_surface_ = new gfx::PbufferGLSurfaceEGL(software, gfx::Size(1,1)); On 2012/12/03 21:23:59, apatrick wrote: > Neither GLSurface or GLContext implementations are thread-safe in general. I > don't object to changing that. They would need to derive from > RefCountedThreadSafe. There are, unfortunatley, a bunch of globals. In > particular there is a glocal current context pointer that GLContext::MakeCurrent > updates. That would need to become TLS or something. > > Alternatively, and this is not necessarily a good idea, you could just call into > EGL directly on the async upload thread and not worry about the thread safety of > these classes. I see! I think since this is self-contained and platform-specific it might be okay to just use raw egl calls. At least temporarily. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:232: bool is_current = thread_context_->MakeCurrent(thread_surface_); On 2012/12/03 21:23:59, apatrick wrote: > This will modify the current context pointer, shared with the main thread. Very good to know, thanks!
https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:227: thread_surface_ = new gfx::PbufferGLSurfaceEGL(software, gfx::Size(1,1)); On 2012/12/03 21:23:59, apatrick wrote: > Neither GLSurface or GLContext implementations are thread-safe in general. I > don't object to changing that. They would need to derive from > RefCountedThreadSafe. There are, unfortunatley, a bunch of globals. In > particular there is a glocal current context pointer that GLContext::MakeCurrent > updates. That would need to become TLS or something. > > Alternatively, and this is not necessarily a good idea, you could just call into > EGL directly on the async upload thread and not worry about the thread safety of > these classes. Actually after looking at this it appears some of what you are mentioning has been done (eg the global context uses TLS etc.) https://cs.corp.google.com/#chrome/src/ui/gl/gl_context.cc&sq=package:chrome&... Possibly they are indeed thread-safe now? Daniel is also interested since I believe we use GL on different threads for android webview.
Maybe I'm out-of-date. I'll check it tomorrow.
ptal. Addressed feedback thus far and added asyncTexImage2D. I think pending a solution to the threaded use-after-free issue it's getting close. I'll also be testing it with impl-side painting once 11412043 lands. https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gles2_cmd_decoder.cc:9418: // TODO(epenner): We should really do this level clear as part of the On 2012/12/03 22:48:45, epennerAtGoogle wrote: > On 2012/12/03 21:23:59, apatrick wrote: > > It might be a bit tricky to synchronize that. Maybe it's okay to do it here if > > it is known to be rare. Let's see what gman thinks. > > Hmm.. You are right. If we keep the clear here, then everything is safe. Since > the cost can easily be avoided, best to just keep it here. Done. https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/qu... File gpu/command_buffer/service/query_manager.cc (right): https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/qu... gpu/command_buffer/service/query_manager.cc:169: , public base::SupportsWeakPtr<AsyncPixelTransfersCompletedQuery> { On 2012/12/03 21:23:59, apatrick wrote: > style nit: comma wants to be on line above. Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:27: On 2012/12/03 21:23:59, apatrick wrote: > nit: delete blank line. Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:28: bool check_errors(const char* file, int line) { On 2012/12/03 21:23:59, apatrick wrote: > nit: CheckErrors Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:49: , egl_image_(0) {} On 2012/12/03 21:23:59, apatrick wrote: > nit: comma to line above. Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:118: return make_scoped_refptr((AsyncPixelTransferState*) On 2012/12/03 21:23:59, apatrick wrote: > nit: no c-style casts. Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:150: void no_op() {} On 2012/12/03 21:23:59, apatrick wrote: > nit: no indentation. NoOp. Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:177: AsyncTransferStateAndroid* state =(AsyncTransferStateAndroid*)transfer_state; On 2012/12/03 21:23:59, apatrick wrote: > nit: no c-style casts. Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:190: EGLClientBuffer egl_buffer = (EGLClientBuffer) state->texture_id_; On 2012/12/03 21:23:59, apatrick wrote: > nit: c-style casts. Done.
More annoying nits for you. I think it might be worthwhile seeing what the performance cost of dup, mmap and friends is on Android to determine if that is a viable way to fix the use after free issue. https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... File gpu/command_buffer/service/texture_manager.h (right): https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... gpu/command_buffer/service/texture_manager.h:196: if (!async_transfer_state_) nit: chromium style is to use braces for body of if here. https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... File ui/gl/async_pixel_transfer_delegate.h (right): https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate.h:18: : public base::RefCounted<AsyncPixelTransferState> { RefCountedThreadSafe https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate.h:52: AsyncPixelTransferState*, nit: indentation is out by 2 spaces https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate_android.cc:48: // The EGLImage is accessed by either thread, but the everything nit: fix comment https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate_android.cc:60: if (egl_image_) { nit: indentation out by 1 space https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate_android.cc:253: // the client texture is not effected. nit: effected -> affected https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate_android.cc:342: thread_surface_->Initialize(); What if this fails? https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate_android.cc:343: thread_context_ = gfx::GLContext::CreateGLContext(share_group, And this. https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate_android.cc:346: bool is_current = thread_context_->MakeCurrent(thread_surface_); Ditto.
https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gles2_cmd_decoder.cc:9405: SetGLErrorInvalidEnum("glTexSubImage2D", type, "type"); s/glTexSubImage2D/glAsyncTexSubImage2DCHROMIUM/ here and a bunch of other places https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gles2_cmd_decoder.cc:9436: gfx::AsyncPixelTransferDelegate::Get()->AsyncTexSubImage2D( I thought singletons are discouraged in Chromium? Other than the driver setup I'm not aware of any other singleton's in the gpu code. Is there a reason for this exception? It's certainly easier then passing in or setting some AsyncPixelTransferManager but it also makes it harder to mock for testing. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate.h (right): https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate.h:15: class AsyncPixelTransferState I'm just guessing that in order for this to work in the shared library build this is going to have to be class GL_EXPORT AsyncPixelTransfrerState, similarly for the class below and include "ui/gl/gl_export.h" https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate.h:20: private: nit: one empty line before private https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate.h:53: private: nit: one empty line before private https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:56: void no_op() {} nit: functions are CamelCase https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:124: { nit: { wants to be on previous line https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:129: { nit: { wants to be on previous line https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... gpu/command_buffer/service/gles2_cmd_decoder.cc:3517: if (info->AsyncTransferIsInProgress()) { Why is this needed? Binding a texture should not be dependent on state. For example if you want to query a texture you should always be able to do this glBindTexture(target, tex); glGetTextParameteriv(target, pname, &value); This will break other code that assuming binding always works which is what GL expects
https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gles2_cmd_decoder.cc:9436: gfx::AsyncPixelTransferDelegate::Get()->AsyncTexSubImage2D( On 2012/12/05 02:23:42, greggman wrote: > I thought singletons are discouraged in Chromium? Other than the driver setup > I'm not aware of any other singleton's in the gpu code. Is there a reason for > this exception? It's certainly easier then passing in or setting some > AsyncPixelTransferManager but it also makes it harder to mock for testing. It's one of my TODOs. Yeah I wasn't sure initially where to put it and I still haven't investigated it. If you have a good home in mind I'm all ears. It needs to exist at a pretty high level in the GPU process so multiple don't get created. https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... gpu/command_buffer/service/gles2_cmd_decoder.cc:3517: if (info->AsyncTransferIsInProgress()) { On 2012/12/05 02:23:42, greggman wrote: > Why is this needed? Binding a texture should not be dependent on state. For > example if you want to query a texture you should always be able to do this > > glBindTexture(target, tex); > glGetTextParameteriv(target, pname, &value); > > This will break other code that assuming binding always works which is what GL > expects Good point! I was trying to preserve the 'level cleared' safety while not doing a black-fill. An async upload might clear the level, but we still need to not mark the level cleared until it has actually happened. Hmm. I think I can avoid the bind restriction entirely and just make a conditional SetLevelCleared for async textures. There is also some lazy binding that needs to happen for asyncTexSubImage2D, since the allocation has occurred on the other thread. I figured bind is the best place to do these things.
On 2012/12/05 04:01:54, epenner wrote: > https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... > gpu/command_buffer/service/gles2_cmd_decoder.cc:9436: > gfx::AsyncPixelTransferDelegate::Get()->AsyncTexSubImage2D( > On 2012/12/05 02:23:42, greggman wrote: > > I thought singletons are discouraged in Chromium? Other than the driver setup > > I'm not aware of any other singleton's in the gpu code. Is there a reason for > > this exception? It's certainly easier then passing in or setting some > > AsyncPixelTransferManager but it also makes it harder to mock for testing. > > It's one of my TODOs. Yeah I wasn't sure initially where to put it and I still > haven't investigated it. If you have a good home in mind I'm all ears. It needs > to exist at a pretty high level in the GPU process so multiple don't get > created. > > https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... > gpu/command_buffer/service/gles2_cmd_decoder.cc:3517: if > (info->AsyncTransferIsInProgress()) { > On 2012/12/05 02:23:42, greggman wrote: > > Why is this needed? Binding a texture should not be dependent on state. For > > example if you want to query a texture you should always be able to do this > > > > glBindTexture(target, tex); > > glGetTextParameteriv(target, pname, &value); > > > > This will break other code that assuming binding always works which is what GL > > expects > > Good point! I was trying to preserve the 'level cleared' safety while not doing > a black-fill. An async upload might clear the level, but we still need to not > mark the level cleared until it has actually happened. > > Hmm. I think I can avoid the bind restriction entirely and just make a > conditional SetLevelCleared for async textures. > > There is also some lazy binding that needs to happen for asyncTexSubImage2D, > since the allocation has occurred on the other thread. I figured bind is the > best place to do these things. can you allocate the texture in AsyncTexImage2D and just upload in the other thread? I'm a little worried about how GL_OUT_OF_MEMORY gets reported for AsyncTexImage2D.
On 2012/12/05 05:19:20, greggman wrote: > On 2012/12/05 04:01:54, epenner wrote: > > > https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:9436: > > gfx::AsyncPixelTransferDelegate::Get()->AsyncTexSubImage2D( > > On 2012/12/05 02:23:42, greggman wrote: > > > I thought singletons are discouraged in Chromium? Other than the driver > setup > > > I'm not aware of any other singleton's in the gpu code. Is there a reason > for > > > this exception? It's certainly easier then passing in or setting some > > > AsyncPixelTransferManager but it also makes it harder to mock for testing. > > > > It's one of my TODOs. Yeah I wasn't sure initially where to put it and I still > > haven't investigated it. If you have a good home in mind I'm all ears. It > needs > > to exist at a pretty high level in the GPU process so multiple don't get > > created. > > > > > https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:3517: if > > (info->AsyncTransferIsInProgress()) { > > On 2012/12/05 02:23:42, greggman wrote: > > > Why is this needed? Binding a texture should not be dependent on state. For > > > example if you want to query a texture you should always be able to do this > > > > > > glBindTexture(target, tex); > > > glGetTextParameteriv(target, pname, &value); > > > > > > This will break other code that assuming binding always works which is what > GL > > > expects > > > > Good point! I was trying to preserve the 'level cleared' safety while not > doing > > a black-fill. An async upload might clear the level, but we still need to not > > mark the level cleared until it has actually happened. > > > > Hmm. I think I can avoid the bind restriction entirely and just make a > > conditional SetLevelCleared for async textures. > > > > There is also some lazy binding that needs to happen for asyncTexSubImage2D, > > since the allocation has occurred on the other thread. I figured bind is the > > best place to do these things. > > can you allocate the texture in AsyncTexImage2D and just upload in the other > thread? I'm a little worried about how GL_OUT_OF_MEMORY gets reported for > AsyncTexImage2D. That is a great point, and one that was previously in the back of my mind, but I since forgot about. OOM is the only major error I can think of which we can't screen for before issuing the async call. I thought about it some more and I'd really like to keep it async at first, at least in the Android implementation. This is mainly for performance, but also because we have found that OOM is never reported on Android (the app will just crash first), or worse it is reported as a 'catch all' error that has nothing to do with low memory. So on Android I think its safe to say that it will never happen on the upload thread, and the only way to protect against crashing is to enforce a hard limit ourselves.
Possibly ptal. I didn't address the decoder stuff but I added the shared memory pieces. https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... File gpu/command_buffer/service/texture_manager.h (right): https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... gpu/command_buffer/service/texture_manager.h:196: if (!async_transfer_state_) On 2012/12/04 20:59:22, apatrick_chromium wrote: > nit: chromium style is to use braces for body of if here. Done.
I guess you mentioned you hadn't done the decoder changes but just in case. Looking forward to them ;-) https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9375: SetGLErrorInvalidEnum("glTexSubImage2D", type, "type"); "glAsyncTexImage2DCHROMIUM" https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9381: SetGLError(GL_INVALID_VALUE, "glTexSubImage2D", "level != 0"); "glAsyncTexImage2DCHROMIUM" https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9389: "glAsyncTexImage2D", "transfer already in progress"); "glAsyncTexImage2DCHROMIUM" https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9445: SetGLErrorInvalidEnum("glTexSubImage2D", format, "format"); "glAsyncTexSubImage2DCHROMIUM" https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9449: SetGLErrorInvalidEnum("glTexSubImage2D", type, "type"); "glAsyncTexImage2DCHROMIUM" https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9454: if (pixels == NULL) "glAsyncTexImage2DCHROMIUM" https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9455: return error::kOutOfBounds; Does this need to be death or should it be SetGLError(GL_INVALID_OPERATION, ...)? https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9459: SetGLErrorInvalidEnum("glTexSubImage2D", type, "type"); "glAsyncTexImage2DCHROMIUM" https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9465: SetGLError(GL_INVALID_VALUE, "glTexSubImage2D", "level != 0"); "glAsyncTexImage2DCHROMIUM" https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9473: "glTexSubImage2D", "transfer already in progress"); "glAsyncTexImage2DCHROMIUM" https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9486: SetGLError(GL_OUT_OF_MEMORY, "glTexSubImage2D", "dimensions too big"); "glAsyncTexImage2DCHROMIUM" https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... File ui/gl/async_pixel_transfer_delegate.h (right): https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate.h:13: class SharedMemory; nit: don't intent inside namespaces https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:80: DCHECK(shm_data_offset + shm_data_size <= shm_size); DCHECK_LE? https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:100: if (egl_image_) { nit: As al pointed out. there's 3 spaces of indent here. Should be 2 https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:168: SharedMemory*, should this be const? https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:183: SharedMemory*, should this be const? https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:194: struct MemoryParams { nit: type definition come before function declarations https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:353: DCHECK(target == GL_TEXTURE_2D); DCHECK_EQ? You probably need DCHECK_EQ(static_cast<GLenum>(GL_TEXTURE_2D), target) https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:354: DCHECK(level == 0); DCHECK_EQ? or DCHECK(!level)? The linter used to complain about comparisons inside DCHECK https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:379: width, height, format, type}; nit: indentation is off https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:444: AsyncTexImage2DParams t, nit: except for i for index single letter names are discouraged in the style guide. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:454: DCHECK(0 == t.level); DCHECK_EQ? https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:455: DCHECK(EGL_NO_IMAGE_KHR == state->egl_image_); DCHECK_EQ? https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:507: DCHECK(EGL_NO_IMAGE_KHR != state->egl_image_); DCHECK_EQ? https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:508: DCHECK(0 == t.level); same
I clicked the lint buttons on the right. Examples: Some others https://codereview.chromium.org/lint_patch/issue11428140_17001_15008 https://codereview.chromium.org/lint_patch/issue11428140_17001_15006 https://codereview.chromium.org/lint_patch/issue11428140_17001_15007
https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:434: // EGLSyncKHR fence = eglCreateSyncKHR(display, EGL_SYNC_FENCE_KHR, NULL); btw: If you want these functions I think you need to add them to ui/gl/generate_bindings.py
https://chromiumcodereview.appspot.com/11428140/diff/17001/ui/gl/async_pixel_... File ui/gl/async_pixel_transfer_delegate.h (right): https://chromiumcodereview.appspot.com/11428140/diff/17001/ui/gl/async_pixel_... ui/gl/async_pixel_transfer_delegate.h:25: virtual ~AsyncPixelTransferState() {}; nit: destructor of ref counted class should not be public.
Extra notes: I added a stub implementation and initially the stub is enabled for all platforms. I'll enable android in an initialization call somewhere. Regarding the singleton, I didn't find a good owner yet, but I did make a stub class which lets us test the decoder handling separately from the real implementations of the delegate. A mock class would help further. Perhaps that enough to address the testing concerns initially? I think the best owner for the delegate will be more obvious if we do a single-threaded version for Linux. Lastly, I didn't change to thread-safe ref pointer because I don't think its needed (explained inline), but let me know and I can change it if needed. https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/1/gpu/command_buffer/service/gl... gpu/command_buffer/service/gles2_cmd_decoder.cc:9405: SetGLErrorInvalidEnum("glTexSubImage2D", type, "type"); On 2012/12/05 02:23:42, greggman wrote: > s/glTexSubImage2D/glAsyncTexSubImage2DCHROMIUM/ > > here and a bunch of other places Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate.h (right): https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate.h:15: class AsyncPixelTransferState On 2012/12/05 02:23:42, greggman wrote: > I'm just guessing that in order for this to work in the shared library build > this is going to have to be > > class GL_EXPORT AsyncPixelTransfrerState, similarly for the class below and > include "ui/gl/gl_export.h" Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate.h:20: private: On 2012/12/05 02:23:42, greggman wrote: > nit: one empty line before private Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate.h:53: private: On 2012/12/05 02:23:42, greggman wrote: > nit: one empty line before private Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:56: void no_op() {} On 2012/12/05 02:23:42, greggman wrote: > nit: functions are CamelCase Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:124: { On 2012/12/05 02:23:42, greggman wrote: > nit: { wants to be on previous line Done. https://codereview.chromium.org/11428140/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:129: { On 2012/12/05 02:23:42, greggman wrote: > nit: { wants to be on previous line Done. https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/8002/gpu/command_buffer/service... gpu/command_buffer/service/gles2_cmd_decoder.cc:3517: if (info->AsyncTransferIsInProgress()) { On 2012/12/05 04:01:54, epenner wrote: > On 2012/12/05 02:23:42, greggman wrote: > > Why is this needed? Binding a texture should not be dependent on state. For > > example if you want to query a texture you should always be able to do this > > > > glBindTexture(target, tex); > > glGetTextParameteriv(target, pname, &value); > > > > This will break other code that assuming binding always works which is what GL > > expects > > Good point! I was trying to preserve the 'level cleared' safety while not doing > a black-fill. An async upload might clear the level, but we still need to not > mark the level cleared until it has actually happened. > > Hmm. I think I can avoid the bind restriction entirely and just make a > conditional SetLevelCleared for async textures. > > There is also some lazy binding that needs to happen for asyncTexSubImage2D, > since the allocation has occurred on the other thread. I figured bind is the > best place to do these things. Done. https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... File ui/gl/async_pixel_transfer_delegate.h (right): https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate.h:18: : public base::RefCounted<AsyncPixelTransferState> { On 2012/12/04 20:59:22, apatrick_chromium wrote: > RefCountedThreadSafe I haven't made this thread safe as the lifetime is currently controlled entirely on the main thread. For tasks, only the reply has a reference, so the deref happens back on the main thread after the task is complete. https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate.h:52: AsyncPixelTransferState*, On 2012/12/04 20:59:22, apatrick_chromium wrote: > nit: indentation is out by 2 spaces Done. https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate_android.cc:48: // The EGLImage is accessed by either thread, but the everything On 2012/12/04 20:59:22, apatrick_chromium wrote: > nit: fix comment Is this regarding thread-safe ref counting? Let me know if you are still concerned about that (explanation is in AsyncPixelTransferState) https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate_android.cc:60: if (egl_image_) { On 2012/12/04 20:59:22, apatrick_chromium wrote: > nit: indentation out by 1 space Done. https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate_android.cc:253: // the client texture is not effected. On 2012/12/04 20:59:22, apatrick_chromium wrote: > nit: effected -> affected Done. https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate_android.cc:342: thread_surface_->Initialize(); On 2012/12/04 20:59:22, apatrick_chromium wrote: > What if this fails? Hmm, is this normally treated as a recoverable error with a resonable fallback? Or do you just mean I should change to CHECK below? I could fall back on the stub implementation possibly, but I don't think we want this to happen without knowing about it. https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate_android.cc:343: thread_context_ = gfx::GLContext::CreateGLContext(share_group, On 2012/12/04 20:59:22, apatrick_chromium wrote: > And this. See above. https://codereview.chromium.org/11428140/diff/8002/ui/gl/async_pixel_transfer... ui/gl/async_pixel_transfer_delegate_android.cc:346: bool is_current = thread_context_->MakeCurrent(thread_surface_); On 2012/12/04 20:59:22, apatrick_chromium wrote: > Ditto. See above. https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9375: SetGLErrorInvalidEnum("glTexSubImage2D", type, "type"); On 2012/12/06 06:52:00, greggman wrote: > "glAsyncTexImage2DCHROMIUM" Done. https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9381: SetGLError(GL_INVALID_VALUE, "glTexSubImage2D", "level != 0"); On 2012/12/06 06:52:00, greggman wrote: > "glAsyncTexImage2DCHROMIUM" Done. https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9389: "glAsyncTexImage2D", "transfer already in progress"); On 2012/12/06 06:52:00, greggman wrote: > "glAsyncTexImage2DCHROMIUM" Done. https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9449: SetGLErrorInvalidEnum("glTexSubImage2D", type, "type"); On 2012/12/06 06:52:00, greggman wrote: > "glAsyncTexImage2DCHROMIUM" Done. https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9454: if (pixels == NULL) On 2012/12/06 06:52:00, greggman wrote: > "glAsyncTexImage2DCHROMIUM" Done. https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9455: return error::kOutOfBounds; It's an SetGLError now when no transfer buffer is provided. https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9459: SetGLErrorInvalidEnum("glTexSubImage2D", type, "type"); On 2012/12/06 06:52:00, greggman wrote: > "glAsyncTexImage2DCHROMIUM" Done. https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9465: SetGLError(GL_INVALID_VALUE, "glTexSubImage2D", "level != 0"); On 2012/12/06 06:52:00, greggman wrote: > "glAsyncTexImage2DCHROMIUM" Done. https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9473: "glTexSubImage2D", "transfer already in progress"); On 2012/12/06 06:52:00, greggman wrote: > "glAsyncTexImage2DCHROMIUM" Done. https://codereview.chromium.org/11428140/diff/17001/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9486: SetGLError(GL_OUT_OF_MEMORY, "glTexSubImage2D", "dimensions too big"); On 2012/12/06 06:52:00, greggman wrote: > "glAsyncTexImage2DCHROMIUM" Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... File ui/gl/async_pixel_transfer_delegate.h (right): https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate.h:13: class SharedMemory; On 2012/12/06 06:52:00, greggman wrote: > nit: don't intent inside namespaces Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate.h:25: virtual ~AsyncPixelTransferState() {}; On 2012/12/06 16:01:39, David Reveman wrote: > nit: destructor of ref counted class should not be public. Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:80: DCHECK(shm_data_offset + shm_data_size <= shm_size); On 2012/12/06 06:52:00, greggman wrote: > DCHECK_LE? Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:100: if (egl_image_) { On 2012/12/06 06:52:00, greggman wrote: > nit: As al pointed out. there's 3 spaces of indent here. Should be 2 Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:168: SharedMemory*, On 2012/12/06 06:52:00, greggman wrote: > should this be const? Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:183: SharedMemory*, On 2012/12/06 06:52:00, greggman wrote: > should this be const? Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:194: struct MemoryParams { On 2012/12/06 06:52:00, greggman wrote: > nit: type definition come before function declarations Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:353: DCHECK(target == GL_TEXTURE_2D); On 2012/12/06 06:52:00, greggman wrote: > DCHECK_EQ? > > You probably need > > DCHECK_EQ(static_cast<GLenum>(GL_TEXTURE_2D), target) Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:354: DCHECK(level == 0); On 2012/12/06 06:52:00, greggman wrote: > DCHECK_EQ? or DCHECK(!level)? > > The linter used to complain about comparisons inside DCHECK Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:379: width, height, format, type}; On 2012/12/06 06:52:00, greggman wrote: > nit: indentation is off Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:434: // EGLSyncKHR fence = eglCreateSyncKHR(display, EGL_SYNC_FENCE_KHR, NULL); On 2012/12/06 11:22:20, greggman wrote: > btw: If you want these functions I think you need to add them to > ui/gl/generate_bindings.py > Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:444: AsyncTexImage2DParams t, On 2012/12/06 06:52:00, greggman wrote: > nit: except for i for index single letter names are discouraged in the style > guide. I'll change if you want but I was following the decoder convention of many parameters in a struct. The size of code for descriptive names starts to get unwieldy. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:454: DCHECK(0 == t.level); On 2012/12/06 06:52:00, greggman wrote: > DCHECK_EQ? Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:455: DCHECK(EGL_NO_IMAGE_KHR == state->egl_image_); On 2012/12/06 06:52:00, greggman wrote: > DCHECK_EQ? Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:507: DCHECK(EGL_NO_IMAGE_KHR != state->egl_image_); On 2012/12/06 06:52:00, greggman wrote: > DCHECK_EQ? Done. https://codereview.chromium.org/11428140/diff/17001/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_android.cc:508: DCHECK(0 == t.level); On 2012/12/06 06:52:00, greggman wrote: > same Done.
ptal. This removes the singleton and adds unit tests. The android EGLImage implementation will land in another CL.
https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9704: if (0 != level) { style: the chromium style guide prefers if (var == const) https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9709: if (0 == data) { style: the chromium style guide prefers if (var == const) https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9766: if (info->estimated_size() > 0) { Are you sure you need this check? If so I'd consider adding info->IsDefined() so you can play with the code outside of this function. I'm not sure there is an easy way to check though but for now estimated_size > 0 as the check inside info->IsDefined might be okay. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9772: if (info->IsAttachedToFramebuffer()) { If they are not allowed be bound to a framebuffer then you probably also need a similar check in DoFramebufferTexture2D so you can't bind an texture with a pending async upload to an fbo https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9777: if (type == GL_FLOAT || type == GL_HALF_FLOAT_OES) { Would it would be better to list the valid types rather than assume that any type but FLOAT and HALF_FLOAT is valid? https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9791: DCHECK(!info->GetAsyncTransferState()); It seems like this DCHECK is in conflict with the IF below it. Either the DCHECK is valid or the if is valid. not both? https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9800: texture_manager()->SetLevelInfo( I'm confused. You can't call SetLevelInfo until the texture has actually been created right? Otherwise the command buffer will think this texture is ok to render with. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9860: SetGLError(GL_OUT_OF_MEMORY, "glTexSubImage2D", "dimensions too big"); "glAsyncTexSubImage2D" https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:7962: AsyncTexImage2DCHROMIUM texImagecmd; style: variables are under_score https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:7965: AsyncTexSubImage2DCHROMIUM texSubImagecmd; style: variables are under_score https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8049: BindTexture bindCmd; style: variables are under_score https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... File gpu/command_buffer/service/texture_manager.h (right): https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.h:186: return async_transfer_state_ style: operators want to be at the end of the line https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.h:190: DCHECK(async_transfer_state_); It seems like this DCHECK is in conflict with the if below. Should this function have the if, in which case it shouldn't have the DCHECK? Or should it not have the if and should have the DCHECK? https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... File ui/gl/async_pixel_transfer_delegate.h (right): https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate.h:86: AsyncPixelTransferState*, style: google style requires argument names. https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate.h:87: AsyncTexImage2DParams, any reason this can't be a const ref? https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate.h:91: AsyncPixelTransferState*, style: google style requires argument names. https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate.h:92: AsyncTexSubImage2DParams, any reason this can't be a const ref? https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... File ui/gl/async_pixel_transfer_delegate_stub.cc (right): https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_stub.cc:21: void* GetAddress(SharedMemory* shared_memory, style: this wants to be in an anonymous namespace (preferred) or declare it static https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_stub.cc:69: AsyncTexImage2DParams t, style: single letter names are discouraged in the style guide. https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_stub.cc:73: glTexImage2D( I'm a little confused. This is temporary impl that uploaded immediately right?
https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9704: if (0 != level) { On 2012/12/12 03:51:36, greggman wrote: > style: the chromium style guide prefers if (var == const) Done. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9709: if (0 == data) { On 2012/12/12 03:51:36, greggman wrote: > style: the chromium style guide prefers if (var == const) Done. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9766: if (info->estimated_size() > 0) { On 2012/12/12 03:51:36, greggman wrote: > Are you sure you need this check? If so I'd consider adding info->IsDefined() so > you can play with the code outside of this function. > > I'm not sure there is an easy way to check though but for now estimated_size > 0 > as the check inside info->IsDefined might be okay. Done (added IsDefined). Redefining a texture that has an EGLImage will 'orphan' the EGLImage and result in a lot of complexity. It also gets more complicated tracking the 'cleared' state for the existing texture and the new texture being defined on another thread. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9772: if (info->IsAttachedToFramebuffer()) { On 2012/12/12 03:51:36, greggman wrote: > If they are not allowed be bound to a framebuffer then you probably also need a > similar check in DoFramebufferTexture2D so you can't bind an texture with a > pending async upload to an fbo Let me do a quick check to see if I can just allow FBO binding.. This was just because there was some custom code in DoTexImage2D https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9777: if (type == GL_FLOAT || type == GL_HALF_FLOAT_OES) { On 2012/12/12 03:51:36, greggman wrote: > Would it would be better to list the valid types rather than assume that any > type but FLOAT and HALF_FLOAT is valid? Ditto, maybe I can just allow floats actually. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9791: DCHECK(!info->GetAsyncTransferState()); On 2012/12/12 03:51:36, greggman wrote: > It seems like this DCHECK is in conflict with the IF below it. Either the DCHECK > is valid or the if is valid. not both? DCHECK is correct. Just being defensive against crashes, but I can just keep the DCHECK. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9800: texture_manager()->SetLevelInfo( On 2012/12/12 03:51:36, greggman wrote: > I'm confused. You can't call SetLevelInfo until the texture has actually been > created right? Otherwise the command buffer will think this texture is ok to > render with. My understanding was that we want to avoid reading from a texture which has been defined but not uploaded to. In this case, the texture hasn't even been defined. If this isn't okay, I could define it as a 1x1 texture temporarily. The data is 'replaced' when the async transfer is bound. The end goal is to have it always be 'safe', since the black clear will actually corrupt a pending upload (they both access the same memory at once). https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9860: SetGLError(GL_OUT_OF_MEMORY, "glTexSubImage2D", "dimensions too big"); On 2012/12/12 03:51:36, greggman wrote: > "glAsyncTexSubImage2D" Done. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right): https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:7962: AsyncTexImage2DCHROMIUM texImagecmd; On 2012/12/12 03:51:36, greggman wrote: > style: variables are under_score Done. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:7965: AsyncTexSubImage2DCHROMIUM texSubImagecmd; On 2012/12/12 03:51:36, greggman wrote: > style: variables are under_score Done. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:8049: BindTexture bindCmd; On 2012/12/12 03:51:36, greggman wrote: > style: variables are under_score Done. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... File gpu/command_buffer/service/texture_manager.h (right): https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.h:186: return async_transfer_state_ On 2012/12/12 03:51:36, greggman wrote: > style: operators want to be at the end of the line Done. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.h:190: DCHECK(async_transfer_state_); On 2012/12/12 03:51:36, greggman wrote: > It seems like this DCHECK is in conflict with the if below. Should this function > have the if, in which case it shouldn't have the DCHECK? Or should it not have > the if and should have the DCHECK? Done. https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... File ui/gl/async_pixel_transfer_delegate.h (right): https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate.h:86: AsyncPixelTransferState*, On 2012/12/12 03:51:36, greggman wrote: > style: google style requires argument names. Done. https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate.h:87: AsyncTexImage2DParams, On 2012/12/12 03:51:36, greggman wrote: > any reason this can't be a const ref? Done. https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate.h:91: AsyncPixelTransferState*, On 2012/12/12 03:51:36, greggman wrote: > style: google style requires argument names. Done. https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate.h:92: AsyncTexSubImage2DParams, On 2012/12/12 03:51:36, greggman wrote: > any reason this can't be a const ref? Done. https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... File ui/gl/async_pixel_transfer_delegate_stub.cc (right): https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_stub.cc:21: void* GetAddress(SharedMemory* shared_memory, On 2012/12/12 03:51:36, greggman wrote: > style: this wants to be in an anonymous namespace (preferred) or declare it > static Done. https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_stub.cc:69: AsyncTexImage2DParams t, On 2012/12/12 03:51:36, greggman wrote: > style: single letter names are discouraged in the style guide. Done. https://codereview.chromium.org/11428140/diff/43002/ui/gl/async_pixel_transfe... ui/gl/async_pixel_transfer_delegate_stub.cc:73: glTexImage2D( On 2012/12/12 03:51:36, greggman wrote: > I'm a little confused. This is temporary impl that uploaded immediately right? Yes this is a stub that allows the API to work by just doing synchronous calls. We could remove this and just not support the extension instead, but we would rather use the same code in the compositor, at least for now.
Thanks for all the feedback! And apologies on my sloppy Google style (got used to webkit). https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9772: if (info->IsAttachedToFramebuffer()) { On 2012/12/12 04:49:49, epennerAtGoogle wrote: > On 2012/12/12 03:51:36, greggman wrote: > > If they are not allowed be bound to a framebuffer then you probably also need > a > > similar check in DoFramebufferTexture2D so you can't bind an texture with a > > pending async upload to an fbo > > Let me do a quick check to see if I can just allow FBO binding.. This was just > because there was some custom code in DoTexImage2D I had to copy a small piece of DoTexImage2D, but just enabled FBOs. https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9777: if (type == GL_FLOAT || type == GL_HALF_FLOAT_OES) { On 2012/12/12 04:49:49, epennerAtGoogle wrote: > On 2012/12/12 03:51:36, greggman wrote: > > Would it would be better to list the valid types rather than assume that any > > type but FLOAT and HALF_FLOAT is valid? > > Ditto, maybe I can just allow floats actually. Okay, I allowed floats by adding GetTexInternalFormat(internal_format,format,type), and reusing that. It looks like some more simple refactoring is possible with that,
https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9800: texture_manager()->SetLevelInfo( On 2012/12/12 04:49:49, epennerAtGoogle wrote: > On 2012/12/12 03:51:36, greggman wrote: > > I'm confused. You can't call SetLevelInfo until the texture has actually been > > created right? Otherwise the command buffer will think this texture is ok to > > render with. > > My understanding was that we want to avoid reading from a texture which has been > defined but not uploaded to. > > In this case, the texture hasn't even been defined. If this isn't okay, I could > define it as a 1x1 texture temporarily. The data is 'replaced' when the async > transfer is bound. > > The end goal is to have it always be 'safe', since the black clear will actually > corrupt a pending upload (they both access the same memory at once). Another possibility would be to not set the level info now. It could be deferred until the async texture is bound to the texture id (done lazily).
https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/43002/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9800: texture_manager()->SetLevelInfo( On 2012/12/12 05:53:33, epennerAtGoogle wrote: > On 2012/12/12 04:49:49, epennerAtGoogle wrote: > > On 2012/12/12 03:51:36, greggman wrote: > > > I'm confused. You can't call SetLevelInfo until the texture has actually > been > > > created right? Otherwise the command buffer will think this texture is ok to > > > render with. > > > > My understanding was that we want to avoid reading from a texture which has > been > > defined but not uploaded to. > > > > In this case, the texture hasn't even been defined. If this isn't okay, I > could > > define it as a 1x1 texture temporarily. The data is 'replaced' when the async > > transfer is bound. > > > > The end goal is to have it always be 'safe', since the black clear will > actually > > corrupt a pending upload (they both access the same memory at once). > > Another possibility would be to not set the level info now. It could be deferred > until the async texture is bound to the texture id (done lazily). That seems like that would be best.
Addressed the remaining feedback. Ptal.
On 2012/12/12 21:32:37, epennerAtGoogle wrote: > Addressed the remaining feedback. Ptal. Rebased. Ptal.
https://chromiumcodereview.appspot.com/11428140/diff/38010/gpu/command_buffer... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://chromiumcodereview.appspot.com/11428140/diff/38010/gpu/command_buffer... gpu/command_buffer/service/gles2_cmd_decoder.cc:9809: if (info->IsAttachedToFramebuffer()) { Just to document our conversation This code marks says if the texture is used in an fbo then re-validate fbos. You don't want to ask for fbos to be revalidated until you've actually changed the texture. Since the texture is changing async this needs to happen later. The plans is to add some kind of task list. The async code will take a task with a weakptr to the decoder and to the TextureInfo. Once it's done uploading it will add the task to the decoder's task list. In GLES2DecoderImpl::CheckFramebufferValid any tasks on the list will be executed. those tasks will Call any GL functions needed on this context restore any state those functions changed call SetTextureInfo do the IsAttachedToFramebuffer check here to mark the fbo for re-validation this will also fix the need to bind after async completion.
Grrr! i just remembered another issue. Maybe your idea of posting the tasks to
the main thread solves this. But....textures are shared, so you need to prevent
any context in the same ContextGroup from these issues which means for example
the task list would go in the ContextGroup so that any decoder using that
texture would execute the task.
If putting the tasks in the main thread's task queue solves that great. It also
might mean instead of a weak_ptr to a decoder you want a weak_ptr to the
something else, probably the ContextGroup. The issue is
context1->glBindTexture
context2->glBindTexture
context1->glAsyncTexImage2D
context1->glDeleteTexture
delete context1
Texture is not actually deleted (glDeleteTexture is not called). The command
buffer will keep it around because it's still bound to context2.
There's unfortunately no way to get a decoder/context right now from a
ContextGroup. Ideally you'd pick one, do your stuff, restore any state and be
done with it. I suppose you could add a list of decoder weakptrs to the
ContextGroup and find one that's not NULL.
In that case you'd probably add ContextGroup::RegisterDecoder and put a call in
GLES2DecoderImpl's constructor. Similarly in GLES2DecoderImpl::Destroy you'd
put a call to ContextGroup::UnregisterDecoder. Maybe they can be raw pointers in
that case.
Then, in your last task you'd do something like
if (context_group_weakptr) {
GLES2Decoder* decoder = context_group_weakptr->GetAnyDecoder();
decoder->FinishAsyncStuff(texture_info_weakptr, async_params);
}
also note that if you put the tasks on the main thread then you'll need to get a
GLContext (I think you can get one from the decoder) and call MakeCurrent where
as if the task list is in the ContextGroup and the code to execute the tasks is
inside GLES2DecoderImpl::CheckFramebufferValid then other code has already
called MakeCurrent are the correct place and with the correct checks.
I feel like I went down this route before and stopped for some reason but I
don't remember why.
On 2012/12/13 07:50:54, greggman wrote:
> Grrr! i just remembered another issue. Maybe your idea of posting the tasks to
> the main thread solves this. But....textures are shared, so you need to
prevent
> any context in the same ContextGroup from these issues which means for example
> the task list would go in the ContextGroup so that any decoder using that
> texture would execute the task.
>
> If putting the tasks in the main thread's task queue solves that great. It
also
> might mean instead of a weak_ptr to a decoder you want a weak_ptr to the
> something else, probably the ContextGroup. The issue is
>
> context1->glBindTexture
> context2->glBindTexture
> context1->glAsyncTexImage2D
> context1->glDeleteTexture
> delete context1
>
> Texture is not actually deleted (glDeleteTexture is not called). The command
> buffer will keep it around because it's still bound to context2.
>
> There's unfortunately no way to get a decoder/context right now from a
> ContextGroup. Ideally you'd pick one, do your stuff, restore any state and be
> done with it. I suppose you could add a list of decoder weakptrs to the
> ContextGroup and find one that's not NULL.
>
> In that case you'd probably add ContextGroup::RegisterDecoder and put a call
in
> GLES2DecoderImpl's constructor. Similarly in GLES2DecoderImpl::Destroy you'd
> put a call to ContextGroup::UnregisterDecoder. Maybe they can be raw pointers
in
> that case.
>
> Then, in your last task you'd do something like
>
> if (context_group_weakptr) {
> GLES2Decoder* decoder = context_group_weakptr->GetAnyDecoder();
> decoder->FinishAsyncStuff(texture_info_weakptr, async_params);
> }
>
> also note that if you put the tasks on the main thread then you'll need to get
a
> GLContext (I think you can get one from the decoder) and call MakeCurrent
where
> as if the task list is in the ContextGroup and the code to execute the tasks
is
> inside GLES2DecoderImpl::CheckFramebufferValid then other code has already
> called MakeCurrent are the correct place and with the correct checks.
>
>
>
>
> I feel like I went down this route before and stopped for some reason but I
> don't remember why.
I thought about this last night and I think I have a nice solution that will be
pretty clean. Very good point about the context group sharing textures though.
Although the tasks reply on the main thread, I would rather not grab a context
and make it current since that could get really bad in the worst case. So here's
the plan:
- TextureInfo has a scoped_ptr AsyncTransferState.
- AsyncTransferState supports weak pointer.
- AsyncTexImage2D adds the transfer state to a "to be bound" list in the context
group.
- Async reply marks the transfer as finished.
- CheckFramebufferValid does context_group->bindPendingTransfers
> I thought about this last night and I think I have a nice solution that will be > pretty clean. Very good point about the context group sharing textures though. > Although the tasks reply on the main thread, I would rather not grab a context > and make it current since that could get really bad in the worst case. So here's > the plan: > - TextureInfo has a scoped_ptr AsyncTransferState. > - AsyncTransferState supports weak pointer. > - AsyncTexImage2D adds the transfer state to a "to be bound" list in the context > group. > - Async reply marks the transfer as finished. > - CheckFramebufferValid does context_group->bindPendingTransfers Addressed feedback as per above. Ptal.
https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:2756: // TODO(epenner): Is there a better place to do this? Transfers This seems like it really needs to be in DoCommand? here's the issue BindTexture AsyncTexImage Query till true TexSubimage2D Draw Maybe we can leave that for later? Note: That also suggests that the other texture manipulating functions probably need to check for async in progress. As in BindTexture AsyncTexXXX TexSubImage2D <- should gen INVALID_OPERATION until BindFinishedAsyncPixelTransfers has been called. CopyTexSubImage2D as well as in TextureInfo* info = GetTextureForTarget(target); if (info->IsBusy()) { SetGLError(...INVALID_OPERATION...) return; } https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:2761: GetContextGroup()->texture_manager()->BindFinishedAsyncPixelTransfers( you can just use texture_manager()->BindFinished.... right? No need to use GetContextGroup https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9792: if (!ValidateTexImage2D("glAsyncTexImage2D", target, level, internal_format, s/glAsyncTexImage2D/glAsyncTexImage2DCHROMIUM/ s/glAsyncTexSubImage2D/glAsyncTexSubImage2DCHROMIUM/ ? https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9836: GetContextGroup()->texture_manager()->AddPendingAsyncPixelTransfer( GetConextGroup()-> not needed? https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.cc:1214: DCHECK(texture_dirty); If this is going to be called often (like from DoCommand) then maybe you need 2 lists or something that makes it exit quicker if there is nothing to be done? Or maybe I'm over thinking it. Ideally if there's several pending textures but none of them are ready then this should exit extremely quickly. https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.cc:1223: it++) { always pre-increment stl iterators no? ++it means it = it + 1 it++ means temp = it; it = it + 1; return temp for a complex iterator making that temp can be expensive. I don't know if stl::list::iterator is complex. I'm guessing stl::map::iterator and stl::unordered_map::iterator are complex https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.cc:1227: pending_async_transfers_.erase(it++); I don't think you can call erase in stl like this in a loop can you? I thought the correct way was for (it = list.begin; it != list.end;) { if (want to erase) { it = list.erase(it); } else { ++it; } } https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.cc:1250: pending_async_transfers_.erase(it++); Same as above. Besides this would skip the element after the one erased no?
Guys how do we make progress on this? 18 patchsets is very scary, and this is android coupled. Is there a good-enough and secure v0 we can land and file bugs as followups? Is the entire approach flawed? We're down to the wire here.
The only remaining issue is when to do the BindFinished call. I've left it where it is as I haven't found the ideal place. Ideally there is something like "begin processing commands", which is called once when we resume doing work on the main thread. https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:2756: // TODO(epenner): Is there a better place to do this? Transfers On 2012/12/14 06:08:06, greggman wrote: > This seems like it really needs to be in DoCommand? > > here's the issue > > BindTexture > AsyncTexImage > Query till true > TexSubimage2D > Draw Does DoCommand happen at every GL command? It only needs to happen once per flush, as transfers are marked as completed on the main thread (which can't happen again until we yield the main thread). https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:2761: GetContextGroup()->texture_manager()->BindFinishedAsyncPixelTransfers( On 2012/12/14 06:08:06, greggman wrote: > you can just use > > texture_manager()->BindFinished.... > > right? No need to use GetContextGroup Done. https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9792: if (!ValidateTexImage2D("glAsyncTexImage2D", target, level, internal_format, On 2012/12/14 06:08:06, greggman wrote: > s/glAsyncTexImage2D/glAsyncTexImage2DCHROMIUM/ > s/glAsyncTexSubImage2D/glAsyncTexSubImage2DCHROMIUM/ > ? Done. https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:9836: GetContextGroup()->texture_manager()->AddPendingAsyncPixelTransfer( On 2012/12/14 06:08:06, greggman wrote: > GetConextGroup()-> not needed? Done. https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.cc:1214: DCHECK(texture_dirty); On 2012/12/14 06:08:06, greggman wrote: > If this is going to be called often (like from DoCommand) then maybe you need 2 > lists or something that makes it exit quicker if there is nothing to be done? Or > maybe I'm over thinking it. Ideally if there's several pending textures but none > of them are ready then this should exit extremely quickly. It doesn't need to happen in DoCommand IMO. Only once per flush. But the loop can also terminate early since the uploads occur in order. So it's now only one iteration per completed upload. https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.cc:1223: it++) { On 2012/12/14 06:08:06, greggman wrote: > always pre-increment stl iterators no? > > ++it means it = it + 1 > it++ means temp = it; it = it + 1; return temp > > for a complex iterator making that temp can be expensive. I don't know if > stl::list::iterator is complex. I'm guessing stl::map::iterator and > stl::unordered_map::iterator are complex Done. https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.cc:1227: pending_async_transfers_.erase(it++); On 2012/12/14 06:08:06, greggman wrote: > I don't think you can call erase in stl like this in a loop can you? I thought > the correct way was > > for (it = list.begin; it != list.end;) { > if (want to erase) { > it = list.erase(it); > } else { > ++it; > } > } Sorry, last minute change from remove_if(). Done. https://codereview.chromium.org/11428140/diff/58015/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.cc:1250: pending_async_transfers_.erase(it++); On 2012/12/14 06:08:06, greggman wrote: > Same as above. Besides this would skip the element after the one erased no? See above. Done.
On 2012/12/14 21:01:34, epenner wrote: > The only remaining issue is when to do the BindFinished call. I've left it where > it is as I haven't found the ideal place. Ideally there is something like "begin > processing commands", which is called once when we resume doing work on the main > thread. > I've moved the BindFinishedTransfers to MakeCurrent. From looking at the code this always gets called before processing new commands, which includes processing commands after we have yielded the main thread (which is when we must process finished transfers).
just a few issues then lgtm https://codereview.chromium.org/11428140/diff/57005/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/57005/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:7975: } Don't you need to check here if (info->IsAsyncUploadPending()) { SetGLError(GL_INVALID_OPERATION, "glCopyTexSubImage2D", "async upload pending for texture"); return; } https://codereview.chromium.org/11428140/diff/57005/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:8100: } Don't you need to check that an async upload is not in progress here? if (info->IsAsyncUploadPending()) { SetGLError(GL_INVALID_OPERATION, function_name, "async upload pending for texture"); return; } https://codereview.chromium.org/11428140/diff/57005/gpu/command_buffer/servic... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/11428140/diff/57005/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.cc:1232: pending_async_transfers_.erase(it++); I still believe this needs to be it = pending_async_transfers_.erase(it); so it will work regardless of container https://codereview.chromium.org/11428140/diff/57005/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.cc:1256: pending_async_transfers_.erase(it++); I still believe this needs to be it = pending_async_transfers_.erase(it);
https://codereview.chromium.org/11428140/diff/57005/gpu/command_buffer/servic... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/11428140/diff/57005/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:7975: } On 2012/12/15 06:45:04, greggman wrote: > Don't you need to check here > > if (info->IsAsyncUploadPending()) { > SetGLError(GL_INVALID_OPERATION, "glCopyTexSubImage2D", > "async upload pending for texture"); > return; > } Done. https://codereview.chromium.org/11428140/diff/57005/gpu/command_buffer/servic... gpu/command_buffer/service/gles2_cmd_decoder.cc:8100: } On 2012/12/15 06:45:04, greggman wrote: > Don't you need to check that an async upload is not in progress here? > > if (info->IsAsyncUploadPending()) { > SetGLError(GL_INVALID_OPERATION, function_name, > "async upload pending for texture"); > return; > } Done. https://codereview.chromium.org/11428140/diff/57005/gpu/command_buffer/servic... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/11428140/diff/57005/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.cc:1232: pending_async_transfers_.erase(it++); On 2012/12/15 06:45:04, greggman wrote: > I still believe this needs to be > > it = pending_async_transfers_.erase(it); > > so it will work regardless of container I realized I could use front()/pop_front() in this case since it now never goes past the first element. https://codereview.chromium.org/11428140/diff/57005/gpu/command_buffer/servic... gpu/command_buffer/service/texture_manager.cc:1256: pending_async_transfers_.erase(it++); On 2012/12/15 06:45:04, greggman wrote: > I still believe this needs to be > > it = pending_async_transfers_.erase(it); Ditto
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/11428140/63001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/11428140/65028
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/11428140/65028
Message was sent while issue was closed.
Change committed as 173430 |
