Re: Patch to improve blending accuracy of RGBA/RGB/Gray pixel formats

[John Horigan]

Sorry for the delay in this patch. I thought that the new blenders were failing in the rgba16 formats. But it turned out that the error was in my test framework.

The changes that I have made are:
* Utility methods have been added to classes rgb8, rgb16, gray8, and gray16 for doing fast, accurate fixed point multiplies and linear interpolations.

* The add, gradient, and premultiply methods of classes rgb8, rgb16, gray8, and gray16 use the new utility methods.

* A few new constructors have been added to agg_color_gray.h/agg_color_rgba.h: rgba8(const gray8& c), rgba16(const gray16& c), gray16(const rgba16& c), gray16(const rgba16& c, unsigned a_)

* A new file: agg_color_rgba.cpp was added to store the new rgba8/16 constructors. This was necessary to avoid a dependency loop. I used the same style of header comments that I found in other .cpp files.

* multiplier_rgba<>::premultiply() method in agg_pixfmt_rgba.h uses the new utility methods.

* New blender classes have been added to agg_pixfmt_gray/rbg/rgba.h using the new utility methods. They have the same names as the old blenders with '_ars' (for Alvy Ray Smith) appended. It just occurred to me that this naming convention may cause British AGG users to giggle. I am open to changes in the naming convention.

* All of the pixel format typedefs in agg_pixfmt_gray/rbg/rgba.h use the new '_ars' blenders. Typedefs using the old blenders remain, but have '_legacy' appended to them.

* The interface between the blender classes and the pixfmt classes has been changed slightly. Currently the pixfmt class multiplies the color alpha with pixel coverage factor before calling the blender (even though both alpha and cover are passed to the blender). With this patch, this calculation is moved to the blender class blend_pix() methods.

* I added a utility method to class rect_base in agg_basics.h: overlaps()





-- john horigan

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Vector-agg-general mailing list
Vector-agg-general@...
https://lists.sourceforge.net/lists/listinfo/vector-agg-general

[John Horigan]

My husband is (a software architect) is critical of these parts to my patch. He says that it is a mistake to leave the incorrect blending code in AGG and that the old blenders and the legacy typedefs should be removed. Or if they are to remain that they should be renamed to make it clear that they are deprecated.

Vinnie, you are the advocate for keeping the old blender code. Can you describe the changes that you have made to the blenders in your private copy of AGG.

-- john

On Sep 27, 2009, at 3:12 PM, John Horigan wrote:

* New blender classes have been added to agg_pixfmt_gray/rbg/rgba.h using the new utility methods. They have the same names as the old blenders with '_ars' (for Alvy Ray Smith) appended. It just occurred to me that this naming convention may cause British AGG users to giggle. I am open to changes in the naming convention.

* All of the pixel format typedefs in agg_pixfmt_gray/rbg/rgba.h use the new '_ars' blenders. Typedefs using the old blenders remain, but have '_legacy' appended to them.

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Vector-agg-general mailing list
Vector-agg-general@...
https://lists.sourceforge.net/lists/listinfo/vector-agg-general

[Stephan Assmus]

Hi John,

On 2009-09-28 at 00:12:42 [+0200], John Horigan <john@...> wrote:
> Sorry for the delay in this patch. I thought that the new blenders were
> failing in the rgba16 formats. But it turned out that the error was in my
> test framework.

I've read through the patch and it looks very good! Thanks a lot for this
work. Only one place I am not sure about:

+        
//--------------------------------------------------------------------
+        gray16(const rgba16& c, unsigned a_) :
+        v((value_type)((c.r*77 + c.g*150 + c.b*29) >> 16)),
+        a((value_type)a_) {}

I think the shift is wrong, since 77 + 150 + 29 = 256.

Has anybody else here already commited this patch? I do not follow the AGG
commits, is there a commit notification list I can sign up to?

Best regards,
-Stephan


------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Vector-agg-general mailing list
Vector-agg-general@...
https://lists.sourceforge.net/lists/listinfo/vector-agg-general

[klaas.holwerda-3]

Stephan Assmus wrote:

> Hi John,
>
> On 2009-09-28 at 00:12:42 [+0200], John Horigan <john@...> wrote:
>  
>> Sorry for the delay in this patch. I thought that the new blenders were
>> failing in the rgba16 formats. But it turned out that the error was in my
>> test framework.
>>    
>
> I've read through the patch and it looks very good! Thanks a lot for this
> work. Only one place I am not sure about:
>
> +        
> //--------------------------------------------------------------------
> +        gray16(const rgba16& c, unsigned a_) :
> +        v((value_type)((c.r*77 + c.g*150 + c.b*29) >> 16)),
> +        a((value_type)a_) {}
>
> I think the shift is wrong, since 77 + 150 + 29 = 256.
>
> Has anybody else here already commited this patch?

I think not, can you do it?
Although i thinks legacy and incorrect blending code in AGG should be
removed. I agree with John on that and/or the husband.
It is bad to keep all old code around.
What do you think?
> I do not follow the AGG
> commits, is there a commit notification list I can sign up to?
>  
Not yet, and only the admin can do it.
I suggest our admin gives other devlopers  admin rights too.
Or at least make things happen more quickly. e.g. Vinnie is not on the
members list jet.
And this commit list should be in place.

Leonardo were are you??

regards,

Klaas






------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Vector-agg-general mailing list
Vector-agg-general@...
https://lists.sourceforge.net/lists/listinfo/vector-agg-general

[John Horigan]

Yes, that is a bug in my new gray16() constructor. I will send out a  
new patch with that fixed and the old code removed.

-- john

On Oct 2, 2009, at 10:13 AM, klaas.holwerda wrote:

> Stephan Assmus wrote:
>> Hi John,
>>
>> On 2009-09-28 at 00:12:42 [+0200], John Horigan <john@...>  
>> wrote:
>>
>>> Sorry for the delay in this patch. I thought that the new blenders  
>>> were
>>> failing in the rgba16 formats. But it turned out that the error  
>>> was in my
>>> test framework.
>>>
>>
>> I've read through the patch and it looks very good! Thanks a lot  
>> for this
>> work. Only one place I am not sure about:
>>
>> +
>> //--------------------------------------------------------------------
>> +        gray16(const rgba16& c, unsigned a_) :
>> +        v((value_type)((c.r*77 + c.g*150 + c.b*29) >> 16)),
>> +        a((value_type)a_) {}
>>
>> I think the shift is wrong, since 77 + 150 + 29 = 256.
>>
>> Has anybody else here already commited this patch?
> I think not, can you do it?
> Although i thinks legacy and incorrect blending code in AGG should be
> removed. I agree with John on that and/or the husband.
> It is bad to keep all old code around.
> What do you think?
>> I do not follow the AGG
>> commits, is there a commit notification list I can sign up to?
>>
> Not yet, and only the admin can do it.
> I suggest our admin gives other devlopers  admin rights too.
> Or at least make things happen more quickly. e.g. Vinnie is not on the
> members list jet.
> And this commit list should be in place.
>
> Leonardo were are you??
>
> regards,
>
> Klaas
>
>
>
>
>
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry® Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart  
> your
> developing skills, take BlackBerry mobile applications to market and  
> stay
> ahead of the curve. Join us from November 9-12, 2009. Register  
> now!
> http://p.sf.net/sfu/devconf
> _______________________________________________
> Vector-agg-general mailing list
> Vector-agg-general@...
> https://lists.sourceforge.net/lists/listinfo/vector-agg-general

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Vector-agg-general mailing list
Vector-agg-general@...
https://lists.sourceforge.net/lists/listinfo/vector-agg-general

[John Horigan]

A gzipped patch file is attached. If you don't see an attachment then  
contact me and I will arrange some other method of getting the patch  
to you.





Updated patch for accurate blenders:
1) Old blenders are completely removed
2) both gray16(const rgba16&) constructors are fixed

-- john

On Oct 2, 2009, at 3:19 PM, John Horigan wrote:

> Yes, that is a bug in my new gray16() constructor. I will send out a
> new patch with that fixed and the old code removed.
>
> -- john
>
> On Oct 2, 2009, at 10:13 AM, klaas.holwerda wrote:
>
>> Stephan Assmus wrote:
>>> Hi John,
>>>
>>> On 2009-09-28 at 00:12:42 [+0200], John Horigan <john@...>
>>> wrote:
>>>
>>>> Sorry for the delay in this patch. I thought that the new blenders
>>>> were
>>>> failing in the rgba16 formats. But it turned out that the error
>>>> was in my
>>>> test framework.
>>>>
>>>
>>> I've read through the patch and it looks very good! Thanks a lot
>>> for this
>>> work. Only one place I am not sure about:
>>>
>>> +
>>> //--------------------------------------------------------------------
>>> +        gray16(const rgba16& c, unsigned a_) :
>>> +        v((value_type)((c.r*77 + c.g*150 + c.b*29) >> 16)),
>>> +        a((value_type)a_) {}
>>>
>>> I think the shift is wrong, since 77 + 150 + 29 = 256.
>>>
>>> Has anybody else here already commited this patch?
>> I think not, can you do it?
>> Although i thinks legacy and incorrect blending code in AGG should be
>> removed. I agree with John on that and/or the husband.
>> It is bad to keep all old code around.
>> What do you think?
>>> I do not follow the AGG
>>> commits, is there a commit notification list I can sign up to?
>>>
>> Not yet, and only the admin can do it.
>> I suggest our admin gives other devlopers  admin rights too.
>> Or at least make things happen more quickly. e.g. Vinnie is not on  
>> the
>> members list jet.
>> And this commit list should be in place.
>>
>> Leonardo were are you??
>>
>> regards,
>>
>> Klaas
>>
>>
>>
>>
>>
>>
>> ------------------------------------------------------------------------------
>> Come build with us! The BlackBerry® Developer Conference in SF,  
>> CA
>> is the only developer event you need to attend this year. Jumpstart
>> your
>> developing skills, take BlackBerry mobile applications to market and
>> stay
>> ahead of the curve. Join us from November 9-12, 2009. Register
>> now!
>> http://p.sf.net/sfu/devconf
>> _______________________________________________
>> Vector-agg-general mailing list
>> Vector-agg-general@...
>> https://lists.sourceforge.net/lists/listinfo/vector-agg-general
>
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry® Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart  
> your
> developing skills, take BlackBerry mobile applications to market and  
> stay
> ahead of the curve. Join us from November 9-12, 2009. Register  
> now!
> http://p.sf.net/sfu/devconf
> _______________________________________________
> Vector-agg-general mailing list
> Vector-agg-general@...
> https://lists.sourceforge.net/lists/listinfo/vector-agg-general

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Vector-agg-general mailing list
Vector-agg-general@...
https://lists.sourceforge.net/lists/listinfo/vector-agg-general