Add GL_EXT_multisampled_render_to_texture support to command buffer.

Add GL_EXT_multisampled_render_to_texture support to command buffer.

BUG=257113
R=apatrick@chromium.org, piman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211393

[bsalomon, 2013-07-08 20:14:16]

Hi guys, git cl suggested you as reviewers but feel free to redirect. The
motivation is to use this for canvas2d.

[piman, 2013-07-08 21:06:24]

lgtm

[apatrick_chromium, 2013-07-08 21:09:29]

On 2013/07/08 21:06:24, piman wrote:
> lgtm

Wait! I think it exposes undefined data to clients.

[apatrick_chromium, 2013-07-08 21:14:24]

I mean this:

"When the texture level is used as a source or destination for any operation,
the attachment is flushed, or when the attachment is broken, an implicit resolve
of multisample data from the multisample buffer to the texture level may be
performed. After such a resolve, the contents of the multisample buffer become
undefined.

The operations which may cause a resolve include:
    - Drawing with the texture bound to an active texture unit
    - ReadPixels or CopyTex[Sub]Image* while the texture is 
      attached to the framebuffer
    - CopyTex[Sub]Image*, Tex[Sub]Image*, 
      CompressedTex[Sub]Image* with the specified level as 
      destination
    - GenerateMipmap
    - Flush or Finish while the texture is attached to the
      framebuffer
    - BindFramebuffer while the texture is attached to the currently
      bound framebuffer."

In what cases, if any, might a client be able to access the undefined data?
Maybe we need to mark the attachment as uncleared somewhere.

[piman, 2013-07-08 21:26:34]

On Mon, Jul 8, 2013 at 2:14 PM, <apatrick@chromium.org> wrote:

> I mean this:
>
> "When the texture level is used as a source or destination for any
> operation,
> the attachment is flushed, or when the attachment is broken, an implicit
> resolve
> of multisample data from the multisample buffer to the texture level may be
> performed. After such a resolve, the contents of the multisample buffer
> become
> undefined.
>
> The operations which may cause a resolve include:
>     - Drawing with the texture bound to an active texture unit
>     - ReadPixels or CopyTex[Sub]Image* while the texture is
>       attached to the framebuffer
>     - CopyTex[Sub]Image*, Tex[Sub]Image*,
>       CompressedTex[Sub]Image* with the specified level as
>       destination
>     - GenerateMipmap
>     - Flush or Finish while the texture is attached to the
>       framebuffer
>     - BindFramebuffer while the texture is attached to the currently
>       bound framebuffer."
>
> In what cases, if any, might a client be able to access the undefined data?
> Maybe we need to mark the attachment as uncleared somewhere.
>

That would be surprising.
I don't think the contents is lost, it's just that the sample buffer is
(may be) discarded, meaning it will be refilled with the (resolved) pixel
data, i.e. only losing the per-sample info (in the MS buffer), but keeping
the per-pixel data (in the texture level)


>
>
https://codereview.chromium.**org/18492005/<https://codereview.chromium.org/1...
>

[apatrick_chromium, 2013-07-08 21:28:03]

GL specs like to imply things by omission :) See inline.

https://codereview.chromium.org/18492005/diff/2001/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/18492005/diff/2001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4947: if
(features().use_img_for_multisampled_render_to_texture) {
Does it need something like this? I can't find anything in the spec about the
texture being copied into the implicit multisample buffer.

framebuffer->MarkAttachmentAsCleared(renderbuffer_manager(),
                                     texture_manager(),
                                     attachment,
                                     false);

[apatrick_chromium, 2013-07-08 21:29:59]

Also, why would the spec mention that the multisample buffer becomes undefined
in some cases if that is semantically irrelevant, i.e. invisible to the client.

[piman, 2013-07-08 21:40:08]

On Mon, Jul 8, 2013 at 2:29 PM, <apatrick@chromium.org> wrote:

> Also, why would the spec mention that the multisample buffer becomes
> undefined
> in some cases if that is semantically irrelevant, i.e. invisible to the
> client.
>

Maybe you're right, but in that case, we can't simply "clear". The only way
to clear the MS buffer is to clear the texture, which then has a visible
side effect.


>
>
https://codereview.chromium.**org/18492005/<https://codereview.chromium.org/1...
>

[apatrick_chromium, 2013-07-08 21:47:21]

On 2013/07/08 21:40:08, piman wrote:
> On Mon, Jul 8, 2013 at 2:29 PM, <mailto:apatrick@chromium.org> wrote:
> 
> > Also, why would the spec mention that the multisample buffer becomes
> > undefined
> > in some cases if that is semantically irrelevant, i.e. invisible to the
> > client.
> >
> 
> Maybe you're right, but in that case, we can't simply "clear". The only way
> to clear the MS buffer is to clear the texture, which then has a visible
> side effect.
> 
> 
> >
> >
>
https://codereview.chromium.**org/18492005/%3Chttps://codereview.chromium.org...>
> >

If the contents of the multisample buffer are undefined anyway then it's okay to
clear it, no? There's already code to clear framebuffer attachments if they are
potentially undefined.

I thought there was also code to avoid clearing if it would be redundant, e.g.
if the first thing you do to the FBO is glClear, but I can't find that it.

See the implementation of GLES2DecoderImpl::DoDiscardFramebufferEXT.

[piman, 2013-07-08 21:50:46]

On 2013/07/08 21:47:21, apatrick_chromium wrote:
> On 2013/07/08 21:40:08, piman wrote:
> > On Mon, Jul 8, 2013 at 2:29 PM, <mailto:apatrick@chromium.org> wrote:
> > 
> > > Also, why would the spec mention that the multisample buffer becomes
> > > undefined
> > > in some cases if that is semantically irrelevant, i.e. invisible to the
> > > client.
> > >
> > 
> > Maybe you're right, but in that case, we can't simply "clear". The only way
> > to clear the MS buffer is to clear the texture, which then has a visible
> > side effect.
> > 
> > 
> > >
> > >
> >
>
https://codereview.chromium.**org/18492005/%253Chttps://codereview.chromium.o...>
> > >
> 
> If the contents of the multisample buffer are undefined anyway then it's okay
to
> clear it, no?

Is it?
Say I have a 10x10 texture, I render to it, and a resolve happens, discarding
the MS buffer. Now I write 5x5 of that texture with an opaque quad. Are the rest
of the pixels undefined now? Why?

> There's already code to clear framebuffer attachments if they are
> potentially undefined.
> 
> I thought there was also code to avoid clearing if it would be redundant, e.g.
> if the first thing you do to the FBO is glClear, but I can't find that it.
> 
> See the implementation of GLES2DecoderImpl::DoDiscardFramebufferEXT.

glDiscardFramebufferEXT discards the actual pixels, not the sample buffer, so
it's a different thing.

Antoine

[apatrick_chromium, 2013-07-08 22:04:01]

On 2013/07/08 21:50:46, piman wrote:
> On 2013/07/08 21:47:21, apatrick_chromium wrote:
> > On 2013/07/08 21:40:08, piman wrote:
> > > On Mon, Jul 8, 2013 at 2:29 PM, <mailto:apatrick@chromium.org> wrote:
> > > 
> > > > Also, why would the spec mention that the multisample buffer becomes
> > > > undefined
> > > > in some cases if that is semantically irrelevant, i.e. invisible to the
> > > > client.
> > > >
> > > 
> > > Maybe you're right, but in that case, we can't simply "clear". The only
way
> > > to clear the MS buffer is to clear the texture, which then has a visible
> > > side effect.
> > > 
> > > 
> > > >
> > > >
> > >
> >
>
https://codereview.chromium.**org/18492005/%25253Chttps://codereview.chromium...>
> > > >
> > 
> > If the contents of the multisample buffer are undefined anyway then it's
okay
> to
> > clear it, no?
> 
> Is it?
> Say I have a 10x10 texture, I render to it, and a resolve happens, discarding
> the MS buffer. Now I write 5x5 of that texture with an opaque quad. Are the
rest
> of the pixels undefined now? Why?
> 

It's one way, not necessarily the right way, to interpret this:

"When the texture level is used as a source or destination for any operation,
the attachment is flushed, or when the attachment is broken, an implicit resolve
of multisample data from the multisample buffer to the texture level may be
performed. After such a resolve, the contents of the multisample buffer become
undefined."

I read this as when you break the attachment, e.g. attach some other color
attachment to the FBO, the multisample buffer becomes undefined. Then, if you
cause it to resolve, some of the texture might be undefined. So you would have
to render to every pixel to ensure everything was defined before using the
texture.

I guess the question is whether the texture is copied into the multisample
buffer when it is attached.

> > There's already code to clear framebuffer attachments if they are
> > potentially undefined.
> > 
> > I thought there was also code to avoid clearing if it would be redundant,
e.g.
> > if the first thing you do to the FBO is glClear, but I can't find that it.
> > 
> > See the implementation of GLES2DecoderImpl::DoDiscardFramebufferEXT.
> 
> glDiscardFramebufferEXT discards the actual pixels, not the sample buffer, so
> it's a different thing.
> 

Yeah sorry, I mean for the usage of Framebuffer::MarkAttachmentAsCleared.

Do we have one of these GPUs? It might make a good gl_test.

> Antoine

[piman, 2013-07-08 22:15:47]

On Mon, Jul 8, 2013 at 3:04 PM, <apatrick@chromium.org> wrote:

> On 2013/07/08 21:50:46, piman wrote:
>
>> On 2013/07/08 21:47:21, apatrick_chromium wrote:
>> > On 2013/07/08 21:40:08, piman wrote:
>> > > On Mon, Jul 8, 2013 at 2:29 PM, <mailto:apatrick@chromium.org> wrote:
>> > >
>> > > > Also, why would the spec mention that the multisample buffer becomes
>> > > > undefined
>> > > > in some cases if that is semantically irrelevant, i.e. invisible to
>> the
>> > > > client.
>> > > >
>> > >
>> > > Maybe you're right, but in that case, we can't simply "clear". The
>> only
>>
> way
>
>> > > to clear the MS buffer is to clear the texture, which then has a
>> visible
>> > > side effect.
>> > >
>> > >
>> > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.****org/18492005/%25253Chttps://co**
> dereview.chromium.org/**18492005/<http://codereview.chromium.org/18492005/>
> >
>
>  > > >
>> >
>> > If the contents of the multisample buffer are undefined anyway then it's
>>
> okay
>
>> to
>> > clear it, no?
>>
>
>  Is it?
>> Say I have a 10x10 texture, I render to it, and a resolve happens,
>> discarding
>> the MS buffer. Now I write 5x5 of that texture with an opaque quad. Are
>> the
>>
> rest
>
>> of the pixels undefined now? Why?
>>
>
>
> It's one way, not necessarily the right way, to interpret this:
>
>
> "When the texture level is used as a source or destination for any
> operation,
> the attachment is flushed, or when the attachment is broken, an implicit
> resolve
> of multisample data from the multisample buffer to the texture level may be
> performed. After such a resolve, the contents of the multisample buffer
> become
> undefined."
>
> I read this as when you break the attachment, e.g. attach some other color
> attachment to the FBO, the multisample buffer becomes undefined. Then, if
> you
> cause it to resolve, some of the texture might be undefined. So you would
> have
> to render to every pixel to ensure everything was defined before using the
> texture.
>
> I guess the question is whether the texture is copied into the multisample
> buffer when it is attached.


I'm even more worried about the implicit resolves, e.g. with BindFramebuffer
or Flush. A literal reading of the spec would mean that 2 implicit resolves
in a row would make all pixels undefined. These could even happen under the
hood from the client pov, e.g. on a context switch with virtual context, or
with another context in the share group sampling the texture. If that is
the case, I fear this extension will be unusable as is for us.


>
>
>  > There's already code to clear framebuffer attachments if they are
>> > potentially undefined.
>> >
>> > I thought there was also code to avoid clearing if it would be
>> redundant,
>>
> e.g.
>
>> > if the first thing you do to the FBO is glClear, but I can't find that
>> it.
>> >
>> > See the implementation of GLES2DecoderImpl::**DoDiscardFramebufferEXT.
>>
>
>  glDiscardFramebufferEXT discards the actual pixels, not the sample
>> buffer, so
>> it's a different thing.
>>
>
>
> Yeah sorry, I mean for the usage of Framebuffer::**
> MarkAttachmentAsCleared.
>
> Do we have one of these GPUs? It might make a good gl_test.
>
>  Antoine
>>
>
>
>
https://codereview.chromium.**org/18492005/<https://codereview.chromium.org/1...
>

[piman, 2013-07-08 22:19:13]

On Mon, Jul 8, 2013 at 3:15 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Mon, Jul 8, 2013 at 3:04 PM, <apatrick@chromium.org> wrote:
>
>> On 2013/07/08 21:50:46, piman wrote:
>>
>>> On 2013/07/08 21:47:21, apatrick_chromium wrote:
>>> > On 2013/07/08 21:40:08, piman wrote:
>>> > > On Mon, Jul 8, 2013 at 2:29 PM, <mailto:apatrick@chromium.org>
>>> wrote:
>>> > >
>>> > > > Also, why would the spec mention that the multisample buffer
>>> becomes
>>> > > > undefined
>>> > > > in some cases if that is semantically irrelevant, i.e. invisible
>>> to the
>>> > > > client.
>>> > > >
>>> > >
>>> > > Maybe you're right, but in that case, we can't simply "clear". The
>>> only
>>>
>> way
>>
>>> > > to clear the MS buffer is to clear the texture, which then has a
>>> visible
>>> > > side effect.
>>> > >
>>> > >
>>> > > >
>>> > > >
>>> > >
>>> >
>>>
>>
>> https://codereview.chromium.****org/18492005/%25253Chttps://co**
>> dereview.chromium.org/**18492005/<http://codereview.chromium.org/18492005/>
>> >
>>
>>  > > >
>>> >
>>> > If the contents of the multisample buffer are undefined anyway then
>>> it's
>>>
>> okay
>>
>>> to
>>> > clear it, no?
>>>
>>
>>  Is it?
>>> Say I have a 10x10 texture, I render to it, and a resolve happens,
>>> discarding
>>> the MS buffer. Now I write 5x5 of that texture with an opaque quad. Are
>>> the
>>>
>> rest
>>
>>> of the pixels undefined now? Why?
>>>
>>
>>
>> It's one way, not necessarily the right way, to interpret this:
>>
>>
>> "When the texture level is used as a source or destination for any
>> operation,
>> the attachment is flushed, or when the attachment is broken, an implicit
>> resolve
>> of multisample data from the multisample buffer to the texture level may
>> be
>> performed. After such a resolve, the contents of the multisample buffer
>> become
>> undefined."
>>
>> I read this as when you break the attachment, e.g. attach some other color
>> attachment to the FBO, the multisample buffer becomes undefined. Then, if
>> you
>> cause it to resolve, some of the texture might be undefined. So you would
>> have
>> to render to every pixel to ensure everything was defined before using the
>> texture.
>>
>> I guess the question is whether the texture is copied into the multisample
>> buffer when it is attached.
>
>
> I'm even more worried about the implicit resolves, e.g. with BindFramebuffer
> or Flush. A literal reading of the spec would mean that 2 implicit resolves
> in a row would make all pixels undefined. These could even happen under the
> hood from the client pov, e.g. on a context switch with virtual context, or
> with another context in the share group sampling the texture. If that is
> the case, I fear this extension will be unusable as is for us.
>

Well, I guess a possibility is to manually restore the MS buffer by drawing
the texture onto itself (we may need to double-buffer or something) when we
think the MS buffer has been lost, to restore normal MS semantics. But I
think that'll be overly costly.

>
>
>>
>>
>>  > There's already code to clear framebuffer attachments if they are
>>> > potentially undefined.
>>> >
>>> > I thought there was also code to avoid clearing if it would be
>>> redundant,
>>>
>> e.g.
>>
>>> > if the first thing you do to the FBO is glClear, but I can't find that
>>> it.
>>> >
>>> > See the implementation of GLES2DecoderImpl::**DoDiscardFramebufferEXT.
>>>
>>
>>  glDiscardFramebufferEXT discards the actual pixels, not the sample
>>> buffer, so
>>> it's a different thing.
>>>
>>
>>
>> Yeah sorry, I mean for the usage of Framebuffer::**
>> MarkAttachmentAsCleared.
>>
>> Do we have one of these GPUs? It might make a good gl_test.
>>
>>  Antoine
>>>
>>
>>
>>
https://codereview.chromium.**org/18492005/<https://codereview.chromium.org/1...
>>
>
>

[apatrick_chromium, 2013-07-08 22:38:09]

> >> It's one way, not necessarily the right way, to interpret this:
> >>
> >>
> >> "When the texture level is used as a source or destination for any
> >> operation,
> >> the attachment is flushed, or when the attachment is broken, an implicit
> >> resolve
> >> of multisample data from the multisample buffer to the texture level may
> >> be
> >> performed. After such a resolve, the contents of the multisample buffer
> >> become
> >> undefined."
> >>
> >> I read this as when you break the attachment, e.g. attach some other color
> >> attachment to the FBO, the multisample buffer becomes undefined. Then, if
> >> you
> >> cause it to resolve, some of the texture might be undefined. So you would
> >> have
> >> to render to every pixel to ensure everything was defined before using the
> >> texture.
> >>
> >> I guess the question is whether the texture is copied into the multisample
> >> buffer when it is attached.
> >
> >
> > I'm even more worried about the implicit resolves, e.g. with BindFramebuffer
> > or Flush. A literal reading of the spec would mean that 2 implicit resolves
> > in a row would make all pixels undefined. These could even happen under the
> > hood from the client pov, e.g. on a context switch with virtual context, or
> > with another context in the share group sampling the texture. If that is
> > the case, I fear this extension will be unusable as is for us.
> >
> 
> Well, I guess a possibility is to manually restore the MS buffer by drawing
> the texture onto itself (we may need to double-buffer or something) when we
> think the MS buffer has been lost, to restore normal MS semantics. But I
> think that'll be overly costly.
> 

This wording is clearer IMHO:

"When the texture level is flushed or used as a source or destination for any
operation other than drawing to it, an implicit resolve of multisampled color
data may be performed. After such a resolve, the multisampled color data is
discarded."

And it says later that something as innocuous as a glFlush is sufficient to
cause such a resolve :( I don't know if we can use this extension effectively in
Chrome without incurring the kind of overhead that this extension is intended to
avoid in the first place.

[apatrick_chromium, 2013-07-08 23:23:39]

Here is a link to an AMD presentation on tile based renderers:

http://developer.amd.com/wordpress/media/2012/10/gdc2008_ribble_maurice_TileB...

It suggest that "heavy-weight" resolves can be avoided by ensuring that the
first draw call after a glBindFramebuffer is glClear. Is that what we are trying
to achieve here? Is there reason to believe that IMG does not also have this
optimization? Maybe it doesn't work with multisampling? Or maybe the issue is
that without GL_EXT_multisampled_render_to_texture, resolving requires calls to
heavy-weight resolve in order to set things up for glBlitFramebuffer?

[bsalomon, 2013-07-09 13:11:53]

On 2013/07/08 23:23:39, apatrick_chromium wrote:
> Here is a link to an AMD presentation on tile based renderers:
> 
>
http://developer.amd.com/wordpress/media/2012/10/gdc2008_ribble_maurice_TileB...
> 
> It suggest that "heavy-weight" resolves can be avoided by ensuring that the
> first draw call after a glBindFramebuffer is glClear. Is that what we are
trying
> to achieve here? Is there reason to believe that IMG does not also have this
> optimization? Maybe it doesn't work with multisampling? Or maybe the issue is
> that without GL_EXT_multisampled_render_to_texture, resolving requires calls
to
> heavy-weight resolve in order to set things up for glBlitFramebuffer?

Hey guys, sorry for putting this up and then being away and missing the
discussion. It seems very difficult to determine from the spec exactly when we
would have to clear the sample buffer to avoid uninit data in the sample buffer.
I'm going to try to bring in people from IMG and ARM to see if they can clarify
under what conditions (if any) there might be uninit data in the sample buffer.

[apatrick_chromium, 2013-07-10 20:38:13]

With information from IMG, LGTM.

[bsalomon, 2013-07-12 14:13:20]

Committed patchset #3 manually as r211393 (presubmit successful).