Re: [Pixman] Benchmarked: [PATCH 1/4] Change conditions for setting FAST_PATH_SAMPLES_COVER_CLIP flags

[Bill Spitzak]

On Wed, Sep 16, 2015 at 7:30 AM, Ben Avison  wrote:

Just by thinking things through, I realised that we would regularly fail
> to hit COVER paths if Pixman's caller set the scale factors such that the
> centre of the outermost destination pixels aligned with the centre of the
> outermost source pixels. There has been some argument about whether this
> is representative of how Pixman should be used. I happen to think this is
> a perfectly reasonable thing to expect Pixman to support, but there are
> other models you can follow, notably setting the scale factors such that
> the outer edges of the outermost destination pixels align to the outer
> edges of the outermost source pixels. If the Cairo traces are using this
> latter model, then it's understandable if you aren't hitting the edge
> case that I'm concerned about very often.
>

There are currently very good reasons for clients to use the first model:
it is a method of removing undesirable "fuzzy edges" from zoomed-in images.

The alternative of scaling the outer edge, while not hitting this fast
path, will not hit the other fast path either! (the other fast path is the
one that allows a zero-weighted pixel at one end to be read from the image).

Therefore I believe there is actual usage of this new fast path, while
there is almost zero usage of the old fast path. This is the main reason I
think the current flag should be removed, and a new one indicating that all
the non-zero samples are inside the image, added. This second one will
actually be used by real code.

NOTE: the method of scaling the centers of the pixels is generally wrong.
The math is wonky: does scaling an image by 2 produce a 2*w-1 image, or
does it spread the pixels slightly more than 2 apart? Programs are going to
disagree. And you have to alter the math as soon as the scale goes below 1
or you will get fuzzy edges again. And lots of code are going to get
coordinates in the resulting image wrong, resulting in annoying problems
like overlaid lines not lining up or shifting their alignment from one side
to another.

I very much believe Cairo should change how CAIRO_EXTEND_NONE is done. It
should instead mean that the path is intersected with a quadrilateral that
is the transform of the outer edge of the source, then drawing as though
using CAIRO_EXTEND_REPEAT. This is what most users of scaling expect, and
anybody wanting the previous effect can get it by adding a black pixel to
the outer edge of their source surface.

Now it is true that this result can be achieved with Cairo right now. The
reason for making this the default is because non-experts are unlikely to
figure this out. Currently filling a path equal to the source boundary
results in a double-mulitplication of the edge pixels, a subtle error that
can be very frustrating when graphics gets more complex. Making this error
not happen by default would be very helpful. I also believe making this the
default will result in faster implementations, as EXTEND_REPEAT fast paths
can be used, and this is often directly supported by graphics hardware.
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 0/4] More VMX enhancements

[Oded Gabbay]

On Fri, Sep 18, 2015 at 6:08 AM, Siarhei Siamashka
 wrote:
> On Thu, 17 Sep 2015 18:04:13 +0300
> Oded Gabbay  wrote:
>
>> On Thu, Sep 10, 2015 at 1:47 PM, Pekka Paalanen  wrote:
>> > On Thu, 10 Sep 2015 11:55:56 +0300
>> > Oded Gabbay  wrote:
>> >
>> >> On Mon, Sep 7, 2015 at 2:09 PM, Oded Gabbay  wrote:
>> >>
>> >> > On Mon, Sep 7, 2015 at 2:04 PM, Pekka Paalanen 
>> >> > wrote:
>> >> > > On Sun,  6 Sep 2015 18:27:07 +0300
>> >> > > Oded Gabbay  wrote:
>> >> > >
>> >> > >> This patch-set contains optimizations for two existing VMX fast-paths
>> >> > and a new
>> >> > >> VMX fast-path function.
>> >> > >>
>> >> > >> The optimization ideas came from Siarhei's recent implementation of
>> >> > over_n_
>> >> > >> VMX fast path (see
>> >> > http://lists.freedesktop.org/archives/pixman/2015-September/003951.html).
>> >> > >>
>> >> > >> The new function I added is actually one that I already implemented a
>> >> > couple
>> >> > >> of months ago, but it produced conflicting results regarding the
>> >> > performance.
>> >> > >> However, I now optimized it and it now shows considerable performance
>> >> > >> improvement over the non-vmx path.
>> >> > >>
>> >> > >> The last patch removes many helper functions that caused the less 
>> >> > >> than
>> >> > stellar
>> >> > >> performance the current fast-paths provide. I removed them as I don't
>> >> > want
>> >> > >> anyone to try and use them, because there are much better 
>> >> > >> alternatives,
>> >> > as
>> >> > >> I've demonstrated with this patch-set.
>> >> > >>
>> >> > >> Thanks,
>> >> > >>
>> >> > >>   Oded
>> >> > >>
>> >> > >> Oded Gabbay (4):
>> >> > >>   vmx: optimize scaled_nearest_scanline_vmx___OVER
>> >> > >>   vmx: optimize vmx_composite_over_n___ca
>> >> > >>   vmx: implement fast path vmx_composite_over_n_8_
>> >> > >>   vmx: Remove unused expensive functions
>> >> > >>
>> >> > >>  pixman/pixman-vmx.c | 439
>> >> > ++--
>> >> > >>  1 file changed, 150 insertions(+), 289 deletions(-)
>> >> > >>
>> >> > >
>> >> > > Hi Oded,
>> >> > >
>> >> > > nice diffstat. :-)
>> >> > >
>> >> > > This series is:
>> >> > > Acked-by: Pekka Paalanen 
>> >> > >
>> >> > > I did notice a few minor issues. Patch 1 has a dereference before
>> >> > > NULL-check, and you sometimes forget the space before an opening
>> >> > > parenthesis.
>> >> > >
>> >> > > I suppose there is no danger of regressing operations you didn't
>> >> > > touch? ;-)
>> >> > >
>> >> > >
>> >> > > Thanks,
>> >> > > pq
>> >> >
>> >> > HI Pekka,
>> >> > I run cario benchmark (trimmed) and there was *no* regression.
>> >> > I don't think optimizing some fast-paths affects other, non-related,
>> >> > fast-paths. And, of course, I don't think it has *any* impact on non
>> >> > POWER systems.
>> >> > However, if someone thinks of a specific other function I need to
>> >> > check for regression, I'm open for suggestions :)
>> >> >
>> >> >  Oded
>> >> >
>> >>
>> >> It bugged me that there was no change, neither up nor down in cairo
>> >> benchmark.
>> >> So I rechecked it and I had a wrong setup - cairo used the 
>> >> system-installed
>> >> pixman instead of my pixman.
>> >>
>> >> After fixing that, I saw several modest speedups for this patch series:
>> >>
>> >> Speedups
>> >> 
>> >> imaget-firefox-scrolling  1232.30 (1237.81 0.40%) -> 1080.17
>> >> (1097.06 0.99%):  1.14x speedup
>> >> imaget-gnome-terminal-vim  613.86 (615.04 0.12%) -> 549.73 (551.32
>> >> 0.13%):  1.12x speedup
>> >> imaget-evolution  405.54 (412.06 0.81%) -> 370.57 (379.11 1.89%):
>> >>  1.09x speedup
>> >> imaget-gvim  653.02 (655.16 0.16%) -> 615.31 (618.40 1.68%):  
>> >> 1.06x
>> >> speedup
>> >> imaget-firefox-talos-gfx  919.31 (926.31 0.36%) -> 867.05 (870.01
>> >> 0.35%):  1.06x speedup
>> >>
>> >> I'll add it into the last commit of this patch-set for future references.
>> >
>> > Paranoia pays off!
>> >
>> >
>> > Cheers,
>> > pq
>>
>> Siarhei,
>> Are you planning on reviewing my patch-set ?
>> pq gave his acked-by,
>
> Assuming that the dereference before NULL-check problem (spotted by
> Pekka) gets fixed before pushing these patches:
>
> Acked-by: Siarhei Siamashka 
>
>> and I don't think anyone else than you cares about POWER.
>
> I have powerpc hardware, but I can't say that I really care about it
> too much. Other than just occasionally running some tests on it.
>
>> If not, I would like to push this patch-set to master.
>
> Do you want to have these changes in the upcoming pixman 0.34.0 release
> or in the next one? What are your plans regarding the creation of the
> '0.34' branch?
>
> --
> Best regards,
> Siarhei Siamashka

Patches were pushed to master:
73e586e..8189fad  

[Pixman] Pixman release schedule

[Pekka Paalanen]

On Fri, 18 Sep 2015 08:36:09 +0300
Oded Gabbay  wrote:

> On Fri, Sep 18, 2015 at 6:08 AM, Siarhei Siamashka
>  wrote:

> > Do you want to have these changes in the upcoming pixman 0.34.0 release
> > or in the next one? What are your plans regarding the creation of the
> > '0.34' branch?
> 
> Well, I actually thought of doing one more development-stable release,
> i.e. 0.33.4, before going to 0.34.0
> The reason is that it's been over a year since the last stable
> release, and I would like to have more testing time with the
> distributions. Although 0.33.2 is already in Fedora F23 for the past 2
> months, only a week ago 0.33.2 was released to debian testing. That's
> not enough time, IMO.
> 
> So, I was thinking of doing 0.33.4 sometime at the start of October,
> with all the new patches added by then. Then, after about 2-3 months
> do 0.34.0. Bottom line, the intention is to have 0.34.0 included in
> the Spring releases (Fedora 24, Ubuntu 16.04 lts), which means it
> should be released sometime around December/January.

Hi,

let's have a more appropriate subject, so people will notice. :-)

I'm personally fine with any plan you pick. Just let me know when/if
our criteria for pushing patches to master changes.


Thanks,
pq


pgp84ZYsjECTJ.pgp
Description: OpenPGP digital signature
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH v2 0/2] over_n_8888 fast path for RaspberryPi

[Pekka Paalanen]

On Thu, 17 Sep 2015 18:01:05 +0300
Oded Gabbay  wrote:

> On Thu, Sep 17, 2015 at 5:38 PM, Siarhei Siamashka
>  wrote:
> > On Thu, 17 Sep 2015 17:04:35 +0300
> > Pekka Paalanen  wrote:
> >
> >> On Mon,  7 Sep 2015 14:40:47 +0300
> >> Pekka Paalanen  wrote:
> >>
> >> > From: Pekka Paalanen 
> >> >
> >> > Hi,
> >> >
> >> > this is the second iteration, with the C fast path patch dropped. The 
> >> > remaining
> >> > patches are identical to the previous submission, except they have been
> >> > re-benchmarked, just in case.
> >> >
> >> > Explanation for this patch submission style was given in the previous 
> >> > series:
> >> > http://lists.freedesktop.org/archives/pixman/2015-August/003855.html

> >> Pushed:
> >>4c71f59..73e586e  master -> master
> >
> > Well, nothing bad has really happened yet. But probably it would be
> > great to give at least a 2 days notice before pushing patches, unless
> > they had been actually reviewed by somebody.
> >
> 
> I thought that Ben wrote the patch and Pekka reviewed it, no ?

Yup. Ben wrote it. I reviewed it (except I can't read asm). We both
benchmarked it, and both our benchmarks agreed. I split it in two
patches that together are equal to Ben's original. I ran it through
'make check' on rpi1 just before pushing. I asked Oded if it's ok to
push these, he said yes. I didn't think it was necessary to ask Ben one
more time "are you sure this code you wrote is good?". I made the
judgement call that after all this it's ripe with just my testing.

I *did* think of posting just a note "going to land soon", but then
thought it's been on the list for 10 days with no comments and it's
already been discussed and accepted. Certainly we have a huge log of
related emails and no-one has objected. After all, the original patch is 
http://patchwork.freedesktop.org/patch/49901/ from 2014.

I'll try to remember to post the going-to-land-soon notes always in the
future.


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

Re: [Pixman] [PATCH v2 1/2] armv6: Add over_n_8888 fast path (disabled)

[Pekka Paalanen]

On Thu, 17 Sep 2015 17:20:22 +0300
Siarhei Siamashka  wrote:

> On Mon,  7 Sep 2015 14:40:48 +0300
> Pekka Paalanen  wrote:
> 
> > From: Ben Avison 
> > 
> > This new fast path is initially disabled by putting the entries in the
> > lookup table after the sentinel. The compiler cannot tell the new code
> > is not used, so it cannot eliminate the code. Also the lookup table size
> > will include the new fast path. When the follow-up patch then enables
> > the new fast path, the binary layout (alignments, size, etc.) will stay
> > the same compared to the disabled case.
> > 
> > Keeping the binary layout identical is important for benchmarking on
> > Raspberry Pi 1. The addresses at which functions are loaded will have a
> > significant impact on benchmark results, causing unexpected performance
> > changes. Keeping all function addresses the same across the patch
> > enabling a new fast path improves the reliability of benchmarks.
> 
> Don't we already have the PIXMAN_DISABLE environment variable exactly
> for this purpose (testing different implementations without recompiling
> the library)?

Now that PIXMAN_DISABLE=wholeops has landed, yeah, I think so. A good
point.

The old disable options would have disabled both fast paths and
iterators.

However, we still cannot disable fast paths one by one, which means
benchmarking for unexpected changes is not possible without this patch
arrangement. PIXMAN_DISABLE=wholeops also doesn't help with Cairo
benchmarks, so it's use it pretty limited.


Thanks,
pq


pgp6KpIpv31AN.pgp
Description: OpenPGP digital signature
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman