Re: koffice/krita/image

On 2009-11-04 05:18, Cyrille Berger wrote:
> SVN commit 1044629 by berger:
>
> revert to 936510 (the reason is that after 944988, the algorithm return either a very small value or UINT64_MAX)
>
> It might be worth, at some point to backport to branch 2.1

Um... guys? r93651 doesn't seem to be any better. Also what I'm seeing
out of krita isn't remotely like what I got from my test application.

I'm going to compare notes here... the r944988 version should have been
the same as what's in my test app.

...and I'm going to check in my test app (to krita/image/tests).

--
Matthew
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

Matthew Woehlke wrote:

> On 2009-11-04 05:18, Cyrille Berger wrote:
>> SVN commit 1044629 by berger:
>>
>> revert to 936510 (the reason is that after 944988, the algorithm
>> return either a very small value or UINT64_MAX)
>>
>> It might be worth, at some point to backport to branch 2.1
>
> Um... guys? r93651 doesn't seem to be any better. Also what I'm seeing
> out of krita isn't remotely like what I got from my test application.

Huh, now I am not seeing that. Odd. But still...

> I'm going to compare notes here... the r944988 version should have been
> the same as what's in my test app.
>
> ...and I'm going to check in my test app (to krita/image/tests).

Eureka!

(...But still) there is nothing wrong with the algorithm in r944988.
There is, however, a small error in the code that has a... very, very
bad effect.

Here is mine version of part():
     inline quint64 part(quint64 n1, quint64 n2, int p)
     {
         int b = p * 8;
         int i = (n1 >> b) & 0xFF;
         int j = (n2 >> b) & 0xFF;
         return quint64(salt[i][j]) << b;
     }

...and here is what got committed:
     inline quint64 part(quint64 n1, quint64 n2, int p)
     {
         int b = p * 8;
         int i = (n1 >> b) & 0xFF;
         int j = (n2 >> b) & 0xFF;
         return salt[i][j] << b;
     }

(Granted, mea culpa, though I'm also a bit frightened no one noticed
until now. Well the test app is in koffice's svn now, so hopefully that
will keep this sort of thing from happening again.)

Please don't backport the reversion; there is no need, the old code
(that is now in trunk again) is messy, and the old algorithm is slightly
worse.

If there are no objections within 48 hours, I am going to revert the
revert, and then check in the fix for the above bug. (Of course, I'll do
the work sooner if I hear an affirmative.)

--
Matthew
ENOWIT: .sig file for this machine not set up yet

_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

[Cyrille Berger]

On Wednesday 04 November 2009, Matthew Woehlke wrote:
> (Granted, mea culpa, though I'm also a bit frightened no one noticed
> until now. Well the test app is in koffice's svn now, so hopefully that
> will keep this sort of thing from happening again.)
Well actually we had a test failing for some time on the subject, since that
commit actually. Might be good to test that is still passing (or that the
amount of failure is low).
Might be a good idea to extend on that test, rather than rely on the test app.

> Please don't backport the reversion; there is no need, the old code
> (that is now in trunk again) is messy, and the old algorithm is slightly
> worse.
Ok we will backport your change then.

> If there are no objections within 48 hours, I am going to revert the
> revert, and then check in the fix for the above bug. (Of course, I'll do
> the work sooner if I hear an affirmative.)
fine with me.

--
Cyrille Berger
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

[Matthew Woehlke-2]

(repost with correct address.)
Um... please reply to kimageshop@.... Sorry about that. (I'm trying
to drop -commits from this, I don't think there is any reason to
continue posting there?)

On 2009-11-05 03:39, Cyrille Berger wrote:

> On Wednesday 04 November 2009, Matthew Woehlke wrote:
>> On 2009-11-04 05:18, Cyrille Berger wrote:
>>> SVN commit 1044629 by berger:
>>>
>>> revert to 936510 (the reason is that after 944988, the algorithm return
>>> either a very small value or UINT64_MAX)
>>>
>>> It might be worth, at some point to backport to branch 2.1
>>>
>>> CCMAIL: mw_triad@...
>>
>> Um... 944988 should have been a non-functional change. Are you sure
>> something didn't go wrong since then?
> I tested 944988 first. And the change is rather intrusive.

Well, sure, it was a refactor after all. But it made the code cleaner
(and nuked a bunch of dead stuff). And it *shouldn't* have caused a
functional change.

(It did due to a cast that got lost, as noted on the list.)

>> Also it seems you reverted a number of compile fixes.
> Actually, I made sure that they were in my revert. But I did remove the ugly
> #ifdef and instead define the macro at the same place for everybody.

Okay. I'm starting with a clean revert, so I'll redo this separately
(assuming I can figure out what is the change - if you know offhand,
that would be appreciated). Probably I'll run that diff by you first.

Oh, and I am un-breaking the unit test :-). Right now it passes even
with the broken version.

Specifically, I am pretty sure you meant:
     quint64 number2 = randg.randomAt(...);
...and not:
     quint64 number2 = randg.doubleRandomAt(...);

(The *fixed* algorithm does pass the (fixed) test. This is good :-).)

On an unrelated note, svn is driving me nuts ;-). Does anyone have a
git-svn checkout I can clone?

--
Matthew
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

On 2009-11-04 05:23, Boudewijn Rempt wrote:
> On Wed, 4 Nov 2009, Cyrille Berger wrote:
>
>> SVN commit 1044629 by berger:
>>
>> revert to 936510 (the reason is that after 944988, the algorithm return either a very small value or UINT64_MAX)
>>
>> It might be worth, at some point to backport to branch 2.1
>
> Before we release?

Okay... now that trunk is back in order, r1045274 should be backported.
As Cyrille notes, without the bug fix, the output is... well, bad.
*Really* bad :-). (Also, r1045274 is conveninently far less invasive
than undoing the refactoring, and won't lost the behavior change from
r947541.)

Would you like me to do that (probably won't happen for ~24 hours due to
time zone differences)? Otherwise I have no objection if one of you
wants to do it.

--
Matthew
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

[Cyrille Berger]

On Thursday 05 November 2009, Matthew Woehlke wrote:
> Would you like me to do that (probably won't happen for ~24 hours due to
> time zone differences)? Otherwise I have no objection if one of you
> wants to do it.
I have tested it, sounds ok to backport.

--
Cyrille Berger
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

Cyrille Berger wrote:
> On Thursday 05 November 2009, Matthew Woehlke wrote:
>> Would you like me to do that (probably won't happen for ~24 hours due to
>> time zone differences)? Otherwise I have no objection if one of you
>> wants to do it.
> I have tested it, sounds ok to backport.

Done. Thanks.

--
Matthew
Please do not quote my e-mail address unobfuscated in message bodies.
--
Sorry, fresh out of .sigs. Maybe tomorrow.
(paraphrased German saying)

_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop