Re: [Pixman] Cleaning patchwork - Nemanja

[Oded Gabbay]

On Tue, Dec 22, 2015 at 3:16 PM, Oded Gabbay  wrote:
> Hi Nemanja,
>
> I would like to clean pixman's patchwork and you have about 10
> outstanding patches.
>
> Could you please take a look and tell me which ones are still relevant
> and which ones are not ?
>
> You can take a look at 
> http://patchwork.freedesktop.org/project/pixman/patches/
> I also copied here there names:
>
> [13/13] MIPS: enable prefetch for store only for CPU with 32 byte cache line
> [12/13] MIPS: disabled non 32-bit platforms
> [11/13] MIPS: mips32r2: Added optimization for function pixman_fill_buff16
> [10/13] MIPS: dspr1: Move fast paths implementation from dspr2 to dspr1
> [09/13] MIPS: dspr1: empty implementation with runtime detection
> [08/13] MIPS: mips32r2: Move fast paths implementation from dspr2 to mips32r2
> [07/13] MIPS: mips32r2: empty implementation with runtime detection
> [06/13] MIPS: dspr2: runtime detection extended
> [05/13] Implementing memcpy through pointer
> [03/13] MIPS: dspr2: Removed build restrictions and repair compiler's check
>
> I also suggest that for the relevant ones (if there are any), you
> would rebase them on the latest master and re-send them as a single
> series in order to restart the review process.
>
> Thanks,
>
> Oded

Hi Nemanja,

Just wanted to send a reminder about the email I sent you on 12/22.

Thanks,

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

Re: [Pixman] [PATCH 07/15] pixman-filter: Speed up the BOX+BOX filter

[Oded Gabbay]

On Fri, Jan 8, 2016 at 8:53 PM, Bill Spitzak  wrote:
>
>
> On Thu, Jan 7, 2016 at 11:44 PM, Pekka Paalanen  wrote:
>>
>>
>> Please keep in mind that the filters GOOD and BEST have been as is for
>> a long long time, AFAIU, so changing their behaviour now is likely not
>> a good idea. They are no longer "a good filter" and "whatever best
>> filter", but "the specific filter called GOOD" and "the specific filter
>> called BEST", in lack of documentation saying otherwise.
>
>
> Both GOOD and BEST are identical to BILINEAR in the current version of
> Pixman. Therefore anybody relying on the current behaviour can achieve it by
> using BILINEAR. In addition GOOD is unchanged for any scales larger than .75
> or for a scale of exactly .5.
>
> Also despite their names, bilinear in no way would be considered "good" or
> "best" by any sane person. We should not add illogical names (like NEW_GOOD
> or whatever) just because of paranoia over back-compatibility. It is also
> highly desirable that the default actually be "good", this cannot be done
> unless GOOD is changed, or the default is changed to "NEW_GOOD".
>
> This change was made to Cairo over a year ago with no complaints (except for
> speed issues, which this patch is necessary to solve by moving the fix from
> Cairo to Pixman). Lets get out of the dark ages, and stop doing things that
> are making open source desktops a laughingstock.
>

Hi Bill,
I just now read the new emails (I was on PTO for the last week).
It seems you found some mistakes and you want to resend a new version.
Did I understand you correctly ?
If that is indeed the case, then I prefer to wait for that version
(v9), and skip reviewing v8.
Please ack this.

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

Re: [Pixman] Cleaning patchwork

[Oded Gabbay]

On Mon, Jan 4, 2016 at 4:31 PM, Ben Avison  wrote:
> On Tue, 22 Dec 2015 13:14:21 -, Oded Gabbay 
> wrote:
>
>> Hi Ben,
>>
>> I would like to clean pixman's patchwork and you have about 20
>> outstanding patches.
>
> [...]
>
>> [4/4] pixman-fast-path: Make bilinear cover fetcher use COVER_CLIP_TIGHT
>> flag
>> [3/4] armv7/mips/sse2: Make bilinear cover fast paths use COVER_CLIP_TIGHT
>> flag
>> [2/4] Introduce FAST_PATH_SAMPLES_COVER_CLIP_TIGHT_BILINEAR flag
>
>
> I still think these are a worthwhile improvement and have described some
> conditions under which it should be measurable using affine-bench. I believe
> Pekka wanted to prove it using real-world traces though, and since there was
> no measurable change for better or worse using the normal Cairo traces, he
> was attempting to capture some new ones that would exercise the cases I
> described. Last I heard though, he had found that Cairo's tracer was broken,
> so he was unable to make progress. Under the circumstances, do you think
> affine-bench results alone would be acceptable?

Hi Pekka,
I admit I didn't follow these patches when Ben sent them as you and
Siarhei reviewed them.
May I ask you to write your opinion on the matter ?

>
>> Resolve implementation-defined behaviour for division rounded to -infinity
>
>
> This one got bogged down in an argument about whether C89 should still be
> supported or not, which I'm not sure was ever resolved?

So from reading back the thread, I saw two things:
1. Pekka and Siarhei  wrote about adding test(s) to verify behavior of % and /
2. C89 compatibility issue. Because we have already forked 0.34 and
0.36 seems like a 2017 story, I think we can start phasing it out in
the 0.35 development releases and see if someone complains.

>
>> [05/37,v2] composite flags: Early detection of fully opaque 1x1
>> repeating source images
>> [10/37,v2] pixman-fast-path: Add in__8 fast path
>> [11/37,v2] armv6: Add in__8 fast path
>> [23/37,v2] armv6: Add optimised scanline fetchers and writeback for
>> r5g6b5 and a8
>> [24/37,v2] armv6: Add optimised scanline fetcher for a1r5g5b5
>> [25/37,v2] armv6: Add src_1555_ fast path
>
>
> v1 of these patches were reviewed (excluding the ARM assembly parts) by
> Søren on 05 Oct 2014, and v2 addressed the issue he raised. There haven't
> been any comments on the reposted versions.

Could you re-post only the the ones you fixed with the v2 remarks ?
I'll take a look and hopefully we could merge them.

>
>> armv7: Re-use existing fast paths in more cases
>> armv7: Re-use existing fast paths in more cases
>
>
> These apply the same rules that Søren suggested for my ARMv6 paths in the v2
> patches above to the pre-existing ARMv7 paths as well. The only review
> comment received so far was that the two patches needed different names.

Same comment as above.

>
>> [2/5,v2] armv7: Add in__8 fast path
>
>
> The only difference for v2 here was again just a matter of defining the
> widest possible set of pixel formats to match the fast path.

Same comment as above.

>
>> [5/5] armv7: Add src_1555_ fast path
>> [4/5] armv7: Add in_reverse__ fast path
>> [3/5] armv7: Add in_n_ fast path
>> [1/5] armv7: Use prefetch for small-width images too
>> [3/3] armv7: Use VLD-to-all-lanes
>> [2/3] armv7: Faster fill operations
>> [1/3] armv7: Coalesce scalar accesses where possible
>> Require destination images to be bitmaps
>
>
> None of these have received any comments to date. In many cases, I suspect
> it's the fact that they involve ARM assembly, and we don't have many (any?)
> active reviewers who know it. I'm not sure what we can do about that.

Send them as a different series and I'll take a look (as much as I
can, I also don't know ARM assembly).

>
>> I also suggest that for the relevant ones (if there are any), you
>> would rebase them on the latest master and re-send them as a single
>> series in order to restart the review process.
>
>
> I can certainly do that, but I had previously been asked to split my
> postings into smaller series that were independent. I have a lot more
> patches waiting to post that depend on the ones that are stuck in limbo -
> the complete set can be seen at
>
> https://github.com/bavison/pixman/commits/arm-neon-release1
>
> if you're interested. Or I could just post/repost the whole lot (72 patches
> now), like I did with my 37-patch series from 2014.
>
> Ben

Let's start with the patches you wrote above and move step by step after it.

Thanks,

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

Re: [Pixman] [PATCH 07/15] pixman-filter: Speed up the BOX+BOX filter

[Bill Spitzak]

Yes I will try to send new versions asap.

Known mistakes/problems:

1. I was a bit too fast at deleting the recursion code for the linear
filters. The problems were not very visible with the 16-segment simpsons
integration, but obvious when reducing to 4 (which otherwise works in most
cases). Also it is actually easier to split with my new arguments to the
function, so I really should not have done that.

2. x.LINEAR where x is not IMPULSE or BOX or LINEAR is producing artefacts
that don't seem to exist in other combinations (or are much less visible in
other combinations?). I think I need to investigate this.

3. My comment is somewhat inaccurate and I need to update it. LINEAR.LINEAR
is only a cubic at scale==1, it is not possible to replicate what other
software calls "cubic" or "bicubic" using this

On Mon, Jan 11, 2016 at 1:16 AM, Oded Gabbay  wrote:

> On Fri, Jan 8, 2016 at 8:53 PM, Bill Spitzak  wrote:
> >
> >
> > On Thu, Jan 7, 2016 at 11:44 PM, Pekka Paalanen 
> wrote:
> >>
> >>
> >> Please keep in mind that the filters GOOD and BEST have been as is for
> >> a long long time, AFAIU, so changing their behaviour now is likely not
> >> a good idea. They are no longer "a good filter" and "whatever best
> >> filter", but "the specific filter called GOOD" and "the specific filter
> >> called BEST", in lack of documentation saying otherwise.
> >
> >
> > Both GOOD and BEST are identical to BILINEAR in the current version of
> > Pixman. Therefore anybody relying on the current behaviour can achieve
> it by
> > using BILINEAR. In addition GOOD is unchanged for any scales larger than
> .75
> > or for a scale of exactly .5.
> >
> > Also despite their names, bilinear in no way would be considered "good"
> or
> > "best" by any sane person. We should not add illogical names (like
> NEW_GOOD
> > or whatever) just because of paranoia over back-compatibility. It is also
> > highly desirable that the default actually be "good", this cannot be done
> > unless GOOD is changed, or the default is changed to "NEW_GOOD".
> >
> > This change was made to Cairo over a year ago with no complaints (except
> for
> > speed issues, which this patch is necessary to solve by moving the fix
> from
> > Cairo to Pixman). Lets get out of the dark ages, and stop doing things
> that
> > are making open source desktops a laughingstock.
> >
>
> Hi Bill,
> I just now read the new emails (I was on PTO for the last week).
> It seems you found some mistakes and you want to resend a new version.
> Did I understand you correctly ?
> If that is indeed the case, then I prefer to wait for that version
> (v9), and skip reviewing v8.
> Please ack this.
>
> Oded
>
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman