[Mesa-dev] [PATCH 01/12] mesa/es: Validate FBO attachment enum in Mesa code rather than the ES wrapper

[Ian Romanick]

On 10/03/2011 07:20 AM, Brian Paul wrote:
> On 10/02/2011 04:44 PM, Ian Romanick wrote:
>> From: Ian Romanick<ian.d.romanick at intel.com>
>>
>> Signed-off-by: Ian Romanick<ian.d.romanick at intel.com>
>> ---
>> src/mesa/main/APIspec.xml | 27 ---------------------------
>> src/mesa/main/fbobject.c | 7 ++++++-
>> 2 files changed, 6 insertions(+), 28 deletions(-)
>>
>> diff --git a/src/mesa/main/APIspec.xml b/src/mesa/main/APIspec.xml
>> index 0f82d0a..2b277a0 100644
>> --- a/src/mesa/main/APIspec.xml
>> +++ b/src/mesa/main/APIspec.xml
>> @@ -3217,15 +3217,6 @@
>> <value name="GL_FRAMEBUFFER" category="GLES2.0"/>
>> </desc>
>>
>> - <desc name="attachment">
>> - <value name="GL_COLOR_ATTACHMENT0_OES"
>> category="OES_framebuffer_object"/>
>> - <value name="GL_DEPTH_ATTACHMENT_OES"
>> category="OES_framebuffer_object"/>
>> - <value name="GL_STENCIL_ATTACHMENT_OES"
>> category="OES_framebuffer_object"/>
>> - <value name="GL_COLOR_ATTACHMENT0" category="GLES2.0"/>
>> - <value name="GL_DEPTH_ATTACHMENT" category="GLES2.0"/>
>> - <value name="GL_STENCIL_ATTACHMENT" category="GLES2.0"/>
>> - </desc>
>> -
>> <desc name="renderbuffertarget">
>> <value name="GL_RENDERBUFFER_OES" category="OES_framebuffer_object"/>
>> <value name="GL_RENDERBUFFER" category="GLES2.0"/>
>> @@ -3247,15 +3238,6 @@
>> <value name="GL_FRAMEBUFFER" category="GLES2.0"/>
>> </desc>
>>
>> - <desc name="attachment">
>> - <value name="GL_COLOR_ATTACHMENT0_OES"
>> category="OES_framebuffer_object"/>
>> - <value name="GL_DEPTH_ATTACHMENT_OES"
>> category="OES_framebuffer_object"/>
>> - <value name="GL_STENCIL_ATTACHMENT_OES"
>> category="OES_framebuffer_object"/>
>> - <value name="GL_COLOR_ATTACHMENT0" category="GLES2.0"/>
>> - <value name="GL_DEPTH_ATTACHMENT" category="GLES2.0"/>
>> - <value name="GL_STENCIL_ATTACHMENT" category="GLES2.0"/>
>> - </desc>
>> -
>> <desc name="textarget" error="GL_INVALID_OPERATION">
>> <value name="GL_TEXTURE_2D"/>
>> <value name="GL_TEXTURE_CUBE_MAP_POSITIVE_X" category="GLES2.0"/>
>> @@ -3292,15 +3274,6 @@
>> <value name="GL_FRAMEBUFFER" category="GLES2.0"/>
>> </desc>
>>
>> - <desc name="attachment">
>> - <value name="GL_COLOR_ATTACHMENT0_OES"
>> category="OES_framebuffer_object"/>
>> - <value name="GL_DEPTH_ATTACHMENT_OES"
>> category="OES_framebuffer_object"/>
>> - <value name="GL_STENCIL_ATTACHMENT_OES"
>> category="OES_framebuffer_object"/>
>> - <value name="GL_COLOR_ATTACHMENT0" category="GLES2.0"/>
>> - <value name="GL_DEPTH_ATTACHMENT" category="GLES2.0"/>
>> - <value name="GL_STENCIL_ATTACHMENT" category="GLES2.0"/>
>> - </desc>
>> -
>> <desc name="textarget" error="GL_INVALID_OPERATION">
>> <value name="GL_TEXTURE_3D_OES" category="OES_texture_3D"/>
>> </desc>
>> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
>> index 139ff03..2e0af97 100644
>> --- a/src/mesa/main/fbobject.c
>> +++ b/src/mesa/main/fbobject.c
>> @@ -239,17 +239,22 @@ _mesa_get_attachment(struct gl_context *ctx,
>> struct gl_framebuffer *fb,
>> case GL_COLOR_ATTACHMENT14_EXT:
>> case GL_COLOR_ATTACHMENT15_EXT:
>> i = attachment - GL_COLOR_ATTACHMENT0_EXT;
>> - if (i>= ctx->Const.MaxColorAttachments) {
>> + if (i>= ctx->Const.MaxColorAttachments
>> + || (i> 0&& ctx->API != API_OPENGL)) {
>> return NULL;
>> }
>
> Couldn't we just set ctx->Const.MaxColorAttachments = 1 for ES contexts
> to avoid changing this test (and get short loops elsewhere)?

I had thought about that, and I was on the fence.  My (slight) 
preference was to make a single change here rather than in every driver. 
  A future patch (07/12 of in this series) changes this check to only 
reject ES1 contexts.

The downside is possible interactions with (possible future) metaops code.

> Otherwise, these changes look good to me.
>
> Reviewed-by: Brian Paul <brianp at vmware.com>