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

[Ian Romanick]

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.

>> Interactions with GL_ARB_texture_view?  Really, texture views seem like the
>> right way to do this.
> GL_ARB_texture_view doesn't interact with EGL images at all from what I can see.
>
>> 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.

>> Since this extension
>> depends on KHR_image_base, the enable needs to at least depend on
>> dri2_dpy->image.
>
>> 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.

>> 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.

>> 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.