[Pixman] [PATCH] build: Distinguish SKIP and FAIL on Win32

[Andrea Canciani]

The `check` target in test/Makefile.win32 assumed that any non-0 exit
code from the tests was an error, but the testsuite is currently using
77 as a SKIP exit code (based on the convention used in autotools).

Fixes fence-image-self-test and cover-test (now reported as SKIP).

Signed-off-by: Andrea Canciani <ranm...@gmail.com>
---
 test/Makefile.win32 | 31 ---
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/test/Makefile.win32 b/test/Makefile.win32
index bdd9b7f..d38f0c0 100644
--- a/test/Makefile.win32
+++ b/test/Makefile.win32
@@ -19,26 +19,35 @@ OTHERS  = $(patsubst %,   $(CFG_VAR)/%.exe, 
$(OTHERPROGRAMS))
 all: inform $(TESTS) $(OTHERS)
 
 check: inform $(TESTS)
-   @failures=0 ; \
-   total=0 ; \
+   @total=0 ; \
+   pass=0 ; \
+   skip=0 ; \
+   fail=0 ; \
for test in $(TESTS) ; \
do \
total=`expr $$total + 1` ; \
-   if ./$$test ; \
+   ./$$test ; \
+   r=$$? ; \
+   if test $$r -eq 0 ; \
then echo "PASS: $$test" ; \
+   pass=`expr $$pass + 1` ; \
+   elif test $$r -eq 77 ; \
+   then echo "SKIP: $$test" ; \
+   skip=`expr $$skip + 1` ; \
else echo "FAIL: $$test" ; \
-failures=`expr $$failures + 1` ; \
+   fail=`expr $$fail + 1` ; \
fi ; \
done ; \
-   if test $$failures -eq 0 ; \
-   then banner="All $$total tests passed" ; \
-   else banner="$$failures of $$total tests failed" ; \
-   fi ; \
-   dashes=`echo "$$banner" | sed s/./=/g`; \
+   
dashes=""
 ; \
echo "$$dashes" ; \
-   echo "$$banner" ; \
+   echo "Testsuite summary for pixman:" ; \
echo "$$dashes" ; \
-   test $$failures -eq 0
+   echo "# TOTAL: $$total" ; \
+   echo "# PASS:  $$pass" ; \
+   echo "# SKIP:  $$skip" ; \
+   echo "# FAIL:  $$fail" ; \
+   echo "$$dashes" ; \
+   test $$fail -eq 0
 
 $(CFG_VAR)/libutils.lib: $(libutils_OBJECTS)
@$(AR) $(PIXMAN_ARFLAGS) -OUT:$@ $^
-- 
2.5.4 (Apple Git-61)

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 4/4] build: Use `del` instead of `rm` on `cmd.exe` shells

[Andrea Canciani]

From: Simon Richter <simon.rich...@hogyros.de>

The `rm` command is not usually available when running on Win32 in a
`cmd.exe` shell. Instead the shell provides the `del` builtin, which
has somewhat more limited wildcars expansion and error handling.

This makes all of the Makefile targets work on Win32 both using
`cmd.exe` and using the MSYS environment.

Signed-off-by: Simon Richter <simon.rich...@hogyros.de>
Signed-off-by: Andrea Canciani <ranm...@gmail.com>
---
 Makefile.win32.common | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Makefile.win32.common b/Makefile.win32.common
index a759ddc..756fc94 100644
--- a/Makefile.win32.common
+++ b/Makefile.win32.common
@@ -5,6 +5,10 @@ LD = link
 AR = lib
 PERL = perl
 
+ifneq ($(shell echo ""),)
+RM = del
+endif
+
 ifeq ($(top_builddir),)
 top_builddir = $(top_srcdir)
 endif
@@ -51,7 +55,7 @@ $(CFG_VAR):
 $(CFG_VAR)/%.obj: %.c $(libpixman_headers) | $(CFG_VAR)
@$(CC) -c $(PIXMAN_CFLAGS) -Fo"$@" $<
 
-clean: inform
-   @$(RM) $(CFG_VAR)/*.{exe,ilk,lib,obj,pdb} || exit 0
+clean: inform $(CFG_VAR)
+   @cd $(CFG_VAR) && echo > silence_error.exe && $(RM) *.exe *.ilk *.lib 
*.obj *.pdb
 
 .PHONY: inform clean
-- 
2.5.4 (Apple Git-61)

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 3/4] build: Do not use `mkdir -p` on Windows

[Andrea Canciani]

When the build is performed using `cmd.exe` as shell, the `mkdir`
command does not support the `-p` flag. The ability to create multiple
netsted folder is not used, hence it can be easily replaced by only
creating the directory if it does not exist.

This makes the build work on the `cmd.exe` shell, except for the
`clean` targets.

Signed-off-by: Andrea Canciani <ranm...@gmail.com>
---
 Makefile.win32.common | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Makefile.win32.common b/Makefile.win32.common
index b498c2f..a759ddc 100644
--- a/Makefile.win32.common
+++ b/Makefile.win32.common
@@ -45,9 +45,10 @@ endif
 endif
 endif
 
+$(CFG_VAR):
+   @mkdir $@
 
-$(CFG_VAR)/%.obj: %.c $(libpixman_headers)
-   @mkdir -p $(CFG_VAR)
+$(CFG_VAR)/%.obj: %.c $(libpixman_headers) | $(CFG_VAR)
@$(CC) -c $(PIXMAN_CFLAGS) -Fo"$@" $<
 
 clean: inform
-- 
2.5.4 (Apple Git-61)

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 1/4] build: Remove use of BUILT_SOURCES from Makefile.win32

[Andrea Canciani]

Since 3d81d89c292058522cce91338028d9b4c4a23c24 BUILT_SOURCES is not
used anymore, but it was unintentionally left in Win32 Makefiles.

Signed-off-by: Andrea Canciani <ranm...@gmail.com>
---
 Makefile.win32.common | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.win32.common b/Makefile.win32.common
index 777f94c..b498c2f 100644
--- a/Makefile.win32.common
+++ b/Makefile.win32.common
@@ -51,6 +51,6 @@ $(CFG_VAR)/%.obj: %.c $(libpixman_headers)
@$(CC) -c $(PIXMAN_CFLAGS) -Fo"$@" $<
 
 clean: inform
-   @$(RM) $(CFG_VAR)/*.{exe,ilk,lib,obj,pdb} $(BUILT_SOURCES) || exit 0
+   @$(RM) $(CFG_VAR)/*.{exe,ilk,lib,obj,pdb} || exit 0
 
 .PHONY: inform clean
-- 
2.5.4 (Apple Git-61)

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 2/4] build: Avoid phony `pixman` target in test/Makefile.win32

[Andrea Canciani]

Instead of explicitly depending on "pixman" for the "all" and "check"
targets, rely on the dependency to the .lib file

Signed-off-by: Andrea Canciani <ranm...@gmail.com>
---
 test/Makefile.win32 | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/test/Makefile.win32 b/test/Makefile.win32
index 6cfb4a7..bdd9b7f 100644
--- a/test/Makefile.win32
+++ b/test/Makefile.win32
@@ -16,9 +16,9 @@ OBJECTS = $(patsubst %.c, $(CFG_VAR)/%.obj, $(SOURCES))
 TESTS   = $(patsubst %,   $(CFG_VAR)/%.exe, $(TESTPROGRAMS))
 OTHERS  = $(patsubst %,   $(CFG_VAR)/%.exe, $(OTHERPROGRAMS))
 
-all: pixman inform $(TESTS) $(OTHERS)
+all: inform $(TESTS) $(OTHERS)
 
-check: pixman inform $(TESTS)
+check: inform $(TESTS)
@failures=0 ; \
total=0 ; \
for test in $(TESTS) ; \
@@ -46,9 +46,7 @@ $(CFG_VAR)/libutils.lib: $(libutils_OBJECTS)
 $(CFG_VAR)/%.exe: $(CFG_VAR)/%.obj $(TEST_LDADD)
@$(LD) $(PIXMAN_LDFLAGS) -OUT:$@ $^
 
-$(top_builddir)/pixman/$(CFG_VAR)/$(LIBRARY).lib: pixman
-
-pixman:
+$(top_builddir)/pixman/$(CFG_VAR)/$(LIBRARY).lib:
@$(MAKE) -C $(top_builddir)/pixman -f Makefile.win32
 
-.PHONY: all check pixman
+.PHONY: all check
-- 
2.5.4 (Apple Git-61)

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Allow building on Windows with cmd.exe

[Andrea Canciani]

On Tue, Dec 22, 2015 at 2:19 PM, Oded Gabbay <oded.gab...@gmail.com> wrote:

> On Thu, Jun 4, 2015 at 12:02 PM, Andrea Canciani <ranm...@gmail.com>
> wrote:
> > The patch does not regress the mingw-based build (basically that used in
> > http://cairographics.org/end_to_end_build_for_win32/ )
> > I did not manage to get the build working on the cmd.exe shell. Do you
> have
> > any instructions (or possibly indications about what is the correct
> > environment) for that?
> >
> > AFAICT the change still relies on a Unix-like make command.
> > A bigger (but possibly even more interesting) change would be an
> NMAKE-based
> > build, so that the command-line tools available in Visual Studio are
> > sufficient for building the whole project.
> >
> > Andrea
> >
>
> Hi Andrea,
>
> I know it has been almost 6 months since you last looked at this
> patch, but I would like to ask if you can give it an r-b and then I
> will push it to master.
>

Actually, this is a good time for me to work on pixman, as I have some more
free time :)


> If not, what needs to be done to give it an r-b ?
>

The main thing blocking me from an r-b tag originally was that I had been
unable to get it working.
I set up an environment to test it and found that it mostly behaves as
intended, with the main glitch being that the "clean" target can spit out
undesired error logs.

I pushed a branch which is based on Simon's patch here:
http://cgit.freedesktop.org/~ranma42/pixman/log/?h=win32-build

The commits include:
 - cleanup of the win32 build system (mostly unrelated to the original
patch)
   ea8a070 build: Remove use of BUILT_SOURCES from Makefile.win32
   8989759 build: Avoid phony `pixman` target in test/Makefile.win32
 - shell-generic compatibility
   db72f26 build: Do not use `mkdir -p` on Windows
 - cmd.exe compatibility
   6223be5 build: Use `del` instead of `rm` on `cmd.exe` shells

The last commit is based on the original patch, with some minor differences
to avoid path separator handling (instead I changed the current directory)
and to avoid error messages from `del` (when it finds no file to delete, it
logs a warning that apparently cannot be silenced by any of the flags).

While doing this I discovered that on Win32 fence-image-self-test and
cover-test are currently failing (regardless of these patches and
apparently also with any combination of MMX, SSE2, SSSE3).
I will try to investigate ASAP.

I believe that the first two commits should be "obviously" ok, while the
other two are not as trivial.
The whole patchset has been tested on Win32, both in an MSYS environment
and on cmd.exe (in both cases, using GNU make).

Andrea

PS: it looks like pixman has adopted Reviewed-by, Signed-off-by, etc.
I assume that the first three patches should be marked signed off by me,
while I am unsure about the tagging for the last patch (I guess it should
be credited to Simon).


Unfortunately, I don't have a Windows machine to check this.
>
> Thanks,
>
> Oded
>
> >
> > On Mon, Jun 1, 2015 at 8:44 AM, Andrea Canciani <ranm...@gmail.com>
> wrote:
> >>
> >> The ping was good and the changes make sense. Currently I am replying
> from
> >> the phone and I will not have access to my computer until the 3rd, but
> I am
> >> flagging this mail so that I will test and review the patch asap.
> >>
> >> Sorry for the delay
> >> Andrea
> >>
> >> On Jun 1, 2015 8:09 AM, "Siarhei Siamashka" <
> siarhei.siamas...@gmail.com>
> >> wrote:
> >>>
> >>> On Wed, 22 Apr 2015 04:01:31 +0200
> >>> Simon Richter <simon.rich...@hogyros.de> wrote:
> >>>
> >>> > This finds out whether the standard shell for make is cmd.exe, and
> >>> > adjusts
> >>> > the build process accordingly.
> >>> > ---
> >>> >  Makefile.win32.common | 14 --
> >>> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >>>
> >>> Thanks for the patch.
> >>>
> >>> But it is a bit difficult for non-Windows folks to see if
> >>> it makes any sense or not. So it would be best if some other
> >>> Windows user could review and confirm that it works.
> >>>
> >>> I have added Andrea Canciani (the author of the win32 makefiles)
> >>> to CC.
> >>>
> >>> > diff --git a/Makefile.win32.common b/Makefile.win32.common
> >>> > index 777f94c..335886f 100644
> >>> > --- a/Makefile.win32.common
> >>> > +++ b/Makefile.win32.common
> >>> > @@ -1,5 +1,15 @@
> >>> &

[Pixman] [PATCH] test: Fix fence-image-self-test on Mac

[Andrea Canciani]

On MacOS X an out-of-bounds access to an mmap-ed region typically
results in a SIGBUS, but fence-image-self-test was only accepting a
SIGSEGV as notification of invalid access.

Fixes fence-image-self-test
---
 test/fence-image-self-test.c | 12 +++-
 test/utils.c |  6 +++---
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/test/fence-image-self-test.c b/test/fence-image-self-test.c
index c883038..484c4ac 100644
--- a/test/fence-image-self-test.c
+++ b/test/fence-image-self-test.c
@@ -73,7 +73,7 @@ prinfo (const char *fmt, ...)
 }
 
 static void
-do_expect_segv (void (*fn)(void *), void *data)
+do_expect_signal (void (*fn)(void *), void *data)
 {
 struct sigaction sa;
 
@@ -82,6 +82,8 @@ do_expect_segv (void (*fn)(void *), void *data)
 sa.sa_sigaction = segv_handler;
 if (sigaction (SIGSEGV, , NULL) == -1)
 die ("sigaction failed", errno);
+if (sigaction (SIGBUS, , NULL) == -1)
+die ("sigaction failed", errno);
 
 (*fn)(data);
 
@@ -96,7 +98,7 @@ do_expect_segv (void (*fn)(void *), void *data)
  * to exit with success, and return failure otherwise.
  */
 static pixman_bool_t
-expect_segv (void (*fn)(void *), void *data)
+expect_signal (void (*fn)(void *), void *data)
 {
 pid_t pid, wp;
 int status;
@@ -106,7 +108,7 @@ expect_segv (void (*fn)(void *), void *data)
 die ("fork failed", errno);
 
 if (pid == 0)
-do_expect_segv (fn, data); /* never returns */
+do_expect_signal (fn, data); /* never returns */
 
 wp = waitpid (pid, , 0);
 if (wp != pid)
@@ -131,9 +133,9 @@ test_read_fault (uint8_t *p, int offset)
 {
 prinfo ("*(uint8_t *)(%p + %d)", p, offset);
 
-if (expect_segv (read_u8, p + offset))
+if (expect_signal (read_u8, p + offset))
 {
-prinfo ("\tSEGV OK\n");
+prinfo ("\signal OK\n");
 
 return TRUE;
 }
diff --git a/test/utils.c b/test/utils.c
index 8657966..f8e42a5 100644
--- a/test/utils.c
+++ b/test/utils.c
@@ -471,9 +471,9 @@ fence_image_destroy (pixman_image_t *image, void *data)
  * min_width is only a minimum width for the image. The width is aligned up
  * for the row size to be divisible by both page size and pixel size.
  *
- * If stride_fence is true, the additional page on each row will be armed
- * to cause SIGSEVG on all accesses. This should catch all accesses outside
- * the valid row pixels.
+ * If stride_fence is true, the additional page on each row will be
+ * armed to cause SIGSEGV or SIGBUS on all accesses. This should catch
+ * all accesses outside the valid row pixels.
  */
 pixman_image_t *
 fence_image_create_bits (pixman_format_code_t format,
-- 
2.3.8 (Apple Git-58)

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH] test: Fix fence-image-self-test on Mac

[Andrea Canciani]

On MacOS X, according to the manpage of mprotect(), "When a program
violates the protections of a page, it gets a SIGBUS or SIGSEGV
signal.", but fence-image-self-test was only accepting a SIGSEGV as
notification of invalid access.

Fixes fence-image-self-test

Reviewed-by: Pekka Paalanen 
---
 test/fence-image-self-test.c | 12 +++-
 test/utils.c |  6 +++---
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/test/fence-image-self-test.c b/test/fence-image-self-test.c
index c883038..c80b3cf 100644
--- a/test/fence-image-self-test.c
+++ b/test/fence-image-self-test.c
@@ -73,7 +73,7 @@ prinfo (const char *fmt, ...)
 }
 
 static void
-do_expect_segv (void (*fn)(void *), void *data)
+do_expect_signal (void (*fn)(void *), void *data)
 {
 struct sigaction sa;
 
@@ -82,6 +82,8 @@ do_expect_segv (void (*fn)(void *), void *data)
 sa.sa_sigaction = segv_handler;
 if (sigaction (SIGSEGV, , NULL) == -1)
 die ("sigaction failed", errno);
+if (sigaction (SIGBUS, , NULL) == -1)
+die ("sigaction failed", errno);
 
 (*fn)(data);
 
@@ -96,7 +98,7 @@ do_expect_segv (void (*fn)(void *), void *data)
  * to exit with success, and return failure otherwise.
  */
 static pixman_bool_t
-expect_segv (void (*fn)(void *), void *data)
+expect_signal (void (*fn)(void *), void *data)
 {
 pid_t pid, wp;
 int status;
@@ -106,7 +108,7 @@ expect_segv (void (*fn)(void *), void *data)
 die ("fork failed", errno);
 
 if (pid == 0)
-do_expect_segv (fn, data); /* never returns */
+do_expect_signal (fn, data); /* never returns */
 
 wp = waitpid (pid, , 0);
 if (wp != pid)
@@ -131,9 +133,9 @@ test_read_fault (uint8_t *p, int offset)
 {
 prinfo ("*(uint8_t *)(%p + %d)", p, offset);
 
-if (expect_segv (read_u8, p + offset))
+if (expect_signal (read_u8, p + offset))
 {
-prinfo ("\tSEGV OK\n");
+prinfo ("\tsignal OK\n");
 
 return TRUE;
 }
diff --git a/test/utils.c b/test/utils.c
index 8657966..f8e42a5 100644
--- a/test/utils.c
+++ b/test/utils.c
@@ -471,9 +471,9 @@ fence_image_destroy (pixman_image_t *image, void *data)
  * min_width is only a minimum width for the image. The width is aligned up
  * for the row size to be divisible by both page size and pixel size.
  *
- * If stride_fence is true, the additional page on each row will be armed
- * to cause SIGSEVG on all accesses. This should catch all accesses outside
- * the valid row pixels.
+ * If stride_fence is true, the additional page on each row will be
+ * armed to cause SIGSEGV or SIGBUS on all accesses. This should catch
+ * all accesses outside the valid row pixels.
  */
 pixman_image_t *
 fence_image_create_bits (pixman_format_code_t format,
-- 
2.3.8 (Apple Git-58)

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] Pixman not building on MacOS X 10.11

[Andrea Canciani]

On Sun, Oct 11, 2015 at 5:30 AM, Siarhei Siamashka <
siarhei.siamas...@gmail.com> wrote:

> On Sun, 11 Oct 2015 04:53:08 +0300
> Siarhei Siamashka <siarhei.siamas...@gmail.com> wrote:
>
> > On Sat, 10 Oct 2015 16:03:53 -0700
> > Jeremy Huddleston Sequoia <jerem...@freedesktop.org> wrote:
> >
> > > > On Oct 10, 2015, at 13:48, Andrea Canciani <ranm...@gmail.com>
> wrote:
> > > > The attached hack gets the code to compile on modern clang, but I
> > > > believe first of all we should improve the configure.ac detection
> code
> > > > so that pixman can actually build both on old and on new clang
> > > > versions (possibly with mmx disabled, if the asm constraints we need
> > > > are not implemented).
> >
> > This workaround looks reasonable to me. We should probably just drop
> > the whole "ifdef __OPTIMIZE__" part in
> >
> http://cgit.freedesktop.org/pixman/tree/pixman/pixman-mmx.c?id=pixman-0.32.8#n92
> >
> > I don't quite like the fact that this way of returning results from
> > a macro is a GNU C specific extension. But as you said, the configure
> > test can be updated to better match the code and also check if the
> > compiler supports this particular construct.
> >
> > Could you please submit the final variant of your patch in a
> > "git format-patch" format with a commit message and your
> > Signed-off-by tag?
>
> After looking at this issue a bit more, I realized that we are
> about to add a second layer of workarounds on top of the existing
> old workarounds :-)
>

The attached patch should fix the issue with only minor changes.
It keeps the workarounds :( but somewhat it simplifies them :)
I followed your suggestion of checking block expressions.
Given that the _mm_shuffle_pi16() function is always used in a "return"
statement, if needed we could avoid the usage of block expressions by
defining a macro "_return_mm_shuffle_pi16()" (which would return the result
of the operation instead of making it available as an expression) both for
the xmmintrin branch and for the hand-coded one.

The original problem is that certain compilers (just GCC?) did not
> support some intrinsics when compiling MMX code (_mm_movemask_pi8,
> _mm_mulhi_pu16, _mm_shuffle_pi16) and we got the following code:
>
> http://cgit.freedesktop.org/pixman/tree/pixman/pixman-mmx.c?id=pixman-0.32.8#n66
>
> In fact, these instructions were not available as part of the original
> MMX, but only got introduced later with AMD Extended 3DNow! and Intel
> SSE1. This is mentioned in the commit messages:
>
> http://cgit.freedesktop.org/pixman/commit/?id=84221f4c1687b8ea14e9cbdc78b2ba7258e62c9e
>
> http://cgit.freedesktop.org/pixman/commit/?id=14208344964f341a7b4a704b05cf4804c23792e9
>
> These extra instructions are unofficially known as MMX2. But GCC does
> not have a separate option for "-mmmx2". Instead the GCC manual says
> that these intrinsics are available when either "-msse" or a
> combination of "-m3dnow -march=athlon" is used:
>
> https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/x86-Built-in-Functions.html#x86-Built-in-Functions
>
>
> Now I wonder if the comment "We have to compile with -msse to use
> xmmintrin.h" is still valid. I tried to tweak the following ifdef to
> use the part of code, which includes  and the it compiled
> fine for me with CFLAGS="-O2 -m32" using recent versions of GCC and
> Clang:
>
> http://cgit.freedesktop.org/pixman/tree/pixman/pixman-mmx.c?id=pixman-0.32.8#n63
>
> I believe that this might be somehow related to the new __ALL_ISA__
> define, which had been mentioned in 2013:
> https://gcc.gnu.org/ml/gcc-patches/2013-04/txts5M0c0uU9y.txt
>
> So what about just dropping this ugly stuff and adding a configure
> check, which would verify if the MMX code can include ?
>

I would love getting rid of the workarounds, but I'm somewhat worried about
the possibility of regressions.
If you believe is a valid option, we might definitely try to pursue it.

What is the best way forward?

Andrea

And if this configure check fails (because of using an old compiler),
> then simply disable MMX support when building pixman.
>
> --
> Best regards,
> Siarhei Siamashka
>


0001-mmx-Improve-detection-of-support-for-K-constraint.patch
Description: Binary data
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] Pixman not building on MacOS X 10.11

[Andrea Canciani]

It looks like the latest XCode cannot build pixman:

$ clang --version
Apple LLVM version 7.0.0 (clang-700.0.72)
Target: x86_64-apple-darwin14.5.0
Thread model: posix

pixman-mmx.c:100:20: error: constraint 'K' expects an integer constant
expression
: "y" (__A), "K" (__N)
  ^~~
Reported here:
https://github.com/Homebrew/homebrew/issues/41056

It looks like the test performed in configure.ac does not match the code
with sufficient accuracy.
In particular, the constant is hardcoded in the configure.ac test, while in
the code it is only available after inlining.

The attached hack gets the code to compile on modern clang, but I believe
first of all we should improve the configure.ac detection code so that
pixman can actually build both on old and on new clang versions (possibly
with mmx disabled, if the asm constraints we need are not implemented).

Andrea
diff --git a/pixman/pixman-mmx.c b/pixman/pixman-mmx.c
index 05c48a4..412beb9 100644
--- a/pixman/pixman-mmx.c
+++ b/pixman/pixman-mmx.c
@@ -89,7 +89,7 @@ _mm_mulhi_pu16 (__m64 __A, __m64 __B)
 return __A;
 }
 
-#  ifdef __OPTIMIZE__
+#  if 0
 extern __inline __m64 __attribute__((__gnu_inline__, __always_inline__, 
__artificial__))
 _mm_shuffle_pi16 (__m64 __A, int8_t const __N)
 {
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Allow building on Windows with cmd.exe

[Andrea Canciani]

The patch does not regress the mingw-based build (basically that used in
http://cairographics.org/end_to_end_build_for_win32/ )
I did not manage to get the build working on the cmd.exe shell. Do you have
any instructions (or possibly indications about what is the correct
environment) for that?

AFAICT the change still relies on a Unix-like make command.
A bigger (but possibly even more interesting) change would be an
NMAKE-based build, so that the command-line tools available in Visual
Studio are sufficient for building the whole project.

Andrea

On Mon, Jun 1, 2015 at 8:44 AM, Andrea Canciani ranm...@gmail.com wrote:

 The ping was good and the changes make sense. Currently I am replying from
 the phone and I will not have access to my computer until the 3rd, but I am
 flagging this mail so that I will test and review the patch asap.

 Sorry for the delay
 Andrea
 On Jun 1, 2015 8:09 AM, Siarhei Siamashka siarhei.siamas...@gmail.com
 wrote:

 On Wed, 22 Apr 2015 04:01:31 +0200
 Simon Richter simon.rich...@hogyros.de wrote:

  This finds out whether the standard shell for make is cmd.exe, and
 adjusts
  the build process accordingly.
  ---
   Makefile.win32.common | 14 --
   1 file changed, 12 insertions(+), 2 deletions(-)

 Thanks for the patch.

 But it is a bit difficult for non-Windows folks to see if
 it makes any sense or not. So it would be best if some other
 Windows user could review and confirm that it works.

 I have added Andrea Canciani (the author of the win32 makefiles)
 to CC.

  diff --git a/Makefile.win32.common b/Makefile.win32.common
  index 777f94c..335886f 100644
  --- a/Makefile.win32.common
  +++ b/Makefile.win32.common
  @@ -1,5 +1,15 @@
   LIBRARY = pixman-1
 
  +ifeq ($(shell echo ),)
  +# POSIX style shell
  +mkdir_p = mkdir -p $1
  +rm = $(RM) $1
  +else
  +# DOS/Windows style shell
  +mkdir_p = if not exist $(subst /,\,$1) md $(subst /,\,$1)
  +rm = del $(subst /,\,$1)
  +endif
  +
   CC = cl
   LD = link
   AR = lib
  @@ -47,10 +57,10 @@ endif
 
 
   $(CFG_VAR)/%.obj: %.c $(libpixman_headers)
  - @mkdir -p $(CFG_VAR)
  + @$(call mkdir_p,$(@D))
@$(CC) -c $(PIXMAN_CFLAGS) -Fo$@ $
 
   clean: inform
  - @$(RM) $(CFG_VAR)/*.{exe,ilk,lib,obj,pdb} $(BUILT_SOURCES) ||
 exit 0
  + @-$(call rm,$(CFG_VAR)/*.exe $(CFG_VAR)/*.ilk $(CFG_VAR)/*.lib
 $(CFG_VAR)/*.obj $(CFG_VAR)/*.pdb} $(BUILT_SOURCES))
 
   .PHONY: inform clean



 --
 Best regards,
 Siarhei Siamashka


___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH] test: Fix the win32 build

[Andrea Canciani]

The win32 build has no config.h, so HAVE_CONFIG_H should be checked
before including it, as in utils.h.
---
 test/thread-test.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/test/thread-test.c b/test/thread-test.c
index fa21933..0b07b26 100644
--- a/test/thread-test.c
+++ b/test/thread-test.c
@@ -1,4 +1,4 @@
-#include config.h
+#include utils.h
 
 #ifndef HAVE_PTHREADS
 
@@ -12,7 +12,6 @@ int main ()
 
 #include stdlib.h
 #include pthread.h
-#include utils.h
 
 typedef struct
 {
-- 
1.8.3.4 (Apple Git-47)

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 2/2] test: Add new thread-test program

[Andrea Canciani]

Sorry, I didn't realize it beforehand, but I just noticed that thread-test
fails on MacOS.
This happens because it relies on the availability of OpenMP to have a
thread-local state for the prng.
Would it be ok to have each thread explicitly keep the state of its prng or
is this another element that the test is supposed to be checking?

Andrea

On Sat, Sep 28, 2013 at 5:38 PM, Søren Sandmann sandm...@cs.au.dk wrote:

 From: Søren Sandmann Pedersen s...@redhat.com

 This test program allocates an array of 16 uint32_ts and spawns 16
 threads that each use one of the allocated uint32_ts as destination
 for a large number of composite operations. Each thread then checks
 whether the results have the right crc32 checksum.

 The purpose of this test is catch errors where memory outside images
 is read and then written back. Such out-of-bounds accesses are broken
 when multiple threads are involved.
 ---
  test/Makefile.am  |6 +-
  test/Makefile.sources |1 +
  test/thread-test.c|  183
 +
  3 files changed, 187 insertions(+), 3 deletions(-)
  create mode 100644 test/thread-test.c

 diff --git a/test/Makefile.am b/test/Makefile.am
 index 5d901d5..80f3537 100644
 --- a/test/Makefile.am
 +++ b/test/Makefile.am
 @@ -1,8 +1,8 @@
  include $(top_srcdir)/test/Makefile.sources

 -AM_CFLAGS = $(OPENMP_CFLAGS)
 -AM_LDFLAGS = $(OPENMP_CFLAGS) $(TESTPROGS_EXTRA_LDFLAGS)
 -LDADD = libutils.la $(top_builddir)/pixman/libpixman-1.la -lm
  $(PNG_LIBS)
 +AM_CFLAGS = $(OPENMP_CFLAGS) $(PTHREAD_CFLAGS)$
 +AM_LDFLAGS = $(OPENMP_CFLAGS) $(TESTPROGS_EXTRA_LDFLAGS)
 $(PTHREAD_LDFLAGS)
 +LDADD = libutils.la $(top_builddir)/pixman/libpixman-1.la -lm
  $(PNG_LIBS) $(PTHREAD_LIBS)
  AM_CPPFLAGS = -I$(top_srcdir)/pixman -I$(top_builddir)/pixman
 $(PNG_CFLAGS)

  libutils_la_SOURCES = $(libutils_sources) $(libutils_headers)
 diff --git a/test/Makefile.sources b/test/Makefile.sources
 index 2fabdb5..2ae5d9f 100644
 --- a/test/Makefile.sources
 +++ b/test/Makefile.sources
 @@ -13,6 +13,7 @@ TESTPROGRAMS =\
 infinite-loop   \
 trap-crasher\
 alpha-loop  \
 +   thread-test \
 scaling-crash-test  \
 scaling-helpers-test\
 gradient-crash-test \
 diff --git a/test/thread-test.c b/test/thread-test.c
 new file mode 100644
 index 000..917f417
 --- /dev/null
 +++ b/test/thread-test.c
 @@ -0,0 +1,183 @@
 +#include config.h
 +
 +#ifndef HAVE_PTHREADS
 +
 +int main ()
 +{
 +printf (Skipped thread-test - pthreads not supported\n);
 +return 0;
 +}
 +
 +#else
 +
 +#include stdlib.h
 +#include pthread.h
 +#include utils.h
 +
 +typedef struct
 +{
 +int thread_no;
 +uint32_t *dest;
 +uint32_t crc32;
 +} info_t;
 +
 +static const pixman_op_t operators[] =
 +{
 +PIXMAN_OP_SRC,
 +PIXMAN_OP_OVER,
 +PIXMAN_OP_ADD,
 +PIXMAN_OP_CLEAR,
 +PIXMAN_OP_SRC,
 +PIXMAN_OP_DST,
 +PIXMAN_OP_OVER,
 +PIXMAN_OP_OVER_REVERSE,
 +PIXMAN_OP_IN,
 +PIXMAN_OP_IN_REVERSE,
 +PIXMAN_OP_OUT,
 +PIXMAN_OP_OUT_REVERSE,
 +PIXMAN_OP_ATOP,
 +PIXMAN_OP_ATOP_REVERSE,
 +PIXMAN_OP_XOR,
 +PIXMAN_OP_ADD,
 +PIXMAN_OP_SATURATE,
 +PIXMAN_OP_DISJOINT_CLEAR,
 +PIXMAN_OP_DISJOINT_SRC,
 +PIXMAN_OP_DISJOINT_DST,
 +PIXMAN_OP_DISJOINT_OVER,
 +PIXMAN_OP_DISJOINT_OVER_REVERSE,
 +PIXMAN_OP_DISJOINT_IN,
 +PIXMAN_OP_DISJOINT_IN_REVERSE,
 +PIXMAN_OP_DISJOINT_OUT,
 +PIXMAN_OP_DISJOINT_OUT_REVERSE,
 +PIXMAN_OP_DISJOINT_ATOP,
 +PIXMAN_OP_DISJOINT_ATOP_REVERSE,
 +PIXMAN_OP_DISJOINT_XOR,
 +PIXMAN_OP_CONJOINT_CLEAR,
 +PIXMAN_OP_CONJOINT_SRC,
 +PIXMAN_OP_CONJOINT_DST,
 +PIXMAN_OP_CONJOINT_OVER,
 +PIXMAN_OP_CONJOINT_OVER_REVERSE,
 +PIXMAN_OP_CONJOINT_IN,
 +PIXMAN_OP_CONJOINT_IN_REVERSE,
 +PIXMAN_OP_CONJOINT_OUT,
 +PIXMAN_OP_CONJOINT_OUT_REVERSE,
 +PIXMAN_OP_CONJOINT_ATOP,
 +PIXMAN_OP_CONJOINT_ATOP_REVERSE,
 +PIXMAN_OP_CONJOINT_XOR,
 +PIXMAN_OP_MULTIPLY,
 +PIXMAN_OP_SCREEN,
 +PIXMAN_OP_OVERLAY,
 +PIXMAN_OP_DARKEN,
 +PIXMAN_OP_LIGHTEN,
 +PIXMAN_OP_COLOR_DODGE,
 +PIXMAN_OP_COLOR_BURN,
 +PIXMAN_OP_HARD_LIGHT,
 +PIXMAN_OP_DIFFERENCE,
 +PIXMAN_OP_EXCLUSION,
 +};
 +
 +static const pixman_format_code_t formats[] =
 +{
 +PIXMAN_a8r8g8b8,
 +PIXMAN_r5g6b5,
 +PIXMAN_a8,
 +PIXMAN_a4,
 +PIXMAN_a1,
 +PIXMAN_b5g6r5,
 +PIXMAN_r8g8b8a8,
 +PIXMAN_a4r4g4b4
 +};
 +
 +#define N_ROUNDS 32768
 +
 +#define RAND_ELT(arr)  \
 +arr[prng_rand() % ARRAY_LENGTH (arr)]
 +
 +static void *
 +thread (void *data)
 +{
 +info_t *info = data;
 +uint32_t crc32 = 0x0;
 +uint32_t src_buf[64];
 +pixman_image_t *dst_img, *src_img;
 +int i;
 +
 +prng_srand (info-thread_no);
 +
 +for (i = 0; i  N_ROUNDS; ++i)
 +{
 +   *info-dest = 

[Pixman] [PATCH] Fix thread-test on non-OpenMP systems

[Andrea Canciani]

The non-reentrant versions of prng_* functions are thread-safe only in
OpenMP-enabled builds.

Fixes thread-test failing when compiled with Clang (both on Linux and
on MacOS).
---
 test/thread-test.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/test/thread-test.c b/test/thread-test.c
index f24c31d..088033e 100644
--- a/test/thread-test.c
+++ b/test/thread-test.c
@@ -18,6 +18,7 @@ typedef struct
 {
 int   thread_no;
 uint32_t *dst_buf;
+prng_tprng_state;
 } info_t;
 
 static const pixman_op_t operators[] = 
@@ -90,7 +91,7 @@ static const pixman_format_code_t formats[] =
 #define N_ROUNDS 8192
 
 #define RAND_ELT(arr)  \
-arr[prng_rand() % ARRAY_LENGTH (arr)]
+arr[prng_rand_r(info-prng_state) % ARRAY_LENGTH (arr)]
 
 #define DEST_WIDTH (7)
 
@@ -103,15 +104,17 @@ thread (void *data)
 pixman_image_t *dst_img, *src_img;
 int i;
 
-prng_srand (info-thread_no);
+prng_srand_r (info-prng_state, info-thread_no);
 
 for (i = 0; i  N_ROUNDS; ++i)
 {
pixman_op_t op;
int rand1, rand2;
 
-   prng_randmemset (info-dst_buf, DEST_WIDTH * sizeof (uint32_t), 0);
-   prng_randmemset (src_buf, sizeof (src_buf), 0);
+   prng_randmemset_r (info-prng_state, info-dst_buf,
+  DEST_WIDTH * sizeof (uint32_t), 0);
+   prng_randmemset_r (info-prng_state, src_buf,
+  sizeof (src_buf), 0);
 
src_img = pixman_image_create_bits (
RAND_ELT (formats), 4, 4, src_buf, 16);
@@ -122,8 +125,8 @@ thread (void *data)
image_endian_swap (src_img);
image_endian_swap (dst_img);

-   rand2 = prng_rand() % 4;
-   rand1 = prng_rand() % 4;
+   rand2 = prng_rand_r (info-prng_state) % 4;
+   rand1 = prng_rand_r (info-prng_state) % 4;
op = RAND_ELT (operators);
 
pixman_image_composite32 (
-- 
1.7.12.4 (Apple Git-37)

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 2/2] test: Add new thread-test program

[Andrea Canciani]

I just sent a wannabe patch for this issue.
I did not test on clang/linux, but I expect it to work in that environment
as well.
Can you confirm this?

TY
Andrea



On Wed, Oct 9, 2013 at 5:46 PM, Siarhei Siamashka 
siarhei.siamas...@gmail.com wrote:

 On Wed, 09 Oct 2013 17:27:16 +0200
 sandm...@cs.au.dk (Søren Sandmann) wrote:

  Siarhei Siamashka siarhei.siamas...@gmail.com writes:
 
   On Wed, 09 Oct 2013 17:06:06 +0200
   sandm...@cs.au.dk (Søren Sandmann) wrote:
  
   Andrea Canciani ranm...@gmail.com writes:
  
Sorry, I didn't realize it beforehand, but I just noticed that
 thread-test
fails on MacOS.
This happens because it relies on the availability of OpenMP to
 have a
thread-local state for the prng.
Would it be ok to have each thread explicitly keep the state of its
 prng or
is this another element that the test is supposed to be checking?
  
   It's not intended to test the thread-private-ness of the prng states,
 so
   yes, the simplest fix seems to be to just explicitly keep a prng_t
   for each thread.
  
   Would it really help to pass the test? The fast path cache is using the
   thread-local storage too.
 
  But the fast path cache is using the PIXMAN_DEFINE_THREAD_LOCAL macro
  that uses pthread_setspecific() on Mac OS. I suppose it would make sense
  to use that macro in thread_test as well, just to make sure it works.

 OK, this is a bit messy. The prng code is only compatible with OpenMP
 threads while the thread-test is using pthreads. It also fails with
 clang in Linux.

 Explicitly keeping a prng_t for each thread would be indeed a good
 solution. It also very likely is going to have a lower overhead.

 --
 Best regards,
 Siarhei Siamashka

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 2/2] test: Add new thread-test program

[Andrea Canciani]

I have some work on memleak related issues:

http://cgit.freedesktop.org/~ranma42/pixman/commit/?h=wip/simpleops-to-masterid=77db90242f7b7f12c5792480a10eb3a448e6c55f
and
http://cgit.freedesktop.org/~ranma42/pixman/commit/?h=wip/simpleops-to-masterid=ca41228a66fe9bbec354cdef034c144ce1619793

Do you think it might be worth to revive it and cleanup for review  merge?
Unfortunately they depend on simpleops, a header-library I wrote to
abstract (from compiler/platform) some low-level operations, like atomic
arithmetics, mutexes, threads and library constructors/destructors.
(currently all of them except init/fini are available in C11 and the
simpleops API tries to be as similar to C11 as possible, to ease C11-based
implementations and to make it easy to get rid of the simpleops dependency
when C11 support is sufficiently widespread).
Would it be ok to have it as dependency?

Andrea




On Sat, Sep 28, 2013 at 5:38 PM, Søren Sandmann sandm...@cs.au.dk wrote:

 From: Søren Sandmann Pedersen s...@redhat.com

 This test program allocates an array of 16 uint32_ts and spawns 16
 threads that each use one of the allocated uint32_ts as destination
 for a large number of composite operations. Each thread then checks
 whether the results have the right crc32 checksum.

 The purpose of this test is catch errors where memory outside images
 is read and then written back. Such out-of-bounds accesses are broken
 when multiple threads are involved.
 ---
  test/Makefile.am  |6 +-
  test/Makefile.sources |1 +
  test/thread-test.c|  183
 +
  3 files changed, 187 insertions(+), 3 deletions(-)
  create mode 100644 test/thread-test.c

 diff --git a/test/Makefile.am b/test/Makefile.am
 index 5d901d5..80f3537 100644
 --- a/test/Makefile.am
 +++ b/test/Makefile.am
 @@ -1,8 +1,8 @@
  include $(top_srcdir)/test/Makefile.sources

 -AM_CFLAGS = $(OPENMP_CFLAGS)
 -AM_LDFLAGS = $(OPENMP_CFLAGS) $(TESTPROGS_EXTRA_LDFLAGS)
 -LDADD = libutils.la $(top_builddir)/pixman/libpixman-1.la -lm
  $(PNG_LIBS)
 +AM_CFLAGS = $(OPENMP_CFLAGS) $(PTHREAD_CFLAGS)$
 +AM_LDFLAGS = $(OPENMP_CFLAGS) $(TESTPROGS_EXTRA_LDFLAGS)
 $(PTHREAD_LDFLAGS)
 +LDADD = libutils.la $(top_builddir)/pixman/libpixman-1.la -lm
  $(PNG_LIBS) $(PTHREAD_LIBS)
  AM_CPPFLAGS = -I$(top_srcdir)/pixman -I$(top_builddir)/pixman
 $(PNG_CFLAGS)

  libutils_la_SOURCES = $(libutils_sources) $(libutils_headers)
 diff --git a/test/Makefile.sources b/test/Makefile.sources
 index 2fabdb5..2ae5d9f 100644
 --- a/test/Makefile.sources
 +++ b/test/Makefile.sources
 @@ -13,6 +13,7 @@ TESTPROGRAMS =\
 infinite-loop   \
 trap-crasher\
 alpha-loop  \
 +   thread-test \
 scaling-crash-test  \
 scaling-helpers-test\
 gradient-crash-test \
 diff --git a/test/thread-test.c b/test/thread-test.c
 new file mode 100644
 index 000..917f417
 --- /dev/null
 +++ b/test/thread-test.c
 @@ -0,0 +1,183 @@
 +#include config.h
 +
 +#ifndef HAVE_PTHREADS
 +
 +int main ()
 +{
 +printf (Skipped thread-test - pthreads not supported\n);
 +return 0;
 +}
 +
 +#else
 +
 +#include stdlib.h
 +#include pthread.h
 +#include utils.h
 +
 +typedef struct
 +{
 +int thread_no;
 +uint32_t *dest;
 +uint32_t crc32;
 +} info_t;
 +
 +static const pixman_op_t operators[] =
 +{
 +PIXMAN_OP_SRC,
 +PIXMAN_OP_OVER,
 +PIXMAN_OP_ADD,
 +PIXMAN_OP_CLEAR,
 +PIXMAN_OP_SRC,
 +PIXMAN_OP_DST,
 +PIXMAN_OP_OVER,
 +PIXMAN_OP_OVER_REVERSE,
 +PIXMAN_OP_IN,
 +PIXMAN_OP_IN_REVERSE,
 +PIXMAN_OP_OUT,
 +PIXMAN_OP_OUT_REVERSE,
 +PIXMAN_OP_ATOP,
 +PIXMAN_OP_ATOP_REVERSE,
 +PIXMAN_OP_XOR,
 +PIXMAN_OP_ADD,
 +PIXMAN_OP_SATURATE,
 +PIXMAN_OP_DISJOINT_CLEAR,
 +PIXMAN_OP_DISJOINT_SRC,
 +PIXMAN_OP_DISJOINT_DST,
 +PIXMAN_OP_DISJOINT_OVER,
 +PIXMAN_OP_DISJOINT_OVER_REVERSE,
 +PIXMAN_OP_DISJOINT_IN,
 +PIXMAN_OP_DISJOINT_IN_REVERSE,
 +PIXMAN_OP_DISJOINT_OUT,
 +PIXMAN_OP_DISJOINT_OUT_REVERSE,
 +PIXMAN_OP_DISJOINT_ATOP,
 +PIXMAN_OP_DISJOINT_ATOP_REVERSE,
 +PIXMAN_OP_DISJOINT_XOR,
 +PIXMAN_OP_CONJOINT_CLEAR,
 +PIXMAN_OP_CONJOINT_SRC,
 +PIXMAN_OP_CONJOINT_DST,
 +PIXMAN_OP_CONJOINT_OVER,
 +PIXMAN_OP_CONJOINT_OVER_REVERSE,
 +PIXMAN_OP_CONJOINT_IN,
 +PIXMAN_OP_CONJOINT_IN_REVERSE,
 +PIXMAN_OP_CONJOINT_OUT,
 +PIXMAN_OP_CONJOINT_OUT_REVERSE,
 +PIXMAN_OP_CONJOINT_ATOP,
 +PIXMAN_OP_CONJOINT_ATOP_REVERSE,
 +PIXMAN_OP_CONJOINT_XOR,
 +PIXMAN_OP_MULTIPLY,
 +PIXMAN_OP_SCREEN,
 +PIXMAN_OP_OVERLAY,
 +PIXMAN_OP_DARKEN,
 +PIXMAN_OP_LIGHTEN,
 +PIXMAN_OP_COLOR_DODGE,
 +PIXMAN_OP_COLOR_BURN,
 +PIXMAN_OP_HARD_LIGHT,
 +PIXMAN_OP_DIFFERENCE,
 +PIXMAN_OP_EXCLUSION,
 +};
 +
 +static const pixman_format_code_t formats[] =
 +{
 +PIXMAN_a8r8g8b8,
 +PIXMAN_r5g6b5,
 +PIXMAN_a8,
 +

[Pixman] [PATCH 4/4] Add support for SSSE3 to the MSVC build system

[Andrea Canciani]

Handle SSSE3 just like MMX and SSE2.
---
 pixman/Makefile.win32 | 29 +++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/pixman/Makefile.win32 b/pixman/Makefile.win32
index 57ed7a5..7b64033 100644
--- a/pixman/Makefile.win32
+++ b/pixman/Makefile.win32
@@ -14,8 +14,14 @@ ifeq ($(SSE2_VAR),)
 SSE2_VAR=on
 endif
 
+SSSE3_VAR = $(SSSE3)
+ifeq ($(SSSE3_VAR),)
+SSSE3_VAR=on
+endif
+
 MMX_CFLAGS = -DUSE_X86_MMX -w14710 -w14714
 SSE2_CFLAGS = -DUSE_SSE2
+SSSE3_CFLAGS = -DUSE_SSSE3
 
 # MMX compilation flags
 ifeq ($(MMX_VAR),on)
@@ -29,10 +35,16 @@ PIXMAN_CFLAGS += $(SSE2_CFLAGS)
 libpixman_sources += pixman-sse2.c
 endif
 
+# SSSE3 compilation flags
+ifeq ($(SSSE3_VAR),on)
+PIXMAN_CFLAGS += $(SSSE3_CFLAGS)
+libpixman_sources += pixman-ssse3.c
+endif
+
 OBJECTS = $(patsubst %.c, $(CFG_VAR)/%.obj, $(libpixman_sources))
 
 # targets
-all: inform informMMX informSSE2 $(CFG_VAR)/$(LIBRARY).lib
+all: inform informMMX informSSE2 informSSSE3 $(CFG_VAR)/$(LIBRARY).lib
 
 informMMX:
 ifneq ($(MMX),off)
@@ -60,9 +72,22 @@ endif
 endif
 endif
 
+informSSSE3:
+ifneq ($(SSSE3),off)
+ifneq ($(SSSE3),on)
+ifneq ($(SSSE3),)
+   @echo Invalid specified SSE option : $(SSSE3).
+   @echo
+   @echo Possible choices for SSSE3 are 'on' or 'off'
+   @exit 1
+endif
+   @echo Setting SSSE3 flag to default value 'on'... (use SSSE3=on or 
SSSE3=off)
+endif
+endif
+
 
 # pixman linking
 $(CFG_VAR)/$(LIBRARY).lib: $(OBJECTS)
@$(AR) $(PIXMAN_ARFLAGS) -OUT:$@ $^
 
-.PHONY: all informMMX informSSE2
+.PHONY: all informMMX informSSE2 informSSSE3
-- 
1.7.12.4 (Apple Git-37)

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 2/4] Fix building of other programs on MSVC

[Andrea Canciani]

In d1434d112ca5cd325e4fb85fc60afd1b9e902786 the benchmarks have been
extended to include other programs as well and the variable names have
been updated accordingly in the autotools-based build system, but not
in the MSVC one.
---
 test/Makefile.win32 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/test/Makefile.win32 b/test/Makefile.win32
index b6254a3..6cfb4a7 100644
--- a/test/Makefile.win32
+++ b/test/Makefile.win32
@@ -11,12 +11,12 @@ TEST_LDADD = \
 
 libutils_OBJECTS = $(patsubst %.c, $(CFG_VAR)/%.obj, $(libutils_sources))
 
-SOURCES = $(patsubst %,   %.c,  $(TESTPROGRAMS) $(BENCHMARKS))
+SOURCES = $(patsubst %,   %.c,  $(TESTPROGRAMS) $(OTHERPROGRAMS))
 OBJECTS = $(patsubst %.c, $(CFG_VAR)/%.obj, $(SOURCES))
 TESTS   = $(patsubst %,   $(CFG_VAR)/%.exe, $(TESTPROGRAMS))
-BENCHS  = $(patsubst %,   $(CFG_VAR)/%.exe, $(BENCHMARKS))
+OTHERS  = $(patsubst %,   $(CFG_VAR)/%.exe, $(OTHERPROGRAMS))
 
-all: pixman inform $(TESTS) $(BENCHS)
+all: pixman inform $(TESTS) $(OTHERS)
 
 check: pixman inform $(TESTS)
@failures=0 ; \
-- 
1.7.12.4 (Apple Git-37)

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 1/4] Fix build on MSVC

[Andrea Canciani]

After a4c79d695d52c94647b1aff78548e5892d616b70 the MMX and SSE2 code
has some declarations after the beginning of a block, which is not
allowed by MSVC.

Fixes multiple errors like:

pixman-mmx.c(3625) : error C2275: '__m64' : illegal use of this type
as an expression

pixman-sse2.c(5708) : error C2275: '__m128i' : illegal use of this
type as an expression
---
 pixman/pixman-mmx.c  | 2 +-
 pixman/pixman-sse2.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/pixman/pixman-mmx.c b/pixman/pixman-mmx.c
index a0f59ef..f9a92ce 100644
--- a/pixman/pixman-mmx.c
+++ b/pixman/pixman-mmx.c
@@ -3579,7 +3579,6 @@ do {  
\
 __m64 b_lo = _mm_mullo_pi16 (_mm_unpacklo_pi8 (b, mm_zero), mm_wb);
\
 __m64 hi = _mm_add_pi16 (t_hi, b_hi);  
\
 __m64 lo = _mm_add_pi16 (t_lo, b_lo);  
\
-vx += unit_x;  
\
 /* calculate horizontal weights */ 
\
 __m64 mm_wh = _mm_add_pi16 (mm_addc7, _mm_xor_si64 (mm_xorc7,  
\
  _mm_srli_pi16 (mm_x,  
\
@@ -3587,6 +3586,7 @@ do {  
\
 /* horizontal interpolation */ 
\
 __m64 p = _mm_unpacklo_pi16 (lo, hi);  
\
 __m64 q = _mm_unpackhi_pi16 (lo, hi);  
\
+vx += unit_x;  
\
 lo = _mm_madd_pi16 (p, mm_wh); 
\
 hi = _mm_madd_pi16 (q, mm_wh); 
\
 mm_x = _mm_add_pi16 (mm_x, mm_ux); 
\
diff --git a/pixman/pixman-sse2.c b/pixman/pixman-sse2.c
index 42c7209..2ab2690 100644
--- a/pixman/pixman-sse2.c
+++ b/pixman/pixman-sse2.c
@@ -5626,10 +5626,10 @@ do {
\
 #define BILINEAR_INTERPOLATE_ONE_PIXEL_HELPER(pix, phase)  
\
 do {   
\
 __m128i xmm_wh, xmm_a, xmm_b;  
\
-(void)xmm_ux4; /* suppress warning: unused variable 'xmm_ux4' */   
\
 /* fetch 2x2 pixel block into sse2 registers */
\
 __m128i tltr = _mm_loadl_epi64 ((__m128i *)src_top[vx  16]);
\
 __m128i blbr = _mm_loadl_epi64 ((__m128i *)src_bottom[vx  16]); 
\
+(void)xmm_ux4; /* suppress warning: unused variable 'xmm_ux4' */   
\
 vx += unit_x;  
\
 /* vertical interpolation */   
\
 xmm_a = _mm_mullo_epi16 (_mm_unpacklo_epi8 (tltr, xmm_zero), xmm_wt);  
\
-- 
1.7.12.4 (Apple Git-37)

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [cairo] help building cairo on windows

[Andrea Canciani]

This reminds me that I have some patches to improve building pixman on win32:
http://cgit.freedesktop.org/~ranma42/pixman/commit/?h=wip/simpleops-to-master

Soren, is it ok if I push the attached patches?

I will work a little more on simpleops before proposing it again (in
particular, I'd like to make it behave like the gstreamer git
submodules and I need to investigate how it is integrated with
packaging and releasing), but the first two commits in the branch
should be ok for merge regardless of simpleops.

Andrea

On Mon, Sep 10, 2012 at 8:17 PM, Dimiter 'malkia' Stanev
mal...@gmail.com wrote:
 On 9/10/2012 7:04 AM, Cosmin Apreutesei wrote:

 Hi,

 I have to build cairo on windows because I want to use the recording
 surface and I can't find binaries above 1.10 anywhere on the net (till
 now I have used the binaries from GTK but these are old and the
 recording surface doesn't work with those).

 Trying to build pixman with:

 make CFG=debug -f Makefile.win32


 gives me:

 pixman-mmx.c(545) : error C2143: syntax error : missing ';' before 'type'

 and then 100 syntax errors like that (other c files compile ok until
 the mmx one).

 The sources were pulled today from git. The environment is VSE2008.
 make is 3.81 from mingw (I also had to throw in pkg-config binaries +
 dependencies from GTK).

 I also tried to build the latest pixman (0.26.2) with the same command
 but it failed with:

 Entering directory `/x/work/pixman-0.26.2/test'
 Makefile.win32: No such file or directory

 I also tried building with mozilla-build from the mingw bash prompt,
 with the same results.


 So what's the best (preferably from cmdline) way to build pixman and
 cairo under windows? And which is the best version? The release or the
 git one?

 Btw, if anyone has an up-to-date cairo.dll that would work for me too.


 Hi Cosmin,

 With this patch you should be able to compile.

 Thanks,
 Dimiter 'malkia' Stanev

 pixman-mmx.diff

 diff --git a/pixman/pixman-mmx.c b/pixman/pixman-mmx.c
 index 74a5e87..9e597db 100644
 --- a/pixman/pixman-mmx.c
 +++ b/pixman/pixman-mmx.c
 @@ -541,7 +541,7 @@ expand565 (__m64 pixel, int pos)
  static force_inline void
  expand_4xpacked565 (__m64 vin, __m64 *vout0, __m64 *vout1, int full_alpha)
  {
 -__m64 t0, t1, alpha = _mm_setzero_si64 ();;
 +__m64 t0, t1, alpha = _mm_setzero_si64 ();
  __m64 r = _mm_and_si64 (vin, MC (expand_565_r));
  __m64 g = _mm_and_si64 (vin, MC (expand_565_g));
  __m64 b = _mm_and_si64 (vin, MC (expand_565_b));
 @@ -1902,22 +1902,22 @@ mmx_composite_over__0565
 (pixman_implementation_t *imp,
 while (w = 4)
 {
 __m64 vdest = *(__m64 *)dst;
 -   __m64 v0, v1, v2, v3;
 +   __m64 v0, v1, v2, v3, vsrc0, vsrc1, vsrc2, vsrc3;

 expand_4x565 (vdest, v0, v1, v2, v3, 0);

 -   __m64 vsrc0 = load ((src + 0));
 -   __m64 vsrc1 = load ((src + 1));
 -   __m64 vsrc2 = load ((src + 2));
 -   __m64 vsrc3 = load ((src + 3));
 -
 +   vsrc0 = load ((src + 0));
 +   vsrc1 = load ((src + 1));
 +   vsrc2 = load ((src + 2));
 +   vsrc3 = load ((src + 3));
 +
 v0 = over (vsrc0, expand_alpha (vsrc0), v0);
 v1 = over (vsrc1, expand_alpha (vsrc1), v1);
 v2 = over (vsrc2, expand_alpha (vsrc2), v2);
 v3 = over (vsrc3, expand_alpha (vsrc3), v3);
 -
 +
 *(__m64 *)dst = pack_4x565 (v0, v1, v2, v3);
 -
 +
 w -= 4;
 dst += 4;
 src += 4;
 @@ -2454,22 +2454,22 @@ mmx_composite_over_n_8_0565 (pixman_implementation_t
 *imp,
 else if (m0 | m1 | m2 | m3)
 {
 __m64 vdest = *(__m64 *)dst;
 -   __m64 v0, v1, v2, v3;
 +   __m64 v0, v1, v2, v3, vm0, vm1, vm2, vm3;

 expand_4x565 (vdest, v0, v1, v2, v3, 0);

 -   __m64 vm0 = to_m64 (m0);
 +   vm0 = to_m64 (m0);
 v0 = in_over (vsrc, vsrca, expand_alpha_rev (vm0), v0);
 -
 -   __m64 vm1 = to_m64 (m1);
 +
 +   vm1 = to_m64 (m1);
 v1 = in_over (vsrc, vsrca, expand_alpha_rev (vm1), v1);
 -
 -   __m64 vm2 = to_m64 (m2);
 +
 +   vm2 = to_m64 (m2);
 v2 = in_over (vsrc, vsrca, expand_alpha_rev (vm2), v2);
 -
 -   __m64 vm3 = to_m64 (m3);
 +
 +   vm3 = to_m64 (m3);
 v3 = in_over (vsrc, vsrca, expand_alpha_rev (vm3), v3);
 -
 +
 *(__m64 *)dst = pack_4x565 (v0, v1, v2, v3);;
 }

 @@ -3545,32 +3545,35 @@ mmx_composite_over_reverse_n_
 (pixman_implementation_t *imp,

  #define BILINEAR_INTERPOLATE_ONE_PIXEL(pix)
 \
  do {
 \
 +__m64 t_hi, t_lo, b_hi, b_lo, hi, lo;  \
  /* fetch 2x2 pixel block into 2 mmx registers */
 \
  __m64 t = ldq_u ((__m64 *)src_top [pixman_fixed_to_int (vx)]);
 \
  __m64 b = ldq_u ((__m64 

Re: [Pixman] [PATCH 1/1] Disable implementations mentioned in the PIXMAN_DISABLE environment variable.

[Andrea Canciani]

On Sun, Feb 26, 2012 at 4:12 PM, Matt Turner matts...@gmail.com wrote:
 On Sat, Feb 25, 2012 at 9:39 PM, Søren Sandmann sandm...@cs.au.dk wrote:
 From: Søren Sandmann Pedersen s...@redhat.com

 With this, it becomes possible to do

     PIXMAN_DISABLE=sse2 mmx some_app

 which will run some_app without SSE2 and MMX enabled. This is useful
 for benchmarking, testing and narrowing down bugs.
 ---
  pixman/pixman-cpu.c |   44 
  1 files changed, 32 insertions(+), 12 deletions(-)

 diff --git a/pixman/pixman-cpu.c b/pixman/pixman-cpu.c
 index fcf591a..ba89df5 100644
 --- a/pixman/pixman-cpu.c
 +++ b/pixman/pixman-cpu.c
 @@ -24,6 +24,7 @@
  #endif

  #include string.h
 +#include stdlib.h

  #if defined(USE_ARM_SIMD)  defined(_MSC_VER)
  /* Needed for EXCEPTION_ILLEGAL_INSTRUCTION */
 @@ -328,7 +329,6 @@ pixman_arm_read_auxv_or_cpu_features ()

  #elif defined (__linux__) /* linux ELF */

 -#include stdlib.h
  #include unistd.h
  #include sys/types.h
  #include sys/stat.h
 @@ -711,51 +711,71 @@ pixman_have_sse2 (void)
  #endif /* __amd64__ */
  #endif

 +static pixman_bool_t
 +disabled (const char *name)
 +{
 +    const char *e;
 +
 +    if ((e = getenv (PIXMAN_DISABLE)))
 +    {
 +       if (strstr (e, name))

I think that strstr might cause substrings to also disable unintended backends.
This would already happen for mmx vs arm-iwmmxt.
Is this intentional?
Although in this specific case, it might be ok, I believe that it
might lead to unexpected behavior as other backends are added.

 +       {
 +           printf (pixman: disabled %s implementation\n, name);
 +           return TRUE;
 +       }
 +    }
 +
 +    return FALSE;
 +}
 +
  pixman_implementation_t *
  _pixman_choose_implementation (void)
  {
     pixman_implementation_t *imp;

     imp = _pixman_implementation_create_general();
 -    imp = _pixman_implementation_create_fast_path (imp);
 -
 +
 +    if (!disabled (fast))
 +       imp = _pixman_implementation_create_fast_path (imp);
 +
  #ifdef USE_X86_MMX
 -    if (pixman_have_mmx ())
 +    if (pixman_have_mmx ()  !disabled (mmx))
        imp = _pixman_implementation_create_mmx (imp);
  #endif

  #ifdef USE_SSE2
 -    if (pixman_have_sse2 ())
 +    if (pixman_have_sse2 ()  !disabled (sse2))
        imp = _pixman_implementation_create_sse2 (imp);
  #endif

  #ifdef USE_ARM_SIMD
 -    if (pixman_have_arm_simd ())
 +    if (pixman_have_arm_simd ()  !disabled (arm-simd))
        imp = _pixman_implementation_create_arm_simd (imp);
  #endif

  #ifdef USE_ARM_IWMMXT
 -    if (pixman_have_arm_iwmmxt ())
 +    if (pixman_have_arm_iwmmxt ()  !disabled (arm-iwmmxt))
        imp = _pixman_implementation_create_mmx (imp);
  #endif

  #ifdef USE_ARM_NEON
 -    if (pixman_have_arm_neon ())
 +    if (pixman_have_arm_neon ()  !disabled (arm-neon))
        imp = _pixman_implementation_create_arm_neon (imp);
  #endif

  #ifdef USE_MIPS_DSPR2
 -    if (pixman_have_mips_dspr2 ())
 +    if (pixman_have_mips_dspr2 ()  !disabled (mips-dspr2))
        imp = _pixman_implementation_create_mips_dspr2 (imp);
  #endif

  #ifdef USE_VMX
 -    if (pixman_have_vmx ())
 +    if (pixman_have_vmx ()  !disabled (vmx))
        imp = _pixman_implementation_create_vmx (imp);
  #endif

 -    imp = _pixman_implementation_create_noop (imp);
 -
 +    if (!disabled (noop))
 +       imp = _pixman_implementation_create_noop (imp);
 +
     return imp;
  }

 --
 1.6.0.6

 Indeed, this is very useful.

 I often seen if (!disabled(envvar)  supported()) where the disabling
 check is first in the if-statement. That's the only suggestion I have.
 Otherwise, this looks like a good change.

I agree that if the backend is disabled, we should avoid checking the support.
This way, we can use disable to ensure that the backend cannot break
the application status in any way (example: modify simd registers
without restoring them appropriately).

Andrea


 Reviewed-by: Matt Turner matts...@gmail.com
 ___
 Pixman mailing list
 Pixman@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/pixman
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 1/3] gradient-walker: Remove need_reset field

[Andrea Canciani]

A reset can be forced by making left_x  right_x, because no pos will
satisfy left = pos  right.
---
 pixman/pixman-gradient-walker.c |   17 ++---
 pixman/pixman-private.h |2 --
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/pixman/pixman-gradient-walker.c b/pixman/pixman-gradient-walker.c
index e7e724f..ae917e6 100644
--- a/pixman/pixman-gradient-walker.c
+++ b/pixman/pixman-gradient-walker.c
@@ -35,16 +35,13 @@ _pixman_gradient_walker_init (pixman_gradient_walker_t 
*walker,
 {
 walker-num_stops = gradient-n_stops;
 walker-stops = gradient-stops;
-walker-left_x= 0;
-walker-right_x   = 0x1;
-walker-stepper   = 0;
-walker-left_ag   = 0;
-walker-left_rb   = 0;
-walker-right_ag  = 0;
-walker-right_rb  = 0;
 walker-repeat= repeat;
 
-walker-need_reset = TRUE;
+/* The remaining part of the structure should be initalized by the
+ * reset function. Set left  right to trigger a reset as soon as
+ * the gradient walker is used. */
+walker-left_x= 0x1;
+walker-right_x   = 0;
 }
 
 static void
@@ -134,8 +131,6 @@ gradient_walker_reset (pixman_gradient_walker_t *walker,
int32_t width = right_x - left_x;
walker-stepper = ((1  24) + width / 2) / width;
 }
-
-walker-need_reset = FALSE;
 }
 
 uint32_t
@@ -145,7 +140,7 @@ _pixman_gradient_walker_pixel (pixman_gradient_walker_t 
*walker,
 int dist, idist;
 uint32_t t1, t2, a, color;
 
-if (walker-need_reset || x  walker-left_x || x = walker-right_x)
+if (x  walker-left_x || x = walker-right_x)
gradient_walker_reset (walker, x);
 
 dist  = ((int)(x - walker-left_x) * walker-stepper)  16;
diff --git a/pixman/pixman-private.h b/pixman/pixman-private.h
index 1443bfb..a41d46c 100644
--- a/pixman/pixman-private.h
+++ b/pixman/pixman-private.h
@@ -306,8 +306,6 @@ typedef struct
 pixman_gradient_stop_t *stops;
 int num_stops;
 pixman_repeat_trepeat;
-
-pixman_bool_t   need_reset;
 } pixman_gradient_walker_t;
 
 void
-- 
1.7.5.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 3/3] gradient-walker: Use appropriate size for left and right offset

[Andrea Canciani]

Otherwise if pos cannot be represented as an int32_t, the gradient is
reset at every step.
---
 pixman/pixman-gradient-walker.c |   13 +++--
 pixman/pixman-private.h |4 ++--
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/pixman/pixman-gradient-walker.c b/pixman/pixman-gradient-walker.c
index e0aa3a4..3c34d1b 100644
--- a/pixman/pixman-gradient-walker.c
+++ b/pixman/pixman-gradient-walker.c
@@ -87,12 +87,7 @@ gradient_walker_reset (pixman_gradient_walker_t *walker,
 right_x =  stops[n].x;
 right_c = stops[n].color;
 
-if (walker-repeat == PIXMAN_REPEAT_NORMAL)
-{
-   left_x  += (pos - x);
-   right_x += (pos - x);
-}
-else if (walker-repeat == PIXMAN_REPEAT_REFLECT)
+if (walker-repeat == PIXMAN_REPEAT_REFLECT)
 {
if ((int32_t)pos  0x1)
{
@@ -109,8 +104,6 @@ gradient_walker_reset (pixman_gradient_walker_t *walker,
 
x = 0x1 - x;
}
-   left_x  += (pos - x);
-   right_x += (pos - x);
 }
 else if (walker-repeat == PIXMAN_REPEAT_NONE)
 {
@@ -120,8 +113,8 @@ gradient_walker_reset (pixman_gradient_walker_t *walker,
left_c = right_c;
 }
 
-walker-left_x   = left_x;
-walker-right_x  = right_x;
+walker-left_x   = left_x + (pos - x);
+walker-right_x  = right_x + (pos - x);
 walker-left_ag  = ((left_c-alpha  8)  16)   | (left_c-green  8);
 walker-left_rb  = ((left_c-red  0xff00)  8)  | (left_c-blue  8);
 walker-right_ag = ((right_c-alpha  8)  16)  | (right_c-green  8);
diff --git a/pixman/pixman-private.h b/pixman/pixman-private.h
index a41d46c..2a78ab9 100644
--- a/pixman/pixman-private.h
+++ b/pixman/pixman-private.h
@@ -299,8 +299,8 @@ typedef struct
 uint32_tleft_rb;
 uint32_tright_ag;
 uint32_tright_rb;
-pixman_fixed_t left_x;
-pixman_fixed_t  right_x;
+pixman_fixed_48_16_tleft_x;
+pixman_fixed_48_16_tright_x;
 pixman_fixed_t  stepper;
 
 pixman_gradient_stop_t *stops;
-- 
1.7.5.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] pixman make error

[Andrea Canciani]

On Thu, Nov 17, 2011 at 7:16 AM,  lwin_...@dimension.com.tw wrote:
 hi Sir, Madam:
         When I compiled , the system show  a error message as the
 attachment. Please kindly advise me how to fix the problem. Thanks

Can you check if your make command is a GNU Make?
$ make --version
GNU Make 3.81
Copyright (C) 2006  Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.

If it is not, it might be unable to correctly handle the lastword
command used in those lines.
You might also want to check if GNU make is available under a
different name on your system (sometimes it is called gmake).

Andrea






 BR
 Winston
 ___
 Pixman mailing list
 Pixman@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/pixman


___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH] test: Produce autotools-looking report in the win32 build system

[Andrea Canciani]

Tweak the commands used to run the tests on win32 to make the output
look mostly like that produced by the autotools test system.

In addition to this, make sure that the exit status of the test target
is success (0) if and only if no failure occurred.
---
 test/Makefile.win32 |   21 -
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/test/Makefile.win32 b/test/Makefile.win32
index 307ba0c..c88d087 100644
--- a/test/Makefile.win32
+++ b/test/Makefile.win32
@@ -19,7 +19,26 @@ BENCHS  = $(patsubst %,   $(CFG_VAR)/%.exe, $(BENCHMARKS))
 all: inform $(TESTS) $(BENCHS)
 
 check: inform $(TESTS)
-   @for test in $(TESTS) ; do ./$$test  echo PASS: $$test || echo 
FAIL: $$test ; done
+   @failures=0 ; \
+   total=0 ; \
+   for test in $(TESTS) ; \
+   do \
+   total=`expr $$total + 1` ; \
+   if ./$$test ; \
+   then echo PASS: $$test ; \
+   else echo FAIL: $$test ; \
+failures=`expr $$failures + 1` ; \
+   fi ; \
+   done ; \
+   if test $$failures -eq 0 ; \
+   then banner=All $$total tests passed ; \
+   else banner=$$failures of $$total tests failed ; \
+   fi ; \
+   dashes=`echo $$banner | sed s/./=/g`; \
+   echo $$dashes ; \
+   echo $$banner ; \
+   echo $$dashes ; \
+   test $$failures -eq 0
 
 $(CFG_VAR)/libutils.lib: $(libutils_OBJECTS)
@$(AR) $(PIXMAN_ARFLAGS) -OUT:$@ $^
-- 
1.7.5.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] Code cleanup

[Andrea Canciani]

This patchset should simply cleanup some code, mostly in the tests.

The patchset is also available in
http://cgit.freedesktop.org/~ranma42/pixman/log/?h=wip/cocci

As they are not fixes, it might be a good idea to wait for 0.24 to be
released before merging them (as usual, if nobody raises objections).

Andrea


___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 1/5] Remove useless checks for NULL before freeing

[Andrea Canciani]

This patch has been generated by the following Coccinelle semantic patch:

// Remove useless checks for NULL before freeing
//
// free (NULL) is a no-op, so there is no need to avoid it

@@
expression E;
@@
+ free (E);
+ E = NULL;
- if (unlikely (E != NULL)) {
-   free(E);
(
-   E = NULL;
|
-   E = 0;
)
   ...
- }

@@
expression E;
@@
+ free (E);
- if (unlikely (E != NULL)) {
-   free (E);
- }
---
 pixman/pixman-image.c  |7 ++-
 pixman/pixman-region.c |9 +++--
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
index 09d7cbc..913853c 100644
--- a/pixman/pixman-image.c
+++ b/pixman/pixman-image.c
@@ -145,11 +145,8 @@ _pixman_image_fini (pixman_image_t *image)
 
pixman_region32_fini (common-clip_region);
 
-   if (common-transform)
-   free (common-transform);
-
-   if (common-filter_params)
-   free (common-filter_params);
+   free (common-transform);
+   free (common-filter_params);
 
if (common-alpha_map)
pixman_image_unref ((pixman_image_t *)common-alpha_map);
diff --git a/pixman/pixman-region.c b/pixman/pixman-region.c
index 47beb52..80219c6 100644
--- a/pixman/pixman-region.c
+++ b/pixman/pixman-region.c
@@ -828,8 +828,7 @@ pixman_op (region_type_t *  new_reg,   /* Place 
to store result
 {
 if (!pixman_rect_alloc (new_reg, new_size))
 {
-if (old_data)
-   free (old_data);
+free (old_data);
 return FALSE;
}
 }
@@ -1005,8 +1004,7 @@ pixman_op (region_type_t *  new_reg,   /* 
Place to store result
 APPEND_REGIONS (new_reg, r2_band_end, r2_end);
 }
 
-if (old_data)
-   free (old_data);
+free (old_data);
 
 if (!(numRects = new_reg-data-numRects))
 {
@@ -1027,8 +1025,7 @@ pixman_op (region_type_t *  new_reg,   /* 
Place to store result
 return TRUE;
 
 bail:
-if (old_data)
-   free (old_data);
+free (old_data);
 
 return pixman_break (new_reg);
 }
-- 
1.7.5.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 2/5] test: Cleanup includes

[Andrea Canciani]

All the tests are linked to libutil, hence it makes sence to always
include utils.h and reuse what it provides (config.h inclusion, access
to private pixman APIs, ARRAY_LENGTH, ...).
---
 test/a1-trap-test.c  |2 +-
 test/blitters-test.c |1 -
 test/composite-traps-test.c  |1 -
 test/composite.c |2 --
 test/fetch-test.c|6 +-
 test/lowlevel-blt-bench.c|4 
 test/oob-test.c  |2 +-
 test/region-contains-test.c  |1 -
 test/region-translate-test.c |2 +-
 test/scaling-crash-test.c|2 +-
 test/scaling-test.c  |1 -
 test/stress-test.c   |1 +
 test/trap-crasher.c  |2 +-
 test/utils.c |1 +
 test/utils.h |1 -
 15 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/test/a1-trap-test.c b/test/a1-trap-test.c
index 6163e7c..93c6caa 100644
--- a/test/a1-trap-test.c
+++ b/test/a1-trap-test.c
@@ -2,7 +2,7 @@
 #include stdio.h
 #include stdlib.h
 #include string.h
-#include pixman.h
+#include utils.h
 
 int
 main (int argc, char **argv)
diff --git a/test/blitters-test.c b/test/blitters-test.c
index 4f931c4..6355632 100644
--- a/test/blitters-test.c
+++ b/test/blitters-test.c
@@ -5,7 +5,6 @@
  * Script 'fuzzer-find-diff.pl' can be used to narrow down the problem in
  * the case of test failure.
  */
-#include assert.h
 #include stdlib.h
 #include stdio.h
 #include utils.h
diff --git a/test/composite-traps-test.c b/test/composite-traps-test.c
index fa6d8a9..ff03b50 100644
--- a/test/composite-traps-test.c
+++ b/test/composite-traps-test.c
@@ -1,6 +1,5 @@
 /* Based loosely on scaling-test */
 
-#include assert.h
 #include stdlib.h
 #include stdio.h
 #include utils.h
diff --git a/test/composite.c b/test/composite.c
index 408c363..fe59eae 100644
--- a/test/composite.c
+++ b/test/composite.c
@@ -22,8 +22,6 @@
  * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
  * PERFORMANCE OF THIS SOFTWARE.
  */
-#define PIXMAN_USE_INTERNAL_API
-#include pixman.h
 #include stdio.h
 #include stdlib.h /* abort() */
 #include math.h
diff --git a/test/fetch-test.c b/test/fetch-test.c
index 9f80eec..fa79ad7 100644
--- a/test/fetch-test.c
+++ b/test/fetch-test.c
@@ -1,11 +1,7 @@
-#ifdef HAVE_CONFIG_H
-#include config.h
-#endif
-
 #include assert.h
 #include stdlib.h
 #include stdio.h
-#include pixman.h
+#include utils.h
 
 #define SIZE 1024
 
diff --git a/test/lowlevel-blt-bench.c b/test/lowlevel-blt-bench.c
index bdafb35..67cd939 100644
--- a/test/lowlevel-blt-bench.c
+++ b/test/lowlevel-blt-bench.c
@@ -25,10 +25,6 @@
 #include stdio.h
 #include stdlib.h
 #include string.h
-
-#define PIXMAN_USE_INTERNAL_API
-#include pixman.h
-
 #include utils.h
 
 #define SOLID_FLAG 1
diff --git a/test/oob-test.c b/test/oob-test.c
index 4f9e5a2..9c1a25d 100644
--- a/test/oob-test.c
+++ b/test/oob-test.c
@@ -1,6 +1,6 @@
 #include stdio.h
 #include stdlib.h
-#include pixman.h
+#include utils.h
 
 typedef struct
 {
diff --git a/test/region-contains-test.c b/test/region-contains-test.c
index b660fdf..2372686 100644
--- a/test/region-contains-test.c
+++ b/test/region-contains-test.c
@@ -1,4 +1,3 @@
-#include assert.h
 #include stdlib.h
 #include stdio.h
 #include utils.h
diff --git a/test/region-translate-test.c b/test/region-translate-test.c
index 0e96a5e..5a03027 100644
--- a/test/region-translate-test.c
+++ b/test/region-translate-test.c
@@ -1,5 +1,5 @@
-#include pixman.h
 #include assert.h
+#include utils.h
 
 /* Pixman had a bug where 32bit regions where clipped to 16bit sizes when
  * pixman_region32_translate() was called. This test exercises that bug.
diff --git a/test/scaling-crash-test.c b/test/scaling-crash-test.c
index 40323d4..50d445d 100644
--- a/test/scaling-crash-test.c
+++ b/test/scaling-crash-test.c
@@ -2,7 +2,7 @@
 #include stdlib.h
 #include stdio.h
 #include string.h
-#include pixman.h
+#include utils.h
 
 /*
  * We have a source image filled with solid color, set NORMAL or PAD repeat,
diff --git a/test/scaling-test.c b/test/scaling-test.c
index 82370f7..6f2da14 100644
--- a/test/scaling-test.c
+++ b/test/scaling-test.c
@@ -7,7 +7,6 @@
  * Script 'fuzzer-find-diff.pl' can be used to narrow down the problem in
  * the case of test failure.
  */
-#include assert.h
 #include stdlib.h
 #include stdio.h
 #include utils.h
diff --git a/test/stress-test.c b/test/stress-test.c
index 571420a..08bf1d4 100644
--- a/test/stress-test.c
+++ b/test/stress-test.c
@@ -1,4 +1,5 @@
 #include stdio.h
+#include stdlib.h
 #include utils.h
 #include sys/types.h
 
diff --git a/test/trap-crasher.c b/test/trap-crasher.c
index 7485e62..db7e01a 100644
--- a/test/trap-crasher.c
+++ b/test/trap-crasher.c
@@ -1,5 +1,5 @@
 #include stdlib.h
-#include pixman.h
+#include utils.h
 
 int
 main()
diff --git a/test/utils.c b/test/utils.c
index adabd75..204066f 100644
--- a/test/utils.c
+++ b/test/utils.c
@@ -2,6 +2,7 @@
 
 #include utils.h
 #include signal.h
+#include stdlib.h
 
 

[Pixman] [PATCH 3/5] Use the ARRAY_LENGTH() macro when possible

[Andrea Canciani]

This patch has been generated by the following Coccinelle semantic patch:

// Use the ARRAY_LENGTH() macro when possible
//
// Replace open-coded array length computations with the
// ARRAY_LENGTH() macro

@@
type T;
T[] E;
@@
- (sizeof(E)/sizeof(T))
+ ARRAY_LENGTH (E)
---
 test/blitters-test.c   |2 +-
 test/fetch-test.c  |2 +-
 test/gradient-crash-test.c |6 +++---
 test/lowlevel-blt-bench.c  |2 +-
 test/oob-test.c|2 +-
 test/trap-crasher.c|2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/test/blitters-test.c b/test/blitters-test.c
index 6355632..55b6c73 100644
--- a/test/blitters-test.c
+++ b/test/blitters-test.c
@@ -280,7 +280,7 @@ test_composite (int testnum, int verbose)
 
 lcg_srand (testnum);
 
-op = op_list[lcg_rand_n (sizeof (op_list) / sizeof (op_list[0]))];
+op = op_list[lcg_rand_n (ARRAY_LENGTH (op_list))];
 
 if (lcg_rand_n (8))
 {
diff --git a/test/fetch-test.c b/test/fetch-test.c
index fa79ad7..04e8cc5 100644
--- a/test/fetch-test.c
+++ b/test/fetch-test.c
@@ -103,7 +103,7 @@ static testcase_t testcases[] =
 },
 };
 
-int n_test_cases = sizeof(testcases)/sizeof(testcases[0]);
+int n_test_cases = ARRAY_LENGTH (testcases);
 
 
 static uint32_t
diff --git a/test/gradient-crash-test.c b/test/gradient-crash-test.c
index c85712d..73e5bbc 100644
--- a/test/gradient-crash-test.c
+++ b/test/gradient-crash-test.c
@@ -106,17 +106,17 @@ main (int argc, char **argv)
if (i == 0)
{
stops = onestop;
-   num_stops = sizeof(onestop) / sizeof(onestop[0]);
+   num_stops = ARRAY_LENGTH (onestop);
}
else if (i == 1)
{
stops = subsetstops;
-   num_stops = sizeof(subsetstops) / sizeof(subsetstops[0]);
+   num_stops = ARRAY_LENGTH (subsetstops);
}
else
{
stops = stops01;
-   num_stops = sizeof(stops01) / sizeof(stops01[0]);
+   num_stops = ARRAY_LENGTH (stops01);
}

for (j = 0; j  3; ++j)
diff --git a/test/lowlevel-blt-bench.c b/test/lowlevel-blt-bench.c
index 67cd939..ba7f307 100644
--- a/test/lowlevel-blt-bench.c
+++ b/test/lowlevel-blt-bench.c
@@ -703,7 +703,7 @@ main (int argc, char *argv[])
 x / 100., x / 400);
 printf (---\n);
 
-for (i = 0; i  sizeof(tests_tbl) / sizeof(tests_tbl[0]); i++)
+for (i = 0; i  ARRAY_LENGTH (tests_tbl); i++)
 {
if (strcmp (pattern, all) == 0 || strstr (tests_tbl[i].testname, 
pattern))
{
diff --git a/test/oob-test.c b/test/oob-test.c
index 9c1a25d..0d19b50 100644
--- a/test/oob-test.c
+++ b/test/oob-test.c
@@ -94,7 +94,7 @@ main (int argc, char **argv)
 {
 int i;
 
-for (i = 0; i  sizeof (info) / sizeof (info[0]); ++i)
+for (i = 0; i  ARRAY_LENGTH (info); ++i)
test_composite (info[i]);
 
 return 0;
diff --git a/test/trap-crasher.c b/test/trap-crasher.c
index db7e01a..4e4cac2 100644
--- a/test/trap-crasher.c
+++ b/test/trap-crasher.c
@@ -22,6 +22,6 @@ main()
 
 dst = pixman_image_create_bits (PIXMAN_a8, 1, 1, NULL, -1);
 
-pixman_add_trapezoids (dst, 0, 0, sizeof (traps)/sizeof (traps[0]), traps);
+pixman_add_trapezoids (dst, 0, 0, ARRAY_LENGTH (traps), traps);
 return (0);
 }
-- 
1.7.5.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 4/5] test: Reuse the ARRAY_LENGTH() macro

[Andrea Canciani]

It is provided by utils.h, there is no need to redefine it.
---
 test/alphamap.c   |2 --
 test/scaling-crash-test.c |5 ++---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/test/alphamap.c b/test/alphamap.c
index 554b309..24a350e 100644
--- a/test/alphamap.c
+++ b/test/alphamap.c
@@ -139,8 +139,6 @@ get_alpha (pixman_image_t *image, int x, int y, int orig_x, 
int orig_y)
 return r;
 }
 
-#define ARRAY_LENGTH(A) ((int) (sizeof (A) / sizeof ((A) [0])))
-
 static int
 run_test (int s, int d, int sa, int da, int soff, int doff)
 {
diff --git a/test/scaling-crash-test.c b/test/scaling-crash-test.c
index 50d445d..ed57ae0 100644
--- a/test/scaling-crash-test.c
+++ b/test/scaling-crash-test.c
@@ -130,12 +130,11 @@ do_test (int32_t  dst_size,
 int32_tsrc_offs,
 int32_tscale_factor)
 {
-#define N_ELEMENTS(a)  (sizeof (a) / sizeof ((a)[0]))
 int i, j;
 
-for (i = 0; i  N_ELEMENTS(filters); ++i)
+for (i = 0; i  ARRAY_LENGTH (filters); ++i)
 {
-   for (j = 0; j  N_ELEMENTS (repeats); ++j)
+   for (j = 0; j  ARRAY_LENGTH (repeats); ++j)
{
/* horizontal test */
if (run_test (dst_size, 1,
-- 
1.7.5.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 5/5] demos: Consistently use G_N_ELEMENTS()

[Andrea Canciani]

Instead of open-coding G_N_ELEMENTS(), just use it.
---
 demos/composite-test.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/demos/composite-test.c b/demos/composite-test.c
index f5f352f..bba90c5 100644
--- a/demos/composite-test.c
+++ b/demos/composite-test.c
@@ -123,7 +123,7 @@ main (int argc, char **argv)
 table = gtk_table_new (G_N_ELEMENTS (operators) / 6, 6, TRUE);
 
 src_img = pixman_image_create_linear_gradient (p1, p2, stops,
-  sizeof (stops) / sizeof 
(stops[0]));
+  G_N_ELEMENTS (stops));
 
 pixman_image_set_repeat (src_img, PIXMAN_REPEAT_PAD);
 
-- 
1.7.5.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [RFC] Performance statistics analyzer

[Andrea Canciani]

On Wed, Sep 7, 2011 at 6:46 AM, Maarten Bosmans mkbosm...@gmail.com wrote:
 2011/9/6 Taekyun Kim podai...@gmail.com:
 Hi,

 Currently pixman is used as a lowlevel backend for various applications.
 For doing analysis on performance, it might be desirable to provide some
 performance statistics information to developers.

 The basic idea is based on siarhei's slow-path-reporter. I extended it to
 accumulate information of every composite paths. Final result will be
 shown at the end of the application when destructor is called. Each paths
 are sorted by the amount of time they spent.

 Indeed, this is very useful.

 The main purpose of this utility is to profile cairo traces. We can easily
 figure out hot spots and real world usage patterns.

 You can see it from here.

     http://cgit.freedesktop.org/~podain/pixman/?h=perf_stat

 You can give --enable-perf-stat option to configure script to enable the
 performance statistics analyzer. (I'm not sure that this is done right,
 because I'm not familiar with autoconf, but it works anyway)

 Is it right or good to include this feature within upstream? I think other
 developers can easily profile their applications and report the statistics
 when they suffer from performance.

 I was thinking of some performance log channel, individual composite
 event is printed though that log channel and statistics analysis utilities
 to retrieve meaningful information from the log.

 All suggestions and comments are welcome.

 I took this RFC to also be a request for patches.
 Attached is a patch that strips out the pthread mutex usage and
 implements some macros in pixman-compiler.h, not unlike the TLS
 macros. I also added a win32 implementation and tested it using mingw.

Is there any reason to use win32 Mutexes instead of CriticalRegions?
AFAIK Mutexes are for inter-process synchronization, while each
CriticalRegion can only be used by a single process to synchronize
multiple threads (and has a better performance, because most of the
times it does not require calling into the kernel).


 Some further improvements could be: check whether the win32
 implementation works with MSVC and use it there too. And also a way of
 opting out if you're on a non-win32, non-pthread platform and don't
 want multithreading.

It does not work with MSVC because it obvously does not provide the
__MINGW32__ definition. Replacing it with _WIN32 fixes that build
error but still dies in pixman-fast-path.c because window.h defines
IN and OUT. I have a patch for that in my simpleops branch, I'll try
cherry-picking it.

About the previous part of the patchset:
Would it be possible to avoid the #ifdef PIXMAN_PERF_STAT in
pixman.c and just make perfstat_is_enabled() an inline function
which always returns false when it is not defined?
I believe that the code would look cleaner.

(We might actually need to ifdef the code out in order to be able
to compile when no mutexes are available, but the branch currently
does not try to detect it AFAICT)

Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Only link with -lpng when libpng is actually available

[Andrea Canciani]

How about using pkg-config to detect the availability of libpng and
the flags required to use it?

Andrea

On Sun, Sep 11, 2011 at 1:09 PM, Søren Sandmann sandm...@cs.au.dk wrote:
 From: Søren Sandmann Pedersen s...@redhat.com

 Fixes build on systems that don't have libpng
 ---
  configure.ac     |    1 +
  test/Makefile.am |    5 -
  2 files changed, 5 insertions(+), 1 deletions(-)

 diff --git a/configure.ac b/configure.ac
 index 21613e1..fdb2521 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -811,6 +811,7 @@ if test x$have_libpng = xyes; then
  fi

  AC_SUBST(HAVE_LIBPNG)
 +AM_CONDITIONAL(HAVE_LIBPNG, test $have_libpng = yes)

  AC_OUTPUT([pixman-1.pc
            pixman-1-uninstalled.pc
 diff --git a/test/Makefile.am b/test/Makefile.am
 index 52ef8ad..7bd826e 100644
 --- a/test/Makefile.am
 +++ b/test/Makefile.am
 @@ -1,6 +1,9 @@
  AM_CFLAGS = @OPENMP_CFLAGS@
  AM_LDFLAGS = @OPENMP_CFLAGS@ @TESTPROGS_EXTRA_LDFLAGS@
 -LDADD = $(top_builddir)/pixman/libpixman-1.la -lm -lpng
 +LDADD = $(top_builddir)/pixman/libpixman-1.la -lm
 +if HAVE_LIBPNG
 +LDADD += -lpng
 +endif
  INCLUDES = -I$(top_srcdir)/pixman -I$(top_builddir)/pixman

  TESTPROGRAMS =                 \
 --
 1.6.0.6

 ___
 Pixman mailing list
 Pixman@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/pixman

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 1/7] test: Fix compilation on win32

[Andrea Canciani]

Adding scaling-helpers-test to the testsuite on win32 makes MSVC
complain about int64_t being used as an expression:

scaling-helpers-test.c(27) : error C2275: 'int64_t' : illegal use of
this type as an expression
---
 test/scaling-helpers-test.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/scaling-helpers-test.c b/test/scaling-helpers-test.c
index a38cac5..eb436d1 100644
--- a/test/scaling-helpers-test.c
+++ b/test/scaling-helpers-test.c
@@ -20,12 +20,12 @@ bilinear_pad_repeat_get_scanline_bounds_ref (int32_t
source_image_width,
 int32_t *  right_pad)
 {
 int w = *width;
+int64_t vx = vx_;
 *left_pad = 0;
 *left_tz = 0;
 *width = 0;
 *right_tz = 0;
 *right_pad = 0;
-int64_t vx = vx_;
 while (--w = 0)
 {
if (vx  0)
-- 
1.7.4.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 2/7] build: Reuse sources and pixman-combine build rules

[Andrea Canciani]

Makefile.am and Makefile.win32 should not duplicate content, as this
leads to breaking the build when they are not kept in sync.

This can be avoided by listing sources, headers and common build
variables/rules in a Makefile.sources file.
---
 pixman/Makefile.am  |   61 ++-
 pixman/Makefile.sources |   55 ++
 pixman/Makefile.win32   |   52 +++-
 3 files changed, 77 insertions(+), 91 deletions(-)
 create mode 100644 pixman/Makefile.sources

diff --git a/pixman/Makefile.am b/pixman/Makefile.am
index 44e6b17..2421a4f 100644
--- a/pixman/Makefile.am
+++ b/pixman/Makefile.am
@@ -1,60 +1,25 @@
+include $(top_srcdir)/pixman/Makefile.sources
+
 lib_LTLIBRARIES = libpixman-1.la
+
 libpixman_1_la_LDFLAGS = -version-info $(LT_VERSION_INFO) -no-undefined 
@PTHREAD_LDFLAGS@ 
 libpixman_1_la_LIBADD = @PTHREAD_LIBS@ @DEP_LIBS@ -lm
-libpixman_1_la_SOURCES =   \
-   pixman.h\
-   pixman-accessor.h   \
-   pixman-access.c \
-   pixman-access-accessors.c   \
-   pixman-cpu.c\
-   pixman-gradient-walker.c\
-   pixman-region16.c   \
-   pixman-region32.c   \
-   pixman-compiler.h   \
-   pixman-private.h\
-   pixman-image.c  \
-   pixman-implementation.c \
-   pixman-combine32.c  \
-   pixman-combine32.h  \
-   pixman-combine64.c  \
-   pixman-combine64.h  \
-   pixman-general.c\
-   pixman.c\
-   pixman-noop.c   \
-   pixman-fast-path.c  \
-   pixman-solid-fill.c \
-   pixman-conical-gradient.c   \
-   pixman-linear-gradient.c\
-   pixman-radial-gradient.c\
-   pixman-bits-image.c \
-   pixman-utils.c  \
-   pixman-edge.c   \
-   pixman-edge-accessors.c \
-   pixman-edge-imp.h   \
-   pixman-inlines.h\
-   pixman-trap.c   \
-   pixman-timer.c  \
-   pixman-matrix.c
+libpixman_1_la_SOURCES = $(libpixman_sources) $(libpixman_headers)
 
 libpixmanincludedir = $(includedir)/pixman-1
 libpixmaninclude_HEADERS = pixman.h pixman-version.h
 noinst_LTLIBRARIES = 
 
-BUILT_SOURCES = pixman-combine32.h pixman-combine32.c pixman-combine64.h 
pixman-combine64.c
-
-pixman-combine32.c : pixman-combine.c.template pixman-combine32.h 
make-combine.pl
-   $(PERL) $(srcdir)/make-combine.pl 8  
$(srcdir)/pixman-combine.c.template  $@ || ($(RM) $@; exit 1)
-pixman-combine32.h : pixman-combine.h.template make-combine.pl
-   $(PERL) $(srcdir)/make-combine.pl 8  
$(srcdir)/pixman-combine.h.template  $@ || ($(RM) $@; exit 1)
-
-pixman-combine64.c : pixman-combine.c.template pixman-combine64.h 
make-combine.pl
-   $(PERL) $(srcdir)/make-combine.pl 16  
$(srcdir)/pixman-combine.c.template  $@ || ($(RM) $@; exit 1)
-pixman-combine64.h : pixman-combine.h.template make-combine.pl
-   $(PERL) $(srcdir)/make-combine.pl 16  
$(srcdir)/pixman-combine.h.template  $@ || ($(RM) $@; exit 1)
+EXTRA_DIST =   \
+   Makefile.win32  \
+   make-combine.pl \
+   pixman-combine.c.template   \
+   pixman-combine.h.template   \
+   pixman-region.c \
+   solaris-hwcap.mapfile   \
+   $(NULL)
 
-EXTRA_DIST = Makefile.win32 pixman-combine.c.template make-combine.pl 
pixman-region.c \
-   pixman-combine.h.template solaris-hwcap.mapfile
-CLEANFILES = pixman-combine32.c pixman-combine64.c pixman-combine32.h 
pixman-combine64.h
+DISTCLEANFILES = $(BUILT_SOURCES)
 
 # mmx code
 if USE_MMX
diff --git a/pixman/Makefile.sources b/pixman/Makefile.sources
new file mode 100644
index 000..ca3f001
--- /dev/null
+++ b/pixman/Makefile.sources
@@ -0,0 +1,55 @@
+libpixman_sources =\
+   pixman.c\
+   pixman-access.c \
+   pixman-access-accessors.c   \
+   pixman-bits-image.c \
+   pixman-combine32.c  \
+   pixman-combine64.c  \
+   pixman-conical-gradient.c   \
+   pixman-cpu.c\
+   pixman-edge.c   \
+   pixman-edge-accessors.c \
+   pixman-fast-path.c  \
+   pixman-general.c\
+   

[Pixman] [PATCH 5/7] build-win32: Add root Makefile.win32

[Andrea Canciani]

Add Makefile.win32 to the pixman root. This makefile can recursively
run the other ones to compile the library or the test suite.
---
 Makefile.win32 |   20 
 1 files changed, 20 insertions(+), 0 deletions(-)
 create mode 100644 Makefile.win32

diff --git a/Makefile.win32 b/Makefile.win32
new file mode 100644
index 000..5b74878
--- /dev/null
+++ b/Makefile.win32
@@ -0,0 +1,20 @@
+default: all
+
+top_srcdir = .
+include $(top_srcdir)/Makefile.win32.common
+
+# Recursive targets
+pixman_r:
+   @$(MAKE) -C pixman -f Makefile.win32
+
+test_r:
+   @$(MAKE) -C test -f Makefile.win32
+
+clean_r:
+   @$(MAKE) -C pixman -f Makefile.win32 clean
+   @$(MAKE) -C test   -f Makefile.win32 clean
+
+# Base targets
+all: test_r
+
+clean: clean_r
-- 
1.7.4.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 6/7] test: Do not include config.h unless HAVE_CONFIG_H is defined

[Andrea Canciani]

The win32 build system does not generate config.h and correctly runs
the compiler without defining HAVE_CONFIG_H. Nevertheless some files
include config.h without checking for its availability, breaking the
build from a clean directory:

test\utils.h(2) : fatal error C1083: Cannot open include file:
'config.h': No such file or directory
...
---
 test/blitters-test.c|1 -
 test/composite.c|1 -
 test/fetch-test.c   |5 -
 test/pdf-op-test.c  |1 -
 test/scaling-helpers-test.c |1 -
 test/utils.h|5 -
 6 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/test/blitters-test.c b/test/blitters-test.c
index 594ec54..790a27f 100644
--- a/test/blitters-test.c
+++ b/test/blitters-test.c
@@ -8,7 +8,6 @@
 #include assert.h
 #include stdlib.h
 #include stdio.h
-#include config.h
 #include utils.h
 
 static pixman_indexed_t rgb_palette[9];
diff --git a/test/composite.c b/test/composite.c
index edea9a9..408c363 100644
--- a/test/composite.c
+++ b/test/composite.c
@@ -27,7 +27,6 @@
 #include stdio.h
 #include stdlib.h /* abort() */
 #include math.h
-#include config.h
 #include time.h
 #include utils.h
 
diff --git a/test/fetch-test.c b/test/fetch-test.c
index feb98d9..9f80eec 100644
--- a/test/fetch-test.c
+++ b/test/fetch-test.c
@@ -1,8 +1,11 @@
+#ifdef HAVE_CONFIG_H
+#include config.h
+#endif
+
 #include assert.h
 #include stdlib.h
 #include stdio.h
 #include pixman.h
-#include config.h
 
 #define SIZE 1024
 
diff --git a/test/pdf-op-test.c b/test/pdf-op-test.c
index dc7a4fd..99cb7df 100644
--- a/test/pdf-op-test.c
+++ b/test/pdf-op-test.c
@@ -1,4 +1,3 @@
-#include config.h
 #include stdlib.h
 #include utils.h
 
diff --git a/test/scaling-helpers-test.c b/test/scaling-helpers-test.c
index eb436d1..a8b94b0 100644
--- a/test/scaling-helpers-test.c
+++ b/test/scaling-helpers-test.c
@@ -1,4 +1,3 @@
-#include config.h
 #include stdint.h
 #include stdlib.h
 #include stdio.h
diff --git a/test/utils.h b/test/utils.h
index f0c9c30..7d16a9c 100644
--- a/test/utils.h
+++ b/test/utils.h
@@ -1,5 +1,8 @@
-#include stdlib.h
+#ifdef HAVE_CONFIG_H
 #include config.h
+#endif
+
+#include stdlib.h
 #include assert.h
 #include pixman-private.h /* For 'inline' definition */
 
-- 
1.7.4.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 7/7] build-win32: Add 'check' target

[Andrea Canciani]

On win32 the tests are built but they are not run automatically by the
build system.

A minimal 'check' target (depending on the tests being built) can
simply run them and log to the console their success/failure.
---
 Makefile.win32  |5 +
 test/Makefile.win32 |3 +++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/Makefile.win32 b/Makefile.win32
index 5b74878..91cd12a 100644
--- a/Makefile.win32
+++ b/Makefile.win32
@@ -14,7 +14,12 @@ clean_r:
@$(MAKE) -C pixman -f Makefile.win32 clean
@$(MAKE) -C test   -f Makefile.win32 clean
 
+check_r:
+   @$(MAKE) -C test -f Makefile.win32 check
+
 # Base targets
 all: test_r
 
 clean: clean_r
+
+check: check_r
diff --git a/test/Makefile.win32 b/test/Makefile.win32
index c857db9..307ba0c 100644
--- a/test/Makefile.win32
+++ b/test/Makefile.win32
@@ -18,6 +18,9 @@ BENCHS  = $(patsubst %,   $(CFG_VAR)/%.exe, $(BENCHMARKS))
 
 all: inform $(TESTS) $(BENCHS)
 
+check: inform $(TESTS)
+   @for test in $(TESTS) ; do ./$$test  echo PASS: $$test || echo 
FAIL: $$test ; done
+
 $(CFG_VAR)/libutils.lib: $(libutils_OBJECTS)
@$(AR) $(PIXMAN_ARFLAGS) -OUT:$@ $^
 
-- 
1.7.4.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Workaround bug in llvm-gcc

[Andrea Canciani]

On Mon, Aug 22, 2011 at 3:24 PM, Soeren Sandmann sandm...@cs.au.dk wrote:
 Andrea Canciani ranm...@gmail.com writes:

 llvm-gcc (shipped in Apple XCode 4.1.1 as the default compiler or in
 the 2.9 release of LLVM) performs an invalid optimization which
 unifies the empty_region and the bad_region structures because they
 have the same content.

 A bug has been filed against Apple Developers Tool for this
 issue. This commit works around this bug by making one of the two
 structures volatile, so that it cannot be merged.

 Is the bug report publicly readable?

  static const box_type_t PREFIX (_empty_box_) = { 0, 0, 0, 0 };
  static const region_data_type_t PREFIX (_empty_data_) = { 0, 0 };
 +#if defined (__llvm__)  !defined (__clang__)
 +static const volatile region_data_type_t PREFIX (_broken_data_) = { 0, 0 };
 +#else
  static const region_data_type_t PREFIX (_broken_data_) = { 0, 0 };
 +#endif

 It worries me a little that these #ifdefs will still trigger when Apple
 fix their compiler.

The same issue affects non-Apple llvm-gcc, but it's deprecated and
there will be no further releases. I would not be surprised if Apple
just started using clang as the default compiler in future XCode
versions.

 Can we detect it at configure time instead by
 running something like this:

It's not that easy. Unfortunately simple code snippets like this one
compute the correct result, because the if condition gets optimized
to a constant and the blah_t structures are not emitted at all.

It is definitely possible to do it, but the code will probably be
ugly or not that obvious.

Andrea


        typedef struct { int x, y; } blah_t;

        static const blah_t b1 = { 0, 0 };
        static const blah_t b2 = { 0, 0 };

        int
        main ()
        {
                if (b1 == b2)
                        return 1;
                else
                        return 0;
        }


 Soren

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH] Workaround bug in llvm-gcc

[Andrea Canciani]

llvm-gcc (shipped in Apple XCode 4.1.1 as the default compiler or in
the 2.9 release of LLVM) performs an invalid optimization which
unifies the empty_region and the bad_region structures because they
have the same content.

A bug has been filed against Apple Developers Tool for this
issue. This commit works around this bug by making one of the two
structures volatile, so that it cannot be merged.

Fixes region-contains-test.
---
 pixman/pixman-region.c |4 
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/pixman/pixman-region.c b/pixman/pixman-region.c
index 9ff5157..47beb52 100644
--- a/pixman/pixman-region.c
+++ b/pixman/pixman-region.c
@@ -102,7 +102,11 @@
 
 static const box_type_t PREFIX (_empty_box_) = { 0, 0, 0, 0 };
 static const region_data_type_t PREFIX (_empty_data_) = { 0, 0 };
+#if defined (__llvm__)  !defined (__clang__)
+static const volatile region_data_type_t PREFIX (_broken_data_) = { 0, 0 };
+#else
 static const region_data_type_t PREFIX (_broken_data_) = { 0, 0 };
+#endif
 
 static box_type_t *pixman_region_empty_box =
 (box_type_t *)PREFIX (_empty_box_);
-- 
1.7.4.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH] Workaround bug in llvm-gcc

[Andrea Canciani]

llvm-gcc (shipped in Apple XCode 4.1.1) as the default compiler
performs an invalid optimization which unifies the empty_region and
the bad_region structures because they have the same content.

A bug has been filed against Apple Developers Tool for this
issue. This commit works around this bug by making the content of the
two structures different.

Fixes region-contains-test.
---
 pixman/pixman-region.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/pixman/pixman-region.c b/pixman/pixman-region.c
index 9ff5157..9074fe4 100644
--- a/pixman/pixman-region.c
+++ b/pixman/pixman-region.c
@@ -102,7 +102,7 @@
 
 static const box_type_t PREFIX (_empty_box_) = { 0, 0, 0, 0 };
 static const region_data_type_t PREFIX (_empty_data_) = { 0, 0 };
-static const region_data_type_t PREFIX (_broken_data_) = { 0, 0 };
+static const region_data_type_t PREFIX (_broken_data_) = { -1, 0 };
 
 static box_type_t *pixman_region_empty_box =
 (box_type_t *)PREFIX (_empty_box_);
-- 
1.7.4.4

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH] win32: Build benchmarks

[Andrea Canciani]

Add the makefile rules needed to compile lowlevel-blt-bench on win32
and fix the compilation errors.
---
 test/Makefile.win32   |5 -
 test/lowlevel-blt-bench.c |6 ++
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/test/Makefile.win32 b/test/Makefile.win32
index c71afe1..a62b6fc 100644
--- a/test/Makefile.win32
+++ b/test/Makefile.win32
@@ -35,6 +35,7 @@ SOURCES = \
scaling-test.c  \
affine-test.c   \
composite.c \
+   lowlevel-blt-bench.c\
utils.c
 
 TESTS =\
@@ -56,6 +57,8 @@ TESTS =   \
$(CFG_VAR)/affine-test.exe  \
$(CFG_VAR)/composite.exe
 
+BENCHMARKS =   \
+   $(CFG_VAR)/lowlevel-blt-bench.exe
 
 OBJECTS = $(patsubst %.c, $(CFG_VAR)/%.obj, $(SOURCES))
 
@@ -66,7 +69,7 @@ $(CFG_VAR)/%.obj: %.c
 $(CFG_VAR)/%.exe: $(CFG_VAR)/%.obj
$(LINK) /NOLOGO /OUT:$@ $ $(CFG_VAR)/utils.obj $(TEST_LDADD)
 
-all: $(OBJECTS) $(TESTS)
+all: $(OBJECTS) $(TESTS) $(BENCHMARKS)
@exit 0
 
 clean:
diff --git a/test/lowlevel-blt-bench.c b/test/lowlevel-blt-bench.c
index d58587d..3c74fed 100644
--- a/test/lowlevel-blt-bench.c
+++ b/test/lowlevel-blt-bench.c
@@ -27,11 +27,9 @@
 #include stdlib.h
 #include string.h
 
-#ifdef HAVE_CONFIG_H
-#include config.h
-#endif
+#define PIXMAN_USE_INTERNAL_API
+#include pixman.h
 
-#include pixman-private.h
 #include utils.h
 
 #define SOLID_FLAG 1
-- 
1.7.1

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 1/4] Fix lcg_rand_u32() to return 32 random bits.

[Andrea Canciani]

On Thu, Aug 4, 2011 at 4:21 AM, Søren Sandmann sandm...@cs.au.dk wrote:
 From: Søren Sandmann Pedersen s...@redhat.com

 The lcg_rand() function only returns 15 random bits, so lcg_rand_u32()
 would always have 0 in bit 31 and bit 15. Fix that by calling
 lcg_rand() three times, to generate 15, 15, and 2 random bits
 respectively.
 ---
  test/utils.h |    5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)

 diff --git a/test/utils.h b/test/utils.h
 index 615ad78..a249627 100644
 --- a/test/utils.h
 +++ b/test/utils.h
 @@ -45,9 +45,10 @@ static inline uint32_t
  lcg_rand_u32 (void)
  {
     uint32_t lo = lcg_rand();
 -    uint32_t hi = lcg_rand();
 +    uint32_t mid = lcg_rand()  15;
 +    uint32_t hi = lcg_rand()  30;

 -    return (hi  16) | lo;
 +    return (hi | mid | lo);

lcg has more randomness in the high bits, so we might want
to keep them (hi discards everything but the last two bits).
We could probably get slightly better randomness by doing
something like:

-uint32_t lo = lcg_rand();
-uint32_t hi = lcg_rand();
+uint32_t lo = lcg_rand()  -(32 - 15 - 11*2);
+uint32_t mid = lcg_rand()  (32 - 15 - 11*1);
+uint32_t hi = lcg_rand()  (32 - 15 - 11*0);

-return (hi  16) | lo;
+return hi ^ mid ^ lo;

This uses the 10/11 most significant bits from the 3 lcg results
(and mixes them with the low from the adjacent one... instead
of mixing we could just discard the low ones, but it just makes
the code more complicated).

I'm not sure if this is worth the effort, because lcg is not a great
RNG anyway, but maybe the change is simple enough to make
it reasonable.

Andrea

  }

  /* CRC 32 computation
 --
 1.7.4

 ___
 Pixman mailing list
 Pixman@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/pixman

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 3/4] Speed up pixman_region{, 32}_contains_rectangle()

[Andrea Canciani]

On Thu, Aug 4, 2011 at 4:21 AM, Søren Sandmann sandm...@cs.au.dk wrote:
 From: Søren Sandmann Pedersen s...@redhat.com

 When someone selects some text in Firefox under a non-composited X
 server and initiates a drag, a shaped window is created with a complex
 shape corresponding to the outline of the text. Then, on every mouse
 movement pixman_region_contains_rectangle() is called many times on
 that complicated region. And pixman_region_contains_rectangle() is
 doing a linear scan through the rectangles in the region, although the
 scan does does when it finds the first box that can't possibly
 intersect the passed-in rectangle.

 This patch changes the loop so that it begins with a binary search for
 the first box that could possibly overlap the passed-in rectangle.
 The performance improvement for the text dragging case is easily
 noticable.
 ---
  pixman/pixman-region.c |   44 +++-
  1 files changed, 39 insertions(+), 5 deletions(-)

 diff --git a/pixman/pixman-region.c b/pixman/pixman-region.c
 index 142493b..a199405 100644
 --- a/pixman/pixman-region.c
 +++ b/pixman/pixman-region.c
 @@ -2086,6 +2086,40 @@ PIXMAN_EXPORT PREFIX (_inverse) (region_type_t 
 *new_reg,  /* Destination region
     return TRUE;
  }

 +/* In time O(log n), locate the first box whose y2 is greater than y.
 + * Return @end if no such box exists.
 + */
 +static box_type_t *
 +find_box_for_y (box_type_t *begin, box_type_t *end, int y)
 +{
 +    box_type_t *mid;
 +
 +    if (end == begin)
 +       return end;
 +
 +    if (end - begin == 1)
 +    {
 +       if (begin-y2  y)
 +           return begin;
 +       else
 +           return end;
 +    }
 +
 +    mid = begin + (end - begin) / 2;
 +    if (mid-y2  y)
 +    {
 +       /* If no box is found in [begin, mid], the function
 +        * will return @mid, which is then known to be the
 +        * correct answer.
 +        */
 +       return find_box_for_y (begin, mid, y);
 +    }
 +    else
 +    {
 +       return find_box_for_y (mid, end, y);
 +    }
 +}
 +
  /*
  *   rect_in(region, rect)
  *   This routine takes a pointer to a region and a pointer to a box
 @@ -2102,7 +2136,6 @@ PIXMAN_EXPORT PREFIX (_inverse) (region_type_t 
 *new_reg,  /* Destination region
  *   partially in the region) or is outside the region (we reached a band
  *   that doesn't overlap the box at all and part_in is false)
  */
 -
  pixman_region_overlap_t
  PIXMAN_EXPORT PREFIX (_contains_rectangle) (region_type_t *  region,
                                             box_type_t *     prect)
 @@ -2137,12 +2170,13 @@ PIXMAN_EXPORT PREFIX (_contains_rectangle) 
 (region_type_t *  region,
     x = prect-x1;
     y = prect-y1;

 +    pbox = PIXREGION_BOXPTR (region);
 +    pbox_end = pbox + numRects;
 +
 +    pbox = find_box_for_y (pbox, pbox_end, y);
     /* can stop when both part_out and part_in are TRUE, or we reach 
 prect-y2 */
 -    for (pbox = PIXREGION_BOXPTR (region), pbox_end = pbox + numRects;
 -         pbox != pbox_end;
 -         pbox++)
 +    for (; pbox != pbox_end; pbox++)
     {
 -
         if (pbox-y2 = y)
            continue;   /* getting up to speed or skipping remainder of band */

If this test is not needed anymore, I think it should be deleted,
otherwise the documentation of find_box_for_y should be modified.

The same objection applies to patch 4/4.

Again, some trailing whitespace (both in 3/4 and in 4/4).

Andrea


 --
 1.7.4

 ___
 Pixman mailing list
 Pixman@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/pixman

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 3/3] BILINEAR-NEAREST filter optimization for simple rotation and translation

[Andrea Canciani]

On Sun, May 22, 2011 at 11:15 PM, Siarhei Siamashka
siarhei.siamas...@gmail.com wrote:
 From: Siarhei Siamashka siarhei.siamas...@nokia.com

 Simple rotation and translation are the additional cases when BILINEAR
 filter can be safely reduced to NEAREST.

I believe that this reduction is valid for any matrix which describes
a unit transform followed
by an integer translation (i.e. rotation(90 degrees), scale(-1,1),
translate(1,0) composed
in any possible way).

This happens whenever the translational component of the matrix is
integral, the projective
component is identity (already tested by AFFINE_TRANSFORM) and the
upper-left 2x2
matrix multiplied by its transpose results in the 2x2 identity.

Andrea

 ---
  pixman/pixman-image.c |   37 +
  1 files changed, 37 insertions(+), 0 deletions(-)

 diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
 index 4784c22..583e7a7 100644
 --- a/pixman/pixman-image.c
 +++ b/pixman/pixman-image.c
 @@ -256,6 +256,43 @@ compute_image_info (pixman_image_t *image)
        {
            flags |= FAST_PATH_NEAREST_FILTER;
        }
 +       else if (
 +                /* affine and integer translation components in matrix ... */
 +                ((flags  FAST_PATH_AFFINE_TRANSFORM) 
 +                 !pixman_fixed_frac (image-common.transform-matrix[0][2] |
 +                                     image-common.transform-matrix[1][2])) 
 
 +                (
 +                 /* ... combined with a simple rotation */
 +                 (flags  (FAST_PATH_ROTATE_90_TRANSFORM |
 +                           FAST_PATH_ROTATE_180_TRANSFORM |
 +                           FAST_PATH_ROTATE_270_TRANSFORM)) ||
 +                 /* ... or combined with a simple non-rotated translation */
 +                 (image-common.transform-matrix[0][0] == pixman_fixed_1 
 +                  image-common.transform-matrix[1][1] == pixman_fixed_1 
 +                  image-common.transform-matrix[0][1] == 0 
 +                  image-common.transform-matrix[1][0] == 0)
 +                )
 +               )
 +       {
 +           /* FIXME: there are some affine-test failures, showing that
 +            * handling of BILINEAR and NEAREST filter is not quite
 +            * equivalent when getting close to 32K for the translation
 +            * components of the matrix. That's likely some bug, but for
 +            * now just skip BILINEAR-NEAREST optimization in this case.
 +            */
 +           pixman_fixed_t magic_limit = pixman_int_to_fixed (3);
 +           if (image-common.transform-matrix[0][2]  magic_limit  ||
 +               image-common.transform-matrix[1][2]  magic_limit  ||
 +               image-common.transform-matrix[0][2]  -magic_limit ||
 +               image-common.transform-matrix[1][2]  -magic_limit)
 +           {
 +               flags |= FAST_PATH_BILINEAR_FILTER;
 +           }
 +           else
 +           {
 +               flags |= FAST_PATH_NEAREST_FILTER;
 +           }
 +       }
        else
        {
            flags |= FAST_PATH_BILINEAR_FILTER;
 --
 1.7.3.4

 ___
 Pixman mailing list
 Pixman@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/pixman

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 3/3] BILINEAR-NEAREST filter optimization for simple rotation and translation

[Andrea Canciani]

On Mon, May 23, 2011 at 8:43 AM, Andrea Canciani ranm...@gmail.com wrote:
 On Sun, May 22, 2011 at 11:15 PM, Siarhei Siamashka
 siarhei.siamas...@gmail.com wrote:
 From: Siarhei Siamashka siarhei.siamas...@nokia.com

 Simple rotation and translation are the additional cases when BILINEAR
 filter can be safely reduced to NEAREST.

 I believe that this reduction is valid for any matrix which describes
 a unit transform followed
 by an integer translation (i.e. rotation(90 degrees), scale(-1,1),
 translate(1,0) composed
 in any possible way).

 This happens whenever the translational component of the matrix is
 integral, the projective
 component is identity (already tested by AFFINE_TRANSFORM) and the
 upper-left 2x2
 matrix multiplied by its transpose results in the 2x2 identity.

Uh, we already have an implementation of this (which is what cairo
uses to optimize filters to nearest):
http://cgit.freedesktop.org/cairo/tree/src/cairo-matrix.c#n728
(lines 728-762)


 Andrea

 ---
  pixman/pixman-image.c |   37 +
  1 files changed, 37 insertions(+), 0 deletions(-)

 diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
 index 4784c22..583e7a7 100644
 --- a/pixman/pixman-image.c
 +++ b/pixman/pixman-image.c
 @@ -256,6 +256,43 @@ compute_image_info (pixman_image_t *image)
        {
            flags |= FAST_PATH_NEAREST_FILTER;
        }
 +       else if (
 +                /* affine and integer translation components in matrix ... 
 */
 +                ((flags  FAST_PATH_AFFINE_TRANSFORM) 
 +                 !pixman_fixed_frac (image-common.transform-matrix[0][2] |
 +                                     
 image-common.transform-matrix[1][2])) 
 +                (
 +                 /* ... combined with a simple rotation */
 +                 (flags  (FAST_PATH_ROTATE_90_TRANSFORM |
 +                           FAST_PATH_ROTATE_180_TRANSFORM |
 +                           FAST_PATH_ROTATE_270_TRANSFORM)) ||
 +                 /* ... or combined with a simple non-rotated translation */
 +                 (image-common.transform-matrix[0][0] == pixman_fixed_1 
 +                  image-common.transform-matrix[1][1] == pixman_fixed_1 
 +                  image-common.transform-matrix[0][1] == 0 
 +                  image-common.transform-matrix[1][0] == 0)
 +                )
 +               )
 +       {
 +           /* FIXME: there are some affine-test failures, showing that
 +            * handling of BILINEAR and NEAREST filter is not quite
 +            * equivalent when getting close to 32K for the translation
 +            * components of the matrix. That's likely some bug, but for
 +            * now just skip BILINEAR-NEAREST optimization in this case.
 +            */
 +           pixman_fixed_t magic_limit = pixman_int_to_fixed (3);
 +           if (image-common.transform-matrix[0][2]  magic_limit  ||
 +               image-common.transform-matrix[1][2]  magic_limit  ||
 +               image-common.transform-matrix[0][2]  -magic_limit ||
 +               image-common.transform-matrix[1][2]  -magic_limit)
 +           {
 +               flags |= FAST_PATH_BILINEAR_FILTER;
 +           }
 +           else
 +           {
 +               flags |= FAST_PATH_NEAREST_FILTER;
 +           }
 +       }
        else
        {
            flags |= FAST_PATH_BILINEAR_FILTER;
 --
 1.7.3.4

 ___
 Pixman mailing list
 Pixman@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/pixman


___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH] Include noop in win32 builds

[Andrea Canciani]

---
 pixman/Makefile.win32 |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/pixman/Makefile.win32 b/pixman/Makefile.win32
index b5f9397..7c92722 100644
--- a/pixman/Makefile.win32
+++ b/pixman/Makefile.win32
@@ -49,6 +49,7 @@ SOURCES = \
pixman-radial-gradient.c\
pixman-bits-image.c \
pixman.c\
+   pixman-noop.c   \
pixman-cpu.c\
pixman-fast-path.c  \
pixman-implementation.c \
-- 
1.7.1

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 3/3] BILINEAR-NEAREST filter optimization for simple rotation and translation

[Andrea Canciani]

On Mon, May 23, 2011 at 3:16 PM, Siarhei Siamashka
siarhei.siamas...@gmail.com wrote:
 On Mon, May 23, 2011 at 9:48 AM, Andrea Canciani ranm...@gmail.com wrote:
 On Mon, May 23, 2011 at 8:43 AM, Andrea Canciani ranm...@gmail.com wrote:
 On Sun, May 22, 2011 at 11:15 PM, Siarhei Siamashka
 siarhei.siamas...@gmail.com wrote:
 From: Siarhei Siamashka siarhei.siamas...@nokia.com

 Simple rotation and translation are the additional cases when BILINEAR
 filter can be safely reduced to NEAREST.

 I believe that this reduction is valid for any matrix which describes
 a unit transform followed
 by an integer translation (i.e. rotation(90 degrees), scale(-1,1),
 translate(1,0) composed
 in any possible way).

 This happens whenever the translational component of the matrix is
 integral, the projective
 component is identity (already tested by AFFINE_TRANSFORM) and the
 upper-left 2x2
 matrix multiplied by its transpose results in the 2x2 identity.

 Uh, we already have an implementation of this (which is what cairo
 uses to optimize filters to nearest):
 http://cgit.freedesktop.org/cairo/tree/src/cairo-matrix.c#n728
 (lines 728-762)

 Yes, I know that cairo has such optimizations, you mentioned them earlier :)

I wanted to point to an existing (hence at least somewhat tested and
working) implementation of that reduction.


 I intentionally tried to keep the initial patch simple, lightweight
 and deal only with the identity transform because this is the most
 important case. The last patch in the series attempts to extend
 bilinear-nearest reduction to handle a bit more cases (check for the
 already set ROTATE flags is really cheap), but looks like not
 everything is so smooth (the FIXME part), which which needs some extra
 investigation.

A similar magic number came up in cairo tests:
http://cgit.freedesktop.org/cairo/tree/test/large-source-roi.c#n52

It looks like some checks against overflow conditions are too restrictive.


 Now we are going to basically have duplicated functionality in cairo
 and pixman, but pixman needs this optimization for the applications
 which use XRender directly or bypass cairo in some other way.

Yes, we already talked about this and if there are apps whose performance
can be enhanced by simplifying filtering in pixman (i.e. they do not rely on
cairo or something elso to do the reduction), then it is something appropriate
(and easy) to do in pixman.

Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH] test: Fix compilation on win32

[Andrea Canciani]

MSVC complains about uint32_t being used as an expression:

composite.c(902) : error C2275: 'uint32_t' : illegal use of this type
as an expression
---
 test/composite.c |4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/test/composite.c b/test/composite.c
index 9a001e5..edea9a9 100644
--- a/test/composite.c
+++ b/test/composite.c
@@ -877,7 +877,7 @@ main (int argc, char **argv)
 {
 #define N_TESTS (8 * 1024 * 1024)
 int result = 0;
-uint32_t i;
+uint32_t i, seed;
 
 if (argc  1)
 {
@@ -899,8 +899,6 @@ main (int argc, char **argv)
}
 }
 
-uint32_t seed;
-
 if (getenv (PIXMAN_RANDOMIZE_TESTS))
seed = get_random_seed();
 else
-- 
1.7.1

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] Pixman on iOS

[Andrea Canciani]

On Mon, Mar 7, 2011 at 8:09 PM, cu cairou...@yahoo.com wrote:

 I'm actually more worried that if this build for iPhone ends up being
 a one shot experiment without any maintainer to keep it alive, then it
 is going to bitrot soon. IMHO adding support for more marginal
 platforms (in cairo/pixman context) is not something that is clearly
 beneficial. More like it will attract the users who can't solve their
 problems themselves but complaining about bugs and breakages.



 That might be me :) That said, I think iOS is not a marginal platform -
 perhaps differently abled :) That it is not a major target for cairo is
 a matter of time and a bit of chicken  egg issue. It would be if there
 was support for it.

Cairo should already provide the needed support for iOS in the
Quartz backend. If that is the main backend used by the application,
Pixman should already be quite usable without NEON optimization,
as it will only be used for fallbacks. Of course it would be nice to get
NEON fast paths, if they can actually be used on that architecture.


 As for users that can't solve their own problems - that describes
 majority of users of any library, pixman not being any different.

 I'll give your pre-compiled optimized code a try. I wonder if there is a
 way to make some sort of a step-by-step writeup of what's needed to make
 code like that, so that someone with access to both Linux and iOS/MacOS
 could repeat these steps.

An example/model of such a guide could be:
http://www.cairographics.org/end_to_end_build_for_mac_os_x/

It gives you the whole sequence of commands needed to compile
the library (in the boxes) and explains what's happening and why.

Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [cairo] Planar YUV support

[Andrea Canciani]

On Thu, Mar 3, 2011 at 6:58 PM, Siarhei Siamashka
siarhei.siamas...@gmail.com wrote:
 On Wed, Mar 2, 2011 at 4:16 PM, Soeren Sandmann sandm...@cs.au.dk wrote:
 Siarhei Siamashka siarhei.siamas...@gmail.com writes:

 I'm not a big fan of let's make this totally universal and future
 proof approach if only a very small fraction of this functionality is
 going to be actually used. Moreover, I suspect that trying to be too
 general was responsible for slowing down the original Planar YUV
 support plan.

 Part of what was derailing that plan may have been my insisting on being
 precise about how these formats fit into the image processing pipeline,
 including how it related to gamma correction and other colorspace
 issues. I still think this is important. However, we can probably leave
 out some specific features, if there is a credible story about how to do
 them later.

 The pipeline as it is now:

    1 convert image sample to a8r8g8b8
    2 extend sample grid in all directions according to repeat
    3 interpolate between sample according to filter
    4 transform
    5 resample
    6 combine
    7 store

 What is the difference between 3 interpolate between sample according
 to filter and 5 resample?

 To add support for potentially subsampled YUV, some additional stages
 have to be inserted before the first:

   -2 interpolate subsampled components of YUV to get the same
      resolution as the Y plane

   -1 if the format is planar, stitch together components to form YUV
      pixels

    0 convert to sRGB

 Stage -2 is important because the filter used in that interpolation
 should probably be user-specifiable eventually, which has the
 implication that whatever simple support is added first, it needs to be
 clear what filter precisely is being used.

 Stage 0 is a color space conversion and need to eventually be
 configurable too, which means it has to be specified which matrix is
 being used.

 I'm not totally sure about stage -2. So source interpolation is going
 to happen twice in the pipeline, once when getting rid of subsampling,
 and the second time when applying transform? To me it looks more
 natural to just start with the transform, for each pixel in the
 destination space get a transformed fractional coordinate in the
 source image, look it up in the source YUV grid and interpolate each
 color component for each color plane separately according to the
 selected filter, and finally convert interpolated YUV color to
 a8r8g8b8 according to the used color matrix. Though I see some trouble
 using such approach when handling repeat for odd sized images with
 subsampling (imagine handling of NORMAL repeat for fractional sized
 RGB images as a somewhat similar example). I don't know what would be
 the best solution here.

Are subsampled images allowed to have a size which is not a multiple
of the downsampling? I think that most video formats do not allow this
and pixman should not either.


 The following PDF file has some nice pictures showing chroma siting in
 a very comprehensive way (for the people not familiar with this
 concept, but who might be interested to know what we are talking
 about):
 http://www.poynton.com/PDFs/Chroma_subsampling_notation.pdf

 There is also the question of how to *write* to a subsampled YUV
 image. I don't particularly like read-only image formats, but writing to
 YUV is not simple when you consider clip rectangles, because subsampling
 involves a filter that probably extends outside the clip boxes.

 Writing to subsampled YUV images somewhat resembles writing to
 transformed RGB images to me (if this was supported by pixman of
 course). Both are inherently lossy to the point of becoming
 ridiculous. And if a sequence of compositing operations has to be
 applied to YUV destination image, then each step would introduce a
 major distortion by itself. I see only one recipe here - never allow
 to do this :-) If really high quality image processing has to be done,
 then any subsampled YUV destination image needs to be converted to
 some intermediate format (preferably losslessly). Then all the
 composition has to be done with this intermediate format and only
 converted to the target format at the very last step. A variation of
 this is to hide all these steps from the end user by doing everything
 behind the scene, but it may be too complex and inefficient.

There are two possible approaches to writing on subsampled images,
but both are quite complicated. Since you do not need to add write
support to subsampled images, this can be safely deferred and it
probably should be, as we're already tackling multiple problems
with new formats and YUV support.


 As I don't have a serious need for writing to YUV images at this
 point, I would like to skip this YUV writing part completely for now
 :-)

 What Andrea is getting at, is presumably how to specify image formats in
 the API. A fully general API like he suggests is perhaps interesting at
 some point, but I agree it 

Re: [Pixman] [cairo] Planar YUV support

[Andrea Canciani]

On Wed, Mar 2, 2011 at 3:16 PM, Soeren Sandmann sandm...@cs.au.dk wrote:
 Siarhei Siamashka siarhei.siamas...@gmail.com writes:

 I'm not a big fan of let's make this totally universal and future
 proof approach if only a very small fraction of this functionality is
 going to be actually used. Moreover, I suspect that trying to be too
 general was responsible for slowing down the original Planar YUV
 support plan.

 Part of what was derailing that plan may have been my insisting on being
 precise about how these formats fit into the image processing pipeline,
 including how it related to gamma correction and other colorspace
 issues. I still think this is important. However, we can probably leave
 out some specific features, if there is a credible story about how to do
 them later.

 The pipeline as it is now:

    1 convert image sample to a8r8g8b8
    2 extend sample grid in all directions according to repeat
    3 interpolate between sample according to filter
    4 transform
    5 resample
    6 combine
    7 store

 To add support for potentially subsampled YUV, some additional stages
 have to be inserted before the first:

   -2 interpolate subsampled components of YUV to get the same
      resolution as the Y plane

   -1 if the format is planar, stitch together components to form YUV
      pixels

    0 convert to sRGB

 Stage -2 is important because the filter used in that interpolation
 should probably be user-specifiable eventually, which has the
 implication that whatever simple support is added first, it needs to be
 clear what filter precisely is being used.

 Stage 0 is a color space conversion and need to eventually be
 configurable too, which means it has to be specified which matrix is
 being used.

There are mainly two possibilities for the color space conversion:
a) convert to a standard color space in step 0 and convert the
   result of the combining  to the destination color space in step 6.5
b) perform the color conversion source color space - destination
   space color in step 5.5

SVG seems to use (a), with sRGB and linearRGB as possible color
spaces, but I'd rather have pixman implement the (b) option, which
is consistent with what PDF does.


 There is also the question of how to *write* to a subsampled YUV
 image. I don't particularly like read-only image formats, but writing to
 YUV is not simple when you consider clip rectangles, because subsampling
 involves a filter that probably extends outside the clip boxes.

 What Andrea is getting at, is presumably how to specify image formats in
 the API. A fully general API like he suggests is perhaps interesting at
 some point, but I agree it shouldn't be prerequisite for getting some
 YUV support in.

Yes, definitely. There are actually two independent problems:
 - extend formats (planar formats, subsampled formats, etc)
 - add color space support

Pixman can already do RGB-YUV conversion, so adding support for
a new YUV format with only a different memory layout should be quite
simple, but I'd like not to add new public functions just for this special
format (YUV formats are already quite special, being read-only).

Anyway, I think that if the number of formats you want to add is small,
there is no problem with adding them simply as format codes, but if
you find yourself adding a lot of formats which are the same as existing
ones beside for some generalization already included in my proposal,
you might want to consider it again

Example: YV12 can be reused to implement:
 - IYUV and I420 simply by changing the base pointer of each plane
 - NV12 and NV21 by specifying a different pixel_stride
 - IMC1, IMC2, IMC3, IMC4 by specifying appropriate row_strides
   and base pointers for each plane
None of these formats would require changes to pixman if we already
had pixman_format_t

Andrea


 Surely an alternative solution for html5 video fallbacks is to do
 YUV-RGB conversion of video frames outside of cairo/pixman, and only
 let cairo/pixman handle the RGB data. The only real problem with this
 solution is that such multi pass data processing is somewhat slower.
 We are speaking about at least tens of percents of performance here.
 And pixman support for native single pass YUV-RGB conversion combined
 with scaling could help.

 YUV conversion is definitely within the scope of pixman, both YUV-RGB
 and RGB-YUV. However, I'm not sure the existing YUV formats are very
 well though out.


 Soren

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] Win32 fixes and improvements

[Andrea Canciani]

On Tue, Feb 22, 2011 at 11:07 PM, Andrea Canciani ranm...@gmail.com wrote:
 In order to make pixman more maintainable on windows, having working
 Makefiles for the library and the tests is probably needed. Today I
 took the Makefile attached to
 https://bugs.freedesktop.org/show_bug.cgi?id=33069
 and tried to use it to build but it didn't build all the tests because
 of some incompatibilities between cl and gcc.

 The following patches should make it possible to build pixman and the
 entire test suite on Windows from git in a properly configured Cygwin
 environment.

 There are some remaining warnings:

 c:\cygwin\home\ranma42\code\fdo\pixman\pixman\pixman-mmx.c(317) :
 warning C4799: function 'store' has no EMMS instruction
 c:\cygwin\home\ranma42\code\fdo\pixman\pixman\pixman-mmx.c(166) :
 warning C4799: function 'to_uint64' has no EMMS instruction
 c:\cygwin\home\ranma42\code\fdo\pixman\pixman\pixman-mmx.c(437) :
 warning C4799: function 'combine' has no EMMS instruction

 These are wanrings about some missing MMX registers cleanup. I don't
 know if this is required or if the compiler just does not notice that
 it is already performed somewhere else.

 c:\cygwin\home\ranma42\code\fdo\pixman\test\fetch-test.c(114) :
 warning C4715: 'reader' : not all control paths return a value
 c:\cygwin\home\ranma42\code\fdo\pixman\test\stress-test.c(133) :
 warning C4715: 'real_reader' : not all control paths return a value
 c:\cygwin\home\ranma42\code\fdo\pixman\test\composite.c(431) :
 warning C4715: 'calc_op' : not all control paths return a value

 These are non-returning functions (abort() / assert(0)). They can be
 silenced by adding a return after the termination call, if we aim at a
 warning-free build on Windows.


I improved the patchset a little (I fixed the library makefile for real
and silenced the test warnings) and pushed it as a git branch here:
http://cgit.freedesktop.org/~ranma42/pixman/log/?h=wip/win32

These are my first step in the win32 build environment, so review
and testing are greatly appreciated.

Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 2/3] test: Fix tests for compilation on Windows

[Andrea Canciani]

On Wed, Feb 23, 2011 at 6:26 PM, Brent Fulgham bfulg...@gmail.com wrote:
 Thanks for making these fixes -- they look great!

 I have one small suggestion regarding your update to composite.c;
 while MSVC does not provide an snprintf symbol, it does provide
 _snprintf.

The documentation states that it is deprecated and that it does not behave
as snprintf:
The _snprintf function formats and stores count or fewer characters
in buffer, and appends a terminating null character if the formatted
string length is strictly less than count characters.
(from http://msdn.microsoft.com/en-us/library/2ts7cx93%28v=vs.80%29.aspx )

snprintf, instead, guarantees the output sting to always be nul-terminated.

The code can be safely and easily rewritten to avoid snprintf altogether,
so I modified it this way.

If the previous snprintf-based logic is to be preserved, on WIN32 we should
define the snprintf function to _snprintf and then nul-terminate the string.


 To be consistent with other ports, I would propose the following change:

 bfulgham@bfulgham3 ~/requirements_sources/cairo/git/pixman/test
 $ git diff composite.c
 diff --git a/test/composite.c b/test/composite.c
 index e14f954..b4bde09 100644
 --- a/test/composite.c
 +++ b/test/composite.c
 @@ -36,6 +36,10 @@ typedef struct format_t format_t;
  typedef struct image_t image_t;
  typedef struct operator_t operator_t;

 +#ifdef _WIN32
 +#define snprintf _snprintf
 +#endif
 +
  struct color_t
  {
     double r, g, b, a;

 This builds and runs properly for me.

Yes, we're not hitting problems. If we don't actually care about
the buffer overrun, we can just use sprintf.

Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 1/3] Fix compilation on Win32

[Andrea Canciani]

Building the library from a clean git repository fails with:
pixman-image.c(33) : fatal error C1083: Cannot open include file:
'pixman-combine32.h': No such file or directory

pixman-combine32.h is not used by pixman-image.c, so its inclusion can
simply be removed.
---
 pixman/pixman-image.c |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
index 9103ca6..84bacf8 100644
--- a/pixman/pixman-image.c
+++ b/pixman/pixman-image.c
@@ -30,7 +30,6 @@
 #include assert.h
 
 #include pixman-private.h
-#include pixman-combine32.h
 
 pixman_bool_t
 _pixman_init_gradient (gradient_t *  gradient,
-- 
1.7.1

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 2/3] test: Fix tests for compilation on Windows

[Andrea Canciani]

The Microsoft C compiler cannot handle subobject initialization and
Win32 does not provide snprintf.

Work around these limitations by using normal struct initailization
and directly using printf.
---
 test/composite.c|   48 +++---
 test/fetch-test.c   |   52 ++
 test/trap-crasher.c |   20 +-
 3 files changed, 61 insertions(+), 59 deletions(-)

diff --git a/test/composite.c b/test/composite.c
index e14f954..33e8d97 100644
--- a/test/composite.c
+++ b/test/composite.c
@@ -616,22 +616,20 @@ eval_diff (color_t *expected, color_t *test, 
pixman_format_code_t format)
 return MAX (MAX (MAX (rdiff, gdiff), bdiff), adiff);
 }
 
-static char *
-describe_image (image_t *info, char *buf, int buflen)
+static void
+describe_image (image_t *info)
 {
 if (info-size)
 {
-   snprintf (buf, buflen, %s %dx%d%s,
+   printf (%s %dx%d%s,
  info-format-name,
  info-size, info-size,
  info-repeat ? R :);
 }
 else
 {
-   snprintf (buf, buflen, solid);
+   printf (solid);
 }
-
-return buf;
 }
 
 /* Test a composite of a given operation, source, mask, and destination
@@ -708,18 +706,13 @@ composite_test (image_t *dst,
  */
 if (diff  3.0)
 {
-   char buf[40];
-
-   snprintf (buf, sizeof (buf),
- %s %scomposite,
- op-name,
- component_alpha ? CA  : );
-
-   printf (%s test error of %.4f --\n
+   printf (%s %scomposite test error of %.4f --\n
   RGBA\n
got:   %.2f %.2f %.2f %.2f [%08lx]\n
expected:  %.2f %.2f %.2f %.2f\n,
-   buf, diff,
+   op-name,
+   component_alpha ? CA  : ,
+   diff,
result.r, result.g, result.b, result.a,
*(unsigned long *) pixman_image_get_data (dst-image),
expected.r, expected.g, expected.b, expected.a);
@@ -735,9 +728,18 @@ composite_test (image_t *dst,
mask-color-b, mask-color-a,
dst-color-r, dst-color-g,
dst-color-b, dst-color-a);
-   printf (src: %s, , describe_image (src, buf, sizeof (buf)));
-   printf (mask: %s, , describe_image (mask, buf, sizeof (buf)));
-   printf (dst: %s\n\n, describe_image (dst, buf, sizeof (buf)));
+
+   printf (src: );
+   describe_image (src);
+   printf (, );
+
+   printf (mask: );
+   describe_image (mask);
+   printf (, );
+
+   printf (dst: );
+   describe_image (dst);
+   printf (\n\n);
}
else
{
@@ -747,8 +749,14 @@ composite_test (image_t *dst,
src-color-b, src-color-a,
dst-color-r, dst-color-g,
dst-color-b, dst-color-a);
-   printf (src: %s, , describe_image (src, buf, sizeof (buf)));
-   printf (dst: %s\n\n, describe_image (dst, buf, sizeof (buf)));
+
+   printf (src: );
+   describe_image (src);
+   printf (, );
+
+   printf (dst: );
+   describe_image (dst);
+   printf (\n\n);
}
 
success = FALSE;
diff --git a/test/fetch-test.c b/test/fetch-test.c
index 2ca16dd..314a072 100644
--- a/test/fetch-test.c
+++ b/test/fetch-test.c
@@ -8,7 +8,7 @@
 
 static pixman_indexed_t mono_palette =
 {
-.rgba = { 0x, 0x00ff },
+0, { 0x, 0x00ff },
 };
 
 
@@ -24,57 +24,53 @@ typedef struct {
 static testcase_t testcases[] =
 {
 {
-   .format = PIXMAN_a8r8g8b8,
-   .width = 2, .height = 2,
-   .stride = 8,
-   .src = { 0x00112233, 0x44556677,
-0x8899aabb, 0xccddeeff },
-   .dst = { 0x00112233, 0x44556677,
-0x8899aabb, 0xccddeeff },
-   .indexed = NULL,
+   PIXMAN_a8r8g8b8,
+   2, 2,
+   8,
+   { 0x00112233, 0x44556677,
+ 0x8899aabb, 0xccddeeff },
+   { 0x00112233, 0x44556677,
+ 0x8899aabb, 0xccddeeff },
+   NULL,
 },
 {
-   .format = PIXMAN_g1,
-   .width = 8, .height = 2,
-   .stride = 4,
+   PIXMAN_g1,
+   8, 2,
+   4,
 #ifdef WORDS_BIGENDIAN
-   .src =
{
0xaa00,
0x5500
},
 #else
-   .src =
{
0x0055,
0x00aa
},
 #endif
-   .dst =
{
0x00ff, 0x, 0x00ff, 0x, 0x00ff, 
0x, 0x00ff, 0x,
0x, 0x00ff, 0x, 0x00ff, 0x, 
0x00ff, 0x, 0x00ff
},
-   .indexed = mono_palette,
+   mono_palette,
 },
 #if 0
 {
-   .format = PIXMAN_g8,
-   .width = 4, .height = 2,
-   .stride = 4,
-   .src = { 0x01234567,
-

[Pixman] [PATCH 3/3] test: add Makefile for Win32

[Andrea Canciani]

---
 test/Makefile.win32 |   73 +++
 1 files changed, 73 insertions(+), 0 deletions(-)
 create mode 100644 test/Makefile.win32

diff --git a/test/Makefile.win32 b/test/Makefile.win32
new file mode 100644
index 000..c71afe1
--- /dev/null
+++ b/test/Makefile.win32
@@ -0,0 +1,73 @@
+CC   = cl
+LINK = link
+
+CFG_VAR = $(CFG)
+ifeq ($(CFG_VAR),)
+CFG_VAR=release
+endif
+
+CFLAGS = -MD -nologo -D_CRT_SECURE_NO_DEPRECATE 
-D_CRT_NONSTDC_NO_DEPRECATE -D_BIND_TO_CURRENT_VCLIBS_VERSION -D_MT -I../pixman 
-I. -I../
+TEST_LDADD = ../pixman/$(CFG_VAR)/pixman-1.lib
+INCLUDES = -I../pixman -I$(top_builddir)/pixman
+
+# optimization flags
+ifeq ($(CFG_VAR),debug)
+CFLAGS += -Od -Zi
+else
+CFLAGS += -O2
+endif
+
+SOURCES =  \
+   a1-trap-test.c  \
+   pdf-op-test.c   \
+   region-test.c   \
+   region-translate-test.c \
+   fetch-test.c\
+   oob-test.c  \
+   trap-crasher.c  \
+   alpha-loop.c\
+   scaling-crash-test.c\
+   gradient-crash-test.c   \
+   alphamap.c  \
+   stress-test.c   \
+   composite-traps-test.c  \
+   blitters-test.c \
+   scaling-test.c  \
+   affine-test.c   \
+   composite.c \
+   utils.c
+
+TESTS =\
+   $(CFG_VAR)/a1-trap-test.exe \
+   $(CFG_VAR)/pdf-op-test.exe  \
+   $(CFG_VAR)/region-test.exe  \
+   $(CFG_VAR)/region-translate-test.exe\
+   $(CFG_VAR)/fetch-test.exe   \
+   $(CFG_VAR)/oob-test.exe \
+   $(CFG_VAR)/trap-crasher.exe \
+   $(CFG_VAR)/alpha-loop.exe   \
+   $(CFG_VAR)/scaling-crash-test.exe   \
+   $(CFG_VAR)/gradient-crash-test.exe  \
+   $(CFG_VAR)/alphamap.exe \
+   $(CFG_VAR)/stress-test.exe  \
+   $(CFG_VAR)/composite-traps-test.exe \
+   $(CFG_VAR)/blitters-test.exe\
+   $(CFG_VAR)/scaling-test.exe \
+   $(CFG_VAR)/affine-test.exe  \
+   $(CFG_VAR)/composite.exe
+
+
+OBJECTS = $(patsubst %.c, $(CFG_VAR)/%.obj, $(SOURCES))
+
+$(CFG_VAR)/%.obj: %.c
+   @mkdir -p $(CFG_VAR)
+   @$(CC) -c $(CFLAGS) -Fo$@ $
+
+$(CFG_VAR)/%.exe: $(CFG_VAR)/%.obj
+   $(LINK) /NOLOGO /OUT:$@ $ $(CFG_VAR)/utils.obj $(TEST_LDADD)
+
+all: $(OBJECTS) $(TESTS)
+   @exit 0
+
+clean:
+   @rm -f $(CFG_VAR)/*.obj $(CFG_VAR)/*.pdb || exit 0
-- 
1.7.1

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Avoid marking images dirty when properties are reset

[Andrea Canciani]

On Tue, Feb 15, 2011 at 10:55 AM, Soeren Sandmann sandm...@cs.au.dk wrote:
 When an image property is set to the same value that it already is,
 there is no reason to mark the image dirty and incur a recomputation
 of the flags.
 ---
  pixman/pixman-image.c |   20 +++-
  1 files changed, 19 insertions(+), 1 deletions(-)

 diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
 index e91d87c..9103ca6 100644
 --- a/pixman/pixman-image.c
 +++ b/pixman/pixman-image.c
 @@ -502,7 +502,7 @@ pixman_image_set_transform (pixman_image_t *          
 image,
     if (common-transform == transform)
        return TRUE;

 -    if (memcmp (id, transform, sizeof (pixman_transform_t)) == 0)
 +    if (!transform || memcmp (id, transform, sizeof (pixman_transform_t)) 
 == 0)
     {
        free (common-transform);
        common-transform = NULL;

This change looks like a bugfix unrelated to the commit message.
Overall, the patch looks correct.

Andrea

 @@ -511,6 +511,12 @@ pixman_image_set_transform (pixman_image_t *          
 image,
        goto out;
     }

 +    if (common-transform 
 +       memcmp (common-transform, transform, sizeof (pixman_transform_t) == 
 0))
 +    {
 +       return TRUE;
 +    }
 +
     if (common-transform == NULL)
        common-transform = malloc (sizeof (pixman_transform_t));

 @@ -535,6 +541,9 @@ PIXMAN_EXPORT void
  pixman_image_set_repeat (pixman_image_t *image,
                          pixman_repeat_t repeat)
  {
 +    if (image-common.repeat == repeat)
 +       return;
 +
     image-common.repeat = repeat;

     image_property_changed (image);
 @@ -579,6 +588,9 @@ PIXMAN_EXPORT void
  pixman_image_set_source_clipping (pixman_image_t *image,
                                   pixman_bool_t   clip_sources)
  {
 +    if (image-common.clip_sources == clip_sources)
 +       return;
 +
     image-common.clip_sources = clip_sources;

     image_property_changed (image);
 @@ -594,6 +606,9 @@ pixman_image_set_indexed (pixman_image_t *        image,
  {
     bits_image_t *bits = (bits_image_t *)image;

 +    if (bits-indexed == indexed)
 +       return;
 +
     bits-indexed = indexed;

     image_property_changed (image);
 @@ -656,6 +671,9 @@ PIXMAN_EXPORT void
  pixman_image_set_component_alpha   (pixman_image_t *image,
                                     pixman_bool_t   component_alpha)
  {
 +    if (image-common.component_alpha == component_alpha)
 +       return;
 +
     image-common.component_alpha = component_alpha;

     image_property_changed (image);
 --
 1.6.0.6

 ___
 Pixman mailing list
 Pixman@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/pixman

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [cairo] pixman: New ARM NEON optimizations

[Andrea Canciani]

On Fri, Feb 11, 2011 at 11:30 AM, Soeren Sandmann sandm...@cs.au.dk wrote:
 Chris Wilson ch...@chris-wilson.co.uk writes:

 But at the moment, about the only thing I truly want to add to the API
 is a LERP operator.

 Cairo has a slightly different definition for [clip-] masked operators
 where the operation is defined as:

   ((src IN mask) OP dst) LERP_clip dst

 and for SOURCE and CLEAR:

   (src OP dst) LERP_(clip IN mask) dst

 Cairo currently implements LERP as:

   a LERP_t b := (a IN t) ADD (b OUT t)

 So for example, for the typical unbounded operation (such as IN) this
 translates to:

   result = (((src IN mask) OP dst) IN clip) ADD (dst OUT clip)

 which currently requires 3 composite operations:

   dst' = dst; /* may require a blt using SRC */
   pixman_image_composite (OP, src, mask, dst');
   pixman_image_composite (OUT_REVERSE, clip, NULL, dst);
   pixman_image_composite (ADD, dst', clip, dst);

 By introducing the LERP operator we can reduce this down to 2:

   dst' = dst; /* may require a blt using SRC */
   pixman_image_composite (OP, src, mask, dst');
   pixman_image_composite (LERP, dst', clip, dst);

 Obviously, we could introduce a quaternary operator and do this in a
 single pass - though the complexity would seem to outweigh this rarely
 used slow path.

 So, I realized I never actually responded to this, sorry about that.

 If I had a time machine, I would go back and make - among others -
 these two changes to Render:

 (1) The equation would be

        (src OP dst) LERP_mask dst

    and not

        (src IN mask) OP dst

 (2) The RGB channels of Alpha-only images would be considered to be
    the same as the alpha channel, and not 0 as they are now. For
    example, a 0xb9 pixel in an a8 image would be considered
    equivalent to 0xb9b9b9b9 and not to 0xb900. That is, they
    would be considered a translucent white rather than a translucent
    black.

Isn't it possible for pixman to dynamically do that using a new iterator
type (mask)?
Pixman would then only need component-alpha operators, but I'm
afraid it would still need to provide fastpaths for the old non-ca
operators to avoid performance regressions.


 These two changes together would have the effect that (a) the equation
 would be much easier to understand visually (composite src and dst,
 then clip to the mask and write back), and (b) component alpha would
 become completely regular with no need for the component_alpha bit
 in pictures.

 Given the lack of a time machine, a possible direction might be to add
 a new set of operators CLEAR_LERP, SRC_LERP, DST_LERP, ..., that would
 follow the equation above, and also a new format type
 PIXMAN_FORMAT_TYPE_W where W stands for 'white', and the missing color
 channels would be defined to be copies of the W channel.

 Even better might be to add a new entry-point:

   pixman_image_composite_lerp (...):

 that would use the LERP equation and treat alpha-only formats as
 described above.

 With either, your LERP operator would simply be SRC_LERP, but the
 other other _LERP operators might be useful for cairo too. For
 example, the

  ((src IN mask) OP dst) LERP_clip dst

 equation could become

         composite (SRC, src, mask, tmp);
         composite (OP_LERP, tmp, clip, dst);

 instead of

         composite (SRC, dst, NULL, tmp);
         composite (OP, src, mask, tmp);
         composite (SRC_LERP, tmp, clip, dst);

 Adding all those operators is obviously a lot of work, so I can see
 why it would be tempting to just add the LERP operator. However, it
 would annoy me to have one single weird operator that would behave
 according to a totally different equation than all the other
 operators.

 If it were a huge speedup, then maybe, but it seems to be that it's
 only useful in relatively few cases. Or alternatively, if we have to
 bite the bullet and add special-cased support for this, then maybe it
 would be better to just add the full quaternary operator as a new
 entry point, and not pretend it's just another regular operator.

Quaternary operators would avoid the temporary image, which might
provide a very nice performance boost. An even better performance
improvement might be possible if the clip was handled in pixman
as spans (either emitted by cairo or by a pixman polygon image).
Unfortunately I don't have any data to back there opinions, but IIRC
Chris has been experimenting with spans in xcb-xrender, so he
might be able to confirm or correct me.

Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] warnings in pixman 0.21.4

[Andrea Canciani]

On Tue, Jan 25, 2011 at 4:38 PM, Rolland Dudemaine roll...@ghs.com wrote:

 - *max_vx.diff corrects the initialization of max_vx. Contrary to what
 you mention, the variable is declared in a local block, so the scope of
 the variable is restricted to that block, regardless of whether it is
 part of a macro or not. So the compiler is correct saying the value may
 be used before being initialized. Also, INT32_MAX is a safe value,
 because pixman-compiler.h defines it. INT32_MAX is the right value to
 use since max_vx is a clamping value passed on to a callee. 0 would just
 clamp down to 0, which would be plainly incorrect.

 Pushed this one too. Even though I think that your analysis is wrong, I
 don't
 want to waste time arguing about the choice of the constant ;)

 I agree with Siarhei, max_vx is used in scanline_func if and only if
 the repeat mode is NORMAL, so it should not actually matter.
 Anyway, INT32_MAX is obviously a safe value so your patch is ok for me.
 Since any value is fine in that initialization, the problem with
 pixman_fixed_t
 becoming a different type in future is moot as well. I think that
 initializing
 to 0 would be ok as well, but I doubt that it's an optimization worth the
 additional readability cost.

 Ultimately, the function should probably be splitted in two so as not to
 rely on the compiler to optimize away half of the code in each case.

 - *correct_dimmy_PIXMAN_types.diff is the most complex part of the
 patches. My compiler was producing a number of warnings due to incorrect
 uses of int instead of the correct corresponding pixman_*_t type. Also,
 it was complaining (rightfully) that some values were passed as
 pixman_format_code_t that were not defined in that enum. The best
 resolution I could come up with was to logically move PIXMAN_null,
 PIXMAN_any and the related dummy pixmap types to the enum, so that it is
 possible to use them as argument where a pixman_format_code_t argument
 is required. (Note : the attached patch contains one more correction for
 blitters-test.c that I did not correct previously)

 The biggest complaint against 'correct_dimmy_PIXMAN_types.diff' is that
 it
 moves the definitions of PIXMAN_solid, PIXMAN_pixbuf, PIXMAN_rpixbuf,
 PIXMAN_unknown, PIXMAN_any, PIXMAN_OP_any to the public header
 'pixman.h'.
 And these are intended for internal use only and not supposed to be part
 of
 external API. Are there any good alternative fixes for the problem?

 We can add a cast to pixman_op_t or pixman_format_code_t in the #defines.
 Another solution (which I don't like) would be to protect the new enum
 values
 with an #ifdef PIXMAN_USE_INTERNAL_API, just like in pixman.h for
 PIXMAN_N_OPERATORS.

 I'm not sure what a clever compiled would do with a value that doesn't
 belong to the enum is casted to the enum, but I would expect it to complain
 that the value is not defined in the enum, resulting in a different but
 nonetheless replacement warning.

I think that upon an explicit cast, a C compiler should not complain.
Otherwise the cast operator (which is obviously legitimate in C) would
warn in almost all cases in that compiler. Cast behavior is defined in the
standard and it looks like the compiler is not allowed to complain about
the value being converted not being present in the enum.

 I'm not sure what the issue is with adding those dummy ones to the enum. If
 your concern is that you cannot count the number of elements in the enum,
 then maybe one entry should be added to define a _MAX value.

The problem is that they should not be public and that they are special
values, not really elements of that enumeration, they are more like
wildcards.


 --Rolland Dudemaine
 ___
 Pixman mailing list
 Pixman@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/pixman

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 0/18] Add iterators to images

[Andrea Canciani]

On Sat, Jan 8, 2011 at 12:15 PM, Soeren Sandmann sandm...@cs.au.dk wrote:
 Iterators gets initialized with a buffer which can always contain at
 least width pixels. This is sufficient for current image types,
 but future image types might need additional space.

 Would it be possible to have the general_composite_rect ask to the
 iterator about how much memory it will need (and provide a buffer
 with at least this much memory)?

 If future images need something like this, the iterators can be
 extended to support it. It's always difficult to predict what
 infrastructure might be needed in the future.

 Or would it be better to have the iterator allocate that memory internally
 upon initialization?
 In this case, we should probably have the initializer return success/error
 and also have some kind of iter_fini()

 Right, that's another possibility, although if memory allocation
 fails, the image might also simply install a noop() fetcher. If we get
 out-of-memory errors during rendering, we generally don't guarantee
 the results, except that it obeys they clipping rules. This is
 something pixman has inherited from the X server.

 Apart from this, I skimmed through the patches and noticed that
 sometimes a change is fixed or improved by a following patch
 (example: patch 15/18 is immediately generalized by patch 16/18). Is
 this intentional?

 Yeah, generally, I'm trying to avoid patches being too
 clairvoyant. In principle, new infrastructure shouldn't be
 introduced until something else makes it necessary. It isn't always
 possible or convenient to do it that way, though.

 Wrt. localized alpha:

 I thought I understood the definition after the first two lines, then
 the OVER/ADD explanation confused me. After reading again everything,
 I was convinced again that I had understood it correctly the first
 time.

 I don't know if this is actually better (I still think that it needs a
 better wording), but I'd suggest something like:

 +    /* Separable alpha is when the alpha channel is used only to compute 
 the
 +     * alpha value of the destination. This means that the computation of 
 the
 +     * RGB values of the result is independent of the alpha value.
 +     *
 +     * For example, the OVER operator has separable alpha for the 
 destination,
 +     * because the RGB values of the result can be computed without knowing
 +     * the destination alpha. Similarly, ADD has separable alpha for both 
 source
 +     * and destination because the RGB values of the result can be
 +     * computed without knowing the alpha value of source or destination.

 I'm proposing the separable term because it is consistent with
 separable blend modes (compositing operators in which each channel
 can be computed individually), whereas localized didn't tell me
 anything until I read the explanation.

 I think your new description is better, but I don't like using the
 word separable for two things that are not really the same. A
 separable operator is one that can be applied separately for each R,
 G, and B channel, whereas the localized alpha means that you can
 compute the R, G, and B channels independent from the alpha channel in
 question.

 I have pushed a new branch here:

    http://cgit.freedesktop.org/~sandmann/pixman/log/?h=iterators4

 that shold take care of all your other comments. In addition, I also
 squashed together moving the gradient initializers; there didn't seem
 to be much gained from having them in separate commits.

Thank you! I find this branch much easier to read.

In Move get_scanline_32/64 to the bits part of the image struct.
(commit ffbd7a2722fff502f1ad6200ba3a009704df9d66)
_pixman_image_get_scanline_generic_64 could easily be improved
by pixman_contract()-ing mask8 only if it is not NULL instead of returning
in case of an allocation failure (i.e. if it can't malloc, it fetches the whole
row, which is slower but correct).
(This change might belong to a different commit, because the wrong code
is already present in pixman/master and your commit was only moving it)

In Skip fetching pixels when possible
(commit b2842d6d2b9ab7b1380c3cb95956dc886fcd1ee7), would it be possible
to use get_scanline_null() in _pixman_implementation_src_iter_init() when the
flags are NOOP or, which is the same, to replace both src and mask with
NULL when their flags are NOOP?

This won't give a performance boost (the noop and null iterators should
have very similar speed), but it should immediately point out if a compositing
function incorrectly tries to access data which it should be ignoring.

nitpicking

In Skip fetching pixels when possible it would also be nice to have the
NOOP macro defined once and used everywhere (pixman-bits-image.c,
pixman-general.c, pixman-implementation.c)

The punctuation is slightly inconsistent between different commit messages
and in Linear: Optimize for horizontal gradients.
(46a0115bf62fb61da357c3ece654b57c9947202c) the commit message ends
with ... 

Re: [Pixman] [PATCH 0/18] Add iterators to images

[Andrea Canciani]

On Thu, Jan 6, 2011 at 3:43 AM, Soeren Sandmann sandm...@cs.au.dk wrote:
 Søren Sandmann sandm...@cs.au.dk writes:

 The following patch series changes the scanline access to be based on
 iterators instead of direct calls to virtual functions. There are
 several benefits to this:

It also provides a natural place to initialize the colorspace conversion :)
Or, much more likely to be useful in the short term, a place where
gradient walkers can be initialized (and the color function can be
sampled if the underlying implementation benefits from this).


 The patches are also available in the iterators3 branch here:

 http://cgit.freedesktop.org/~sandmann/pixman/log/?h=iterators3

Iterators gets initialized with a buffer which can always contain at least
width pixels. This is sufficient for current image types, but future image
types might need additional space.

Would it be possible to have the general_composite_rect ask to the iterator
about how much memory it will need (and provide a buffer with at least this
much memory)?
Or would it be better to have the iterator allocate that memory internally
upon initialization?
In this case, we should probably have the initializer return success/error
and also have some kind of iter_fini()

I think that it would be nice to make some additional promises about
what the code outside the iterator will do with the pointers it gets from
get_scanline. Having it in the code (i.e. returning const pointers) would
be good because the compiler would be able to warn us about incorrect
use, but it would require a different get_scanline_src and get_scanline_dst,
which isn't worth it as long as we only use iterators in a single place.

Apart from this, I skimmed through the patches and noticed that sometimes
a change is fixed or improved by a following patch  (example: patch 15/18
is immediately generalized by patch 16/18). Is this intentional?

I also have some comments on specific patches, which will follow.

Andrea

PS: Well done!
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 14/18] Get rid of the classify methods.

[Andrea Canciani]

On Thu, Jan 6, 2011 at 3:27 AM, Søren Sandmann sandm...@cs.au.dk wrote:
 From: Søren Sandmann Pedersen s...@redhat.com

 They are not used anymore, and the linear gradient is now doing the
 optimization in a different way.
 ---
  pixman/pixman-image.c           |   14 --
  pixman/pixman-linear-gradient.c |   28 +---
  pixman/pixman-private.h         |   19 ---
  pixman/pixman-solid-fill.c      |   12 
  4 files changed, 13 insertions(+), 60 deletions(-)

 diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
 index 74efa6f..e276eac 100644
 --- a/pixman/pixman-image.c
 +++ b/pixman/pixman-image.c
 @@ -72,7 +72,6 @@ _pixman_image_allocate (void)
        common-alpha_map = NULL;
        common-component_alpha = FALSE;
        common-ref_count = 1;
 -       common-classify = NULL;
        common-property_changed = NULL;
        common-client_clip = FALSE;
        common-destroy_func = NULL;
 @@ -83,19 +82,6 @@ _pixman_image_allocate (void)
     return image;
  }

 -source_image_class_t
 -_pixman_image_classify (pixman_image_t *image,
 -                        int             x,
 -                        int             y,
 -                        int             width,
 -                        int             height)
 -{
 -    if (image-common.classify)
 -       return image-common.classify (image, x, y, width, height);
 -    else
 -       return SOURCE_IMAGE_CLASS_UNKNOWN;
 -}
 -
  static void
  image_property_changed (pixman_image_t *image)
  {
 diff --git a/pixman/pixman-linear-gradient.c b/pixman/pixman-linear-gradient.c
 index 20be249..b778601 100644
 --- a/pixman/pixman-linear-gradient.c
 +++ b/pixman/pixman-linear-gradient.c
 @@ -31,21 +31,21 @@
  #include stdlib.h
  #include pixman-private.h

 -static source_image_class_t
 -linear_gradient_classify (pixman_image_t *image,
 -                          int             x,
 -                          int             y,
 -                          int             width,
 -                          int             height)
 +static pixman_bool_t
 +linear_gradient_is_horizontal (pixman_image_t *image,
 +                              int             x,
 +                              int             y,
 +                              int             width,
 +                              int             height)
  {
     linear_gradient_t *linear = (linear_gradient_t *)image;
     pixman_vector_t v;
     pixman_fixed_32_32_t l;
     pixman_fixed_48_16_t dx, dy;
     double inc;
 -    source_image_class_t class;
 +    pixman_bool_t result;

Why don't you just return FALSE/TRUE?
Initializing the result to FALSE and returning it
makes the code harder to follow.


 -    class = SOURCE_IMAGE_CLASS_UNKNOWN;
 +    result = FALSE;

     if (image-common.transform)
     {
 @@ -54,7 +54,7 @@ linear_gradient_classify (pixman_image_t *image,
            image-common.transform-matrix[2][1] != 0 ||
            image-common.transform-matrix[2][2] == 0)
        {
 -           return class;
 +           return result;
        }

        v.vector[0] = image-common.transform-matrix[0][1];
 @@ -74,7 +74,7 @@ linear_gradient_classify (pixman_image_t *image,
     l = dx * dx + dy * dy;

     if (l == 0)
 -       return class;
 +       return result;

     /*
      * compute how much the input of the gradient walked changes
 @@ -86,9 +86,9 @@ linear_gradient_classify (pixman_image_t *image,

     /* check that casting to integer would result in 0 */
     if (-1  inc  inc  1)
 -       class = SOURCE_IMAGE_CLASS_HORIZONTAL;
 +       result = TRUE;

 -    return class;
 +    return result;
  }

  static uint32_t *
 @@ -243,8 +243,7 @@ _pixman_linear_gradient_iter_init (pixman_image_t *image,
                                   uint8_t        *buffer,
                                   iter_flags_t    flags)
  {
 -    if (linear_gradient_classify (image, x, y, width, height) ==
 -       SOURCE_IMAGE_CLASS_HORIZONTAL)
 +    if (linear_gradient_is_horizontal (image, x, y, width, height))
     {
        if (flags  ITER_NARROW)
            linear_gradient_get_scanline_narrow (iter, NULL);
 @@ -291,7 +290,6 @@ pixman_image_create_linear_gradient (pixman_point_fixed_t 
 *        p1,
     linear-p2 = *p2;

     image-type = LINEAR;
 -    image-common.classify = linear_gradient_classify;

     return image;
  }
 diff --git a/pixman/pixman-private.h b/pixman/pixman-private.h
 index d62f4cd..8d3a604 100644
 --- a/pixman/pixman-private.h
 +++ b/pixman/pixman-private.h
 @@ -60,17 +60,6 @@ typedef enum
     SOLID
  } image_type_t;

 -typedef enum
 -{
 -    SOURCE_IMAGE_CLASS_UNKNOWN,
 -    SOURCE_IMAGE_CLASS_HORIZONTAL,
 -} source_image_class_t;
 -
 -typedef source_image_class_t (*classify_func_t) (pixman_image_t *image,
 -                                               int             x,
 -                                               int             y,
 -                                               int             width,
 -      

Re: [Pixman] [PATCH 18/18] Fix destination fetching.

[Andrea Canciani]

On Thu, Jan 6, 2011 at 3:28 AM, Søren Sandmann sandm...@cs.au.dk wrote:
 From: Søren Sandmann Pedersen s...@redhat.com

 When fetching from destinations, we need to ignore transformations,
 repeat and filtering. Currently we don't ignore them, which means all
 kinds of bad things can happen.

 This bug fixes this problem by separating the concepts of source and
 destination iterator instead of using the ITER_WRITE flag to indicate
 which is which.

 Apart from making it simple for destination iterators to plug in
 alternate fetchers, this also allows a small optimization where source
 iterators no longer have a separate next line call; instead they are
 supposed to prepare themselves for the next line in the get_scanline
 call.

This patch changes the iterators interface. How about starting with
this interface? Would it bloat the previous commits a lot?


 Destination iterators do now have a separate write_back call that is
 supposed to write the pixels back to the destination and prepare the
 iterator for the next scanline.
 ---
  pixman/pixman-bits-image.c       |  107 ++---
  pixman/pixman-conical-gradient.c |    3 +-
  pixman/pixman-general.c          |   59 ++--
  pixman/pixman-image.c            |    2 +-
  pixman/pixman-implementation.c   |   77 +++-
  pixman/pixman-linear-gradient.c  |    4 +-
  pixman/pixman-private.h          |   60 -
  pixman/pixman-radial-gradient.c  |    3 +-
  pixman/pixman-solid-fill.c       |    1 -
  pixman/pixman-utils.c            |   11 
  10 files changed, 208 insertions(+), 119 deletions(-)

 diff --git a/pixman/pixman-bits-image.c b/pixman/pixman-bits-image.c
 index 7d588a9..4d8c1dc 100644
 --- a/pixman/pixman-bits-image.c
 +++ b/pixman/pixman-bits-image.c
 @@ -1350,7 +1350,7 @@ static uint32_t *
  get_scanline_narrow (pixman_iter_t *iter, const uint32_t *mask)
  {
     iter-image-bits.get_scanline_32 (
 -       iter-image, iter-x, iter-y, iter-width, iter-buffer, mask);
 +       iter-image, iter-x, iter-y++, iter-width, iter-buffer, mask);

     return iter-buffer;
  }
 @@ -1359,13 +1359,59 @@ static uint32_t *
  get_scanline_wide (pixman_iter_t *iter, const uint32_t *mask)
  {
     iter-image-bits.get_scanline_64 (
 -       iter-image, iter-x, iter-y, iter-width, iter-buffer, mask);
 +       iter-image, iter-x, iter-y++, iter-width, iter-buffer, mask);
 +
 +    return iter-buffer;
 +}
 +
 +static uint32_t *
 +dest_get_scanline_narrow (pixman_iter_t *iter, const uint32_t *mask)
 +{
 +    pixman_image_t *image  = iter-image;
 +    int             x      = iter-x;
 +    int             y      = iter-y;
 +    int             width  = iter-width;
 +    uint32_t *     buffer = iter-buffer;
 +
 +    image-bits.fetch_scanline_32 (image, x, y, width, buffer, mask);
 +    if (image-common.alpha_map)
 +    {
 +       x -= image-common.alpha_origin_x;
 +       y -= image-common.alpha_origin_y;
 +
 +       image-common.alpha_map-fetch_scanline_32 (
 +           (pixman_image_t *)image-common.alpha_map,
 +           x, y, width, buffer, mask);
 +    }
 +
 +    return iter-buffer;
 +}
 +
 +static uint32_t *
 +dest_get_scanline_wide (pixman_iter_t *iter, const uint32_t *mask)
 +{
 +    bits_image_t *  image  = iter-image-bits;
 +    int             x      = iter-x;
 +    int             y      = iter-y;
 +    int             width  = iter-width;
 +    uint32_t *     buffer = iter-buffer;
 +
 +    image-fetch_scanline_64 (
 +       (pixman_image_t *)image, x, y, width, buffer, mask);
 +    if (image-common.alpha_map)
 +    {
 +       x -= image-common.alpha_origin_x;
 +       y -= image-common.alpha_origin_y;
 +
 +       image-common.alpha_map-fetch_scanline_64 (
 +           (pixman_image_t *)image-common.alpha_map, x, y, width, buffer, 
 mask);
 +    }

     return iter-buffer;
  }

  static void
 -next_line_write_narrow (pixman_iter_t *iter)
 +write_back_narrow (pixman_iter_t *iter)
  {
     bits_image_t *  image  = iter-image-bits;
     int             x      = iter-x;
 @@ -1388,7 +1434,7 @@ next_line_write_narrow (pixman_iter_t *iter)
  }

  static void
 -next_line_write_wide (pixman_iter_t *iter)
 +write_back_wide (pixman_iter_t *iter)
  {
     bits_image_t *  image  = iter-image-bits;
     int             x      = iter-x;
 @@ -1410,25 +1456,19 @@ next_line_write_wide (pixman_iter_t *iter)
     iter-y++;
  }

 -static uint32_t *
 -get_scanline_direct_write (pixman_iter_t *iter, const uint32_t *mask)
 -{
 -    return iter-buffer;
 -}
 -
  static void
 -next_line_direct_write (pixman_iter_t *iter)
 +write_back_direct (pixman_iter_t *iter)
  {
     iter-buffer += iter-image-bits.rowstride;
  }

  void
 -_pixman_bits_image_iter_init (pixman_image_t *image,
 -                             pixman_iter_t *iter,
 -                             int x, int y, int width, int height,
 -                             uint8_t *buffer, iter_flags_t flags)
 +_pixman_bits_image_dest_iter_init 

Re: [Pixman] [PATCH 15/18] Add direct-write optimization back

[Andrea Canciani]

On Thu, Jan 6, 2011 at 3:27 AM, Søren Sandmann sandm...@cs.au.dk wrote:
 From: Søren Sandmann Pedersen s...@redhat.com

 Introduce a new ITER_LOCALIZED_ALPHA flag that indicates that the
 alpha value computed is used only for the alpha channel of the output;
 it doesn't affect the RGB channels.

 Then in pixman-bits-image.c, if a destination is either a8r8g8b8 or
 x8r8g8b8 with localized alpha, the iterator will return a pointer
 directly into the image.
 ---
  pixman/pixman-bits-image.c |   31 +--
  pixman/pixman-general.c    |   19 +--
  pixman/pixman-private.h    |   19 +--
  3 files changed, 63 insertions(+), 6 deletions(-)

 diff --git a/pixman/pixman-bits-image.c b/pixman/pixman-bits-image.c
 index 9cd8b5a..df8d56e 100644
 --- a/pixman/pixman-bits-image.c
 +++ b/pixman/pixman-bits-image.c
 @@ -1410,6 +1410,18 @@ next_line_write_wide (pixman_iter_t *iter)
     iter-y++;
  }

 +static uint32_t *
 +get_scanline_direct_write (pixman_iter_t *iter, const uint32_t *mask)
 +{
 +    return iter-buffer;
 +}
 +
 +static void
 +next_line_direct_write (pixman_iter_t *iter)
 +{
 +    iter-buffer += iter-image-bits.rowstride;
 +}
 +
  void
  _pixman_bits_image_iter_init (pixman_image_t *image,
                              pixman_iter_t *iter,
 @@ -1418,8 +1430,23 @@ _pixman_bits_image_iter_init (pixman_image_t *image,
  {
     if ((flags  (ITER_NARROW | ITER_WRITE)) == (ITER_NARROW | ITER_WRITE))
     {
 -       iter-get_scanline = get_scanline_narrow;
 -       iter-next_line = next_line_write_narrow;
 +       if (((image-common.flags 
 +             (FAST_PATH_NO_ALPHA_MAP | FAST_PATH_NO_ACCESSORS)) ==
 +            (FAST_PATH_NO_ALPHA_MAP | FAST_PATH_NO_ACCESSORS)) 
 +           (image-bits.format == PIXMAN_a8r8g8b8      ||
 +            (image-bits.format == PIXMAN_x8r8g8b8     
 +             (flags  ITER_LOCALIZED_ALPHA
 +       {
 +           iter-buffer = image-bits.bits + y * image-bits.rowstride + x;
 +
 +           iter-get_scanline = get_scanline_direct_write;
 +           iter-next_line = next_line_direct_write;
 +       }
 +       else
 +       {
 +           iter-get_scanline = get_scanline_narrow;
 +           iter-next_line = next_line_write_narrow;
 +       }
     }
     else if (flags  ITER_WRITE)
     {
 diff --git a/pixman/pixman-general.c b/pixman/pixman-general.c
 index 4f73331..84b52c2 100644
 --- a/pixman/pixman-general.c
 +++ b/pixman/pixman-general.c
 @@ -106,7 +106,7 @@ general_composite_rect  (pixman_implementation_t *imp,
     pixman_iter_t src_iter, mask_iter, dest_iter;
     pixman_combine_32_func_t compose;
     pixman_bool_t component_alpha;
 -    iter_flags_t narrow;
 +    iter_flags_t narrow, dest_flags;
     int Bpp;
     int i;

 @@ -143,9 +143,24 @@ general_composite_rect  (pixman_implementation_t *imp,
                                      mask_x, mask_y, width, height,
                                      mask_buffer, narrow);

 +    if (op == PIXMAN_OP_CLEAR          ||
 +       op == PIXMAN_OP_SRC             ||
 +       op == PIXMAN_OP_DST             ||
 +       op == PIXMAN_OP_OVER            ||
 +       op == PIXMAN_OP_IN_REVERSE      ||
 +       op == PIXMAN_OP_OUT_REVERSE     ||
 +       op == PIXMAN_OP_ADD)
 +    {
 +       dest_flags = narrow | ITER_WRITE | ITER_LOCALIZED_ALPHA;
 +    }
 +    else
 +    {
 +       dest_flags = narrow | ITER_WRITE;
 +    }
 +
     _pixman_implementation_iter_init (imp-toplevel, dest_iter, dest,
                                      dest_x, dest_y, width, height,
 -                                     dest_buffer, narrow | ITER_WRITE);
 +                                     dest_buffer, dest_flags);

     component_alpha =
         mask                            
 diff --git a/pixman/pixman-private.h b/pixman/pixman-private.h
 index 8d3a604..8532b98 100644
 --- a/pixman/pixman-private.h
 +++ b/pixman/pixman-private.h
 @@ -185,8 +185,23 @@ union pixman_image
  typedef struct pixman_iter_t pixman_iter_t;
  typedef enum
  {
 -    ITER_NARROW        = (1  0),
 -    ITER_WRITE =  (1  1)
 +    ITER_NARROW =              (1  0),
 +    ITER_WRITE =               (1  1),
 +
 +    /* Localized alpha is when the alpha channel is used only to compute 
 the
 +     * alpha value of the destination and doesn't influence the RGB part.
 +     *
 +     * For example, the OVER operator has localized alpha for the 
 destination,
 +     * because the destination alpha doesn't affect the results in the RGB
 +     * channels. Similarly, ADD has localized alpha for both source and
 +     * destination because the alpha values never 'leak' into the resulting
 +     * RGB values.
 +     *

I thought I understood the definition after the first two lines, then
the OVER/ADD
explanation confused me. After reading again everything, I was convinced again
that I had understood it correctly the first time.

I don't know if this is actually better (I still think that it needs a
better wording),
but I'd 

Re: [Pixman] [PATCH 3/3] Add SSSE3 fast path skeleton

[Andrea Canciani]

On Tue, Jan 4, 2011 at 10:22 AM, Siarhei Siamashka
siarhei.siamas...@gmail.com wrote:
 On Wednesday 08 December 2010 16:13:48 Liu Xinyun wrote:
 There is a performance test between commits
 commit 56777f3f675869806cd30bcd21a5b39d788507cb
 Author: Dmitri Vorobiev dmitri.vorob...@movial.com
 Date:   Wed Sep 22 12:34:57 2010 +0300

     Use sys/mman.h macros only when they are available

 commit 3d094997b1820719d15cec7dc633ed37e1912bfc
 Author: Siarhei Siamashka siarhei.siamas...@nokia.com
 Date:   Tue Nov 30 00:31:06 2010 +0200

     Fix for potential unaligned memory accesses

 Right, if we look at the changes between these two commits:

 new: ba69989374fe9cbe5151c5aac7b824da0806f94a
 Speedups
 
 image-rgba                  ocitysmap-0    7476.29 (7520.52 3.09%) -
 6817.63 (6822.39 3.04%):  1.10x speedup ▏
 image-rgba                    poppler-0    15028.69 (15029.97 0.71%) -
 13748.83 (13793.77 0.91%):  1.09x speedup ▏
 image-rgba         gnome-terminal-vim-0    23378.23 (23494.57 0.70%) -
 21916.17 (21926.92 0.52%):  1.07x speedup ▏
 image-rgba          xfce4-terminal-a1-0    16632.31 (16637.27 0.60%) -
 15630.85 (15650.89 0.63%):  1.06x speedup ▏
 image-rgba       firefox-planet-gnome-0    87751.74 (87809.43 0.18%) -
 82620.93 (82949.20 0.27%):  1.06x speedup

 image-rgba          firefox-talos-gfx-0    51169.66 (51542.49 0.36%) -
 48572.45 (48610.13 0.26%):  1.05x speedup

 image-rgba         swfdec-giant-steps-0    11619.84 (11646.91 0.46%) -
 11056.41 (11057.69 0.40%):  1.05x speedup

 Removal of software prefetch provides a good performance improvement. It was a
 nice catch, thanks for that patch. But as a side effect, it also makes SSE2
 optimizations more competitive and somewhat harder to beat by SSSE3.

 new: 1ca715ed1e6914e9bd9f050065e827d7a9e2efc9
 Slowdowns
 =
 image-rgba          firefox-talos-svg-0    177350.25 (181525.48 1.11%) -
 198170.08 (202124.06 0.96%):  1.12x slowdown ▏
 image-rgba             swfdec-youtube-0    15843.51 (15850.03 0.76%) -
 18104.51 (18224.94 0.40%):  1.14x slowdown ▏

 new: 1d4f2d71facd5f2bbce74fbe3407ccea6cf4bea1
 Slowdowns
 =
 image-rgba          firefox-talos-svg-0    197582.42 (202455.51 1.19%) -
 208636.52 (212057.02 0.90%):  1.06x slowdown

 And these two slowdowns are caused by the changes in radial gradients code.
 I guess it can't be helped because now radial gradients are supposed to be 
 more
 correct.

I'm quite confident that SIMD makes it possible to speed up the computation of
the interpolation parameter of gradients (both linear and radial).
If someone is interested in improving the performance of radial gradients, I can
point out some transformations which remove basically all of the branching and
make the computation very easy to perform in parallel.

I already implemented them (the idea should be correct but they are
untested) in:
http://cgit.freedesktop.org/~ranma42/cairo/commit/?h=wip/gl2id=5814ec1c2cec04c4e6cbb70ff2b24d8e157ff321

As a side note, if we want SIMD gradients, we should probably profile
the gradient
walker. I expect quite a lot of time to be spent there, at least for
the linear gradients.

Andrea

PS: I'm guilty of the gradient slowdown, but on the good side it seems to be the
best implementation of radial gradients available, surpassing in
correctness and/or
numerical stability Quartz, GhostScript, poppler, Adobe Reader, Acrobat Pro X...
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Improve handling of tangent circles

[Andrea Canciani]

On Tue, Jan 4, 2011 at 10:59 AM, Siarhei Siamashka
siarhei.siamas...@gmail.com wrote:
 On Monday 03 January 2011 10:15:09 Andrea Canciani wrote:
 When b is 0, avoid the division by zero and just return transparent
 black.

 When the solution t would have an invalid radius (negative or outside
 [0,1] for none-extended gradients), return transparent black.

 Thanks for keeping improving radial gradients code.

 I just wonder if it would be difficult to add a test to pixman for this
 particular division by zero case? Or is it somehow triggered by cairo
 tests?

It is triggered by the new radial-gradient tests in cairo:
http://cgit.freedesktop.org/cairo/commit/?id=ada6057b8ccab133909b127850c41abb3216a842

The refimages have been created with a patched pixman, so the tests
currently fail on cairo-image, but would pass with this change.


 One of the problems with the pixman radial code is that it is slow.
 And this path further slows it down a bit by adding a new branch in the
 inner loop. This is perfectly fine for a reference implementation, but if
 somebody decides to add some performance optimizations, then we need to have
 at least some basic tests which can catch possible errors and regressions.

I plan to rewrite the radial-gradient test to make it only use pixman
so that it can be added to pixman testsuite.

I know that branches are bad, but I believe that this slow implementation
should not try to be too clever. I'll rebase and clean up my cairo/wip/gl2
branch, which shows a branch-less radial gradient implementation
(using some tricks to use values as conditions).

Architectures that have a fast floating point SIMD (single precision
should be sufficient for most gradients) with square root (or which
can implement it in a reasonable way using Newton iteration) would
get much better performance by computing the gradient in parallel.

Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH] Improve handling of tangent circles

[Andrea Canciani]

When b is 0, avoid the division by zero and just return transparent
black.

When the solution t would have an invalid radius (negative or outside
[0,1] for none-extended gradients), return transparent black.
---
 pixman/pixman-radial-gradient.c |   20 ++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/pixman/pixman-radial-gradient.c b/pixman/pixman-radial-gradient.c
index b595ba7..7b92307 100644
--- a/pixman/pixman-radial-gradient.c
+++ b/pixman/pixman-radial-gradient.c
@@ -96,8 +96,24 @@ radial_compute_color (doublea,
 
 if (a == 0)
 {
-   return _pixman_gradient_walker_pixel (walker,
- pixman_fixed_1 / 2 * c / b);
+   double t;
+
+   if (b == 0)
+   return 0;
+
+   t = pixman_fixed_1 / 2 * c / b;
+   if (repeat == PIXMAN_REPEAT_NONE)
+   {
+   if (0 = t  t = pixman_fixed_1)
+   return _pixman_gradient_walker_pixel (walker, t);
+   }
+   else
+   {
+   if (t * dr  mindr)
+   return _pixman_gradient_walker_pixel (walker, t);
+   }
+
+   return 0;
 }
 
 det = fdot (b, a, 0, b, -c, 0);
-- 
1.7.1

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 2/3] Fix opacity check

[Andrea Canciani]

On Thu, Nov 4, 2010 at 2:05 AM, Soeren Sandmann sandm...@daimi.au.dk wrote:
 Andrea Canciani ranm...@gmail.com writes:

 @@ -436,6 +436,12 @@ compute_image_info (pixman_image_t *image)
       {
           int i;

 +         if (image-common.type == RADIAL 
 +             image-radial.a = 0)
 +         {
 +             break;
 +         }

 I think this code needs a comment explaining why it's necessary, and
 why checking for a = 0 works. Also, instead of having the extra if
 statement, I think I'd prefer to refactor the switch like this:

        case RADIAL:
            /* Comment explaining why this test is necessary */
            if (image-radial.a = 0)
                break;

            /* Fall through */

        case LINEAR:
            ...

 Other than, this and the other two patches look good to me.

The patch in the attachment should provide the requested improvements.
I tried to only give a high-level explanation of the condition, leaving the
maths in pixman-radial-gradient.c (but referencing it).
Is the comment ok or should I also add the definition of a (and maybe
the equivalence A0 = every point is colored)?


 It would of course also be very useful to extend the test suite to
 catch this problem.

Yes, I know :(. These days I'm really busy with my thesis work,
but I'll add this problem to the ones that the pixman radial test
should check.

Andrea


0002-Fix-opacity-check.patch
Description: Binary data
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] How pixman_implementation_t is getting free

[Andrea Canciani]

2010/11/2 kb pachauri kb.pacha...@gmail.com:
 Actually what i mean for freeing memory is .. freeing the opencl context,
 releasing command queue, all opencl kernels etc...
 sorry for not too clear in my words..
 I dont think this will be freed until i free the context..

Then you might look this thread:
http://lists.freedesktop.org/archives/pixman/2010-October/000642.html

Both Siarhei and I have been working towards thread-safe and valgrind-clean
pixman, but nothing to handle it is in master yet.

Siarhei directly hooks on compiler attributes that allow a function to be
called upon initialization or termination. He only did it for the constructor,
but the destructor can be implemented with the same approach.

My wip/simpleops branch provides a pixman_fini which is (supposed to be)
automatically called upon library termination, so any deallocation for
pixman implementations would go there.

Please notice that my wip/simpleops adds a dependency to the simpleops
library, which is currently under development and not released yet and
(being in very early stages of development) afaik only tested on my laptop
(so it probably won't work on win32 and might have problems with
non-gcc compilers).

Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 1/3] Remove unused stop_range field

[Andrea Canciani]

---
 pixman/pixman-image.c   |2 --
 pixman/pixman-private.h |1 -
 2 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
index fabcd63..fc0677d 100644
--- a/pixman/pixman-image.c
+++ b/pixman/pixman-image.c
@@ -47,8 +47,6 @@ _pixman_init_gradient (gradient_t *  gradient,
 
 gradient-n_stops = n_stops;
 
-gradient-stop_range = 0x;
-
 return TRUE;
 }
 
diff --git a/pixman/pixman-private.h b/pixman/pixman-private.h
index c43172b..27679b6 100644
--- a/pixman/pixman-private.h
+++ b/pixman/pixman-private.h
@@ -127,7 +127,6 @@ struct gradient
 source_image_t  common;
 int n_stops;
 pixman_gradient_stop_t *stops;
-int stop_range;
 };
 
 struct linear_gradient
-- 
1.7.1

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 2/3] Fix opacity check

[Andrea Canciani]

Radial gradients are conical, thus they can have some non-opaque
parts even if all of their stops are completely opaque.

To guarantee that a radial gradient is actually opaque, it needs to
also have one of the two circles containing the other one. In this
case when extrapolating, the whole plane is completely covered (as
explained in the comment in pixman-radial-gradient.c).
---
 pixman/pixman-image.c |6 ++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
index fc0677d..ec277c6 100644
--- a/pixman/pixman-image.c
+++ b/pixman/pixman-image.c
@@ -436,6 +436,12 @@ compute_image_info (pixman_image_t *image)
{
int i;
 
+   if (image-common.type == RADIAL 
+   image-radial.a = 0)
+   {
+   break;
+   }
+
flags |= FAST_PATH_IS_OPAQUE;
for (i = 0; i  image-gradient.n_stops; ++i)
{
-- 
1.7.1

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 3/3] Improve conical gradients opacity check

[Andrea Canciani]

Conical gradients are completely opaque if all of their stops are
opaque and the repeat mode is not 'none'.
---
 pixman/pixman-image.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c
index ec277c6..0511f2f 100644
--- a/pixman/pixman-image.c
+++ b/pixman/pixman-image.c
@@ -429,6 +429,7 @@ compute_image_info (pixman_image_t *image)
break;
 
 case LINEAR:
+case CONICAL:
 case RADIAL:
code = PIXMAN_unknown;
 
-- 
1.7.1

___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] Gradients patches

[Andrea Canciani]

On Tue, Oct 12, 2010 at 1:09 PM, Soeren Sandmann sandm...@daimi.au.dk wrote:
...

 It would be useful for someone to update the Render spec to say that
 the radials are PDF Type 3 Shadings, defined in section 8.7.4.5.4 of
 the PDF specification, and delete the stuff about the circles being
 contained in each other.

I'm pinging the ML about this again, because I would like to add
a couple of facts.

Cairo has been abusing Render radial gradients by requesting their
rasterization even when the outer circle did not contain the inner
circle. The specification explicitly disallows this, but the server
doesn't return an error and draws the gradient, instead.

The change suggested by Soeren seems to be very appropriate
to make specification and implementation more consistent.

Additionally the gradient definition did not change for the values
allowed by the specification, so extending the range of valid values
should be pretty safe.

Andrea Canciani
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] Radial gradients (again)

[Andrea Canciani]

I don't remember the timeline for pixman 0.20, but I think that the
attached patch should be reviewed and merged (if it looks good),
because it is a straightforward change that extends the valid domain.

http://cgit.freedesktop.org/~ranma42/pixman/commit/?h=wip/radialid=ce1738313e0a3e98bc004f1b27e5644929f2427c

I've been working on a fragment-shader implementation of the
new gradients. This might be a good starting point for fast paths
using SIMD instructions, because there is no branching.

http://cgit.freedesktop.org/~ranma42/cairo/log/?h=wip/gl2

In the same branch I also tried to extend one of the tests to
hit all the important cases (in particular tangent circles and
constant radius were not tested at all).
I want to port this test or something very similar to pixman
and check it against a different (and slow, but numerically
stable) implementation of the gradients, but it will take some
time, because I will be quite busy until the end of October.

Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Do CPU features detection from 'constructor' function when compiled with gcc

[Andrea Canciani]

On Mon, Oct 4, 2010 at 11:43 PM, Siarhei Siamashka
siarhei.siamas...@gmail.com wrote:
 On Monday 04 October 2010 13:19:45 Andrea Canciani wrote:
 On Sun, Oct 3, 2010 at 10:52 PM, Siarhei Siamashka

 siarhei.siamas...@gmail.com wrote:
  From: Siarhei Siamashka siarhei.siamas...@nokia.com
 
  There is attribute 'constructor' supported since gcc 2.7 which allows
  to have a constructor function for library initialization. This
  eliminates an extra branch for each composite operation and also helps
  to avoid complains from race condition detection tools like helgrind.

 I'm working on threadsafety issues, too, but I don't have a patch ready
 I pushed
 http://cgit.freedesktop.org/~ranma42/pixman/commit/?h=wip/simpleops which
 adds atomic refcounting and (private, automatically called)
 init/fini functions to pixman.

 Currently the simpleops library (available at
 http://cgit.freedesktop.org/~ranma42/simpleops )
 is not complete, but in a not very distant future it should provide
 some commonly
 available features that with different syntax on each os/compiler (in
 particular, atomics,
 mutexes, integers of various sizes, constructors/destructors, thread
 local storage).
 It should basically be a bunch of headers to hide the
 platform-specific naming and some
 autotools magic to detect what is available.

 I'm working on MacOS X and Ubuntu and compiling with gcc for x86_64,
 so most of the
 library is untested, but I expect only minor changes to be needed in
 most cases, since
 it is based on code from cairo and/or pixman.

 I'm also working on a wip/simpleops branch in cairo, so that the same
 code can be shared
 between the two libraries (and moving non graphic-related code out of
 them).

 Does this look like a viable approach for having these functionalities
 in pixman?
 Right now I'm mainly interested in atomic refcounting and valgrind
 cleanness (threadsafe
 initialization, destroying pixman_implementation's upon finalization),
 but cleaning up the
 TLS stuff would be nice, too.

 Yes, I remember. I just was a bit worried about the absence of recent news
 from
 this front. It's not like you are the only one interested in this stuff ;)

 Should I just drop this simple patch and wait for something better to come
 from you soon?
I'm trying to get some gradient-related changes into master before 0.20,
so I'm afraid that I won't be able to do much work on thread-safety soon.

The atomic part of simpleops is supposed to work, but untested on most
systems.

Even if there is some code for DllMain (win32) and #pragma init (suncc),
constructors/destructors are actually detected only on gcc-like compilers
(i.e. __attribute__).

I'd like to put up a win32 dev environment, but last time I tried I didn't
manage to, so I'm afraid that completion and testing of simpleops will
take more time than you should be willing to wait (before cairo 1.12?).
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Do CPU features detection from 'constructor' function when compiled with gcc

[Andrea Canciani]

On Sun, Oct 3, 2010 at 10:52 PM, Siarhei Siamashka
siarhei.siamas...@gmail.com wrote:
 From: Siarhei Siamashka siarhei.siamas...@nokia.com

 There is attribute 'constructor' supported since gcc 2.7 which allows
 to have a constructor function for library initialization. This eliminates
 an extra branch for each composite operation and also helps to avoid
 complains from race condition detection tools like helgrind.

I'm working on threadsafety issues, too, but I don't have a patch ready
I pushed http://cgit.freedesktop.org/~ranma42/pixman/commit/?h=wip/simpleops
which adds atomic refcounting and (private, automatically called)
init/fini functions to pixman.

Currently the simpleops library (available at
http://cgit.freedesktop.org/~ranma42/simpleops )
is not complete, but in a not very distant future it should provide
some commonly
available features that with different syntax on each os/compiler (in
particular, atomics,
mutexes, integers of various sizes, constructors/destructors, thread
local storage).
It should basically be a bunch of headers to hide the
platform-specific naming and some
autotools magic to detect what is available.

I'm working on MacOS X and Ubuntu and compiling with gcc for x86_64,
so most of the
library is untested, but I expect only minor changes to be needed in
most cases, since
it is based on code from cairo and/or pixman.

I'm also working on a wip/simpleops branch in cairo, so that the same
code can be shared
between the two libraries (and moving non graphic-related code out of them).

Does this look like a viable approach for having these functionalities
in pixman?
Right now I'm mainly interested in atomic refcounting and valgrind
cleanness (threadsafe
initialization, destroying pixman_implementation's upon finalization),
but cleaning up the
TLS stuff would be nice, too.

Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] PDF radial gradients

[Andrea Canciani]

On Fri, Sep 10, 2010 at 8:44 AM, Soeren Sandmann sandm...@daimi.au.dk wrote:
 Andrea Canciani ranm...@gmail.com writes:

 I updated the documentation of radial gradients in my wip/radial branch
 (http://cgit.freedesktop.org/~ranma42/pixman/log/?h=wip/radial) to reflect
 my changes (which make pixman gradients behave as specified by pdf
 reference manual).

 It's of course important to point out here that changing the radial
 gradients to behave as specified in the PDF spec, really is a change
 from they used to do.

 As I understand it, for most common cases, the output is identical to
 what it used be, is that correct? It would be helpful to have a
 description of the cases where the behavior differs.

The behavior is the same when one of the two circle completely contains
the other one, otherwise it is different.
What is the right place for this observation? The code documentation?
Or is it better to write it in the commit message?


 Other than that, I'm in favor of just using the PDF spec here. I don't
 think there is much real value in coming up with our own
 specification.

 Regarding this:

    When a point does not belong to any of the circles, it is
    transparent.

 It's probably worthwhile to say explicitly that the point has the RGBA
 value (0, 0, 0, 0) instead of transparent. That way it's clear that
 you can't leave out the point when using the SOURCE operator for
 example.

Yes, right.


 I'm not completely clear on what cairo is doing (or supposed to do) in
 this case. The cairo SOURCE operator is bounded by the mask, but is it
 also bounded by the source?
In cairo CLEAR and SOURCE are bounded by mask, but not by source.

 The code is not yet ready to be merged (it uses __int128 variables, which
 probably won't work on 32 bit architectures), but I would appreciate
 reviews
 (expecially of the new documentation) and testing.

 If __int128 is intended to help with non-FPU systems, then let me
 repeat that using floating point in pixman is totally fine, and almost
 certainly faster than any __int128 would be.

The purpose of using 128 bits is avoiding approximations (thus retaining
full precision), but I also expect it to be faster than floating point, because
in the inner loop the values are updated using multiple differentiation
(i.e. just 3 additions), beside the double-precision computations when
actually needed.

Why do you think that floating point would be faster?
(I expect that int to double conversion is the slow operation in this code,
but it only happens when a sqrt is computed, so it's probably not a big deal)
Did you also consider the changes required to retain full precision?

Thank you for your comments, I'll fix the documentation and benchmark
fixed vs float soon
Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] Valgrind-clean pixman

[Andrea Canciani]

On Sat, Aug 28, 2010 at 5:34 PM, Siarhei Siamashka
siarhei.siamas...@gmail.com wrote:
 On Saturday 28 August 2010 12:21:52 Andrea Canciani wrote:
 On Fri, Aug 27, 2010 at 3:58 PM, Siarhei Siamashka wrote:
  This code which is setting a global implementation pointer is also not
  quite thread safe (though very unlikely to cause any practical problems
  other than a bit bigger one time memory leak). I did not look at it in
  details, but with your changes for the use of static structures, it may
  become a bit worse because two or more concurrent threads now can
  potentially try to initialize the same data structures at once when
  setting up implementation pointer.

 Pixman is not thread safe right now (reference counting, in particular).
 When we fix (probably by using atomics) reference count, it should be very
 easy to make implementations thread safe, too.

 I may be wrong here, but seems like everyone has his own definition of what is
 considered to be thread safe. Currently pixman appears to be thread safe as
 long as same pixman objects (pixman_image_t and the others) are not used from
 multiple threads simultaneously. This is somewhat similar to the thread safety
 assumptions of the GNU implementation of a standard C++ template library. I'm
 adding this link here because they took care of documenting what can be
 guaranteed when working in a multi-threaded environment, and what can't be:
 http://gcc.gnu.org/onlinedocs/libstdc++/manual/using_concurrency.html#manual.intro.using.concurrency.thread_safety
 http://www.sgi.com/tech/stl/thread_safety.html
I see. Currently cairo makes stronger assumptions about pixman thread safety
(something like: a pixman_image can be used as source from multiple threads
as long as no threads is writing to it).
We should probably make cairo and pixman thread safety models converge, or at
least be compatible (using pixman/master cairo test suite crashes on
pthread tests).

 I'm working on a library of simple inline functions (based on cairo
 atomic and mutex implementations) to share some common utility functions
 which have different availability on different platforms (right now: atomics,
 wideint, mutexes). I hope to make it available soon (after I get autotools
 do the right things to make it work).

 Sounds nice, but also a bit scary at the same time. Let's hope that pixman is
 not going to become a performance disaster like gstreamer due to adding extra
 thread safety guarantees ;)
The performance hit of thread safety depends heavily on the implementation we
choose (and cairo isn't doing the right thing about MT, so some evolution will
probably happen in it, maybe relaxing again the constraints in pixman).
Anyway, what about the constructive comments in my previous email (library
constructors/destructors)?
Those would solve some thread safety issues and make the code cleaner, too.
Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] Valgrind-clean pixman

[Andrea Canciani]

On Fri, Aug 27, 2010 at 3:58 PM, Siarhei Siamashka
siarhei.siamas...@gmail.com wrote:
 On Thursday 26 August 2010 20:40:08 Andrea Canciani wrote:
 Currently pixman allocates dynamically its implementations, but does
 not free them.
 This is not a true memory leak, since implementations are only
 allocated once, but
 many debugging tools will report the asyimmetric malloc/free as a memory
 leak. I wrote a patch that avoids allocating pixman_implementation_t's (by
 declaring them
 to be static) in
 http://cgit.freedesktop.org/~ranma42/pixman/commit/?h=wip/valgrind2
 More general solutions exists (teardown/reset functions), but
 currently they would not
 provide additional advantages and would be more complicated.
 Objections, suggestions, comments, reviews,... are appreciated.

 This code which is setting a global implementation pointer is also not quite
 thread safe (though very unlikely to cause any practical problems other than a
 bit bigger one time memory leak). I did not look at it in details, but with
 your changes for the use of static structures, it may become a bit worse
 because two or more concurrent threads now can potentially try to initialize
 the same data structures at once when setting up implementation pointer.
Pixman is not thread safe right now (reference counting, in particular). When we
fix (probably by using atomics) reference count, it should be very easy to make
implementations thread safe, too.
I'm working on a library of simple inline functions (based on cairo
atomic and mutex
implementations) to share some common utility functions which have different
availability on different platforms (right now: atomics, wideint, mutexes).
I hope to make it available soon (after I get autotools do the right
things to make it
work).

 While we are at it, helgrind complains a lot about 'blitters-test' and
 'scaling-test' programs. These messages are related to implementation setup 
 and
 can be suppressed if a dummy compositing operation is done before starting the
 main loop which uses OpenMP there.

 Having something like pixman_init()/pixman_cleanup() functions could solve all
 the problems, but it's an API change. Other solutions (using atexit(), using
 mutexes or atomic oparations when initializing implementation, putting
 'global_implementation' into thread local storage, etc.) seem to all have some
 risk of introducing portability issues and/or add extra runtime overhead.
Would using library ctor/dtor be ok? They should be available both on win32
(DllMain) and as a gcc attribute and would allow to initialize things in a very
simple way (without having to use any synchronization).

See:
http://msdn.microsoft.com/en-us/library/ms682583(VS.85).aspx
http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html

We probably want to implement DllMain on win32 to allocate/free TLS anyway.
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] Valgrind-clean pixman

[Andrea Canciani]

Currently pixman allocates dynamically its implementations, but does
not free them.
This is not a true memory leak, since implementations are only
allocated once, but
many debugging tools will report the asyimmetric malloc/free as a memory leak.
I wrote a patch that avoids allocating pixman_implementation_t's (by
declaring them
to be static) in
http://cgit.freedesktop.org/~ranma42/pixman/commit/?h=wip/valgrind2
More general solutions exists (teardown/reset functions), but
currently they would not
provide additional advantages and would be more complicated.
Objections, suggestions, comments, reviews,... are appreciated.
Andrea Canciani
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [cairo] Floating point API in Pixman

[Andrea Canciani]

On Thu, Aug 12, 2010 at 10:02 AM, Mathieu Lacage
mathieu.lac...@sophia.inria.fr wrote:
 On Thu, 2010-08-12 at 09:54 +0200, Andrea Canciani wrote:
 (I would *really* love if int128_t was actually available!)

 It's available on most 64bit systems (at least x86_64) with gcc as
 __int128_t and __uint128_t. I use this:

 #if defined(HAVE___UINT128_T) and !defined(HAVE_UINT128_T)
 typedef __uint128_t uint128_t;
 typedef __int128_t int128_t;
 #endif
If we can't assume that we have a (fast) FPU, we probably can't assume
that we are on a 64 bits architecture (even less on x86_64).
:(
Andrea
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman