[Mesa-dev] [PATCH] mesa: Fix render buffer initial internal format type

[Brian Paul]

On 01/13/2015 06:09 PM, Chad Versace wrote:
> Brian and Ian, me and Michael have a Mesa TLS question at the message's bottom.
>
>
> On 01/13/2015 04:41 PM, Mason, Michael W wrote:
>>> -----Original Message-----
>>> From: Versace, Chad
>>> Sent: Tuesday, January 13, 2015 3:42 PM
>>> To: Mason, Michael W; mesa-dev at lists.freedesktop.org
>>> Subject: Re: [Mesa-dev] [PATCH] mesa: Fix render buffer initial internal format type
>>>
>>> On 01/13/2015 02:34 PM, Mason, Michael W wrote:
>>>>> -----Original Message-----
>>>>> From: Versace, Chad
>>>>> Sent: Tuesday, January 13, 2015 10:47 AM
>>>>> To: Mason, Michael W; mesa-dev at lists.freedesktop.org
>>>>> Subject: Re: [Mesa-dev] [PATCH] mesa: Fix render buffer initial
>>>>> internal format type
>>>>>
>>>>> On 01/09/2015 05:21 PM, michael.w.mason at intel.com wrote:
>>>>>> From: Mike Mason <michael.w.mason at intel.com>
>>>>>>
>>>>>> Changes the initial internal format of a render buffer to GL_RGBA4.
>>>>>> This fixes a failure in the following DrawElements test:
>>>>>>
>>>>>> dEQP-GLES3.functional.state_query.rbo.renderbuffer_internal_format
>>>>>> ---
>>>>>>   src/mesa/main/renderbuffer.c | 6 +++++-
>>>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/src/mesa/main/renderbuffer.c
>>>>>> b/src/mesa/main/renderbuffer.c index 0bc7f2b..b339491 100644
>>>>>> --- a/src/mesa/main/renderbuffer.c
>>>>>> +++ b/src/mesa/main/renderbuffer.c
>>>>>> @@ -53,7 +53,11 @@ _mesa_init_renderbuffer(struct gl_renderbuffer *rb, GLuint name)
>>>>>>      rb->Width = 0;
>>>>>>      rb->Height = 0;
>>>>>>      rb->Depth = 0;
>>>>>> -   rb->InternalFormat = GL_RGBA;
>>>>>> +   /* Default internal format should be GL_RGBA4, per page 258,
>>>>>> +    * table 6.15 of the GLES 3.0.4 spec. Same default is expected
>>>>>> +    * in all OpenGL specs that support BindRenderbuffer().
>>>>>> +    */
>>>>>> +   rb->InternalFormat = GL_RGBA4;
>>>>>>      rb->Format = MESA_FORMAT_NONE;
>>>>>>   }
>>>>>>
>>>>>>
>>>>> The patch needs to choose the initial internalformat based on the
>>>>> context API.  Table 6.26 in the GL 3.3 Core spec says the initial
>>>>> renderbuffer internalformat is GL_RGBA. Table 6.15 of the GLES 3.0 spec says GL_RGBA4.
>>>>>
>>>>> I think this is the correct logic:
>>>>>
>>>>>    if (_mesa_is_desktop_gl(ctx)) {
>>>>>        rb->InternalFormat = GL_RGBA;
>>>>>    } else {
>>>>>        rb->InternalFormat = GL_RGBA4;
>>>>>    }
>>>>
>>>> "ctx" isn't available in _mesa_init_renderbuffer (where this code resides) and GET_CURRENT_CONTEXT(ctx) gives nil for
>>> ctx.  Is there any other way to get a pointer to the gl_context?
>>>
>>> Drivers call _mesa_init_renderbuffer for 3 reasons, as far as I can tell:
>>>
>>>      1. To initialize Mesa's gl_renderbuffer structure in response to a real GL
>>>         renderbuffer object created with glGenRenderbuffers.
>>>
>>>      2. To initialize a driver-private renderbuffer in response of the user
>>>         attaching a texture to a framebuffer object, such as with glFramebufferTexture2D.
>>>
>>>      3. To initialize a driver-private renderbuffer that serves as backing storage
>>>         for window system surfaces, such as eglCreateWindowSurface or a GLX Window.
>>>
>>> Only in case 1 will the user have a handle to the renderbuffer object and be able to call glGetRenderbufferParameteriv on
>>> it. In case 2, there is always a context present (I think...), but the driver will immediately overwrite the value of rb-
>>>> InternalFormat anyway, so it doesn't really matter what value _mesa_init_renderbuffer chooses. In case 3, there may or
>>> may not be a context present, but again the internalformat doesn't matter because the driver will immediately overwrite it.
>>>
>>>  From that follows:
>>>
>>>      - If a context is current, we are in case 1 or 2. Case 1 is the more restrictive
>>>        case. It requires that we select the internalformat based on the context's API.
>>>
>>>      - If a context is not current, we are in case 3, or possibly case 2 if I don't
>>>        fully understand the problem space. Either way, in neither case 2 nor 3 does
>>>        the value of internalformat matter, so we should just continue to initialize
>>>        it to GL_RGBA as we have always done.
>>>
>>
>> So would something like this be safe?  Is ctx guaranteed to be either nil or a pointer to a valid context?
>>
>>     GET_CURRENT_CONTEXT(ctx);
>>     if (ctx && _mesa_is_gles3(ctx)) {
>>        rb->InternalFormat = GL_RGBA4;
>>     } else {
>>        rb->InternalFormat = GL_RGBA;
>>     }
>
> Brian and Ian, _mesa_init_renderbuffer() sometimes gets called with a current context and
> sometimes not, because it's used to initialize winsys renderbuffers. So me and Michael
> have arrived at this question:
>
> If no context is current, does Mesa guarantee that GET_CURRENT_CONTEXT(ctx) sets ctx = NULL?

That's the intention and I've never seen otherwise.

-Brian