[Mesa-dev] [PATCH 1/6] mesa: GetTexLevelParameter{if}v changes for OpenGL ES 3.1
Ilia Mirkin
imirkin at alum.mit.edu
Sun Aug 23 23:07:16 PDT 2015
On Aug 24, 2015 2:01 AM, "Tapani Pälli" <tapani.palli at intel.com> wrote:
>
>
>
> On 08/24/2015 08:38 AM, Ilia Mirkin wrote:
>>
>>
>> On Aug 24, 2015 1:25 AM, "Tapani Pälli" <tapani.palli at intel.com
>> <mailto:tapani.palli at intel.com>> wrote:
>> >
>> >
>> >
>> > On 08/24/2015 08:17 AM, Ilia Mirkin wrote:
>> >>
>> >>
>> >> On Aug 24, 2015 1:07 AM, "Tapani Pälli" <tapani.palli at intel.com
>> <mailto:tapani.palli at intel.com>
>> >> <mailto:tapani.palli at intel.com <mailto:tapani.palli at intel.com>>>
wrote:
>> >> >
>> >> >
>> >> >
>> >> > On 08/21/2015 05:14 PM, Ilia Mirkin wrote:
>> >> >>
>> >> >> On Fri, Aug 21, 2015 at 3:22 AM, Tapani Pälli
>> >> <tapani.palli at intel.com <mailto:tapani.palli at intel.com>
>> <mailto:tapani.palli at intel.com <mailto:tapani.palli at intel.com>>> wrote:
>> >> >>>
>> >> >>> Patch refactors existing parameters check to first check common
>> enums
>> >> >>> between desktop GL and GLES 3.1 and modifies
>> >> get_tex_level_parameter_image
>> >> >>> to be compatible with enums specified in 3.1.
>> >> >>>
>> >> >>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com
>> <mailto:tapani.palli at intel.com>
>> >> <mailto:tapani.palli at intel.com <mailto:tapani.palli at intel.com>>>
>>
>> >>
>> >> >>> ---
>> >> >>> src/mesa/main/texparam.c | 34
+++++++++++++++++++++++-----------
>> >> >>> 1 file changed, 23 insertions(+), 11 deletions(-)
>> >> >>>
>> >> >>> diff --git a/src/mesa/main/texparam.c b/src/mesa/main/texparam.c
>> >> >>> index 16739f1..947a2a1 100644
>> >> >>> --- a/src/mesa/main/texparam.c
>> >> >>> +++ b/src/mesa/main/texparam.c
>> >> >>> @@ -1208,20 +1208,34 @@ static GLboolean
>> >> >>> legal_get_tex_level_parameter_target(struct gl_context *ctx,
>> >> GLenum target,
>> >> >>> bool dsa)
>> >> >>> {
>> >> >>> + /* Common targets for desktop GL and GLES 3.1. */
>> >> >>> switch (target) {
>> >> >>> - case GL_TEXTURE_1D:
>> >> >>> - case GL_PROXY_TEXTURE_1D:
>> >> >>> case GL_TEXTURE_2D:
>> >> >>> - case GL_PROXY_TEXTURE_2D:
>> >> >>> case GL_TEXTURE_3D:
>> >> >>> - case GL_PROXY_TEXTURE_3D:
>> >> >>> return GL_TRUE;
>> >> >>> + case GL_TEXTURE_2D_ARRAY_EXT:
>> >> >>> + return (_mesa_is_gles31(ctx) ||
>> >> ctx->Extensions.EXT_texture_array);
>> >> >>> case GL_TEXTURE_CUBE_MAP_POSITIVE_X_ARB:
>> >> >>> case GL_TEXTURE_CUBE_MAP_NEGATIVE_X_ARB:
>> >> >>> case GL_TEXTURE_CUBE_MAP_POSITIVE_Y_ARB:
>> >> >>> case GL_TEXTURE_CUBE_MAP_NEGATIVE_Y_ARB:
>> >> >>> case GL_TEXTURE_CUBE_MAP_POSITIVE_Z_ARB:
>> >> >>> case GL_TEXTURE_CUBE_MAP_NEGATIVE_Z_ARB:
>> >> >>> + return (_mesa_is_gles31(ctx) ||
>> >> ctx->Extensions.ARB_texture_cube_map);
>> >> >>> + case GL_TEXTURE_2D_MULTISAMPLE:
>> >> >>> + return (_mesa_is_gles31(ctx) ||
>> >> ctx->Extensions.ARB_texture_multisample);
>> >> >>> + }
>> >> >>> +
>> >> >>> + if (!_mesa_is_desktop_gl(ctx))
>> >> >>> + return GL_FALSE;
>> >> >>> +
>> >> >>> + /* Rest of the desktop GL targets. */
>> >> >>> + switch (target) {
>> >> >>> + case GL_TEXTURE_1D:
>> >> >>> + case GL_PROXY_TEXTURE_1D:
>> >> >>> + case GL_PROXY_TEXTURE_2D:
>> >> >>> + case GL_PROXY_TEXTURE_3D:
>> >> >>> + return GL_TRUE;
>> >> >>> case GL_PROXY_TEXTURE_CUBE_MAP_ARB:
>> >> >>> return ctx->Extensions.ARB_texture_cube_map;
>> >> >>> case GL_TEXTURE_CUBE_MAP_ARRAY_ARB:
>> >> >>> @@ -1232,7 +1246,6 @@
legal_get_tex_level_parameter_target(struct
>> >> gl_context *ctx, GLenum target,
>> >> >>> return ctx->Extensions.NV_texture_rectangle;
>> >> >>> case GL_TEXTURE_1D_ARRAY_EXT:
>> >> >>> case GL_PROXY_TEXTURE_1D_ARRAY_EXT:
>> >> >>> - case GL_TEXTURE_2D_ARRAY_EXT:
>> >> >>> case GL_PROXY_TEXTURE_2D_ARRAY_EXT:
>> >> >>> return ctx->Extensions.EXT_texture_array;
>> >> >>> case GL_TEXTURE_BUFFER:
>> >> >>> @@ -1254,7 +1267,6 @@
legal_get_tex_level_parameter_target(struct
>> >> gl_context *ctx, GLenum target,
>> >> >>> * "target may also be TEXTURE_BUFFER, indicating the
>> >> texture buffer."
>> >> >>> */
>> >> >>> return ctx->API == API_OPENGL_CORE && ctx->Version >=
31;
>> >> >>> - case GL_TEXTURE_2D_MULTISAMPLE:
>> >> >>> case GL_TEXTURE_2D_MULTISAMPLE_ARRAY:
>> >> >>> case GL_PROXY_TEXTURE_2D_MULTISAMPLE:
>> >> >>> case GL_PROXY_TEXTURE_2D_MULTISAMPLE_ARRAY:
>> >> >>> @@ -1381,8 +1393,8 @@ get_tex_level_parameter_image(struct
>> >> gl_context *ctx,
>> >> >>> *params = _mesa_get_format_bits(texFormat, pname);
>> >> >>> break;
>> >> >>> case GL_TEXTURE_SHARED_SIZE:
>> >> >>> - if (ctx->Version < 30 &&
>> >> >>> - !ctx->Extensions.EXT_texture_shared_exponent)
>> >> >>> + if (!_mesa_is_gles31(ctx) && (ctx->Version < 30 &&
>> >> >>> + !ctx->Extensions.EXT_texture_shared_exponent))
>> >> >>> goto invalid_pname;
>> >> >>> *params = texFormat == MESA_FORMAT_R9G9B9E5_FLOAT ?
>> 5 : 0;
>> >> >>> break;
>> >> >>> @@ -1415,7 +1427,7 @@ get_tex_level_parameter_image(struct
>> >> gl_context *ctx,
>> >> >>> case GL_TEXTURE_BLUE_TYPE_ARB:
>> >> >>> case GL_TEXTURE_ALPHA_TYPE_ARB:
>> >> >>> case GL_TEXTURE_DEPTH_TYPE_ARB:
>> >> >>> - if (!ctx->Extensions.ARB_texture_float)
>> >> >>> + if (!_mesa_is_gles31(ctx) &&
>> >> !ctx->Extensions.ARB_texture_float)
>> >> >>> goto invalid_pname;
>> >> >>> if (_mesa_base_format_has_channel(img->_BaseFormat,
>> pname))
>> >> >>> *params = _mesa_get_format_datatype(texFormat);
>> >> >>> @@ -1425,13 +1437,13 @@ get_tex_level_parameter_image(struct
>> >> gl_context *ctx,
>> >> >>>
>> >> >>> /* GL_ARB_texture_multisample */
>> >> >>> case GL_TEXTURE_SAMPLES:
>> >> >>> - if (!ctx->Extensions.ARB_texture_multisample)
>> >> >>> + if (!_mesa_is_gles31(ctx) &&
>> >> !ctx->Extensions.ARB_texture_multisample)
>> >> >>> goto invalid_pname;
>> >> >>> *params = img->NumSamples;
>> >> >>> break;
>> >> >>>
>> >> >>> case GL_TEXTURE_FIXED_SAMPLE_LOCATIONS:
>> >> >>> - if (!ctx->Extensions.ARB_texture_multisample)
>> >> >>> + if (!_mesa_is_gles31(ctx) &&
>> >> !ctx->Extensions.ARB_texture_multisample)
>> >> >>
>> >> >>
>> >> >> I think you guys have gone a little nuts with sticking
>> _mesa_is_gles31
>> >> >> all over the place... This is accounting for drivers which want
to
>> >> >> expose gles3.1 but don't have ARB_texture_multisample supported.
Are
>> >> >> there any such drivers? Are all these changes really worthwhile?
>> >> >
>> >> >
>> >> > This particular patch was to address
>> >> >
>> >> > "glGetTexLevelParameter[fi]v - needs updates to restrict to GLES
>> enums"
>> >> >
>> >> > in docs/GL3.txt GLES3.1 section.
>> >> >
>> >> > I do understand your worry about sticking is_gles31() here and
there
>> >> but remember that most (if not all) these changes will apply also for
>> >> gles32. In Mesa there are 232 instances of is_gles(), 156 of
is_gles3()
>> >> and only 34 instances of is_gles31() so I think the overall changes
are
>> >> still quite small, not so intrusive as it may look.
>> >>
>> >> I think you may have misunderstood my comment.
>> >>
>> >> Old code: if not texture ms
>> >> New code: if not gles31 and not texture ms
>> >>
>> >> Aka "not (gles31 or texture ms)"
>> >>
>> >> I posit that the situation where gles31 and not texture ms will never
>> >> ever happen.
>> >>
>> >> So you haven't changed the outcome of that conditional for any
>> >> reasonable circumstances. Why are you adding that code?
>> >
>> >
>> > There are some differences between ARB_texture_multisample and GLES
>> 31, at least multisample array textures. Agreeably this is 'academical'
>> and may not happen in real life (like many other error checks in Mesa
>> though). I guess these could be removed if others also feel like this.
>>
>> Sure, but are you ever going to get a driver that supports gles31 but
>> does not have that bit set in extensions? I don't think so.
>>
>> I'm concerned that tons and tons of these needless checks are being
>> added all over.
>>
>> I'm all for api correctness, but this seems to be about totally academic
>> situations. The code literally has no effect for any actual drivers in
>> Mesa, nor any that I can imagine in the future.
>>
>> That's what I meant about it being ok for compute, since its a practical
>> issue -- its not supported, but you want it to still be faked in es31.
>> Fine. But here? What's the usecase?
>
>
> Is it the same thing then for EXT_texture_array and ARB_texture_cube_map?
I could send a new version with removing checks, the comment on top ensures
that reader knows which enums are only for ES 3.1.
Yes. Here's how I think about it -- if it's going to be included in
compute_version (which those are, for es2/3) then you can assume it'll be
set. Otherwise you have to check separately. I think that the es31 check
will have texture ms in it, even though, like you said, its not a perfect
match. That's the case for a lot of these.
>
>
>> >
>> >
>> >> >
>> >> >
>> >> >> The original argument was that you wanted to pass some CTS tests
>> >> >> before you actually had compute/etc shaders going... which seems
>> >> >> reasonable. This all feels like going overboard though.
>> >> >
>> >> >
>> >> > For the question if changes are worthwhile, we have gone from 0
>> >> passing CTS tests (+ a few crashes) to > 200 passing tests,
personally I
>> >> think this is good progress, thanks to you and others who have
corrected
>> >> errors in our changes.
>> >> >
>> >> > Our goal is to pass the whole suite when rest of the missing
>> >> extensions are integrated. There are still things to do, API
differences
>> >> here and there. Nothing major but important to get done.
>> >> >
>> >> >
>> >> >
>> >> >
>> >> >> -ilia
>> >> >>
>> >> >>> goto invalid_pname;
>> >> >>> *params = img->FixedSampleLocations;
>> >> >>> break;
>> >> >>> --
>> >> >>> 2.4.3
>> >> >>>
>> >> >>> _______________________________________________
>> >> >>> mesa-dev mailing list
>> >> >>> mesa-dev at lists.freedesktop.org
>> <mailto:mesa-dev at lists.freedesktop.org>
>> <mailto:mesa-dev at lists.freedesktop.org
>>
>> <mailto:mesa-dev at lists.freedesktop.org>>
>> >> >>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> >> >
>> >> >
>> >> >
>> >> > // Tapani
>> >>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150824/ca3dd6a9/attachment-0001.html>
More information about the mesa-dev
mailing list