<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<title></title>
</head>
<body bgcolor="#ffffff" text="#000000">
On 02/06/2011 09:46 AM, Cyrille Berger Skott wrote:
<blockquote cite="mid:201102060946.32245.cberger@cberger.net"
type="cite">
<pre wrap="">Hi,
Here is some code review:
* I did some more testing and it is indeed a regression for this bug
<a class="moz-txt-link-freetext" href="https://bugs.kde.org/show_bug.cgi?id=176536">https://bugs.kde.org/show_bug.cgi?id=176536</a> , since I am assuming that you
wanted it that way, having transparent pixel being erased.
For instance look at the result of the following file:
<a class="moz-txt-link-freetext" href="http://cyrille.diwi.org/tmp/krita/colored_leaves.kra">http://cyrille.diwi.org/tmp/krita/colored_leaves.kra</a>
Before patch:
<a class="moz-txt-link-freetext" href="http://cyrille.diwi.org/tmp/krita/colored_leaves_before.png">http://cyrille.diwi.org/tmp/krita/colored_leaves_before.png</a>
After patch:
<a class="moz-txt-link-freetext" href="http://cyrille.diwi.org/tmp/krita/colored_leaves_after.png">http://cyrille.diwi.org/tmp/krita/colored_leaves_after.png</a>
Since I am not convinced that "alpha lock" should be used for that, since it
is there to prevent editing of the alpha channel, if the having transparent
pixel erased is a wanted behaviour we should probably introduce a "keep
transparency when compositing" and have it on by default
</pre>
</blockquote>
Oh, yes you are right. This is a problem for layer compositing.<br>
I don't know how easily it can be done but I would like to have a
button, to keep<br>
transparency when compositing layers, for every layer next to the
alpha lock button<br>
and of course turned on by default if you want it that way.<br>
I have the rough guess that the layer compositing simply works with
the CompositeOps alpha<br>
locking turned off by default. So turning it on by default and
adding an option to turn<br>
it off (per layer) should hopefully solve the problem :D if you
agree.<br>
<br>
<blockquote cite="mid:201102060946.32245.cberger@cberger.net"
type="cite">
<pre wrap="">* the optimized integers operations do provide a significant boost in speed,
about 10% on the composite operations, that translate to about 5% when
drawing. I don't know at what size of brush you tried, but even on high-end
hardware it makes a noticeable difference when using large size brushes. On
the other hand, it does provide an unnoticeable difference in the results.
This is why those optimizwd integers operations are used in Gimp, imagemagick
and many other graphics applications. So I am against dropping them. If it is
really a problem for you, I would suggest the use of a compile switch,
ENABLE_EXACT_PRECISION_COMPOSITE_OPS , defaulted to off.
</pre>
</blockquote>
Ok, my biggest brush only has a diameter of 300px.<br>
I didn't meant to drop those optimized functions. I just had some
problems with the<br>
result and wanted to get everything working right before i start
optimizing anything :D<br>
<br>
<blockquote cite="mid:201102060946.32245.cberger@cberger.net"
type="cite">
<pre wrap="">
* KoCompositeOpFunction.h should not duplicate the content of
KoColorSpaceMath.h (especially lerp, mul) and the functions that does not
exist in KoColorSpaceMath should be added (like inv) Similary use "clamp" from
KoColorSpaceMath, since your version clamp values for floating points as well,
we should not happen.
</pre>
</blockquote>
Would you agree if I add those functions as aliases to
KoColorSpaceMath.h<br>
because i think in my opinion it makes the code more readable to
have a<br>
simple function mul(..., ...) then to specify every time
KoColorSpaceMaths<...>::multiply(..., ...).<br>
And I would like to have two versions of the multiplication
functions (optimized and unoptimized) like:<br>
<br>
// fast version<br>
template<class T><br>
inline T mul(T a, T b) { T(KoColorSpaceMaths<T>::multiply(a,
b)); }<br>
<br>
// accurate version<br>
template<class T><br>
inline T mulac(T a, T b) {<br>
typedef typename KoColorSpaceMathsTraits<T>::compositetype
composite_type;<br>
return T(composite_type(a) * b /
KoColorSpaceMathsTraits<T>::unitValue);<br>
}<br>
<br>
So i can add the fast version everywhere where it works and the
accurate version everywhere else.<br>
<br>
About the clamp function. I thought even the FP versions need to be
clamped to the range<br>
between 0.0 and 1.0 (I used the camp function only where the values
could exceed this bound e.g. after divisions)<br>
because without clamping the FP versions would behave different to
the integer versions or do<br>
I understand something wrong here?<br>
<br>
Ah, yes something else. I've got a question about the composite type
for float data types.<br>
The KoColorSpaceMathsTraits<float>::compositetype is double.
is this necessary?<br>
wouldn't float be sufficient here?<br>
<br>
And the KoColorSpaceMaths::divide method takes values of type T and
returns a value of type T.<br>
But i think it should return a value of type T->compositetype.
For example dividing 1.0 by 0.5<br>
will result in 2.0 what would be 510 in 8bit per channel mode and we
have a data loss here because it will be<br>
truncated to T.<br>
<br>
<blockquote cite="mid:201102060946.32245.cberger@cberger.net"
type="cite">
<pre wrap="">
* have you cheked if compositeFunc is inlined inside KoCompositeOpGenericSC ?
</pre>
</blockquote>
Well, yes I'm pretty sure. I compiled a little test program:<br>
<br>
<small><tt>#include <cstdio><br>
<br>
template<class T><br>
inline T bfAdd(T src, T dst) { return src + dst; }<br>
<br>
template<class T, T blendFunc(T,T)><br>
struct CompositingOp<br>
{<br>
inline void doSomething(T src, T dst)<br>
{<br>
double result = double(blendFunc(src, dst));<br>
std::printf("%f\n", result);<br>
}<br>
};<br>
<br>
int main()<br>
{<br>
CompositingOp<int, &bfAdd> opAdd;<br>
int src = 40;<br>
int dst = 20;<br>
<br>
opAdd.doSomething(src, dst);<br>
return 0;<br>
}</tt></small><br>
<br>
The ASM result should speak for itself :D<br>
<br>
<u>Unoptimized code compiled and disassembled with:</u><br>
g++ -c -O0 -g3 main.cpp<br>
objdump -d -s main.o<br>
<br>
<small><tt>0000000000000000 <main>:<br>
0: 55 push %rbp<br>
1: 48 89 e5 mov %rsp,%rbp<br>
4: 48 83 ec 10 sub $0x10,%rsp<br>
8: c7 45 f8 28 00 00 00 movl $0x28,-0x8(%rbp)<br>
f: c7 45 f4 14 00 00 00 movl $0x14,-0xc(%rbp)<br>
16: 8b 55 f4 mov -0xc(%rbp),%edx<br>
19: 8b 4d f8 mov -0x8(%rbp),%ecx<br>
1c: 48 8d 45 ff lea -0x1(%rbp),%rax<br>
20: 89 ce mov %ecx,%esi<br>
22: 48 89 c7 mov %rax,%rdi<br>
25: e8 00 00 00 00 callq 2a <main+0x2a><br>
2a: b8 00 00 00 00 mov $0x0,%eax<br>
2f: c9 leaveq <br>
30: c3 retq <br>
<br>
Disassembly of section .text._Z5bfAddIiET_S0_S0_:<br>
<br>
0000000000000000 <_Z5bfAddIiET_S0_S0_>:<br>
0: 55 push %rbp<br>
1: 48 89 e5 mov %rsp,%rbp<br>
4: 89 7d fc mov %edi,-0x4(%rbp)<br>
7: 89 75 f8 mov %esi,-0x8(%rbp)<br>
a: 8b 45 f8 mov -0x8(%rbp),%eax<br>
d: 8b 55 fc mov -0x4(%rbp),%edx<br>
10: 8d 04 02 lea (%rdx,%rax,1),%eax<br>
13: c9 leaveq <br>
14: c3 retq <br>
<br>
Disassembly of section
.text._ZN13CompositingOpIiXadL_Z5bfAddIiET_S1_S1_EEE11doSomethingEii:<br>
<br>
0000000000000000
<_ZN13CompositingOpIiXadL_Z5bfAddIiET_S1_S1_EEE11doSomethingEii>:<br>
0: 55 push %rbp<br>
1: 48 89 e5 mov %rsp,%rbp<br>
4: 48 83 ec 20 sub $0x20,%rsp<br>
8: 48 89 7d e8 mov %rdi,-0x18(%rbp)<br>
c: 89 75 e4 mov %esi,-0x1c(%rbp)<br>
f: 89 55 e0 mov %edx,-0x20(%rbp)<br>
12: 8b 55 e0 mov -0x20(%rbp),%edx<br>
15: 8b 45 e4 mov -0x1c(%rbp),%eax<br>
18: 89 d6 mov %edx,%esi<br>
1a: 89 c7 mov %eax,%edi<br>
1c: e8 00 00 00 00 callq 21
<_ZN13CompositingOpIiXadL_Z5bfAddIiET_S1_S1_EEE11doSomethingEii+0x21><br>
21: f2 0f 2a c0 cvtsi2sd %eax,%xmm0<br>
25: f2 0f 11 45 f8 movsd %xmm0,-0x8(%rbp)<br>
2a: f2 0f 10 45 f8 movsd -0x8(%rbp),%xmm0<br>
2f: bf 00 00 00 00 mov $0x0,%edi<br>
34: b8 01 00 00 00 mov $0x1,%eax<br>
39: e8 00 00 00 00 callq 3e
<_ZN13CompositingOpIiXadL_Z5bfAddIiET_S1_S1_EEE11doSomethingEii+0x3e><br>
3e: c9 leaveq <br>
3f: c3 retq<br>
</tt></small><br>
<u>Optimized code compiled and disassembled with:</u><br>
g++ -c -O1 -g3 main.cpp<br>
objdump -d -s main.o<br>
<br>
<small><tt>0000000000000000 <main>:<br>
0: 48 83 ec 08 sub $0x8,%rsp<br>
4: f2 0f 10 05 00 00 00 movsd 0x0(%rip),%xmm0
# c <main+0xc><br>
b: 00 <br>
c: be 00 00 00 00 mov $0x0,%esi<br>
11: bf 01 00 00 00 mov $0x1,%edi<br>
16: b8 01 00 00 00 mov $0x1,%eax<br>
1b: e8 00 00 00 00 callq 20 <main+0x20><br>
20: b8 00 00 00 00 mov $0x0,%eax<br>
25: 48 83 c4 08 add $0x8,%rsp<br>
29: c3 retq</tt></small><br>
<br>
<br>
</body>
</html>