Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12892005:
Implement client side PBOs for glReadPixel (Closed)
|
Created:
7 years, 9 months ago by hubbe Modified:
7 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, David Trainor- moved to gerrit, aelias_OOO_until_Jul13 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement client side PBOs for glReadPixel
Use PBOs in gl_helper
swizzle bytes on gpu
flip vertically on gpu
remove gl_helper_thread
Implements GLHelper::CropScaleReadbackAndCleanTexture() to be non-blocking in the client by using shared memory combined with a query (not requiring actual GL PBO support, which would not be available on Android).
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191737
Patch Set 1 #Patch Set 2 : unsigned fix #Patch Set 3 : android fix #Patch Set 4 : another unsigned int fix #Patch Set 5 : update glReadPixels test #Patch Set 6 : made test better #Patch Set 7 : pbo unit test added #
Total comments: 34
Patch Set 8 : lots of cleanup, made sure callbacks come back in right order #
Total comments: 9
Patch Set 9 : restore bindings #
Total comments: 1
Patch Set 10 : don't restore bindings #
Total comments: 4
Patch Set 11 : updated tests #
Total comments: 8
Patch Set 12 : git cl try #
Total comments: 4
Patch Set 13 : more comments, GL_STREAM_READ added #
Total comments: 33
Patch Set 14 : feedback incorporated #Patch Set 15 : use callbacks #
Total comments: 1
Patch Set 16 : make GLHelper constructor explicit #Messages
Total messages: 43 (0 generated)
I've added Gregg because he's the better reviewer for this. In general it looks good. A few comments. https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:383: " gl_FragColor = swizzle * texture2D(s_texture, v_texcoord);" Will this work? Seems a bit simpler. gl_FragColor = texture2D(s_texture, v_texcoord).bgra; https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:560: context_->endQueryEXT(GL_COMMANDS_ISSUED_CHROMIUM); You might need to call flush here to guarantee that the query eventually ends. Gregg will know is endQueryEXT does an implicit flush. https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:561: context_->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0); What if a different buffer was bound to this target previously? https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:639: fprintf(stderr,"mapBuffer Returns NULL\n"); Use LOG / VLOG instead of fprintf. https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:641: context->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0); Same as above. There might have been a different buffer bound in the target. https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:644: fprintf(stderr,"Buffer is negative!\n"); Use LOG / VLOG instead of fprintf. https://codereview.chromium.org/12892005/diff/14001/gpu/GLES2/gl2extchromium.h File gpu/GLES2/gl2extchromium.h (right): https://codereview.chromium.org/12892005/diff/14001/gpu/GLES2/gl2extchromium.... gpu/GLES2/gl2extchromium.h:74: #define GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM 0x78EC The range for dev work is 0x6000 to 0x7FFF so this falls inside it now. Thanks. https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:2117: return; Should this not generate INVALID_OPERATION? https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation.h (right): https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.h:83: xo #define GPU_CLIENT_VALIDATE_DESTINATION_INITALIZATION_ASSERT(v) ASSERT(v) Don't think this is intended for this CL.
https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:560: context_->endQueryEXT(GL_COMMANDS_ISSUED_CHROMIUM); On 2013/03/19 18:34:48, apatrick_chromium wrote: > You might need to call flush here to guarantee that the query eventually ends. > Gregg will know is endQueryEXT does an implicit flush. yes, you need to call flush. endQueryEXT may or may not do a flush but it's not guaranteed. https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:561: context_->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0); You can't use UNPACK_TRANSFER_BUFFER for READing. Only for WRITEing. I know it happens to work but the semantic for GL is UNPACK for for providing data to GL, PACK is for getting data from GL. https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:2107: if (bound_pixel_unpack_transfer_buffer_id_) { As pointed out else where, "unpack" is for providing data to GL, "pack" is for reading data. https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3340: if (buffer->transfer_ready_token()) { It seems like if you're going to add this check you should add it for all operations on a buffer, not just readPixels. In other words, TexImage2D when using a PBO should set a token and block in map if it's busy. https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation_unittest.cc (right): https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation_unittest.cc:1423: TEST_F(GLES2ImplementationTest, ReadPixelsWithPBO) { Tests for failure would be good? *) Test that a buffer that can't fit the result fails? *) Test that a buffer than can fit the result but the result won't fit at the given offset fails? *) Test that mapping a buffer that's in use waits for a token?
https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:383: " gl_FragColor = swizzle * texture2D(s_texture, v_texcoord);" On 2013/03/19 18:34:48, apatrick_chromium wrote: > Will this work? Seems a bit simpler. > > gl_FragColor = texture2D(s_texture, v_texcoord).bgra; Done. https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:560: context_->endQueryEXT(GL_COMMANDS_ISSUED_CHROMIUM); On 2013/03/19 20:54:11, greggman wrote: > On 2013/03/19 18:34:48, apatrick_chromium wrote: > > You might need to call flush here to guarantee that the query eventually ends. > > Gregg will know is endQueryEXT does an implicit flush. > > yes, you need to call flush. endQueryEXT may or may not do a flush but it's not > guaranteed. Does the flush need to happen before endQueryExt? If not, there is a ScopedFlush higher up which should work, no? https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:561: context_->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0); On 2013/03/19 18:34:48, apatrick_chromium wrote: > What if a different buffer was bound to this target previously? Good question. Should I document what buffers are used, or should I attempt to set it back? (If set it back, what is the proper way to get the buffer given a binding?) https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:561: context_->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0); On 2013/03/19 20:54:11, greggman wrote: > You can't use UNPACK_TRANSFER_BUFFER for READing. Only for WRITEing. I know it > happens to work but the semantic for GL is UNPACK for for providing data to GL, > PACK is for getting data from GL. I guess I have to implement that extension then, right? Is there a good resource for this kind of knowledge? Learning openGL piece by piece is not exactly fun. https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:639: fprintf(stderr,"mapBuffer Returns NULL\n"); On 2013/03/19 18:34:48, apatrick_chromium wrote: > Use LOG / VLOG instead of fprintf. Old useless debugging junk. Gone. https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:641: context->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0); On 2013/03/19 18:34:48, apatrick_chromium wrote: > Same as above. There might have been a different buffer bound in the target. This is in an async callback. Is it really reasonable for someone to expect a buffer binding to persist between callbacks? https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:644: fprintf(stderr,"Buffer is negative!\n"); On 2013/03/19 18:34:48, apatrick_chromium wrote: > Use LOG / VLOG instead of fprintf. Gone. https://codereview.chromium.org/12892005/diff/14001/gpu/GLES2/gl2extchromium.h File gpu/GLES2/gl2extchromium.h (right): https://codereview.chromium.org/12892005/diff/14001/gpu/GLES2/gl2extchromium.... gpu/GLES2/gl2extchromium.h:74: #define GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM 0x78EC On 2013/03/19 18:34:48, apatrick_chromium wrote: > The range for dev work is 0x6000 to 0x7FFF so this falls inside it now. Thanks. Done. https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:2107: if (bound_pixel_unpack_transfer_buffer_id_) { On 2013/03/19 20:54:11, greggman wrote: > As pointed out else where, "unpack" is for providing data to GL, "pack" is for > reading data. Done. https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:2117: return; On 2013/03/19 18:34:48, apatrick_chromium wrote: > Should this not generate INVALID_OPERATION? Done. (And in other similar places) https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3340: if (buffer->transfer_ready_token()) { On 2013/03/19 20:54:11, greggman wrote: > It seems like if you're going to add this check you should add it for all > operations on a buffer, not just readPixels. In other words, TexImage2D when > using a PBO should set a token and block in map if it's busy. I can do it either way. I think I prefer implementing the blocking semantics as certain kinds of patters get a lot easier this way. https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation.h (right): https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.h:83: xo #define GPU_CLIENT_VALIDATE_DESTINATION_INITALIZATION_ASSERT(v) ASSERT(v) On 2013/03/19 18:34:48, apatrick_chromium wrote: > Don't think this is intended for this CL. Done. https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation_unittest.cc (right): https://codereview.chromium.org/12892005/diff/14001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation_unittest.cc:1423: TEST_F(GLES2ImplementationTest, ReadPixelsWithPBO) { On 2013/03/19 20:54:11, greggman wrote: > Tests for failure would be good? > > *) Test that a buffer that can't fit the result fails? > *) Test that a buffer than can fit the result but the result > won't fit at the given offset fails? > *) Test that mapping a buffer that's in use waits for a > token? Is there a good test that I can use as a template for these kinds of tests?
https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:560: context_->endQueryEXT(GL_COMMANDS_ISSUED_CHROMIUM); On 2013/03/19 22:42:42, hubbe wrote: > On 2013/03/19 20:54:11, greggman wrote: > > On 2013/03/19 18:34:48, apatrick_chromium wrote: > > > You might need to call flush here to guarantee that the query eventually > ends. > > > Gregg will know is endQueryEXT does an implicit flush. > > > > yes, you need to call flush. endQueryEXT may or may not do a flush but it's > not > > guaranteed. > > Does the flush need to happen before endQueryExt? > If not, there is a ScopedFlush higher up which should work, no? I missed the ScopedFlush. It will work. https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:561: context_->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0); On 2013/03/19 22:42:42, hubbe wrote: > On 2013/03/19 18:34:48, apatrick_chromium wrote: > > What if a different buffer was bound to this target previously? > > Good question. > Should I document what buffers are used, or should I attempt to set it back? (If > set it back, what is the proper way to get the buffer given a binding?) I think the issue is, before this code was using context_for_thread_ as the context for the read-back. So it was free to modify the context state without affecting the state of context_. Now it is changing the state of context_. This might be okay but I don't know what state it is safe to change on it. BTW, I noticed that there might be a similar issue with the framebuffer and texture bindings since ScopedFramebufferBinder etc always restore ID zero, which might not have been what was previously bound on context_. Generally, unless these bindings are cached somewhere on the client side, there is not going to be an efficient way to find out what they were before because you would need to do a synchronous round-trip to the GPU process to get that information. Perhaps what you could do is retain the second GL context (but rename the variable because you don't need the helper thread) so that you can safely modify its state. Specifically, you would be able to reset all the bindings to zero. I seem to remember this code had exactly this issue before and that was why the second context was introduced. https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:641: context->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0); On 2013/03/19 22:42:42, hubbe wrote: > On 2013/03/19 18:34:48, apatrick_chromium wrote: > > Same as above. There might have been a different buffer bound in the target. > > This is in an async callback. Is it really reasonable for someone to expect a > buffer binding to persist between callbacks? The assumption might have been made somewhere. Need to find out.
https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:561: context_->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0); On 2013/03/19 23:31:20, apatrick_chromium wrote: > On 2013/03/19 22:42:42, hubbe wrote: > > On 2013/03/19 18:34:48, apatrick_chromium wrote: > > > What if a different buffer was bound to this target previously? > > > > Good question. > > Should I document what buffers are used, or should I attempt to set it back? > (If > > set it back, what is the proper way to get the buffer given a binding?) > > I think the issue is, before this code was using context_for_thread_ as the > context for the read-back. So it was free to modify the context state without > affecting the state of context_. Now it is changing the state of context_. This > might be okay but I don't know what state it is safe to change on it. > > BTW, I noticed that there might be a similar issue with the framebuffer and > texture bindings since ScopedFramebufferBinder etc always restore ID zero, which > might not have been what was previously bound on context_. > > Generally, unless these bindings are cached somewhere on the client side, there > is not going to be an efficient way to find out what they were before because > you would need to do a synchronous round-trip to the GPU process to get that > information. > > Perhaps what you could do is retain the second GL context (but rename the > variable because you don't need the helper thread) so that you can safely modify > its state. Specifically, you would be able to reset all the bindings to zero. > > I seem to remember this code had exactly this issue before and that was why the > second context was introduced. This things are cached in the client side, so restoring them would not be slow. Is this a general concern, or are we simply trying to make sure that callers to this particular function are not caught unaware? (Currently, there is exactly one call to this function in the Chrome code base.) https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:641: context->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0); On 2013/03/19 23:31:20, apatrick_chromium wrote: > On 2013/03/19 22:42:42, hubbe wrote: > > On 2013/03/19 18:34:48, apatrick_chromium wrote: > > > Same as above. There might have been a different buffer bound in the target. > > > > This is in an async callback. Is it really reasonable for someone to expect a > > buffer binding to persist between callbacks? > > The assumption might have been made somewhere. Need to find out. While it is possible, the browser still works when using this code, so at least that assumption has not been made anywhere in the critical path on linux-aura.
https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:561: context_->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0); Yes it looks like getInteger(GL_PIXEL_PACK_TRANSFER_BUFFER_BINDING_CHROMIUM, ...) will not round-trip to the GPU process so I think you can just call it prior to the first bindBuffer and then restore the previous buffer rather than buffer zero. Likewise, now that this code is no longer using the special helper context as it was before, modify ScopedFramebufferBinder and ScopedTextureBinder to work similarly with GL_DRAW_FRAMEBUFFER_BINDING, GL_READ_FRAMEBUFFER_BINDING and GL_TEXTURE_BINDING as the argument to getInteger. https://codereview.chromium.org/12892005/diff/14001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:641: context->bindBuffer(GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM, 0); On 2013/03/19 23:39:27, hubbe wrote: > On 2013/03/19 23:31:20, apatrick_chromium wrote: > > On 2013/03/19 22:42:42, hubbe wrote: > > > On 2013/03/19 18:34:48, apatrick_chromium wrote: > > > > Same as above. There might have been a different buffer bound in the > target. > > > > > > This is in an async callback. Is it really reasonable for someone to expect > a > > > buffer binding to persist between callbacks? > > > > The assumption might have been made somewhere. Need to find out. > > While it is possible, the browser still works when using this code, so at least > that assumption has not been made anywhere in the critical path on linux-aura. You can do the same thing to get the buffer binding here and there should be no issues.
All buffer bindings should now be restored I think.
I can't see an easy way to make the unit tests useful. How about writing a few gl_tests instead. See the stuff in gpu/command_buffer/tests https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1423: if (buffer->mapped()) { You removed the check for buffer being NULL but you're dereferencing it here? https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1506: buffer->set_transfer_ready_token(helper_->InsertToken()); Add a CheckGLError here (was missing from before) https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3465: // If there's no data/buffer just issue the AsyncTexImage2D Probably need at least a TODO that async uploads need to block on MapBuffer if they are still pending for the same buffer. https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation_unittest.cc (right): https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation_unittest.cc:1459: gl_->DeleteBuffers(1, &b); Ideally you want to check that gl_->ReadPixels inserts the commands you expect it to insert if there's a PBO. so, you need EXPECT_EQ(0, memcmp(&expected, commands_, sizeof(expected))) You don't need the unmap, bind, delete (lines 1457-1459) Up above you don't need ExpectedMemoryInfo mem1 or result1. Those are figuring out what memory would be allocated by ReadPixels. Instead you'd expect BufferData to allocate shared memory. sigh....I don't see an easy way to figure out what that will be though. If we can't figure out an easy way to do that you can write integration tests in gl_tests. See src/gpu/command_buffer/tests
#(*@#$ One of the glGetIntegerv() calls is going to the GPU process, which slows the whole thing down a lot. Is restoring the buffer bindings really needed? I don't see that behavior elsewhere in the code On Tue, Mar 19, 2013 at 5:42 PM, <gman@chromium.org> wrote: > I can't see an easy way to make the unit tests useful. How about writing a > few > gl_tests instead. See the stuff in gpu/command_buffer/tests > > > > > https://codereview.chromium.**org/12892005/diff/24001/gpu/** > command_buffer/client/gles2_**implementation.cc<https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client/gles2_implementation.cc> > File gpu/command_buffer/client/**gles2_implementation.cc (right): > > https://codereview.chromium.**org/12892005/diff/24001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode1423<https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client/gles2_implementation.cc#newcode1423> > gpu/command_buffer/client/**gles2_implementation.cc:1423: if > (buffer->mapped()) { > You removed the check for buffer being NULL but you're dereferencing it > here? > > https://codereview.chromium.**org/12892005/diff/24001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode1506<https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client/gles2_implementation.cc#newcode1506> > gpu/command_buffer/client/**gles2_implementation.cc:1506: > buffer->set_transfer_ready_**token(helper_->InsertToken()); > Add a CheckGLError here (was missing from before) > > https://codereview.chromium.**org/12892005/diff/24001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode3465<https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client/gles2_implementation.cc#newcode3465> > gpu/command_buffer/client/**gles2_implementation.cc:3465: // If there's no > data/buffer just issue the AsyncTexImage2D > Probably need at least a TODO that async uploads need to block on > MapBuffer if they are still pending for the same buffer. > > https://codereview.chromium.**org/12892005/diff/24001/gpu/** > command_buffer/client/gles2_**implementation_unittest.cc<https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client/gles2_implementation_unittest.cc> > File gpu/command_buffer/client/**gles2_implementation_unittest.**cc > (right): > > https://codereview.chromium.**org/12892005/diff/24001/gpu/** > command_buffer/client/gles2_**implementation_unittest.cc#**newcode1459<https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client/gles2_implementation_unittest.cc#newcode1459> > gpu/command_buffer/client/**gles2_implementation_unittest.**cc:1459: > gl_->DeleteBuffers(1, &b); > Ideally you want to check that gl_->ReadPixels inserts the commands you > expect it to insert if there's a PBO. > > so, you need > > EXPECT_EQ(0, memcmp(&expected, commands_, sizeof(expected))) > > You don't need the unmap, bind, delete (lines 1457-1459) > > Up above you don't need ExpectedMemoryInfo mem1 or result1. Those are > figuring out what memory would be allocated by ReadPixels. Instead you'd > expect BufferData to allocate shared memory. > > sigh....I don't see an easy way to figure out what that will be though. > > If we can't figure out an easy way to do that you can write integration > tests in gl_tests. See src/gpu/command_buffer/tests > > https://codereview.chromium.**org/12892005/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/12892005/diff/32001/content/common/gpu/client... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/12892005/diff/32001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:124: context_->getIntegerv(target_binding, &prev_id_); I know al said it was okay to call getInteger but I disagree. Even if it was cached because this is supposed to be GL semantics and they make no guarantee You'd have to ask someone on the compositor team, maybe jamesr or enne if it's okay not to restore this stuff. If it needs to be restored then some class in the compositor should be storing what it needs to be.
I've changed it back to bind zero when done. I think this is safe, and here
is why:
These three bindings:
GL_FRAMEBUFFER
GL_TEXTURE_2D
GL_ARRAY_BUFFER
Were already called in ScaleTexture, which was called on the main thread
already.
This binding is new, and cannot possibly be used anywhere else yet:
GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM
As for the callback, ScaleTexture is already called from a callback. Ergo,
this change should not substantially change how bindings are cleared.
/Hubbe
On Tue, Mar 19, 2013 at 5:59 PM, <gman@chromium.org> wrote:
>
> https://codereview.chromium.**org/12892005/diff/32001/**
>
content/common/gpu/client/gl_**helper.cc<https://codereview.chromium.org/12892005/diff/32001/content/common/gpu/client/gl_helper.cc>
> File content/common/gpu/client/gl_**helper.cc (right):
>
> https://codereview.chromium.**org/12892005/diff/32001/**
>
content/common/gpu/client/gl_**helper.cc#newcode124<https://codereview.chromium.org/12892005/diff/32001/content/common/gpu/client/gl_helper.cc#newcode124>
> content/common/gpu/client/gl_**helper.cc:124:
> context_->getIntegerv(target_**binding, &prev_id_);
> I know al said it was okay to call getInteger but I disagree. Even if it
> was cached because this is supposed to be GL semantics and they make no
> guarantee
>
> You'd have to ask someone on the compositor team, maybe jamesr or enne
> if it's okay not to restore this stuff. If it needs to be restored then
> some class in the compositor should be storing what it needs to be.
>
>
https://codereview.chromium.**org/12892005/<https://codereview.chromium.org/1...
>
Thanks for checking. I'm comfortable with changing the bindings now. A few comments inline. Also, I agree with Gregg that you might be better off writing some gl_tests for this. They're easier to write. On 2013/03/20 02:20:52, hubbe1 wrote: > I've changed it back to bind zero when done. I think this is safe, and here > is why: > > These three bindings: > GL_FRAMEBUFFER > GL_TEXTURE_2D > GL_ARRAY_BUFFER > > Were already called in ScaleTexture, which was called on the main thread > already. > > This binding is new, and cannot possibly be used anywhere else yet: > GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM > > As for the callback, ScaleTexture is already called from a callback. Ergo, > this change should not substantially change how bindings are cleared. > > /Hubbe > > On Tue, Mar 19, 2013 at 5:59 PM, <mailto:gman@chromium.org> wrote: > > > > > https://codereview.chromium.**org/12892005/diff/32001/** > > > content/common/gpu/client/gl_**helper.cc<https://codereview.chromium.org/12892005/diff/32001/content/common/gpu/client/gl_helper.cc> > > File content/common/gpu/client/gl_**helper.cc (right): > > > > https://codereview.chromium.**org/12892005/diff/32001/** > > > content/common/gpu/client/gl_**helper.cc#newcode124<https://codereview.chromium.org/12892005/diff/32001/content/common/gpu/client/gl_helper.cc#newcode124> > > content/common/gpu/client/gl_**helper.cc:124: > > context_->getIntegerv(target_**binding, &prev_id_); > > I know al said it was okay to call getInteger but I disagree. Even if it > > was cached because this is supposed to be GL semantics and they make no > > guarantee > > > > You'd have to ask someone on the compositor team, maybe jamesr or enne > > if it's okay not to restore this stuff. If it needs to be restored then > > some class in the compositor should be storing what it needs to be. > > > > > https://codereview.chromium.**org/12892005/%3Chttps://codereview.chromium.org...> > >
https://codereview.chromium.org/12892005/diff/39001/content/common/gpu/client... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/12892005/diff/39001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:16: #include "base/synchronization/waitable_event.h" I think some of these includes are not needed now, like base/threading/thread.h. https://codereview.chromium.org/12892005/diff/39001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:616: context_->flush(); You could just flush once at the end of this function. Or maybe put a ScopedFlush at the top to defend against this function earlying out in the future.
When you guys say I should write a "gl_test", you mean adding a file to
gpu/command_buffer/tests/ ?
How are those tests different? It kind of looks like they talk to the GL
driver directly, but that's not really true, is it?
Is there a document/comment that describes what should go in "gl_test" and
what should go in "gpu_unittests"?
/Hubbe
On Wed, Mar 20, 2013 at 11:39 AM, <apatrick@chromium.org> wrote:
>
> https://codereview.chromium.**org/12892005/diff/39001/**
>
content/common/gpu/client/gl_**helper.cc<https://codereview.chromium.org/12892005/diff/39001/content/common/gpu/client/gl_helper.cc>
> File content/common/gpu/client/gl_**helper.cc (right):
>
> https://codereview.chromium.**org/12892005/diff/39001/**
>
content/common/gpu/client/gl_**helper.cc#newcode16<https://codereview.chromium.org/12892005/diff/39001/content/common/gpu/client/gl_helper.cc#newcode16>
> content/common/gpu/client/gl_**helper.cc:16: #include
> "base/synchronization/**waitable_event.h"
> I think some of these includes are not needed now, like
> base/threading/thread.h.
>
> https://codereview.chromium.**org/12892005/diff/39001/**
>
content/common/gpu/client/gl_**helper.cc#newcode616<https://codereview.chromium.org/12892005/diff/39001/content/common/gpu/client/gl_helper.cc#newcode616>
> content/common/gpu/client/gl_**helper.cc:616: context_->flush();
> You could just flush once at the end of this function. Or maybe put a
> ScopedFlush at the top to defend against this function earlying out in
> the future.
>
>
https://codereview.chromium.**org/12892005/<https://codereview.chromium.org/1...
>
Implemented a gl_test for this. CL Should hopefully be in pretty reasonable shape now. Working to make sure all trybots pass. https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1423: if (buffer->mapped()) { On 2013/03/20 00:42:17, greggman wrote: > You removed the check for buffer being NULL but you're dereferencing it here? Done. https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1506: buffer->set_transfer_ready_token(helper_->InsertToken()); On 2013/03/20 00:42:17, greggman wrote: > Add a CheckGLError here (was missing from before) Done. https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3465: // If there's no data/buffer just issue the AsyncTexImage2D On 2013/03/20 00:42:17, greggman wrote: > Probably need at least a TODO that async uploads need to block on MapBuffer if > they are still pending for the same buffer. What buffer is that? https://codereview.chromium.org/12892005/diff/39001/content/common/gpu/client... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/12892005/diff/39001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:16: #include "base/synchronization/waitable_event.h" On 2013/03/20 18:39:09, apatrick_chromium wrote: > I think some of these includes are not needed now, like base/threading/thread.h. Done (Is there something like iwyu for chrome sources?) https://codereview.chromium.org/12892005/diff/39001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:616: context_->flush(); On 2013/03/20 18:39:09, apatrick_chromium wrote: > You could just flush once at the end of this function. Or maybe put a > ScopedFlush at the top to defend against this function earlying out in the > future. Done.
Ping?
Thanks for the tests. A few comments. https://codereview.chromium.org/12892005/diff/47001/gpu/command_buffer/tests/... File gpu/command_buffer/tests/gl_readback_unittests.cc (right): https://codereview.chromium.org/12892005/diff/47001/gpu/command_buffer/tests/... gpu/command_buffer/tests/gl_readback_unittests.cc:30: TEST_F(GLReadbackTest, ReadPixelsWithPBO) { Can you clear the FBO to a particular color and then check that the color that is read back is the same? glClearColor and then glClear. https://codereview.chromium.org/12892005/diff/47001/gpu/command_buffer/tests/... gpu/command_buffer/tests/gl_readback_unittests.cc:44: glFlush(); You shouldn't need this glFlush() because glMapBufferCHROMIUM should do it. https://codereview.chromium.org/12892005/diff/47001/gpu/command_buffer/tests/... gpu/command_buffer/tests/gl_readback_unittests.cc:61: glGenQueriesEXT(1, &q); The query object leaks. https://codereview.chromium.org/12892005/diff/47001/gpu/command_buffer/tests/... gpu/command_buffer/tests/gl_readback_unittests.cc:72: while (!done) { I think you need a glFlush before this loop.
Sorry about the "git cl try" patch name (typeahead error) https://codereview.chromium.org/12892005/diff/47001/gpu/command_buffer/tests/... File gpu/command_buffer/tests/gl_readback_unittests.cc (right): https://codereview.chromium.org/12892005/diff/47001/gpu/command_buffer/tests/... gpu/command_buffer/tests/gl_readback_unittests.cc:30: TEST_F(GLReadbackTest, ReadPixelsWithPBO) { On 2013/03/21 22:51:27, apatrick_chromium wrote: > Can you clear the FBO to a particular color and then check that the color that > is read back is the same? glClearColor and then glClear. Done. https://codereview.chromium.org/12892005/diff/47001/gpu/command_buffer/tests/... gpu/command_buffer/tests/gl_readback_unittests.cc:44: glFlush(); On 2013/03/21 22:51:27, apatrick_chromium wrote: > You shouldn't need this glFlush() because glMapBufferCHROMIUM should do it. Done. https://codereview.chromium.org/12892005/diff/47001/gpu/command_buffer/tests/... gpu/command_buffer/tests/gl_readback_unittests.cc:61: glGenQueriesEXT(1, &q); On 2013/03/21 22:51:27, apatrick_chromium wrote: > The query object leaks. Done. https://codereview.chromium.org/12892005/diff/47001/gpu/command_buffer/tests/... gpu/command_buffer/tests/gl_readback_unittests.cc:72: while (!done) { On 2013/03/21 22:51:27, apatrick_chromium wrote: > I think you need a glFlush before this loop. Done.
LGTM for my part. Over to gman.
just a few nit then LGTM https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3465: // If there's no data/buffer just issue the AsyncTexImage2D On 2013/03/20 22:03:47, hubbe wrote: > On 2013/03/20 00:42:17, greggman wrote: > > Probably need at least a TODO that async uploads need to block on MapBuffer > if > > they are still pending for the same buffer. > > What buffer is that? The buffer referenced by bound_pixel_unpack_transfer_buffer_id_. If someone calls AsyncTexImage2DCHROMIUM and then calls MapBufferCHROMIUM there's no token inserted and it won't block so MapBufferCHROMIUM will not wait like you've made it do for other use-cases. You can't use a token in this case either so in order to make MapBufferCHROMIUM block until the async upload is finished someone needs to add code. That's probably a lot of work so it would be good to add a TODO somewhere, possibly in 3 places (1) here, (2) in AsyncTexSubImage2D and (3) in MapBufferCHROMIUM that waiting for a token is not sufficient to make sure the buffer is not in use. https://codereview.chromium.org/12892005/diff/58001/gpu/command_buffer/tests/... File gpu/command_buffer/tests/gl_readback_unittests.cc (right): https://codereview.chromium.org/12892005/diff/58001/gpu/command_buffer/tests/... gpu/command_buffer/tests/gl_readback_unittests.cc:43: 0 /* should be GL_STREAM_READ, but that is not defined in Maybe add GL_STREAM_READ to gpu/GLES2/gl2extchromium.h https://codereview.chromium.org/12892005/diff/58001/gpu/gpu.gyp File gpu/gpu.gyp (right): https://codereview.chromium.org/12892005/diff/58001/gpu/gpu.gyp#newcode266 gpu/gpu.gyp:266: 'command_buffer/tests/gl_readback_unittests.cc', nit: these go in alphabetical order.
https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3465: // If there's no data/buffer just issue the AsyncTexImage2D On 2013/03/21 23:18:13, greggman wrote: > On 2013/03/20 22:03:47, hubbe wrote: > > On 2013/03/20 00:42:17, greggman wrote: > > > Probably need at least a TODO that async uploads need to block on MapBuffer > > if > > > they are still pending for the same buffer. > > > > What buffer is that? > > The buffer referenced by bound_pixel_unpack_transfer_buffer_id_. > If someone calls AsyncTexImage2DCHROMIUM and then calls MapBufferCHROMIUM > there's no token inserted and it won't block so MapBufferCHROMIUM will not wait > like you've made it do for other use-cases. You can't use a token in this case > either so in order to make MapBufferCHROMIUM block until the async upload is > finished someone needs to add code. That's probably a lot of work so it would be > good to add a TODO somewhere, possibly in 3 places (1) here, (2) in > AsyncTexSubImage2D and (3) in MapBufferCHROMIUM that waiting for a token is not > sufficient to make sure the buffer is not in use. Done. https://codereview.chromium.org/12892005/diff/58001/gpu/command_buffer/tests/... File gpu/command_buffer/tests/gl_readback_unittests.cc (right): https://codereview.chromium.org/12892005/diff/58001/gpu/command_buffer/tests/... gpu/command_buffer/tests/gl_readback_unittests.cc:43: 0 /* should be GL_STREAM_READ, but that is not defined in On 2013/03/21 23:18:13, greggman wrote: > Maybe add GL_STREAM_READ to gpu/GLES2/gl2extchromium.h Done. https://codereview.chromium.org/12892005/diff/58001/gpu/gpu.gyp File gpu/gpu.gyp (right): https://codereview.chromium.org/12892005/diff/58001/gpu/gpu.gyp#newcode266 gpu/gpu.gyp:266: 'command_buffer/tests/gl_readback_unittests.cc', On 2013/03/21 23:18:13, greggman wrote: > nit: these go in alphabetical order. Done.
Does anybody have any more comments on this CL, or can I check it in?
/Hubbe
On 2013/03/21 23:36:51, hubbe wrote:
>
https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client...
> File gpu/command_buffer/client/gles2_implementation.cc (right):
>
>
https://codereview.chromium.org/12892005/diff/24001/gpu/command_buffer/client...
> gpu/command_buffer/client/gles2_implementation.cc:3465: // If there's no
> data/buffer just issue the AsyncTexImage2D
> On 2013/03/21 23:18:13, greggman wrote:
> > On 2013/03/20 22:03:47, hubbe wrote:
> > > On 2013/03/20 00:42:17, greggman wrote:
> > > > Probably need at least a TODO that async uploads need to block on
> MapBuffer
> > > if
> > > > they are still pending for the same buffer.
> > >
> > > What buffer is that?
> >
> > The buffer referenced by bound_pixel_unpack_transfer_buffer_id_.
> > If someone calls AsyncTexImage2DCHROMIUM and then calls MapBufferCHROMIUM
> > there's no token inserted and it won't block so MapBufferCHROMIUM will not
> wait
> > like you've made it do for other use-cases. You can't use a token in this
case
> > either so in order to make MapBufferCHROMIUM block until the async upload is
> > finished someone needs to add code. That's probably a lot of work so it
would
> be
> > good to add a TODO somewhere, possibly in 3 places (1) here, (2) in
> > AsyncTexSubImage2D and (3) in MapBufferCHROMIUM that waiting for a token is
> not
> > sufficient to make sure the buffer is not in use.
>
> Done.
>
>
https://codereview.chromium.org/12892005/diff/58001/gpu/command_buffer/tests/...
> File gpu/command_buffer/tests/gl_readback_unittests.cc (right):
>
>
https://codereview.chromium.org/12892005/diff/58001/gpu/command_buffer/tests/...
> gpu/command_buffer/tests/gl_readback_unittests.cc:43: 0 /* should be
> GL_STREAM_READ, but that is not defined in
> On 2013/03/21 23:18:13, greggman wrote:
> > Maybe add GL_STREAM_READ to gpu/GLES2/gl2extchromium.h
>
> Done.
>
> https://codereview.chromium.org/12892005/diff/58001/gpu/gpu.gyp
> File gpu/gpu.gyp (right):
>
> https://codereview.chromium.org/12892005/diff/58001/gpu/gpu.gyp#newcode266
> gpu/gpu.gyp:266: 'command_buffer/tests/gl_readback_unittests.cc',
> On 2013/03/21 23:18:13, greggman wrote:
> > nit: these go in alphabetical order.
>
> Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hubbe@chromium.org/12892005/66001
Presubmit check for 12892005-66001 failed and returned exit status 1.
INFO:root:Found 13 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
content/browser/renderer_host/image_transport_factory_android.cc
content/browser/renderer_host/image_transport_factory.cc
Presubmit checks took 1.7s to calculate.
Adding Daniel Sievers, OWNER for content/browser/render_host/image_transport_factory*
On 2013/03/28 06:11:00, hubbe wrote: > Adding Daniel Sievers, OWNER for > content/browser/render_host/image_transport_factory* From glancing over it this looks like it implements GLHelper::CropScaleReadbackAndCleanTexture() to be non-blocking in the client by using shared memory combined with a query (not requiring actual GL PBO support, which would not be available on Android). So looks like cool stuff! (nit: maybe update the comment/cl description?) lgtm content/browser/renderer_host/image_transport_factory_android.cc We probably want to update our code to take advantage of this (currently we use ReadbackTextureSync(), +dtrainor). +piman for the aura image_transport_factory
Your summary is absolutely accurate. Added it to the CL description. On 2013/03/28 18:37:10, Daniel Sievers wrote: > On 2013/03/28 06:11:00, hubbe wrote: > > Adding Daniel Sievers, OWNER for > > content/browser/render_host/image_transport_factory* > > From glancing over it this looks like it implements > GLHelper::CropScaleReadbackAndCleanTexture() to be non-blocking in the client by > using shared memory combined with a query (not requiring actual GL PBO support, > which would not be available on Android). So looks like cool stuff! (nit: maybe > update the comment/cl description?) > > lgtm content/browser/renderer_host/image_transport_factory_android.cc > We probably want to update our code to take advantage of this (currently we use > ReadbackTextureSync(), +dtrainor). > > +piman for the aura image_transport_factory
https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:474: #ifdef USE_SKIA ? What does USE_SKIA mean? All platforms use skia... I also don't see where it's even defined anywhere. https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:556: base::TimeDelta::FromMilliseconds(2)); Rather than a poll (with a completely arbitrary 2ms), we have a mechanism to get an asynchronous completion callback, with sync points. I realize it's only exposed on CommandBufferProxyImpl, not directly on WebGraphicsContext3D, but it seems that it's just a matter of exposing this, and we'd have a much more efficient signaling mechanism. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1257: if (!buffer_id) return; nit: return on its own line https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1334: if (!buffer_id) return; nit: put return on its own line https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1406: // Unknown target Set error here, rather than the callsites. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1410: SetGLError(GL_INVALID_VALUE, function_name, "no buffer bound"); GL_INVALID_OPERATION (according to spec). Should you return false, saving some extra tests on the callsites? https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1469: SetGLError(GL_INVALID_VALUE, "glCompressedTexImage2D", "invalid buffer"); GetBoundPixelUnpackTransferBufferIfValid already set the correct error https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1513: SetGLError(GL_INVALID_VALUE, GetBoundPixelUnpackTransferBufferIfValid already set the right error https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1603: SetGLError(GL_INVALID_VALUE, "glTexImage2D", "invalid buffer"); GetBoundPixelUnpackTransferBufferIfValid already set the right error https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1711: SetGLError(GL_INVALID_VALUE, "glTexSubImage2D", "invalid buffer"); GetBoundPixelUnpackTransferBufferIfValid already set the right error https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:2168: SetGLError(GL_INVALID_VALUE, "glReadPixels", "invalid readback buffer"); GetBoundPixelUnpackTransferBufferIfValid already set the right error https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:2174: SetGLError(GL_INVALID_VALUE, "glReadPixels", "pixels = NULL"); note: spec doesn't say that an error would be generated. It's almost expected that it would crash on this user error (hint: passing any non-zero offset cast into a pointer would be equally bad) If anything, GL_INVALID_OPERATION would be more appropriate (since it's a matter of state combination, the value itself being valid in other contexts). https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3397: if (!buffer_id) return NULL; nit: return on its own line https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3432: if (!buffer_id) return false; nit: return on its own line https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3484: // the buffer before the transfer is finished. (Currently sunch typo: sunch https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3530: // the buffer before the transfer is finished. (Currently sunch typo: sunch
(note: I'm really excited about this change, it's really cool!) On Thu, Mar 28, 2013 at 1:22 PM, <piman@chromium.org> wrote: > > https://codereview.chromium.**org/12892005/diff/66001/** > content/common/gpu/client/gl_**helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc> > File content/common/gpu/client/gl_**helper.cc (right): > > https://codereview.chromium.**org/12892005/diff/66001/** > content/common/gpu/client/gl_**helper.cc#newcode474<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode474> > content/common/gpu/client/gl_**helper.cc:474: #ifdef USE_SKIA > ? > What does USE_SKIA mean? All platforms use skia... > I also don't see where it's even defined anywhere. > > https://codereview.chromium.**org/12892005/diff/66001/** > content/common/gpu/client/gl_**helper.cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556> > content/common/gpu/client/gl_**helper.cc:556: > base::TimeDelta::**FromMilliseconds(2)); > Rather than a poll (with a completely arbitrary 2ms), we have a > mechanism to get an asynchronous completion callback, with sync points. > I realize it's only exposed on CommandBufferProxyImpl, not directly on > WebGraphicsContext3D, but it seems that it's just a matter of exposing > this, and we'd have a much more efficient signaling mechanism. > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc> > File gpu/command_buffer/client/**gles2_implementation.cc (right): > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode1257<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode1257> > gpu/command_buffer/client/**gles2_implementation.cc:1257: if (!buffer_id) > return; > nit: return on its own line > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode1334<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode1334> > gpu/command_buffer/client/**gles2_implementation.cc:1334: if (!buffer_id) > return; > nit: put return on its own line > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode1406<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode1406> > gpu/command_buffer/client/**gles2_implementation.cc:1406: // Unknown > target > Set error here, rather than the callsites. > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode1410<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode1410> > gpu/command_buffer/client/**gles2_implementation.cc:1410: > SetGLError(GL_INVALID_VALUE, function_name, "no buffer bound"); > GL_INVALID_OPERATION (according to spec). > > Should you return false, saving some extra tests on the callsites? > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode1469<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode1469> > gpu/command_buffer/client/**gles2_implementation.cc:1469: > SetGLError(GL_INVALID_VALUE, "glCompressedTexImage2D", "invalid > buffer"); > GetBoundPixelUnpackTransferBuf**ferIfValid already set the correct error > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode1513<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode1513> > gpu/command_buffer/client/**gles2_implementation.cc:1513: > SetGLError(GL_INVALID_VALUE, > GetBoundPixelUnpackTransferBuf**ferIfValid already set the right error > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode1603<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode1603> > gpu/command_buffer/client/**gles2_implementation.cc:1603: > SetGLError(GL_INVALID_VALUE, "glTexImage2D", "invalid buffer"); > GetBoundPixelUnpackTransferBuf**ferIfValid already set the right error > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode1711<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode1711> > gpu/command_buffer/client/**gles2_implementation.cc:1711: > SetGLError(GL_INVALID_VALUE, "glTexSubImage2D", "invalid buffer"); > GetBoundPixelUnpackTransferBuf**ferIfValid already set the right error > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode2168<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode2168> > gpu/command_buffer/client/**gles2_implementation.cc:2168: > SetGLError(GL_INVALID_VALUE, "glReadPixels", "invalid readback buffer"); > GetBoundPixelUnpackTransferBuf**ferIfValid already set the right error > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode2174<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode2174> > gpu/command_buffer/client/**gles2_implementation.cc:2174: > SetGLError(GL_INVALID_VALUE, "glReadPixels", "pixels = NULL"); > note: spec doesn't say that an error would be generated. It's almost > expected that it would crash on this user error (hint: passing any > non-zero offset cast into a pointer would be equally bad) > If anything, GL_INVALID_OPERATION would be more appropriate (since it's > a matter of state combination, the value itself being valid in other > contexts). > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode3397<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode3397> > gpu/command_buffer/client/**gles2_implementation.cc:3397: if (!buffer_id) > return NULL; > nit: return on its own line > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode3432<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode3432> > gpu/command_buffer/client/**gles2_implementation.cc:3432: if (!buffer_id) > return false; > nit: return on its own line > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode3484<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode3484> > gpu/command_buffer/client/**gles2_implementation.cc:3484: // the buffer > before the transfer is finished. (Currently sunch > typo: sunch > > https://codereview.chromium.**org/12892005/diff/66001/gpu/** > command_buffer/client/gles2_**implementation.cc#newcode3530<https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client/gles2_implementation.cc#newcode3530> > gpu/command_buffer/client/**gles2_implementation.cc:3530: // the buffer > before the transfer is finished. (Currently sunch > typo: sunch > > https://codereview.chromium.**org/12892005/<https://codereview.chromium.org/1... >
Most feedback incorporated. Need pointer on how to use sync points with callbacks. https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:474: #ifdef USE_SKIA On 2013/03/28 20:22:27, piman wrote: > ? > What does USE_SKIA mean? All platforms use skia... > I also don't see where it's even defined anywhere. It seems that USE_SKIA is beinging erased from history. I based this on code that is no longer there. (see http://src.chromium.org/viewvc/chrome/trunk/src/content/common/gpu/client/web...) I'm removing it. https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:556: base::TimeDelta::FromMilliseconds(2)); On 2013/03/28 20:22:27, piman wrote: > Rather than a poll (with a completely arbitrary 2ms), we have a mechanism to get > an asynchronous completion callback, with sync points. > I realize it's only exposed on CommandBufferProxyImpl, not directly on > WebGraphicsContext3D, but it seems that it's just a matter of exposing this, and > we'd have a much more efficient signaling mechanism. Excellent. But how? Do you have a pointer or an example? If difficult, can I make this a TODO()? https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1257: if (!buffer_id) return; On 2013/03/28 20:22:27, piman wrote: > nit: return on its own line Done. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1334: if (!buffer_id) return; On 2013/03/28 20:22:27, piman wrote: > nit: put return on its own line Done. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1406: // Unknown target On 2013/03/28 20:22:27, piman wrote: > Set error here, rather than the callsites. The idea is for this function to handle certain targets but not all possible targets. Returning false just means that this function did not handle the specified target. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1410: SetGLError(GL_INVALID_VALUE, function_name, "no buffer bound"); On 2013/03/28 20:22:27, piman wrote: > GL_INVALID_OPERATION (according to spec). > > Should you return false, saving some extra tests on the callsites? See explanation above. Also, could you point me to where in the spec this is found so I know better next time? :) https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1469: SetGLError(GL_INVALID_VALUE, "glCompressedTexImage2D", "invalid buffer"); On 2013/03/28 20:22:27, piman wrote: > GetBoundPixelUnpackTransferBufferIfValid already set the correct error Done. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1513: SetGLError(GL_INVALID_VALUE, On 2013/03/28 20:22:27, piman wrote: > GetBoundPixelUnpackTransferBufferIfValid already set the right error Done. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1603: SetGLError(GL_INVALID_VALUE, "glTexImage2D", "invalid buffer"); On 2013/03/28 20:22:27, piman wrote: > GetBoundPixelUnpackTransferBufferIfValid already set the right error Done. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:1711: SetGLError(GL_INVALID_VALUE, "glTexSubImage2D", "invalid buffer"); On 2013/03/28 20:22:27, piman wrote: > GetBoundPixelUnpackTransferBufferIfValid already set the right error Done. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:2168: SetGLError(GL_INVALID_VALUE, "glReadPixels", "invalid readback buffer"); On 2013/03/28 20:22:27, piman wrote: > GetBoundPixelUnpackTransferBufferIfValid already set the right error Done. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:2174: SetGLError(GL_INVALID_VALUE, "glReadPixels", "pixels = NULL"); On 2013/03/28 20:22:27, piman wrote: > note: spec doesn't say that an error would be generated. It's almost expected > that it would crash on this user error (hint: passing any non-zero offset cast > into a pointer would be equally bad) > If anything, GL_INVALID_OPERATION would be more appropriate (since it's a matter > of state combination, the value itself being valid in other contexts). Changed to GL_INVALID_OPERATION. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3397: if (!buffer_id) return NULL; On 2013/03/28 20:22:27, piman wrote: > nit: return on its own line Done. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3432: if (!buffer_id) return false; On 2013/03/28 20:22:27, piman wrote: > nit: return on its own line Done. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3484: // the buffer before the transfer is finished. (Currently sunch On 2013/03/28 20:22:27, piman wrote: > typo: sunch Done. https://codereview.chromium.org/12892005/diff/66001/gpu/command_buffer/client... gpu/command_buffer/client/gles2_implementation.cc:3530: // the buffer before the transfer is finished. (Currently sunch On 2013/03/28 20:22:27, piman wrote: > typo: sunch Done.
https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client... content/common/gpu/client/gl_helper.cc:556: base::TimeDelta::FromMilliseconds(2)); On 2013/03/28 20:52:35, hubbe wrote: > On 2013/03/28 20:22:27, piman wrote: > > Rather than a poll (with a completely arbitrary 2ms), we have a mechanism to > get > > an asynchronous completion callback, with sync points. > > I realize it's only exposed on CommandBufferProxyImpl, not directly on > > WebGraphicsContext3D, but it seems that it's just a matter of exposing this, > and > > we'd have a much more efficient signaling mechanism. > > Excellent. But how? Do you have a pointer or an example? > If difficult, can I make this a TODO()? It's not difficult per se. The call is CommandBufferProxyImpl::SignalSyncPoint. You first insert a sync point using context_->insertSyncPoint (think, like a command buffer token, but that also works cross-command buffer), and you use SignalSyncPoint so that the GPU process sends you a callback once the sync point has passed. The only thing is, as it's not exposed on WebGraphicsContext3D, we need to add it there, as well as an implementation in WebGraphicsContext3DCommandBufferImpl (that just forwards to its CommandBufferProxyImpl), so that you can use it here. The advantage is you save all the code related to queries and polling, and you also save at run-time by not having to poll on arbitrary intervals. I think it's well worth doing.
On 2013/03/28 21:10:06, piman wrote: > https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client... > File content/common/gpu/client/gl_helper.cc (right): > > https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client... > content/common/gpu/client/gl_helper.cc:556: > base::TimeDelta::FromMilliseconds(2)); > On 2013/03/28 20:52:35, hubbe wrote: > > On 2013/03/28 20:22:27, piman wrote: > > > Rather than a poll (with a completely arbitrary 2ms), we have a mechanism to > > get > > > an asynchronous completion callback, with sync points. > > > I realize it's only exposed on CommandBufferProxyImpl, not directly on > > > WebGraphicsContext3D, but it seems that it's just a matter of exposing this, > > and > > > we'd have a much more efficient signaling mechanism. > > > > Excellent. But how? Do you have a pointer or an example? > > If difficult, can I make this a TODO()? > > It's not difficult per se. > The call is CommandBufferProxyImpl::SignalSyncPoint. You first insert a sync > point using context_->insertSyncPoint (think, like a command buffer token, but > that also works cross-command buffer), and you use SignalSyncPoint so that the > GPU process sends you a callback once the sync point has passed. > > The only thing is, as it's not exposed on WebGraphicsContext3D, we need to add > it there, as well as an implementation in WebGraphicsContext3DCommandBufferImpl > (that just forwards to its CommandBufferProxyImpl), so that you can use it here. > > The advantage is you save all the code related to queries and polling, and you > also save at run-time by not having to poll on arbitrary intervals. I think it's > well worth doing. On 2013/03/28 21:10:06, piman wrote: > https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client... > File content/common/gpu/client/gl_helper.cc (right): > > https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client... > content/common/gpu/client/gl_helper.cc:556: > base::TimeDelta::FromMilliseconds(2)); > On 2013/03/28 20:52:35, hubbe wrote: > > On 2013/03/28 20:22:27, piman wrote: > > > Rather than a poll (with a completely arbitrary 2ms), we have a mechanism to > > get > > > an asynchronous completion callback, with sync points. > > > I realize it's only exposed on CommandBufferProxyImpl, not directly on > > > WebGraphicsContext3D, but it seems that it's just a matter of exposing this, > > and > > > we'd have a much more efficient signaling mechanism. > > > > Excellent. But how? Do you have a pointer or an example? > > If difficult, can I make this a TODO()? > > It's not difficult per se. > The call is CommandBufferProxyImpl::SignalSyncPoint. You first insert a sync > point using context_->insertSyncPoint (think, like a command buffer token, but > that also works cross-command buffer), and you use SignalSyncPoint so that the > GPU process sends you a callback once the sync point has passed. > > The only thing is, as it's not exposed on WebGraphicsContext3D, we need to add > it there, as well as an implementation in WebGraphicsContext3DCommandBufferImpl > (that just forwards to its CommandBufferProxyImpl), so that you can use it here. > > The advantage is you save all the code related to queries and polling, and you > also save at run-time by not having to poll on arbitrary intervals. I think it's > well worth doing. Now I'm derailing this even more (maybe the callback thing is really worth a separate patch?).... It might also be useful to have the callback triggered from the query completion rather than sync point. Because then it would work for async texture uploads also (they are async on the service side and therefore async from the main command stream).
I'm really tempted to just cast the WebGraphicsContext3D to a WebGraphicsContext3DCommandBufferImpl and call GetCommandBufferProxy()->SignalSyncPoint(....) but that's not a very pretty solution. What do I need to do to land a change in WebGraphicsContex3D.h ? Submit it to the webkit site? Also WebGraphicsContext3D.h doesn't seem to like our callbacks, it has a class for each type of callback instead. (Which probably means I can't use weak pointers, which will mess up my code even more...) This really feels like a TODO to me. On Thu, Mar 28, 2013 at 4:33 PM, <sievers@chromium.org> wrote: > On 2013/03/28 21:10:06, piman wrote: > > https://codereview.chromium.**org/12892005/diff/66001/** > content/common/gpu/client/gl_**helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc> > >> File content/common/gpu/client/gl_**helper.cc (right): >> > > > https://codereview.chromium.**org/12892005/diff/66001/** > content/common/gpu/client/gl_**helper.cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556> > >> content/common/gpu/client/gl_**helper.cc:556: >> base::TimeDelta::**FromMilliseconds(2)); >> On 2013/03/28 20:52:35, hubbe wrote: >> > On 2013/03/28 20:22:27, piman wrote: >> > > Rather than a poll (with a completely arbitrary 2ms), we have a >> mechanism >> > to > >> > get >> > > an asynchronous completion callback, with sync points. >> > > I realize it's only exposed on CommandBufferProxyImpl, not directly on >> > > WebGraphicsContext3D, but it seems that it's just a matter of exposing >> > this, > >> > and >> > > we'd have a much more efficient signaling mechanism. >> > >> > Excellent. But how? Do you have a pointer or an example? >> > If difficult, can I make this a TODO()? >> > > It's not difficult per se. >> The call is CommandBufferProxyImpl::**SignalSyncPoint. You first insert >> a sync >> point using context_->insertSyncPoint (think, like a command buffer >> token, but >> that also works cross-command buffer), and you use SignalSyncPoint so >> that the >> GPU process sends you a callback once the sync point has passed. >> > > The only thing is, as it's not exposed on WebGraphicsContext3D, we need >> to add >> it there, as well as an implementation in >> > WebGraphicsContext3DCommandBuf**ferImpl > >> (that just forwards to its CommandBufferProxyImpl), so that you can use it >> > here. > > The advantage is you save all the code related to queries and polling, >> and you >> also save at run-time by not having to poll on arbitrary intervals. I >> think >> > it's > >> well worth doing. >> > > On 2013/03/28 21:10:06, piman wrote: > > https://codereview.chromium.**org/12892005/diff/66001/** > content/common/gpu/client/gl_**helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc> > >> File content/common/gpu/client/gl_**helper.cc (right): >> > > > https://codereview.chromium.**org/12892005/diff/66001/** > content/common/gpu/client/gl_**helper.cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556> > >> content/common/gpu/client/gl_**helper.cc:556: >> base::TimeDelta::**FromMilliseconds(2)); >> On 2013/03/28 20:52:35, hubbe wrote: >> > On 2013/03/28 20:22:27, piman wrote: >> > > Rather than a poll (with a completely arbitrary 2ms), we have a >> mechanism >> > to > >> > get >> > > an asynchronous completion callback, with sync points. >> > > I realize it's only exposed on CommandBufferProxyImpl, not directly on >> > > WebGraphicsContext3D, but it seems that it's just a matter of exposing >> > this, > >> > and >> > > we'd have a much more efficient signaling mechanism. >> > >> > Excellent. But how? Do you have a pointer or an example? >> > If difficult, can I make this a TODO()? >> > > It's not difficult per se. >> The call is CommandBufferProxyImpl::**SignalSyncPoint. You first insert >> a sync >> point using context_->insertSyncPoint (think, like a command buffer >> token, but >> that also works cross-command buffer), and you use SignalSyncPoint so >> that the >> GPU process sends you a callback once the sync point has passed. >> > > The only thing is, as it's not exposed on WebGraphicsContext3D, we need >> to add >> it there, as well as an implementation in >> > WebGraphicsContext3DCommandBuf**ferImpl > >> (that just forwards to its CommandBufferProxyImpl), so that you can use it >> > here. > > The advantage is you save all the code related to queries and polling, >> and you >> also save at run-time by not having to poll on arbitrary intervals. I >> think >> > it's > >> well worth doing. >> > > Now I'm derailing this even more (maybe the callback thing is really worth > a > separate patch?).... > > It might also be useful to have the callback triggered from the query > completion > rather than sync point. > Because then it would work for async texture uploads also (they are async > on the > service side and therefore async from the main command stream). > > https://codereview.chromium.**org/12892005/<https://codereview.chromium.org/1... >
On Thu, Mar 28, 2013 at 4:33 PM, <sievers@chromium.org> wrote: > On 2013/03/28 21:10:06, piman wrote: > > https://codereview.chromium.**org/12892005/diff/66001/** > content/common/gpu/client/gl_**helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc> > >> File content/common/gpu/client/gl_**helper.cc (right): >> > > > https://codereview.chromium.**org/12892005/diff/66001/** > content/common/gpu/client/gl_**helper.cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556> > >> content/common/gpu/client/gl_**helper.cc:556: >> base::TimeDelta::**FromMilliseconds(2)); >> On 2013/03/28 20:52:35, hubbe wrote: >> > On 2013/03/28 20:22:27, piman wrote: >> > > Rather than a poll (with a completely arbitrary 2ms), we have a >> mechanism >> > to > >> > get >> > > an asynchronous completion callback, with sync points. >> > > I realize it's only exposed on CommandBufferProxyImpl, not directly on >> > > WebGraphicsContext3D, but it seems that it's just a matter of exposing >> > this, > >> > and >> > > we'd have a much more efficient signaling mechanism. >> > >> > Excellent. But how? Do you have a pointer or an example? >> > If difficult, can I make this a TODO()? >> > > It's not difficult per se. >> The call is CommandBufferProxyImpl::**SignalSyncPoint. You first insert >> a sync >> point using context_->insertSyncPoint (think, like a command buffer >> token, but >> that also works cross-command buffer), and you use SignalSyncPoint so >> that the >> GPU process sends you a callback once the sync point has passed. >> > > The only thing is, as it's not exposed on WebGraphicsContext3D, we need >> to add >> it there, as well as an implementation in >> > WebGraphicsContext3DCommandBuf**ferImpl > >> (that just forwards to its CommandBufferProxyImpl), so that you can use it >> > here. > > The advantage is you save all the code related to queries and polling, >> and you >> also save at run-time by not having to poll on arbitrary intervals. I >> think >> > it's > >> well worth doing. >> > > On 2013/03/28 21:10:06, piman wrote: > > https://codereview.chromium.**org/12892005/diff/66001/** > content/common/gpu/client/gl_**helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc> > >> File content/common/gpu/client/gl_**helper.cc (right): >> > > > https://codereview.chromium.**org/12892005/diff/66001/** > content/common/gpu/client/gl_**helper.cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556> > >> content/common/gpu/client/gl_**helper.cc:556: >> base::TimeDelta::**FromMilliseconds(2)); >> On 2013/03/28 20:52:35, hubbe wrote: >> > On 2013/03/28 20:22:27, piman wrote: >> > > Rather than a poll (with a completely arbitrary 2ms), we have a >> mechanism >> > to > >> > get >> > > an asynchronous completion callback, with sync points. >> > > I realize it's only exposed on CommandBufferProxyImpl, not directly on >> > > WebGraphicsContext3D, but it seems that it's just a matter of exposing >> > this, > >> > and >> > > we'd have a much more efficient signaling mechanism. >> > >> > Excellent. But how? Do you have a pointer or an example? >> > If difficult, can I make this a TODO()? >> > > It's not difficult per se. >> The call is CommandBufferProxyImpl::**SignalSyncPoint. You first insert >> a sync >> point using context_->insertSyncPoint (think, like a command buffer >> token, but >> that also works cross-command buffer), and you use SignalSyncPoint so >> that the >> GPU process sends you a callback once the sync point has passed. >> > > The only thing is, as it's not exposed on WebGraphicsContext3D, we need >> to add >> it there, as well as an implementation in >> > WebGraphicsContext3DCommandBuf**ferImpl > >> (that just forwards to its CommandBufferProxyImpl), so that you can use it >> > here. > > The advantage is you save all the code related to queries and polling, >> and you >> also save at run-time by not having to poll on arbitrary intervals. I >> think >> > it's > >> well worth doing. >> > > Now I'm derailing this even more (maybe the callback thing is really worth > a > separate patch?).... > > It might also be useful to have the callback triggered from the query > completion > rather than sync point. Because then it would work for async texture uploads also (they are async > on the > service side and therefore async from the main command stream). > That's something that's very different from a sync point though, and you'd need to build significantly different code in the backend for that. > > https://codereview.chromium.**org/12892005/<https://codereview.chromium.org/1... >
On Thu, Mar 28, 2013 at 4:46 PM, Fredrik Hubinette <hubbe@google.com> wrote: > I'm really tempted to just cast the WebGraphicsContext3D to a > WebGraphicsContext3DCommandBufferImpl and call > GetCommandBufferProxy()->SignalSyncPoint(....) but that's not a very > pretty solution. > > What do I need to do to land a change in WebGraphicsContex3D.h ? Submit > it to the webkit site? > Yes - http://dev.chromium.org/developers/contributing-to-webkit > Also WebGraphicsContext3D.h doesn't seem to like our callbacks, it has a > class for each type of callback instead. > (Which probably means I can't use weak pointers, which will mess up my > code even more...) > You can still use weak pointers, in the implementation of the callback class, it's not a problem. > This really feels like a TODO to me. > Then I will ask you to convince me that 2ms is the right number here. We have anecdotal evidence that in some cases on Android, posting a task takes .4 ms - so you'd be burning 20% of the time just to poll for this. > > On Thu, Mar 28, 2013 at 4:33 PM, <sievers@chromium.org> wrote: > >> On 2013/03/28 21:10:06, piman wrote: >> >> https://codereview.chromium.**org/12892005/diff/66001/** >> content/common/gpu/client/gl_**helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc> >> >>> File content/common/gpu/client/gl_**helper.cc (right): >>> >> >> >> https://codereview.chromium.**org/12892005/diff/66001/** >> content/common/gpu/client/gl_**helper.cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556> >> >>> content/common/gpu/client/gl_**helper.cc:556: >>> base::TimeDelta::**FromMilliseconds(2)); >>> On 2013/03/28 20:52:35, hubbe wrote: >>> > On 2013/03/28 20:22:27, piman wrote: >>> > > Rather than a poll (with a completely arbitrary 2ms), we have a >>> mechanism >>> >> to >> >>> > get >>> > > an asynchronous completion callback, with sync points. >>> > > I realize it's only exposed on CommandBufferProxyImpl, not directly >>> on >>> > > WebGraphicsContext3D, but it seems that it's just a matter of >>> exposing >>> >> this, >> >>> > and >>> > > we'd have a much more efficient signaling mechanism. >>> > >>> > Excellent. But how? Do you have a pointer or an example? >>> > If difficult, can I make this a TODO()? >>> >> >> It's not difficult per se. >>> The call is CommandBufferProxyImpl::**SignalSyncPoint. You first insert >>> a sync >>> point using context_->insertSyncPoint (think, like a command buffer >>> token, but >>> that also works cross-command buffer), and you use SignalSyncPoint so >>> that the >>> GPU process sends you a callback once the sync point has passed. >>> >> >> The only thing is, as it's not exposed on WebGraphicsContext3D, we need >>> to add >>> it there, as well as an implementation in >>> >> WebGraphicsContext3DCommandBuf**ferImpl >> >>> (that just forwards to its CommandBufferProxyImpl), so that you can use >>> it >>> >> here. >> >> The advantage is you save all the code related to queries and polling, >>> and you >>> also save at run-time by not having to poll on arbitrary intervals. I >>> think >>> >> it's >> >>> well worth doing. >>> >> >> On 2013/03/28 21:10:06, piman wrote: >> >> https://codereview.chromium.**org/12892005/diff/66001/** >> content/common/gpu/client/gl_**helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc> >> >>> File content/common/gpu/client/gl_**helper.cc (right): >>> >> >> >> https://codereview.chromium.**org/12892005/diff/66001/** >> content/common/gpu/client/gl_**helper.cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556> >> >>> content/common/gpu/client/gl_**helper.cc:556: >>> base::TimeDelta::**FromMilliseconds(2)); >>> On 2013/03/28 20:52:35, hubbe wrote: >>> > On 2013/03/28 20:22:27, piman wrote: >>> > > Rather than a poll (with a completely arbitrary 2ms), we have a >>> mechanism >>> >> to >> >>> > get >>> > > an asynchronous completion callback, with sync points. >>> > > I realize it's only exposed on CommandBufferProxyImpl, not directly >>> on >>> > > WebGraphicsContext3D, but it seems that it's just a matter of >>> exposing >>> >> this, >> >>> > and >>> > > we'd have a much more efficient signaling mechanism. >>> > >>> > Excellent. But how? Do you have a pointer or an example? >>> > If difficult, can I make this a TODO()? >>> >> >> It's not difficult per se. >>> The call is CommandBufferProxyImpl::**SignalSyncPoint. You first insert >>> a sync >>> point using context_->insertSyncPoint (think, like a command buffer >>> token, but >>> that also works cross-command buffer), and you use SignalSyncPoint so >>> that the >>> GPU process sends you a callback once the sync point has passed. >>> >> >> The only thing is, as it's not exposed on WebGraphicsContext3D, we need >>> to add >>> it there, as well as an implementation in >>> >> WebGraphicsContext3DCommandBuf**ferImpl >> >>> (that just forwards to its CommandBufferProxyImpl), so that you can use >>> it >>> >> here. >> >> The advantage is you save all the code related to queries and polling, >>> and you >>> also save at run-time by not having to poll on arbitrary intervals. I >>> think >>> >> it's >> >>> well worth doing. >>> >> >> Now I'm derailing this even more (maybe the callback thing is really >> worth a >> separate patch?).... >> >> It might also be useful to have the callback triggered from the query >> completion >> rather than sync point. >> Because then it would work for async texture uploads also (they are async >> on the >> service side and therefore async from the main command stream). >> >> https://codereview.chromium.**org/12892005/<https://codereview.chromium.org/1... >> > >
On 2013/03/28 23:46:08, hubbe1 wrote: > I'm really tempted to just cast the WebGraphicsContext3D to a > WebGraphicsContext3DCommandBufferImpl and call > GetCommandBufferProxy()->SignalSyncPoint(....) but that's not a very > pretty solution. I've been tempted by this before too! I believe there will eventually be a pure chromium interface instead of the webkit one. Syncpoints seem ideal if you really want a callback, but as an alternative to polling and callbacks, can you just rely on frames and poll your oldest query every frame? After PIPELINE_DEPTH frames, one will always complete every frame.
On 2013/03/28 23:46:08, hubbe1 wrote:
> I'm really tempted to just cast the WebGraphicsContext3D to a
> WebGraphicsContext3DCommandBufferImpl and call
> GetCommandBufferProxy()->SignalSyncPoint(....) but that's not a very
> pretty solution.
>
To avoid a cast, you might be able to change the call sites to pass in a
WGC3DCBImpl when constructing GLHelper.
For Android it's trivial:
GLHelper* CmdBufferImageTransportFactory::GetGLHelper() {
if (!gl_helper_.get())
- gl_helper_.reset(new GLHelper(GetContext3D(), NULL));
+ gl_helper_.reset(new GLHelper(context_.get(), NULL));
The Aura one also looks like it might only ever create the helper from that type
nowadays.
> What do I need to do to land a change in WebGraphicsContex3D.h ? Submit it
> to the webkit site?
> Also WebGraphicsContext3D.h doesn't seem to like our callbacks, it has a
> class for each type of callback instead.
> (Which probably means I can't use weak pointers, which will mess up my code
> even more...)
>
> This really feels like a TODO to me.
>
>
> On Thu, Mar 28, 2013 at 4:33 PM, <mailto:sievers@chromium.org> wrote:
>
> > On 2013/03/28 21:10:06, piman wrote:
> >
> > https://codereview.chromium.**org/12892005/diff/66001/**
> >
>
content/common/gpu/client/gl_**helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc>
> >
> >> File content/common/gpu/client/gl_**helper.cc (right):
> >>
> >
> >
> > https://codereview.chromium.**org/12892005/diff/66001/**
> >
>
content/common/gpu/client/gl_**helper.cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556>
> >
> >> content/common/gpu/client/gl_**helper.cc:556:
> >> base::TimeDelta::**FromMilliseconds(2));
> >> On 2013/03/28 20:52:35, hubbe wrote:
> >> > On 2013/03/28 20:22:27, piman wrote:
> >> > > Rather than a poll (with a completely arbitrary 2ms), we have a
> >> mechanism
> >>
> > to
> >
> >> > get
> >> > > an asynchronous completion callback, with sync points.
> >> > > I realize it's only exposed on CommandBufferProxyImpl, not directly on
> >> > > WebGraphicsContext3D, but it seems that it's just a matter of exposing
> >>
> > this,
> >
> >> > and
> >> > > we'd have a much more efficient signaling mechanism.
> >> >
> >> > Excellent. But how? Do you have a pointer or an example?
> >> > If difficult, can I make this a TODO()?
> >>
> >
> > It's not difficult per se.
> >> The call is CommandBufferProxyImpl::**SignalSyncPoint. You first insert
> >> a sync
> >> point using context_->insertSyncPoint (think, like a command buffer
> >> token, but
> >> that also works cross-command buffer), and you use SignalSyncPoint so
> >> that the
> >> GPU process sends you a callback once the sync point has passed.
> >>
> >
> > The only thing is, as it's not exposed on WebGraphicsContext3D, we need
> >> to add
> >> it there, as well as an implementation in
> >>
> > WebGraphicsContext3DCommandBuf**ferImpl
> >
> >> (that just forwards to its CommandBufferProxyImpl), so that you can use it
> >>
> > here.
> >
> > The advantage is you save all the code related to queries and polling,
> >> and you
> >> also save at run-time by not having to poll on arbitrary intervals. I
> >> think
> >>
> > it's
> >
> >> well worth doing.
> >>
> >
> > On 2013/03/28 21:10:06, piman wrote:
> >
> > https://codereview.chromium.**org/12892005/diff/66001/**
> >
>
content/common/gpu/client/gl_**helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc>
> >
> >> File content/common/gpu/client/gl_**helper.cc (right):
> >>
> >
> >
> > https://codereview.chromium.**org/12892005/diff/66001/**
> >
>
content/common/gpu/client/gl_**helper.cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556>
> >
> >> content/common/gpu/client/gl_**helper.cc:556:
> >> base::TimeDelta::**FromMilliseconds(2));
> >> On 2013/03/28 20:52:35, hubbe wrote:
> >> > On 2013/03/28 20:22:27, piman wrote:
> >> > > Rather than a poll (with a completely arbitrary 2ms), we have a
> >> mechanism
> >>
> > to
> >
> >> > get
> >> > > an asynchronous completion callback, with sync points.
> >> > > I realize it's only exposed on CommandBufferProxyImpl, not directly on
> >> > > WebGraphicsContext3D, but it seems that it's just a matter of exposing
> >>
> > this,
> >
> >> > and
> >> > > we'd have a much more efficient signaling mechanism.
> >> >
> >> > Excellent. But how? Do you have a pointer or an example?
> >> > If difficult, can I make this a TODO()?
> >>
> >
> > It's not difficult per se.
> >> The call is CommandBufferProxyImpl::**SignalSyncPoint. You first insert
> >> a sync
> >> point using context_->insertSyncPoint (think, like a command buffer
> >> token, but
> >> that also works cross-command buffer), and you use SignalSyncPoint so
> >> that the
> >> GPU process sends you a callback once the sync point has passed.
> >>
> >
> > The only thing is, as it's not exposed on WebGraphicsContext3D, we need
> >> to add
> >> it there, as well as an implementation in
> >>
> > WebGraphicsContext3DCommandBuf**ferImpl
> >
> >> (that just forwards to its CommandBufferProxyImpl), so that you can use it
> >>
> > here.
> >
> > The advantage is you save all the code related to queries and polling,
> >> and you
> >> also save at run-time by not having to poll on arbitrary intervals. I
> >> think
> >>
> > it's
> >
> >> well worth doing.
> >>
> >
> > Now I'm derailing this even more (maybe the callback thing is really worth
> > a
> > separate patch?)....
> >
> > It might also be useful to have the callback triggered from the query
> > completion
> > rather than sync point.
> > Because then it would work for async texture uploads also (they are async
> > on the
> > service side and therefore async from the main command stream).
> >
> >
>
https://codereview.chromium.**org/12892005/%3Chttps://codereview.chromium.org...>
> >
On Thu, Mar 28, 2013 at 5:14 PM, <sievers@chromium.org> wrote: > On 2013/03/28 23:46:08, hubbe1 wrote: > >> I'm really tempted to just cast the WebGraphicsContext3D to a >> WebGraphicsContext3DCommandBuf**ferImpl and call >> GetCommandBufferProxy()->**SignalSyncPoint(....) but that's not a very >> pretty solution. >> > > > To avoid a cast, you might be able to change the call sites to pass in a > WGC3DCBImpl when constructing GLHelper. > > For Android it's trivial: > GLHelper* CmdBufferImageTransportFactory**::GetGLHelper() { > if (!gl_helper_.get()) > - gl_helper_.reset(new GLHelper(GetContext3D(), NULL)); > + gl_helper_.reset(new GLHelper(context_.get(), NULL)); > > > The Aura one also looks like it might only ever create the helper from > that type > nowadays. > True, that works too. Antoine > > What do I need to do to land a change in WebGraphicsContex3D.h ? Submit >> it >> to the webkit site? >> Also WebGraphicsContext3D.h doesn't seem to like our callbacks, it has a >> class for each type of callback instead. >> (Which probably means I can't use weak pointers, which will mess up my >> code >> even more...) >> > > This really feels like a TODO to me. >> > > > On Thu, Mar 28, 2013 at 4:33 PM, <mailto:sievers@chromium.org> wrote: >> > > > On 2013/03/28 21:10:06, piman wrote: >> > >> > https://codereview.chromium.****org/12892005/diff/66001/** >> > >> > > content/common/gpu/client/gl_****helper.cc<https://codereview.** > chromium.org/12892005/diff/**66001/content/common/gpu/** > client/gl_helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc> > > > >> > >> >> File content/common/gpu/client/gl_****helper.cc (right): >> >> >> > >> > >> > https://codereview.chromium.****org/12892005/diff/66001/** >> > >> > > content/common/gpu/client/gl_****helper.cc#newcode556<https://** > codereview.chromium.org/**12892005/diff/66001/content/** > common/gpu/client/gl_helper.**cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556> > > > >> > >> >> content/common/gpu/client/gl_****helper.cc:556: >> >> base::TimeDelta::****FromMilliseconds(2)); >> >> >> On 2013/03/28 20:52:35, hubbe wrote: >> >> > On 2013/03/28 20:22:27, piman wrote: >> >> > > Rather than a poll (with a completely arbitrary 2ms), we have a >> >> mechanism >> >> >> > to >> > >> >> > get >> >> > > an asynchronous completion callback, with sync points. >> >> > > I realize it's only exposed on CommandBufferProxyImpl, not >> directly on >> >> > > WebGraphicsContext3D, but it seems that it's just a matter of >> exposing >> >> >> > this, >> > >> >> > and >> >> > > we'd have a much more efficient signaling mechanism. >> >> > >> >> > Excellent. But how? Do you have a pointer or an example? >> >> > If difficult, can I make this a TODO()? >> >> >> > >> > It's not difficult per se. >> >> The call is CommandBufferProxyImpl::****SignalSyncPoint. You first >> insert >> >> >> a sync >> >> point using context_->insertSyncPoint (think, like a command buffer >> >> token, but >> >> that also works cross-command buffer), and you use SignalSyncPoint so >> >> that the >> >> GPU process sends you a callback once the sync point has passed. >> >> >> > >> > The only thing is, as it's not exposed on WebGraphicsContext3D, we need >> >> to add >> >> it there, as well as an implementation in >> >> >> > WebGraphicsContext3DCommandBuf****ferImpl >> >> > >> >> (that just forwards to its CommandBufferProxyImpl), so that you can >> use it >> >> >> > here. >> > >> > The advantage is you save all the code related to queries and polling, >> >> and you >> >> also save at run-time by not having to poll on arbitrary intervals. I >> >> think >> >> >> > it's >> > >> >> well worth doing. >> >> >> > >> > On 2013/03/28 21:10:06, piman wrote: >> > >> > https://codereview.chromium.****org/12892005/diff/66001/** >> > >> > > content/common/gpu/client/gl_****helper.cc<https://codereview.** > chromium.org/12892005/diff/**66001/content/common/gpu/** > client/gl_helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc> > > > >> > >> >> File content/common/gpu/client/gl_****helper.cc (right): >> >> >> > >> > >> > https://codereview.chromium.****org/12892005/diff/66001/** >> > >> > > content/common/gpu/client/gl_****helper.cc#newcode556<https://** > codereview.chromium.org/**12892005/diff/66001/content/** > common/gpu/client/gl_helper.**cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556> > > > >> > >> >> content/common/gpu/client/gl_****helper.cc:556: >> >> base::TimeDelta::****FromMilliseconds(2)); >> >> >> On 2013/03/28 20:52:35, hubbe wrote: >> >> > On 2013/03/28 20:22:27, piman wrote: >> >> > > Rather than a poll (with a completely arbitrary 2ms), we have a >> >> mechanism >> >> >> > to >> > >> >> > get >> >> > > an asynchronous completion callback, with sync points. >> >> > > I realize it's only exposed on CommandBufferProxyImpl, not >> directly on >> >> > > WebGraphicsContext3D, but it seems that it's just a matter of >> exposing >> >> >> > this, >> > >> >> > and >> >> > > we'd have a much more efficient signaling mechanism. >> >> > >> >> > Excellent. But how? Do you have a pointer or an example? >> >> > If difficult, can I make this a TODO()? >> >> >> > >> > It's not difficult per se. >> >> The call is CommandBufferProxyImpl::****SignalSyncPoint. You first >> insert >> >> >> a sync >> >> point using context_->insertSyncPoint (think, like a command buffer >> >> token, but >> >> that also works cross-command buffer), and you use SignalSyncPoint so >> >> that the >> >> GPU process sends you a callback once the sync point has passed. >> >> >> > >> > The only thing is, as it's not exposed on WebGraphicsContext3D, we need >> >> to add >> >> it there, as well as an implementation in >> >> >> > WebGraphicsContext3DCommandBuf****ferImpl >> >> > >> >> (that just forwards to its CommandBufferProxyImpl), so that you can >> use it >> >> >> > here. >> > >> > The advantage is you save all the code related to queries and polling, >> >> and you >> >> also save at run-time by not having to poll on arbitrary intervals. I >> >> think >> >> >> > it's >> > >> >> well worth doing. >> >> >> > >> > Now I'm derailing this even more (maybe the callback thing is really >> worth >> > a >> > separate patch?).... >> > >> > It might also be useful to have the callback triggered from the query >> > completion >> > rather than sync point. >> > Because then it would work for async texture uploads also (they are >> async >> > on the >> > service side and therefore async from the main command stream). >> > >> > >> > > https://codereview.chromium.****org/12892005/%3Chttps://codere** > view.chromium.org/12892005/ <http://codereview.chromium.org/12892005/>> > >> > >> > > > https://codereview.chromium.**org/12892005/<https://codereview.chromium.org/1... >
Ok, I updated gl_helper to take a WebGraphicsContext3DCommandBufferImpl*, so
that I can call SignalSyncPoint. I didn't like the polling anyways, so I'm
happy to be rid of it.
/Hubbe
On 2013/03/29 00:25:16, piman wrote:
> On Thu, Mar 28, 2013 at 5:14 PM, <mailto:sievers@chromium.org> wrote:
>
> > On 2013/03/28 23:46:08, hubbe1 wrote:
> >
> >> I'm really tempted to just cast the WebGraphicsContext3D to a
> >> WebGraphicsContext3DCommandBuf**ferImpl and call
> >> GetCommandBufferProxy()->**SignalSyncPoint(....) but that's not a very
> >> pretty solution.
> >>
> >
> >
> > To avoid a cast, you might be able to change the call sites to pass in a
> > WGC3DCBImpl when constructing GLHelper.
> >
> > For Android it's trivial:
> > GLHelper* CmdBufferImageTransportFactory**::GetGLHelper() {
> > if (!gl_helper_.get())
> > - gl_helper_.reset(new GLHelper(GetContext3D(), NULL));
> > + gl_helper_.reset(new GLHelper(context_.get(), NULL));
> >
> >
> > The Aura one also looks like it might only ever create the helper from
> > that type
> > nowadays.
> >
>
> True, that works too.
>
> Antoine
>
>
> >
> > What do I need to do to land a change in WebGraphicsContex3D.h ? Submit
> >> it
> >> to the webkit site?
> >> Also WebGraphicsContext3D.h doesn't seem to like our callbacks, it has a
> >> class for each type of callback instead.
> >> (Which probably means I can't use weak pointers, which will mess up my
> >> code
> >> even more...)
> >>
> >
> > This really feels like a TODO to me.
> >>
> >
> >
> > On Thu, Mar 28, 2013 at 4:33 PM, <mailto:sievers@chromium.org> wrote:
> >>
> >
> > > On 2013/03/28 21:10:06, piman wrote:
> >> >
> >> > https://codereview.chromium.****org/12892005/diff/66001/**
> >> >
> >>
> >
> > content/common/gpu/client/gl_****helper.cc<https://codereview.**
> > chromium.org/12892005/diff/**66001/content/common/gpu/**
> >
>
client/gl_helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc>
> > >
> >
> >> >
> >> >> File content/common/gpu/client/gl_****helper.cc (right):
> >> >>
> >> >
> >> >
> >> > https://codereview.chromium.****org/12892005/diff/66001/**
> >> >
> >>
> >
> > content/common/gpu/client/gl_****helper.cc#newcode556<https://**
> > codereview.chromium.org/**12892005/diff/66001/content/**
> >
>
common/gpu/client/gl_helper.**cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556>
> > >
> >
> >> >
> >> >> content/common/gpu/client/gl_****helper.cc:556:
> >> >> base::TimeDelta::****FromMilliseconds(2));
> >>
> >> >> On 2013/03/28 20:52:35, hubbe wrote:
> >> >> > On 2013/03/28 20:22:27, piman wrote:
> >> >> > > Rather than a poll (with a completely arbitrary 2ms), we have a
> >> >> mechanism
> >> >>
> >> > to
> >> >
> >> >> > get
> >> >> > > an asynchronous completion callback, with sync points.
> >> >> > > I realize it's only exposed on CommandBufferProxyImpl, not
> >> directly on
> >> >> > > WebGraphicsContext3D, but it seems that it's just a matter of
> >> exposing
> >> >>
> >> > this,
> >> >
> >> >> > and
> >> >> > > we'd have a much more efficient signaling mechanism.
> >> >> >
> >> >> > Excellent. But how? Do you have a pointer or an example?
> >> >> > If difficult, can I make this a TODO()?
> >> >>
> >> >
> >> > It's not difficult per se.
> >> >> The call is CommandBufferProxyImpl::****SignalSyncPoint. You first
> >> insert
> >>
> >> >> a sync
> >> >> point using context_->insertSyncPoint (think, like a command buffer
> >> >> token, but
> >> >> that also works cross-command buffer), and you use SignalSyncPoint so
> >> >> that the
> >> >> GPU process sends you a callback once the sync point has passed.
> >> >>
> >> >
> >> > The only thing is, as it's not exposed on WebGraphicsContext3D, we need
> >> >> to add
> >> >> it there, as well as an implementation in
> >> >>
> >> > WebGraphicsContext3DCommandBuf****ferImpl
> >>
> >> >
> >> >> (that just forwards to its CommandBufferProxyImpl), so that you can
> >> use it
> >> >>
> >> > here.
> >> >
> >> > The advantage is you save all the code related to queries and polling,
> >> >> and you
> >> >> also save at run-time by not having to poll on arbitrary intervals. I
> >> >> think
> >> >>
> >> > it's
> >> >
> >> >> well worth doing.
> >> >>
> >> >
> >> > On 2013/03/28 21:10:06, piman wrote:
> >> >
> >> > https://codereview.chromium.****org/12892005/diff/66001/**
> >> >
> >>
> >
> > content/common/gpu/client/gl_****helper.cc<https://codereview.**
> > chromium.org/12892005/diff/**66001/content/common/gpu/**
> >
>
client/gl_helper.cc<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc>
> > >
> >
> >> >
> >> >> File content/common/gpu/client/gl_****helper.cc (right):
> >> >>
> >> >
> >> >
> >> > https://codereview.chromium.****org/12892005/diff/66001/**
> >> >
> >>
> >
> > content/common/gpu/client/gl_****helper.cc#newcode556<https://**
> > codereview.chromium.org/**12892005/diff/66001/content/**
> >
>
common/gpu/client/gl_helper.**cc#newcode556<https://codereview.chromium.org/12892005/diff/66001/content/common/gpu/client/gl_helper.cc#newcode556>
> > >
> >
> >> >
> >> >> content/common/gpu/client/gl_****helper.cc:556:
> >> >> base::TimeDelta::****FromMilliseconds(2));
> >>
> >> >> On 2013/03/28 20:52:35, hubbe wrote:
> >> >> > On 2013/03/28 20:22:27, piman wrote:
> >> >> > > Rather than a poll (with a completely arbitrary 2ms), we have a
> >> >> mechanism
> >> >>
> >> > to
> >> >
> >> >> > get
> >> >> > > an asynchronous completion callback, with sync points.
> >> >> > > I realize it's only exposed on CommandBufferProxyImpl, not
> >> directly on
> >> >> > > WebGraphicsContext3D, but it seems that it's just a matter of
> >> exposing
> >> >>
> >> > this,
> >> >
> >> >> > and
> >> >> > > we'd have a much more efficient signaling mechanism.
> >> >> >
> >> >> > Excellent. But how? Do you have a pointer or an example?
> >> >> > If difficult, can I make this a TODO()?
> >> >>
> >> >
> >> > It's not difficult per se.
> >> >> The call is CommandBufferProxyImpl::****SignalSyncPoint. You first
> >> insert
> >>
> >> >> a sync
> >> >> point using context_->insertSyncPoint (think, like a command buffer
> >> >> token, but
> >> >> that also works cross-command buffer), and you use SignalSyncPoint so
> >> >> that the
> >> >> GPU process sends you a callback once the sync point has passed.
> >> >>
> >> >
> >> > The only thing is, as it's not exposed on WebGraphicsContext3D, we need
> >> >> to add
> >> >> it there, as well as an implementation in
> >> >>
> >> > WebGraphicsContext3DCommandBuf****ferImpl
> >>
> >> >
> >> >> (that just forwards to its CommandBufferProxyImpl), so that you can
> >> use it
> >> >>
> >> > here.
> >> >
> >> > The advantage is you save all the code related to queries and polling,
> >> >> and you
> >> >> also save at run-time by not having to poll on arbitrary intervals. I
> >> >> think
> >> >>
> >> > it's
> >> >
> >> >> well worth doing.
> >> >>
> >> >
> >> > Now I'm derailing this even more (maybe the callback thing is really
> >> worth
> >> > a
> >> > separate patch?)....
> >> >
> >> > It might also be useful to have the callback triggered from the query
> >> > completion
> >> > rather than sync point.
> >> > Because then it would work for async texture uploads also (they are
> >> async
> >> > on the
> >> > service side and therefore async from the main command stream).
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.****org/12892005/%253Chttps://codere**
> > view.chromium.org/12892005/ <http://codereview.chromium.org/12892005/>>
> >
> >> >
> >>
> >
> >
> >
>
https://codereview.chromium.**org/12892005/%3Chttps://codereview.chromium.org...>
> >
LGTM+nit, this is great. Thanks! https://codereview.chromium.org/12892005/diff/101001/content/common/gpu/clien... File content/common/gpu/client/gl_helper.h (right): https://codereview.chromium.org/12892005/diff/101001/content/common/gpu/clien... content/common/gpu/client/gl_helper.h:27: GLHelper(WebGraphicsContext3DCommandBufferImpl* context); nit: explicit
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hubbe@chromium.org/12892005/109001
Message was sent while issue was closed.
Change committed as 191737 |
