[Piglit] [PATCH] framework: fix binary assignment for shader_runner
Francisco Jerez
currojerez at riseup.net
Tue Aug 9 04:56:42 UTC 2016
Dylan Baker <dylan at pnwbakers.com> writes:
> Currently the python layer tries to dispatch shader_tests to the right
> binary (gles2, gles3 or gl). But the implementation is pretty dumb. It
> basically assumes either >= or == are the only operators used, and makes
> wrong assignments for < or <=, which are allowed. Is also only
> understands 2.0 and 3.0 as GLES versions, which is problematic.
>
> This version uses the closes equivalent of a match statement that python
> has, to try to pick the right GLES version, otherwise it uses the GL
> version. It also shares this picking mechanism with the Fast Skip
> mechanism, so they always have the same result.
>
> Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
> cc: mark.a.janes at intel.com
> cc: currojerez at riseup.net
> ---
>
> I'm waiting for Jenkins to finish to confirm that nothing was broken,
> but the tests at least run now and don't die during initialization of
> the profile like they did before.
>
> framework/test/opengl.py | 20 ++--
> framework/test/shader_test.py | 148 ++++++++++++++++-----------
> unittests/framework/test/test_shader_test.py | 44 +++++---
> 3 files changed, 132 insertions(+), 80 deletions(-)
>
> diff --git a/framework/test/opengl.py b/framework/test/opengl.py
> index 000bd84..51c8f19 100644
> --- a/framework/test/opengl.py
> +++ b/framework/test/opengl.py
> @@ -318,13 +318,15 @@ class FastSkipMixin(object):
> # other than querying each value before starting the thread pool.
> __info = WflInfo()
>
> - def __init__(self, *args, **kwargs):
> - super(FastSkipMixin, self).__init__(*args, **kwargs)
> - self.gl_required = set()
> - self.gl_version = None
> - self.gles_version = None
> - self.glsl_version = None
> - self.glsl_es_version = None
> + def __init__(self, command, gl_required=None, gl_version=None,
> + gles_version=None, glsl_version=None, glsl_es_version=None,
> + **kwargs): # pylint: disable=too-many-arguments
> + super(FastSkipMixin, self).__init__(command, **kwargs)
> + self.gl_required = gl_required or set()
> + self.gl_version = gl_version
> + self.gles_version = gles_version
> + self.glsl_version = glsl_version
> + self.glsl_es_version = glsl_es_version
>
> def is_skip(self):
> """Skip this test if any of it's feature requirements are unmet.
> @@ -340,6 +342,7 @@ class FastSkipMixin(object):
> 'Test requires extension {} '
> 'which is not available'.format(extension))
>
> + # TODO: Be able to handle any operator
> if (self.__info.gl_version is not None
> and self.gl_version is not None
> and self.gl_version > self.__info.gl_version):
> @@ -348,6 +351,7 @@ class FastSkipMixin(object):
> 'but only {} is available'.format(
> self.gl_version, self.__info.gl_version))
>
> + # TODO: Be able to handle any operator
> if (self.__info.gles_version is not None
> and self.gles_version is not None
> and self.gles_version > self.__info.gles_version):
> @@ -356,6 +360,7 @@ class FastSkipMixin(object):
> 'but only {} is available'.format(
> self.gles_version, self.__info.gles_version))
>
> + # TODO: Be able to handle any operator
> if (self.__info.glsl_version is not None
> and self.glsl_version is not None
> and self.glsl_version > self.__info.glsl_version):
> @@ -364,6 +369,7 @@ class FastSkipMixin(object):
> 'but only {} is available'.format(
> self.glsl_version, self.__info.glsl_version))
>
> + # TODO: Be able to handle any operator
> if (self.__info.glsl_es_version is not None
> and self.glsl_es_version is not None
> and self.glsl_es_version > self.__info.glsl_es_version):
> diff --git a/framework/test/shader_test.py b/framework/test/shader_test.py
> index f3c945c..f29c318 100644
> --- a/framework/test/shader_test.py
> +++ b/framework/test/shader_test.py
> @@ -26,8 +26,8 @@
> from __future__ import (
> absolute_import, division, print_function, unicode_literals
> )
> -import re
> import io
> +import re
>
> from framework import exceptions
> from .opengl import FastSkipMixin
> @@ -38,6 +38,53 @@ __all__ = [
> ]
>
>
> +class _SelectProg(object):
> + """A class for selecting the correct program.
> +
> + This is implemented as a class with a call method to avoid having to
> + reinitialize the _find as a function, or placing it in the global
> + namspace.
> + """
> + # This is sort of the python equivalent of a case statement.
> + _find = {
> + # pylint: disable=bad-whitespace
> + ('<=', 2.0): 'shader_runner_gles2',
> + ('=', 2.0): 'shader_runner_gles2',
> + ('>=', 2.0): 'shader_runner_gles2',
> + ('>', 2.0): 'shader_runner_gles3',
> +
> + ('<', 3.0): 'shader_runner_gles2',
> + ('<=', 3.0): 'shader_runner_gles2',
> + ('=', 3.0): 'shader_runner_gles3',
> + ('>=', 3.0): 'shader_runner_gles3',
> + ('>', 3.0): 'shader_runner_gles3',
> +
> + ('<', 3.1): 'shader_runner_gles2',
> + ('<=', 3.1): 'shader_runner_gles2',
> + ('=', 3.1): 'shader_runner_gles3',
> + ('>=', 3.1): 'shader_runner_gles3',
> + ('>', 3.1): 'shader_runner_gles3',
> +
> + ('<', 3.2): 'shader_runner_gles2',
> + ('<=', 3.2): 'shader_runner_gles2',
> + ('=', 3.2): 'shader_runner_gles3',
> + ('>=', 3.2): 'shader_runner_gles3',
Hm... Wouldn't it be easier to do this algorithmically rather than
trying to enumerate every possible combination of comparison operator
and GLES version? If anything it would save us from having to come back
to fix this the next time a GLES version is released. Maybe something
along the lines of:
| 'shader_runner_gles2' if op in ['<', '<='] or
| op in ['=', '>', '>='] and version < 3
| else 'shader_runner_gles3'
IOW, use shader_runner_gles2 if the version requirement intersects the
x < 3 half-line, otherwise use shader_runner_gles3.
> + }
> +
> + def __call__(self, is_gles, version, op):
> + if is_gles:
> + try:
> + return self._find[(op, version)]
> + except KeyError:
> + raise exceptions.PiglitInternalError(
> + 'No binary available for GLES {} {}'.format(op, version))
> + else:
> + return 'shader_runner'
> +
> +
> +select_prog = _SelectProg()
> +
> +
> class ShaderTest(FastSkipMixin, PiglitBaseTest):
> """ Parse a shader test file and return a PiglitTest instance
>
> @@ -52,7 +99,13 @@ class ShaderTest(FastSkipMixin, PiglitBaseTest):
> r'^GLSL\s+(?P<es>ES)?\s*(?P<op>(<|<=|=|>=|>))\s*(?P<ver>\d\.\d+)')
>
> def __init__(self, filename):
> - self.gl_required = set()
> + gl_required = set()
> + gl_version = None
> + gles_version = None
> + glsl_version = None
> + glsl_es_version = None
> + op = None
> + sl_op = None
>
> # Iterate over the lines in shader file looking for the config section.
> # By using a generator this can be split into two for loops at minimal
> @@ -76,74 +129,55 @@ class ShaderTest(FastSkipMixin, PiglitBaseTest):
> raise exceptions.PiglitFatalError(
> "In file {}: Config block not found".format(filename))
>
> - lines = list(lines)
> -
> - prog = self.__find_gl(lines, filename)
> -
> - super(ShaderTest, self).__init__([prog, filename], run_concurrent=True)
> -
> - # This needs to be run after super or gl_required will be reset
> - self.__find_requirements(lines)
> -
> - def __find_gl(self, lines, filename):
> - """Find the OpenGL API to use."""
> - for line in lines:
> - line = line.strip()
> - if line.startswith('GL ES'):
> - if line.endswith('3.0'):
> - prog = 'shader_runner_gles3'
> - elif line.endswith('2.0'):
> - prog = 'shader_runner_gles2'
> - # If we don't set gles2 or gles3 continue the loop,
> - # probably htting the exception in the for/else
> - else:
> - raise exceptions.PiglitFatalError(
> - "In File {}: No GL ES version set".format(filename))
> - break
> - elif line.startswith('[') or self._is_gl.match(line):
> - # In the event that we reach the end of the config black
> - # and an API hasn't been found, it's an old test and uses
> - # "GL"
> - prog = 'shader_runner'
> - break
> - else:
> - raise exceptions.PiglitFatalError(
> - "In file {}: No GL version set".format(filename))
> -
> - return prog
> -
> - def __find_requirements(self, lines):
> - """Find any requirements in the test and record them."""
> for line in lines:
> if line.startswith('GL_') and not line.startswith('GL_MAX'):
> - self.gl_required.add(line.strip())
> + gl_required.add(line.strip())
> continue
>
> - if not (self.gl_version or self.gles_version):
> - # Find any gles requirements
> + # Find any GLES requirements.
> + if not (gl_version or gles_version):
> m = self._match_gl_version.match(line)
> if m:
> - if m.group('op') not in ['<', '<=']:
> - if m.group('es'):
> - self.gles_version = float(m.group('ver'))
> - else:
> - self.gl_version = float(m.group('ver'))
> - continue
> -
> - if not (self.glsl_version or self.glsl_es_version):
> + op = m.group('op')
> + if m.group('es'):
> + gles_version = float(m.group('ver'))
> + else:
> + gl_version = float(m.group('ver'))
> + continue
> +
> + if not (glsl_version or glsl_es_version):
> # Find any GLSL requirements
> m = self._match_glsl_version.match(line)
> if m:
> - if m.group('op') not in ['<', '<=']:
> - if m.group('es'):
> - self.glsl_es_version = float(m.group('ver'))
> - else:
> - self.glsl_version = float(m.group('ver'))
> - continue
> + sl_op = m.group('op')
> + if m.group('es'):
> + glsl_es_version = float(m.group('ver'))
> + else:
> + glsl_version = float(m.group('ver'))
> + continue
>
> if line.startswith('['):
> break
>
> + try:
> + prog = select_prog(bool(gles_version),
> + gles_version or gl_version,
> + op)
> + except exceptions.PiglitInternalError as e:
> + raise exceptions.PiglitFatalError(
> + str(e) + ' in file {}'.format(filename))
> +
> + super(ShaderTest, self).__init__(
> + [prog, filename],
> + run_concurrent=True,
> + gl_required=gl_required,
> + # FIXME: the if here is related to an incomplete feature in the
> + # FastSkipMixin
> + gl_version=gl_version if op not in ['<', '<='] else None,
> + gles_version=gles_version if op not in ['<', '<='] else None,
> + glsl_version=glsl_version if sl_op not in ['<', '<='] else None,
> + glsl_es_version=glsl_es_version if sl_op not in ['<', '<='] else None)
> +
> @PiglitBaseTest.command.getter
> def command(self):
> """ Add -auto to the test command """
> diff --git a/unittests/framework/test/test_shader_test.py b/unittests/framework/test/test_shader_test.py
> index bd54624..9cf8b60 100644
> --- a/unittests/framework/test/test_shader_test.py
> +++ b/unittests/framework/test/test_shader_test.py
> @@ -63,28 +63,40 @@ teardown_module = _setup.teardown
> class TestConfigParsing(object):
> """Tests for ShaderRunner config parsing."""
>
> - def test_no_decimal(self, tmpdir):
> - """test.shader_test.ShaderTest: raises if version lacks decminal."""
> + @pytest.mark.parametrize('gles,operator,expected', [
> + # pylint: disable=bad-whitespace
> + ('2.0', '>=', 'shader_runner_gles2'),
> + ('2.0', '<=', 'shader_runner_gles2'),
> + ('2.0', '=', 'shader_runner_gles2'),
> + ('2.0', '>', 'shader_runner_gles3'),
> + ('3.0', '>=', 'shader_runner_gles3'),
> + ('3.0', '<=', 'shader_runner_gles2'),
> + ('3.0', '=', 'shader_runner_gles3'),
> + ('3.0', '>', 'shader_runner_gles3'),
> + ('3.0', '<', 'shader_runner_gles2'),
> + ('3.1', '>=', 'shader_runner_gles3'),
> + ('3.1', '<=', 'shader_runner_gles2'),
> + ('3.1', '=', 'shader_runner_gles3'),
> + ('3.1', '>', 'shader_runner_gles3'),
> + ('3.1', '<', 'shader_runner_gles2'),
> + ('3.2', '>=', 'shader_runner_gles3'),
> + ('3.2', '<=', 'shader_runner_gles2'),
> + ('3.2', '=', 'shader_runner_gles3'),
> + ('3.2', '<', 'shader_runner_gles2'),
> + ])
> + def test_bin(self, gles, operator, expected, tmpdir):
> + """Test that the shader_runner parse picks the correct binary."""
> p = tmpdir.join('test.shader_test')
> p.write(textwrap.dedent("""\
> [require]
> - GL = 2
> - """))
> -
> - with pytest.raises(exceptions.PiglitFatalError):
> - shader_test.ShaderTest(six.text_type(p))
> -
> - def test_gles2_bin(self, tmpdir):
> - """test.shader_test.ShaderTest: Identifies GLES2 tests successfully."""
> - p = tmpdir.join('test.shader_test')
> - p.write(textwrap.dedent("""\
> - [require]
> - GL ES >= 2.0
> + GL ES {} {}
> GLSL ES >= 1.00
> - """))
> +
> + [next section]
> + """.format(operator, gles)))
> test = shader_test.ShaderTest(six.text_type(p))
>
> - assert os.path.basename(test.command[0]) == "shader_runner_gles2"
> + assert os.path.basename(test.command[0]) == expected
>
> def test_gles3_bin(self, tmpdir):
> """test.shader_test.ShaderTest: Identifies GLES3 tests successfully."""
> --
> 2.9.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20160808/909aeca1/attachment-0001.sig>
More information about the Piglit
mailing list