[Mesa-dev] problem with "meta: Don't stray outside the confines of the API specified in the context"

[Brian Paul]

On Wed, Aug 29, 2012 at 6:16 PM, Paul Berry <stereotype441 at gmail.com> wrote:
> On 29 August 2012 16:16, Brian Paul <brianp at vmware.com> wrote:
>>
>> In enable.c there's a call to _mesa_meta_in_progress().  But the meta code
>> is not always built or linked in when building gallium targets.
>>
>> For now, I think I can put a _mesa_meta_in_progress() dummy function in
>> the state_tracker code.  But that's a pretty ugly hack.
>>
>> What's the reason for calling _mesa_meta_in_progress() for
>> glEnable/Disable(GL_MULTISAMPLE_ARB)?
>>
>> -Brian
>
>
> The difficulty here is that the clear meta-op needs to be able to disable
> GL_MULTISAMPLE_ARB in order to ensure that GL_SAMPLE_ALPHA_TO_COVERAGE,
> GL_SAMPLE_ALPHA_TO_ONE, and GL_SAMPLE_COVERAGE are all properly ignored
> during clear operations.  However, GL_MULTISAMPLE_ARB does not exist in the
> GLES API (GLES behaves as though it is always enabled).  Calling
> _mesa_meta_in_progress allows Meta to disable GL_MULTISAMPLE_ARB even when
> the API would ordinarily prohibit it.
>
> I agree that putting a dummy _mesa_meta_in_progress() function in the state
> tracker is an ugly hack.
>
> Here are some possible alternatives:
>
> 1. We could simply drop the call to _mesa_meta_in_progress(), and let
> glEnable/Disable(GL_MULTISAMPLE_ARB) always succeed, regardless of which API
> is in use.  But this also seems like an ugly hack, because it makes Mesa
> deliberately non-compliant with the GLES spec.
>
> 2. Is there a #define we could consult to determine whether the driver
> currently being compiled includes Meta?  If so, we could use an ifdef to
> avoid the call to _mesa_meta_in_progress() for drivers that don't support
> Meta (those drivers would then prohibit GL_MULTISAMPLE_ARB in GLES mode,
> consistent with the GLES spec).  I think this would be the cleanest
> solution.
>
> 3. We could make a backdoor function that enables or disables
> GL_MULTISAMPLE_ARB without doing error checking, and have Meta call the
> backdoor function instead of _mesa_set_enable().   This seems slightly less
> clean than option 2 because it involves some code duplication, but it's not
> too bad.
>
> 4. We could change Meta so that instead of enabling/disabling
> GL_MULTISAMPLE_ARB it saves and restores the state of
> GL_SAMPLE_ALPHA_TO_COVERAGE, GL_SAMPLE_ALPHA_TO_ONE, and GL_SAMPLE_COVERAGE.
> (It would have to skip saving/restoring of GL_SAMPLE_ALPHA_TO_ONE in GLES
> mode, since that enum doesn't exist in GLES).  This would have the advantage
> that core Mesa wouldn't have to do anything special to account for the needs
> of Meta, at the expense of adding a minor amount of complexity to Meta (Meta
> has to save the state of 2 or 3 enables, depending on API, rather than just
> 1).
>
> My personal preference is slightly in favor of 2, but I could be talked into
> 3 or 4.  I'd really rather avoid option 1, since strictly speaking it's
> non-compliant.
>
> (Cc'ing Ian since I'm guessing he has an opinion about this too)

I'd go with something like #3.  Just add a _meta_set_enable() function
which accepts GL_MULTISAMPLE_ARB and does what _mesa_set_enable() does
for that case, minus the error checking.  There might be other
enable/disables cases in the future that also require special
handling.  So far, the meta code does almost all state changes via
_mesa_foo() calls but that's not a requirement.

BTW, bug 54234 reports this issue too.

-Brian