Bug 87516 - glProgramBinary violates spec
Summary: glProgramBinary violates spec
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: git
Hardware: All All
: medium normal
Assignee: Ian Romanick
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-20 00:15 UTC by Leith Bade
Modified: 2015-01-11 23:03 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Leith Bade 2014-12-20 00:15:32 UTC
The current implementation of ARB_get_program_binary is incomplete and currently violates the spec.

Specifically these are the major issues:
* glGet with GL_NUM_PROGRAM_BINARY_FORMATS is not implemented, it should return a at least 1.
* glGet with GL_PROGRAM_BINARY_FORMATS is not implemented, it should return a list with a single binaryFormat.
* glGetProgramBinary succeeds with a binary length of zero, and also does not set binaryFormat to a valid value, it should always succeed and set binaryFormat to the value returned by GL_PROGRAM_BINARY_FORMATS.
* glProgramBinary always fails with GL_INVALID_OPERATION despite program being valid, it should always succeed if binaryFormat is correct, and return GL_INVALID_ENUM if not. Additionally it should always set GL_LINK_STATUS to GL_FALSE to indicate the program will need to load source anyway.

If these are not to be implemented ARB_program_binary and GL 4.1+ support should not be advertised by Mesa.
Comment 1 Ian Romanick 2014-12-20 01:42:36 UTC
(In reply to Leith Bade from comment #0)
> The current implementation of ARB_get_program_binary is incomplete and
> currently violates the spec.
> 
> Specifically these are the major issues:
> * glGet with GL_NUM_PROGRAM_BINARY_FORMATS is not implemented, it should
> return a at least 1.

The ARB_get_program_binary spec says:

    Get Value                       Type    Get Command          Minimum Value  Description                        Section
    -------------                   ----    -----------          -------------  -----------                        -------
    PROGRAM_BINARY_FORMATS          0* x Z  GetIntegerv          N/A            Enumerated program binary formats  2.14.3
    NUM_PROGRAM_BINARY_FORMATS      Z       GetIntegerv          0              Number of program binary formats   2.14.3

The Khronos conformance tests check for this and behave appropriately.  Your code should also check the number of available formats and behave appropriately.  Like ARB_texture_compression and ARB_multisample, this is an "ask if you can ask" extension.  You first have to ask if the extension is there, then you ask whether you can do anything with it.

> * glGet with GL_PROGRAM_BINARY_FORMATS is not implemented, it should return
> a list with a single binaryFormat.

Given the above, it should return zero formats.

> * glGetProgramBinary succeeds with a binary length of zero, and also does
> not set binaryFormat to a valid value, it should always succeed and set
> binaryFormat to the value returned by GL_PROGRAM_BINARY_FORMATS.

Hmm... the spec doesn't really provide any guidance here, and I think that's why it's implemented in this way.  Since there is no possible binary format, there are zero bytes of binary data.  Nothing in the extension spec or later OpenGL specs give any guidance for an error to be generated in this scenario.  There are no binary formats, so there is no value to write to binaryFormat.

It's really not clear what the correct behavior is... other than not call this function if there are no binary formats. :)

> * glProgramBinary always fails with GL_INVALID_OPERATION despite program
> being valid, it should always succeed if binaryFormat is correct, and return
> GL_INVALID_ENUM if not. Additionally it should always set GL_LINK_STATUS to
> GL_FALSE to indicate the program will need to load source anyway.

You are correct.  It should set link status to false and generate GL_INVALID_ENUM.

> If these are not to be implemented ARB_program_binary and GL 4.1+ support
> should not be advertised by Mesa.

FWIW, no driver in Mesa advertises a GL version higher than 3.3.  Note that this is also part of OpenGL ES 3.0, and at least the i965 driver has passed OpenGL ES 3.0 conformance testing.  The behavior of GL_NUM_PROGRAM_BINARY_FORMATS and GL_PROGRAM_BINARY_FORMATS may be annoying, but it is, strictly speaking, correct.
Comment 2 Leith Bade 2014-12-20 01:59:07 UTC
Yeah the spec is a bit ambiguous. It is almost as if they can't see why anyone would implement ARB_get_program_binary without supporting at least one binaryFormat. Of course they then made it core, so IDK.

OK I will add a check for GL_NUM_PROGRAM_BINARY_FORMATS on my code.

But the GL_INVALID_OPERATION is rather misleading.
Comment 4 Leith Bade 2014-12-21 22:08:13 UTC
Looks good.

Are you able to send feedback upstream to the GL ARB about the vagueness around having no support binaryFormats in the spec?
Comment 5 Ian Romanick 2015-01-11 23:03:05 UTC
Fixed by the following commits on Mesa master.  Once there is some feedback from Khronos on the spec bug, I'll post some updates.

commit f591712efeb9a757379d1e89907e2147749aaf6c
Author: Ian Romanick <ian.d.romanick@intel.com>
Date:   Sun Dec 21 12:06:23 2014 -0800

    mesa: Always generate GL_INVALID_OPERATION in _mesa_GetProgramBinary
    
    There are no binary formats supported, so what are you doing?  At least
    this gives the application developer some feedback about what's going
    on.  The spec gives no guidance about what to do in this scenario.
    
    Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87516
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
    Acked-by: Leight Bade <leith@mapbox.com>

commit 4fd8b3012371a5795a0d272928266c6237e57466
Author: Ian Romanick <ian.d.romanick@intel.com>
Date:   Sun Dec 21 12:03:57 2014 -0800

    mesa: Ensure that length is set to zero in _mesa_GetProgramBinary
    
    v2: Fix assignment of length.  Noticed by Julien Cristau.
    
    Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87516
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
    Acked-by: Leight Bade <leith@mapbox.com>

commit 201b9c181825551559f6d995007de8ff12d1a54c
Author: Ian Romanick <ian.d.romanick@intel.com>
Date:   Sun Dec 21 12:03:09 2014 -0800

    mesa: Add missing error checks in _mesa_ProgramBinary
    
    Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87516
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
    Acked-by: Leight Bade <leith@mapbox.com>


bug/show.html.tmpl processed on Mar 25, 2017 at 09:44:56.
(provided by the Example extension).