[Mesa-dev] [PATCH 3/3] egl: Add MESA_image_sRGB extension.

[John Kåre Alsaker]

On Fri, Mar 1, 2013 at 10:01 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 02/27/2013 10:19 PM, John Kåre Alsaker wrote:
>>
>> On Mon, Feb 25, 2013 at 8:55 PM, Ian Romanick <idr at freedesktop.org> wrote:
>>>
>>> Also... are there piglit tests coming?
>>
>> Not unless you convince me otherwise. I don't think I'll be able to
>> verify that said tests work however.
>>
>>> More recent versions of the spec template include a section for
>>> describing
>>> conformance tests.  This would be a good place to document piglit tests.
>>> It
>>> also helps app developers know how things should work.
>>
>> Where is this template?
>
>
> I don't think the template is officially available anywhere, but the
> ARB_internalformat_query spec is a good example:
>
> http://www.opengl.org/registry/specs/ARB/internalformat_query.txt
>
> I'm uncomfortable adding new functionality that has zero tests.  Every time
> we've done this, we've gotten bitten by it.  At the minimum it should be
> possible to have tests that verify:
>
>  - Calling eglCreateImageKHR with {EGL_GAMMA_MESA, EGL_DEFAULT_MESA} always
> works.
>
>  - Calling eglCreateImageKHR with {EGL_GAMMA_MESA, <some invalid value>}
> always generates EGL_BAD_VIEW_MESA.
That sounds possible.

>>> What if the underlying driver doesn't support sRGB?
>>
>> It should just return EGL_BAD_VIEW_MESA then.
>
>
> Isn't it better to not expose the extension at all in that case?  If the
> only operations you ever want to do with the extension will always fail,
> what's the point? :)  Otherwise, I suspect we'll get spurious bug reports.
I'm not sure how to tell if the driver supports sRGB without actually
trying it, but I should at least make it depend on the DRIimage
version it needs.

>>> Shouldn't attrs sill be const?
>>
>> If Mesa does const by default, then sure.
>
>
> The parameters were const before your patch.  That's why I was asking.
It's a new parameter, the old one is gone.

>
>
>>> Why can't I create an sRGB view of a pixmap? It's just a reinterpretation
>>> of the bits.
>>
>> Because it's not implemented. I prefer not to touch code I won't even
>> manually test after (no pixmaps in Wayland).
>
>
> In that case, since this is likely the only implementation, the spec should
> just disallow it.  Principle of least surprise and all that.
Looking over I think it's probably best to move the code into
dri2_create_image and expose that to the platform_*.c files. Then it
will work for all EGL images.

>
>
>>> The spec doesn't give much guidance about why the
>>> implementation may not "support creating the specified gamma view".  At
>>> least mentioning something in the issues section would be helpful.
>>
>> I could probably add something.
>>
>>> I know the ifdef stuff is used other places, but since this comes from
>>> our
>>> own header file, I don't think it's necessary.
>>
>> Surrounding code had it.
>
>
> I understand that.  I don't think it's necessary in this case, and it's
> probably not necessary in the other cases either.
I'll remove it then.