Redesigned CommandBuffer and NPDevice3D interfaces

Redesigned CommandBuffer and NPDevice3D interfaces.
All status is now in the Device3D context.
It can be retreived with fewer IPC messages.
It can be now be accessed off the main plugin thread, which is necessary to run OpenGL in another thread.
TEST=none
BUG=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=37545

[apatrick_chromium, 2010-01-26 23:33:12]

http://codereview.chromium.org/555020/diff/3029/4055
File chrome/renderer/command_buffer_proxy.cc (right):

http://codereview.chromium.org/555020/diff/3029/4055#newcode167
chrome/renderer/command_buffer_proxy.cc:167: void
CommandBufferProxy::SetParseError(gpu::parse_error::ParseError parse_error) {
80 chars already fixed.

[greggman, 2010-01-27 01:12:03]

LGTM

What are you working on next? I'm going to be touching some of these files so it
would be nice to know I'm not going to be stepping on your toes.

My change would effectively remove get_offset from the status structure and add
put_shm_id. I'm having a hard time adding the stuff I need to add without having
to touch all these files.

http://codereview.chromium.org/555020/diff/3029/4042
File gpu/command_buffer/service/gpu_processor.cc (right):

http://codereview.chromium.org/555020/diff/3029/4042#newcode51
gpu/command_buffer/service/gpu_processor.cc:51: case
parse_error::kParseUnknownCommand:
In some meeting someone said that all errors should be unrecoverable.  Basically
there should be no reason you should ever get any of these errors unless your
code is buggy or malicious.

What do you think?

[apatrick, 2010-01-27 01:46:11]

Next I'll be figuring out why Alok's pepper demos aren't working on my box
and getting the GPU unit tests integrated into the automated build. I don't
expect to be making major changes to this code this week.

I'm cool with making all errors unrecoverable. It would make things a lot
simpler.

[darin (slow to review), 2010-01-27 08:35:57]

http://codereview.chromium.org/555020/diff/3029/4047
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/555020/diff/3029/4047#newcode305
third_party/npapi/bindings/npapi_extensions.h:305: typedef struct
_NPDeviceFlushSemantics3D
does this mean that there is now no way to do an asynchronous flush for 3D?  why
do we want to preclude that mode?

[darin (slow to review), 2010-01-27 08:41:40]

http://codereview.chromium.org/555020/diff/3029/4047
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/555020/diff/3029/4047#newcode305
third_party/npapi/bindings/npapi_extensions.h:305: typedef struct
_NPDeviceFlushSemantics3D
On 2010/01/27 08:35:57, darin wrote:
> does this mean that there is now no way to do an asynchronous flush for 3D? 
why
> do we want to preclude that mode?

also, had you considered making these flush parameters be members of the
context?  i ask because the context includes other 'input' parameters like the
putOffset.  so, it seems like it could have other input parameters.  then, flush
just means commit.

i guess i just favor flush being simply a request to synchronize the context
(either blocking or non-blocking).

[darin (slow to review), 2010-01-27 08:42:33]

similarly, the 2D context has the dirty rect as an 'input' parameter.

[apatrick_chromium, 2010-01-27 19:04:50]

http://codereview.chromium.org/555020/diff/3029/4047
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/555020/diff/3029/4047#newcode305
third_party/npapi/bindings/npapi_extensions.h:305: typedef struct
_NPDeviceFlushSemantics3D
At the moment, a flush in 3D is always asynchronous and you call glFlush() to
get a synchronous flush. There is no way to set a callback for the asynchronous
flush. I don't think OpenGL has a way to do that on the service side.

http://codereview.chromium.org/555020/diff/3029/4047#newcode305
third_party/npapi/bindings/npapi_extensions.h:305: typedef struct
_NPDeviceFlushSemantics3D
On 2010/01/27 08:41:40, darin wrote:
> On 2010/01/27 08:35:57, darin wrote:
> > does this mean that there is now no way to do an asynchronous flush for 3D? 
> why
> > do we want to preclude that mode?
> 
> also, had you considered making these flush parameters be members of the
> context?  i ask because the context includes other 'input' parameters like the
> putOffset.  so, it seems like it could have other input parameters.  then,
flush
> just means commit.
> 
> i guess i just favor flush being simply a request to synchronize the context
> (either blocking or non-blocking).

I did. It seemed that these were more like parameters to a particular call to
flush than persistent properties of the context. I'm open to the idea of
changing it. sehr?

[darin (slow to review), 2010-01-27 21:00:55]

On 2010/01/27 19:04:50, apatrick_chromium wrote:
> http://codereview.chromium.org/555020/diff/3029/4047
> File third_party/npapi/bindings/npapi_extensions.h (right):
> 
> http://codereview.chromium.org/555020/diff/3029/4047#newcode305
> third_party/npapi/bindings/npapi_extensions.h:305: typedef struct
> _NPDeviceFlushSemantics3D
> At the moment, a flush in 3D is always asynchronous and you call glFlush() to
> get a synchronous flush. There is no way to set a callback for the
asynchronous
> flush. I don't think OpenGL has a way to do that on the service side.

Interesting.  I don't think it means that this lowest level API shouldn't
have the ability to ACK an async flush.  That may be useful to other APIs
that one might layer above the Device3D interface.

Mainly, I'm just striving for consistency across the various devices.  I
can also imagine an implementation that proxies Device3D calls, and for
that the asynchronous flush + ACK callback could be useful.


> http://codereview.chromium.org/555020/diff/3029/4047#newcode305
> third_party/npapi/bindings/npapi_extensions.h:305: typedef struct
> _NPDeviceFlushSemantics3D
> On 2010/01/27 08:41:40, darin wrote:
> > On 2010/01/27 08:35:57, darin wrote:
> > > does this mean that there is now no way to do an asynchronous flush for
3D? 
> > why
> > > do we want to preclude that mode?
> > 
> > also, had you considered making these flush parameters be members of the
> > context?  i ask because the context includes other 'input' parameters like
the
> > putOffset.  so, it seems like it could have other input parameters.  then,
> flush
> > just means commit.
> > 
> > i guess i just favor flush being simply a request to synchronize the context
> > (either blocking or non-blocking).
> 
> I did. It seemed that these were more like parameters to a particular call to
> flush than persistent properties of the context. I'm open to the idea of
> changing it. sehr?

The 'dirty' rect in Context2D seems equivalent.  It is meaningful only in the
context of a particular flush call.

What I'm pushing for is consistency.  If we want to move to using parameters
for flush, then we should do so across the board.  I'm not convinced that we
want to do that though.

-Darin

[darin (slow to review), 2010-01-27 21:02:55]

On 2010/01/27 21:00:55, darin wrote:
> On 2010/01/27 19:04:50, apatrick_chromium wrote:
...
> > At the moment, a flush in 3D is always asynchronous and you call glFlush()
to
> > get a synchronous flush. There is no way to set a callback for the
> asynchronous
> > flush. I don't think OpenGL has a way to do that on the service side.

When we talked about this way back when, you told me that flush could be
acknowledged when the consumer advanced the getOffset.  So, it seems that the
behavior of the OpenGL API is not relevant.  Don't we know when we have consumed
part of the command buffer stream?

[apatrick, 2010-01-27 21:17:01]

>
> When we talked about this way back when, you told me that flush could be
> acknowledged when the consumer advanced the getOffset.  So, it seems that
> the
> behavior of the OpenGL API is not relevant.  Don't we know when we have
> consumed
> part of the command buffer stream?
>
> It depends how deep into the pipeline you want the synchronous flush to go.
We could have a callback when the GPU service has flushed everything to
OpenGL. But OpenGL is itself asynchronous. AFAIK, the only way to
synchronize with OpenGL is to call glFlush or glFinish, which block. There's
no callback option. That's what I meant. It seems somewhat arbitrary to have
a synchronize with progress halfway through the pipeline. As a programmer I
think I would expect a synchronous flush to mean everything I had rendered
was on the screen.

[darin (slow to review), 2010-01-27 21:25:41]

On Wed, Jan 27, 2010 at 1:16 PM, Alastair Patrick <apatrick@google.com>wrote:

> When we talked about this way back when, you told me that flush could be
>> acknowledged when the consumer advanced the getOffset.  So, it seems that
>> the
>> behavior of the OpenGL API is not relevant.  Don't we know when we have
>> consumed
>> part of the command buffer stream?
>>
>> It depends how deep into the pipeline you want the synchronous flush to
> go. We could have a callback when the GPU service has flushed everything to
> OpenGL. But OpenGL is itself asynchronous. AFAIK, the only way to
> synchronize with OpenGL is to call glFlush or glFinish, which block. There's
> no callback option. That's what I meant. It seems somewhat arbitrary to have
> a synchronize with progress halfway through the pipeline. As a programmer I
> think I would expect a synchronous flush to mean everything I had rendered
> was on the screen.
>

I agree that it is better to get a signal of when the screen has been
updated.  That would be much more ideal.

The intent of flush / flushcallback in pepper was that you would call flush
when you want to push your state to the device.  Flush would complete
(either synchronously if no callback is specified or asynchronously via the
callback) to indicate that it is okay to call flush again.  From what you
are saying it seems like we would implement this using glFlush / glFinish
calls in the GPU process.  Because it is a different thread than the pepper
thread, it seems trivial to support both Flush modes in the pepper thread.

What am I missing?

-Darin

[apatrick, 2010-01-27 22:39:35]

Oh if the semantics are that synchronous just means that it safe to
immediately flush again and that asynchronous means you have to wait for a
callback first then that should be no problem. In that sense, it is
currently synchronous. It is always safe to flush.

Supporting a synchronous flush that means the output is on the screen is a
bad idea I think. It could easily cripple the performance, not just of 3D
rendering from that plugin instance but for all 3D rendering on the page.
The rendering thread in the GPU process could easily block for a couple of
frames, or 33ms.

On Wed, Jan 27, 2010 at 1:25 PM, Darin Fisher <darin@chromium.org> wrote:

> On Wed, Jan 27, 2010 at 1:16 PM, Alastair Patrick <apatrick@google.com>wrote:
>
>> When we talked about this way back when, you told me that flush could be
>>> acknowledged when the consumer advanced the getOffset.  So, it seems that
>>> the
>>> behavior of the OpenGL API is not relevant.  Don't we know when we have
>>> consumed
>>> part of the command buffer stream?
>>>
>>> It depends how deep into the pipeline you want the synchronous flush to
>> go. We could have a callback when the GPU service has flushed everything to
>> OpenGL. But OpenGL is itself asynchronous. AFAIK, the only way to
>> synchronize with OpenGL is to call glFlush or glFinish, which block. There's
>> no callback option. That's what I meant. It seems somewhat arbitrary to have
>> a synchronize with progress halfway through the pipeline. As a programmer I
>> think I would expect a synchronous flush to mean everything I had rendered
>> was on the screen.
>>
>
> I agree that it is better to get a signal of when the screen has been
> updated.  That would be much more ideal.
>
> The intent of flush / flushcallback in pepper was that you would call flush
> when you want to push your state to the device.  Flush would complete
> (either synchronously if no callback is specified or asynchronously via the
> callback) to indicate that it is okay to call flush again.  From what you
> are saying it seems like we would implement this using glFlush / glFinish
> calls in the GPU process.  Because it is a different thread than the pepper
> thread, it seems trivial to support both Flush modes in the pepper thread.
>
> What am I missing?
>
> -Darin
>

[apatrick_chromium, 2010-01-28 00:54:13]

I got rid of the changes to the flushContext signature. As gman suggested, I
made all errors fatal, so there is no need for the reset parameter. I also
realized that updateOnly was redundant. A status update is just a flush after no
change to the put offset.

[darin (slow to review), 2010-01-28 06:22:37]

On Wed, Jan 27, 2010 at 2:39 PM, Alastair Patrick <apatrick@google.com>wrote:

> Oh if the semantics are that synchronous just means that it safe to
> immediately flush again and that asynchronous means you have to wait for a
> callback first then that should be no problem. In that sense, it is
> currently synchronous. It is always safe to flush.
>

OK, cool.



>
> Supporting a synchronous flush that means the output is on the screen is a
> bad idea I think. It could easily cripple the performance, not just of 3D
> rendering from that plugin instance but for all 3D rendering on the page.
> The rendering thread in the GPU process could easily block for a couple of
> frames, or 33ms.
>
>
Ah, I was wondering what glFlush / glFinish actually meant.  Thanks.

-Darin



>
> On Wed, Jan 27, 2010 at 1:25 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> On Wed, Jan 27, 2010 at 1:16 PM, Alastair Patrick <apatrick@google.com>wrote:
>>
>>> When we talked about this way back when, you told me that flush could be
>>>> acknowledged when the consumer advanced the getOffset.  So, it seems
>>>> that the
>>>> behavior of the OpenGL API is not relevant.  Don't we know when we have
>>>> consumed
>>>> part of the command buffer stream?
>>>>
>>>> It depends how deep into the pipeline you want the synchronous flush to
>>> go. We could have a callback when the GPU service has flushed everything to
>>> OpenGL. But OpenGL is itself asynchronous. AFAIK, the only way to
>>> synchronize with OpenGL is to call glFlush or glFinish, which block. There's
>>> no callback option. That's what I meant. It seems somewhat arbitrary to have
>>> a synchronize with progress halfway through the pipeline. As a programmer I
>>> think I would expect a synchronous flush to mean everything I had rendered
>>> was on the screen.
>>>
>>
>> I agree that it is better to get a signal of when the screen has been
>> updated.  That would be much more ideal.
>>
>> The intent of flush / flushcallback in pepper was that you would call
>> flush when you want to push your state to the device.  Flush would complete
>> (either synchronously if no callback is specified or asynchronously via the
>> callback) to indicate that it is okay to call flush again.  From what you
>> are saying it seems like we would implement this using glFlush / glFinish
>> calls in the GPU process.  Because it is a different thread than the pepper
>> thread, it seems trivial to support both Flush modes in the pepper thread.
>>
>> What am I missing?
>>
>> -Darin
>>
>
>

[darin (slow to review), 2010-01-28 06:24:28]

http://codereview.chromium.org/555020/diff/4069/3062
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/555020/diff/4069/3062#newcode288
third_party/npapi/bindings/npapi_extensions.h:288: // Last processed token.
is this also "Synchronized on flush"?  it looks like parseError is.  is token
just an output field?

[darin (slow to review), 2010-01-28 06:25:09]

Thanks! -Darin

On Wed, Jan 27, 2010 at 4:54 PM, <apatrick@chromium.org> wrote:

> I got rid of the changes to the flushContext signature. As gman suggested,
> I
> made all errors fatal, so there is no need for the reset parameter. I also
> realized that updateOnly was redundant. A status update is just a flush
> after no
> change to the put offset.
>
>
> http://codereview.chromium.org/555020
>

[apatrick_chromium, 2010-01-28 19:40:07]

http://codereview.chromium.org/555020/diff/4069/3062
File third_party/npapi/bindings/npapi_extensions.h (right):

http://codereview.chromium.org/555020/diff/4069/3062#newcode288
third_party/npapi/bindings/npapi_extensions.h:288: // Last processed token.
On 2010/01/28 06:24:28, darin wrote:
> is this also "Synchronized on flush"?  it looks like parseError is.  is token
> just an output field?

Done.

[darin (slow to review), 2010-01-28 21:12:14]

npapi_extensions.h LGTM

[apatrick_chromium, 2010-01-28 23:28:42]

gman, can you review again? So much has changed.

The last change (set 8) was to rename CommandBuffer::Status to State. One of the
X Windows glew headers #defines Status.

[greggman, 2010-01-29 02:33:18]

checking. (sorry I didn't notice this email till now)

On Thu, Jan 28, 2010 at 3:28 PM, <apatrick@chromium.org> wrote:

> gman, can you review again? So much has changed.
>
> The last change (set 8) was to rename CommandBuffer::Status to State. One
> of the
> X Windows glew headers #defines Status.
>
>
> http://codereview.chromium.org/555020
>

[greggman, 2010-01-29 02:39:59]

LGTM