[Mesa-dev] [PATCH] Revert "mesa: check depth, stencil formats (not depths) in glBlitFramebuffer"

[Brian Paul]

On 01/17/2012 11:00 PM, Kenneth Graunke wrote:
> On 01/17/2012 05:27 PM, Ian Romanick wrote:
>> On 01/17/2012 05:12 PM, Chad Versace wrote:
>>> This reverts commit 3f1fab06844f696de44d9a56e83ff62e8ea576bd.
>>>
>>> This loosens the format validation in glBlitFramebuffer. When blitting
>>> depth bits, don't require an exact match between the depth formats;
>>> only
>>> require that the two formats have the same number of depth bits. Ditto
>>> for
>>> stencil.
>>>
>>> Fixes Piglit fbo/fbo-blit-d24s8 on Intel drivers with separate stencil
>>> enabled.
>>>
>>> The problem was that, on Intel drivers with separate stencil, the
>>> default
>>> framebuffer has separate depth and stencil buffers with formats X8_Z24
>>> and
>>> S8. The test attempts to blit the depth bits from a S8_Z24 buffer into
>>> the
>>> default framebuffer.
>>>
>>> Between S8_Z24 buffers, the EXT_framebuffer_blit spec allows
>>> glBlitFramebuffer to blit the depth and stencil bits separately. So
>>> I see
>>> no reason to prevent blitting the depth bits between X8_Z24 and
>>> S8_Z24 or
>>> the stencil bits between S8 and S8_Z24.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44665
>>> Note: This is a candidate for the 8.0 branch.
>>> CC: Brian Paul<brianp at wmware.com>
>>> Reported-by: Xunx Fang<xunx.fang at intel.com>
>>> Signed-off-by: Chad Versace<chad.versace at linux.intel.com>
>>> ---
>>> src/mesa/main/fbobject.c | 10 ++++++----
>>> 1 files changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
>>> index 0524959..9a5e780 100644
>>> --- a/src/mesa/main/fbobject.c
>>> +++ b/src/mesa/main/fbobject.c
>>> @@ -2709,9 +2709,10 @@ _mesa_BlitFramebufferEXT(GLint srcX0, GLint
>>> srcY0, GLint srcX1, GLint srcY1,
>>> if ((readRb == NULL) || (drawRb == NULL)) {
>>> mask&= ~GL_STENCIL_BUFFER_BIT;
>>> }
>>> - else if (readRb->Format != drawRb->Format) {
>>> + else if (_mesa_get_format_bits(readRb->Format, GL_STENCIL_BITS) !=
>>> + _mesa_get_format_bits(drawRb->Format, GL_STENCIL_BITS)) {
>>> _mesa_error(ctx, GL_INVALID_OPERATION,
>>> - "glBlitFramebufferEXT(stencil buffer format mismatch)");
>>> + "glBlitFramebufferEXT(stencil buffer size mismatch)");
>>> return;
>>> }
>>> }
>>> @@ -2731,9 +2732,10 @@ _mesa_BlitFramebufferEXT(GLint srcX0, GLint
>>> srcY0, GLint srcX1, GLint srcY1,
>>> if ((readRb == NULL) || (drawRb == NULL)) {
>>> mask&= ~GL_DEPTH_BUFFER_BIT;
>>> }
>>> - else if (readRb->Format != drawRb->Format) {
>>> + else if (_mesa_get_format_bits(readRb->Format, GL_DEPTH_BITS) !=
>>> + _mesa_get_format_bits(drawRb->Format, GL_DEPTH_BITS)) {
>>
>> I think the check needs to be slightly stronger than this. This would
>> allow Z32 and Z32_FLOAT which is outside the intention of the spec.
>
> Do we want to allow "X-types", like S8Z24->X8Z24 and

Yes, I think we want to support that type of case.


> ARGB8888->XRGB8888,

Yes, the spec explicitly allows color format conversion:

    "If the color formats of the read and draw framebuffers do not
     match, and <mask> includes COLOR_BUFFER_BIT, the pixel groups are
     converted to match the destination format as in CopyPixels, except
     that no pixel transfer operations apply and clamping behaves as if
     CLAMP_FRAGMENT_COLOR_ARB is set to FIXED_ONLY_ARB."

It's the Z / stencil case that's a bit vague and the reason why I 
changed the test in the first place:

    "Calling BlitFramebufferEXT will result in an INVALID_OPERATION
     error if <mask> includes DEPTH_BUFFER_BIT or STENCIL_BUFFER_BIT
     and the source and destination depth and stencil buffer formats do
     not match."


> but not general format mismatches (like
> Z32<->Z32_FLOAT)?

That should probably not be allowed (but it would be interesting to 
test with an AMD or NVIDIA driver to see what they do).

I think you just need to test _mesa_get_format_datatype(src) == 
_mesa_get_format_datatype(dst) in addition to the depth comparison.


>
> Thinking about FBOs...I could see raising an error if the user
> explicitly made ARGB8888 and XRGB8888 FBOs, but if they chose "RGB"
> and the driver internally decided to use one or the other, it seems
> wrong to throw an error.

-Brian