Re: [Pixman] [PATCH 4/5] lowlevel-blt-bench: Parse test name strings in general case
On Tue, 07 Apr 2015 19:15:02 +0100 "Ben Avison" wrote: > Hi, > > Sorry for the delay in following up... > > On Mon, 16 Mar 2015 14:33:47 -, Pekka Paalanen > wrote: > > I understood these arrays should have been replaced by > > format_from_string/operator_from_string functions from patch 2 and > > similar new functions doing lookups in the same arrays. > > > > Hmm, but I see there are several alternative spellings for operators, > > and the formats follow a whole new convention. > > Some background to this might help. Pixman supports a lot of pixel > formats; of particular significance is the fact that in many cases you > can have the same bits-per-pixel, divided up into the same bitpattern of > colour components, but with the red and blue components exchanged. > > Each of Pixman's operations act upon between 1 and 3 bitmaps, each of > which may have any supported pixel format. But in practice, any given > application tends to stick to the same colour component ordering for most > or all of its images, so the actual number of operations you're likely to > encounter in practice is 1/2 (for 2-image operations) or 1/4 (for 3-image > operations) of what you might otherwise expect. Furthermore, > mathematically, an operation on (for example) two a8r8g8b8 images is > identical to one on two a8b8g8r8 images, so typically a fast path written > for one will be listed in the fast path table under both image formats. > Because of this, a naming convention has evolved in the source code where > fast path names include the string as an indication that it can be > applied to either a8r8g8b8 or a8b8g8r8, with the implicit assumption that > the other image has the same colour component ordering. Okay. > lowlevel-blt-bench is most useful when you're testing the effectiveness > of a particular fast path, so it makes sense that its test pattern names > reflect the names of the fast path function that you're interested in. > However, there are a few tests, like "src_0888__rev" or "rpixbuf" > where the limitations of this approach start to show. I suspected I would > get objections if I changed the command line of lowlevel-blt-bench, but > in introducing a new tool, affine-bench, I saw an opportunity to allow > the pixel formats to be specified more directly, and deliberately made > its syntax different. > > It is a matter for debate as to whether the two tools should use the same > syntax or not, and if so, which one they should standardise on. I'd be a > bit uncomfortable with saying that "" is an alias for "a8r8g8b8" or > "0565" for "r5g6b5", because that's not true in both directions, as I > have described. I'd probably choose the more verbose form if the > consensus was that the two tools should match. Is there anyone who cares > either way, though? Well, the aliases I was thinking of would indeed go just one way. "" would produce a specific rgba pixel format, but going back to string would return the accurate name, never "". It is what already happens for the existing tests, doesn't it? They have to pick something to feed into composite32() and friends. Building that into the lookup functions would allow sharing the lookup tables, which I assume was a big point in the original review. It would mean that you could use both ways of naming pixel formats or operators as you wish on the command line etc. It's hard to see that as a drawback: what used to work, will still work exactly the same, and you can use specific formats too if you want. Of course the aliases need to be carefully chosen, so that "0888" and "" produce the expected relative component ordering, so things like src_0888_ actually do what they used to. Maybe I should try writing that code and see how it would look like? I don't really like having these tables in multiple places either. Oh yeah, I just recalled, some operator names can have an underscore. That could make parsing strings similar to src__ hard... well, I'll see that if try it. Thanks, pq ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 4/5] lowlevel-blt-bench: Parse test name strings in general case
Hi, Sorry for the delay in following up... On Mon, 16 Mar 2015 14:33:47 -, Pekka Paalanen wrote: I understood these arrays should have been replaced by format_from_string/operator_from_string functions from patch 2 and similar new functions doing lookups in the same arrays. Hmm, but I see there are several alternative spellings for operators, and the formats follow a whole new convention. Some background to this might help. Pixman supports a lot of pixel formats; of particular significance is the fact that in many cases you can have the same bits-per-pixel, divided up into the same bitpattern of colour components, but with the red and blue components exchanged. Each of Pixman's operations act upon between 1 and 3 bitmaps, each of which may have any supported pixel format. But in practice, any given application tends to stick to the same colour component ordering for most or all of its images, so the actual number of operations you're likely to encounter in practice is 1/2 (for 2-image operations) or 1/4 (for 3-image operations) of what you might otherwise expect. Furthermore, mathematically, an operation on (for example) two a8r8g8b8 images is identical to one on two a8b8g8r8 images, so typically a fast path written for one will be listed in the fast path table under both image formats. Because of this, a naming convention has evolved in the source code where fast path names include the string as an indication that it can be applied to either a8r8g8b8 or a8b8g8r8, with the implicit assumption that the other image has the same colour component ordering. lowlevel-blt-bench is most useful when you're testing the effectiveness of a particular fast path, so it makes sense that its test pattern names reflect the names of the fast path function that you're interested in. However, there are a few tests, like "src_0888__rev" or "rpixbuf" where the limitations of this approach start to show. I suspected I would get objections if I changed the command line of lowlevel-blt-bench, but in introducing a new tool, affine-bench, I saw an opportunity to allow the pixel formats to be specified more directly, and deliberately made its syntax different. It is a matter for debate as to whether the two tools should use the same syntax or not, and if so, which one they should standardise on. I'd be a bit uncomfortable with saying that "" is an alias for "a8r8g8b8" or "0565" for "r5g6b5", because that's not true in both directions, as I have described. I'd probably choose the more verbose form if the consensus was that the two tools should match. Is there anyone who cares either way, though? Personally I'm not really much of a fan of this much nesting, especially where you have a number of parsing steps and each step nests a bit more. I'd refactor this code into a function and use early (error) returns instead of nesting. Quick-and-dirty code there - it was only a test harness after all. But easily fixed. Looks like there is also no complaint, if the given string is not a valid test name. It never did generate any sort of error if you specified a bad test string - you just wouldn't have seen any results. Another easy fix - updated patch following shortly. Ben ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 4/5] lowlevel-blt-bench: Parse test name strings in general case
On Tue, 3 Mar 2015 15:24:19 +
Ben Avison wrote:
> There are many types of composite operation that are useful to benchmark but
> which are omitted from the table. Continually having to add extra entries to
> the table is a nuisance and is prone to human error, so this patch adds the
> ability to break down unknow strings of the format
> _[_[_ca]
> where bitmap formats are specified by number of bits of each component
> (assumed in ARGB order) or 'n' to indicate a solid source or mask.
>
> The only downside to this approach is that specifying 'all' instead of a
> specific operation will remain limited to the set of operations from the
> table.
> ---
> test/lowlevel-blt-bench.c | 131
> ++---
> 1 files changed, 123 insertions(+), 8 deletions(-)
>
> diff --git a/test/lowlevel-blt-bench.c b/test/lowlevel-blt-bench.c
> index 3da094a..f05a663 100644
> --- a/test/lowlevel-blt-bench.c
> +++ b/test/lowlevel-blt-bench.c
> @@ -51,6 +51,12 @@
>
> #define EXCLUDE_OVERHEAD 1
>
> +typedef struct
> +{
> +const char *name;
> +uint32_tnumber;
> +} name_to_number_t;
> +
> uint32_t *dst;
> uint32_t *src;
> uint32_t *mask;
> @@ -367,14 +373,14 @@ bench_RT (pixman_op_t op,
> }
>
> void
> -bench_composite (char * testname,
> - intsrc_fmt,
> - intsrc_flags,
> - intop,
> - intmask_fmt,
> - intmask_flags,
> - intdst_fmt,
> - double npix)
> +bench_composite (const char * testname,
> + int src_fmt,
> + int src_flags,
> + int op,
> + int mask_fmt,
> + int mask_flags,
> + int dst_fmt,
> + double npix)
> {
> pixman_image_t *src_img;
> pixman_image_t *dst_img;
> @@ -719,12 +725,36 @@ tests_tbl[] =
> { "rpixbuf", PIXMAN_x8b8g8r8,0, PIXMAN_OP_SRC,
> PIXMAN_a8b8g8r8, 0, PIXMAN_a8b8g8r8 },
> };
>
> +static uint32_t
> +lookup_name_to_number (const char **string, const name_to_number_t *array,
> size_t entries)
> +{
> +size_t entry, i;
> +for (entry = 0; entry < entries; entry++)
> +{
> +for (i = 0; (*string)[i] == array[entry].name[i]; i++)
> +{
> +if ((*string)[i] == 0)
> +{
> +*string += i;
> +return array[entry].number;
> +}
> +}
> +if ((*string)[i] == '_' && array[entry].name[i] == 0)
> +{
> +*string += i;
> +return array[entry].number;
> +}
> +}
> +return 0;
> +}
> +
> int
> main (int argc, char *argv[])
> {
> double x;
> int i;
> const char *pattern = NULL;
> +int matches = 0;
> for (i = 1; i < argc; i++)
> {
> if (argv[i][0] == '-')
> @@ -805,6 +835,7 @@ main (int argc, char *argv[])
> {
> if (strcmp (pattern, "all") == 0 || strcmp (tests_tbl[i].testname,
> pattern) == 0)
> {
> + ++matches;
> bench_composite (tests_tbl[i].testname,
>tests_tbl[i].src_fmt,
>tests_tbl[i].src_flags,
> @@ -816,6 +847,90 @@ main (int argc, char *argv[])
> }
> }
>
> +if (matches == 0)
> +{
> +/* Try parsing the string instead */
> +static const name_to_number_t combine_type[] = {
> +{ "src", PIXMAN_OP_SRC },
> +{ "over_reverse", PIXMAN_OP_OVER_REVERSE },
> +{ "overrev", PIXMAN_OP_OVER_REVERSE },
> +{ "over", PIXMAN_OP_OVER },
> +{ "in_reverse", PIXMAN_OP_IN_REVERSE },
> +{ "inrev",PIXMAN_OP_IN_REVERSE },
> +{ "in", PIXMAN_OP_IN },
> +{ "out_reverse", PIXMAN_OP_OUT_REVERSE },
> +{ "outrev", PIXMAN_OP_OUT_REVERSE },
> +{ "out", PIXMAN_OP_OUT },
> +{ "atop_reverse", PIXMAN_OP_ATOP_REVERSE },
> +{ "atoprev", PIXMAN_OP_ATOP_REVERSE },
> +{ "atop", PIXMAN_OP_ATOP },
> +{ "xor", PIXMAN_OP_XOR },
> +{ "add", PIXMAN_OP_ADD }
> +};
> +static const name_to_number_t format[] = {
> +{ "", PIXMAN_a8r8g8b8 },
> +{ "x888", PIXMAN_x8r8g8b8 },
> +{ "0565", PIXMAN_r5g6b5 },
> +{ "1555", PIXMAN_a1r5g5b5 },
> +{ "8",PIXMAN_a8 }
> +};
Hi,
reading Søren's comment:
> ==
> 0002: lowlevel parse Needs work
> 0003: affine benchmarker
[Pixman] [PATCH 4/5] lowlevel-blt-bench: Parse test name strings in general case
There are many types of composite operation that are useful to benchmark but
which are omitted from the table. Continually having to add extra entries to
the table is a nuisance and is prone to human error, so this patch adds the
ability to break down unknow strings of the format
_[_[_ca]
where bitmap formats are specified by number of bits of each component
(assumed in ARGB order) or 'n' to indicate a solid source or mask.
The only downside to this approach is that specifying 'all' instead of a
specific operation will remain limited to the set of operations from the
table.
---
test/lowlevel-blt-bench.c | 131 ++---
1 files changed, 123 insertions(+), 8 deletions(-)
diff --git a/test/lowlevel-blt-bench.c b/test/lowlevel-blt-bench.c
index 3da094a..f05a663 100644
--- a/test/lowlevel-blt-bench.c
+++ b/test/lowlevel-blt-bench.c
@@ -51,6 +51,12 @@
#define EXCLUDE_OVERHEAD 1
+typedef struct
+{
+const char *name;
+uint32_tnumber;
+} name_to_number_t;
+
uint32_t *dst;
uint32_t *src;
uint32_t *mask;
@@ -367,14 +373,14 @@ bench_RT (pixman_op_t op,
}
void
-bench_composite (char * testname,
- intsrc_fmt,
- intsrc_flags,
- intop,
- intmask_fmt,
- intmask_flags,
- intdst_fmt,
- double npix)
+bench_composite (const char * testname,
+ int src_fmt,
+ int src_flags,
+ int op,
+ int mask_fmt,
+ int mask_flags,
+ int dst_fmt,
+ double npix)
{
pixman_image_t *src_img;
pixman_image_t *dst_img;
@@ -719,12 +725,36 @@ tests_tbl[] =
{ "rpixbuf", PIXMAN_x8b8g8r8,0, PIXMAN_OP_SRC,
PIXMAN_a8b8g8r8, 0, PIXMAN_a8b8g8r8 },
};
+static uint32_t
+lookup_name_to_number (const char **string, const name_to_number_t *array,
size_t entries)
+{
+size_t entry, i;
+for (entry = 0; entry < entries; entry++)
+{
+for (i = 0; (*string)[i] == array[entry].name[i]; i++)
+{
+if ((*string)[i] == 0)
+{
+*string += i;
+return array[entry].number;
+}
+}
+if ((*string)[i] == '_' && array[entry].name[i] == 0)
+{
+*string += i;
+return array[entry].number;
+}
+}
+return 0;
+}
+
int
main (int argc, char *argv[])
{
double x;
int i;
const char *pattern = NULL;
+int matches = 0;
for (i = 1; i < argc; i++)
{
if (argv[i][0] == '-')
@@ -805,6 +835,7 @@ main (int argc, char *argv[])
{
if (strcmp (pattern, "all") == 0 || strcmp (tests_tbl[i].testname,
pattern) == 0)
{
+ ++matches;
bench_composite (tests_tbl[i].testname,
tests_tbl[i].src_fmt,
tests_tbl[i].src_flags,
@@ -816,6 +847,90 @@ main (int argc, char *argv[])
}
}
+if (matches == 0)
+{
+/* Try parsing the string instead */
+static const name_to_number_t combine_type[] = {
+{ "src", PIXMAN_OP_SRC },
+{ "over_reverse", PIXMAN_OP_OVER_REVERSE },
+{ "overrev", PIXMAN_OP_OVER_REVERSE },
+{ "over", PIXMAN_OP_OVER },
+{ "in_reverse", PIXMAN_OP_IN_REVERSE },
+{ "inrev",PIXMAN_OP_IN_REVERSE },
+{ "in", PIXMAN_OP_IN },
+{ "out_reverse", PIXMAN_OP_OUT_REVERSE },
+{ "outrev", PIXMAN_OP_OUT_REVERSE },
+{ "out", PIXMAN_OP_OUT },
+{ "atop_reverse", PIXMAN_OP_ATOP_REVERSE },
+{ "atoprev", PIXMAN_OP_ATOP_REVERSE },
+{ "atop", PIXMAN_OP_ATOP },
+{ "xor", PIXMAN_OP_XOR },
+{ "add", PIXMAN_OP_ADD }
+};
+static const name_to_number_t format[] = {
+{ "", PIXMAN_a8r8g8b8 },
+{ "x888", PIXMAN_x8r8g8b8 },
+{ "0565", PIXMAN_r5g6b5 },
+{ "1555", PIXMAN_a1r5g5b5 },
+{ "8",PIXMAN_a8 }
+};
+const char *p = pattern;
+int src_fmt;
+int src_flags = 0;
+int mask_fmt;
+int mask_flags = 0;
+int dst_fmt;
+int op = lookup_name_to_number(&p, combine_type, sizeof
combine_type / sizeof *combine_type);
+if (op != 0 && *p++ == '_')
+{
+if (p[0] == 'n' && p[1] == '_')
+{
+src_fmt = PIXMAN_a8r8g8b8;
+src_flags = SOLID_FLAG;
