Bug 40272 - The 2d.imageData.object.round canvas test is failing
Summary: The 2d.imageData.object.round canvas test is failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Dominik Röttsches (drott)
URL: http://philip.html5.org/tests/canvas/...
Keywords: HTML5
Depends on:
Blocks:
 
Reported: 2010-06-07 17:39 PDT by Jan Erik Hanssen
Modified: 2012-08-29 08:32 PDT (History)
32 users (show)

See Also:


Attachments
Proposed patch (5.74 KB, patch)
2010-07-19 19:16 PDT, Helder Correia
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Proposed patch v2 (8.98 KB, patch)
2010-07-20 18:56 PDT, Helder Correia
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Proposed patch v3 (9.90 KB, patch)
2010-07-23 19:14 PDT, Helder Correia
no flags Details | Formatted Diff | Diff
Rounding using lrint, handling halfway case, v1. (11.78 KB, patch)
2012-08-15 03:31 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Rounding using lrint, handling halfway case, v2. (12.87 KB, patch)
2012-08-15 06:02 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Rounding using lrint, handling halfway case, v3. (12.92 KB, patch)
2012-08-15 06:19 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (432.52 KB, application/zip)
2012-08-15 10:41 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-06 (614.50 KB, application/zip)
2012-08-15 11:43 PDT, WebKit Review Bot
no flags Details
Rounding using lrint, handling halfway case, v4. (13.23 KB, patch)
2012-08-16 13:46 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Rounding using lrint, handling halfway case, v5. (13.25 KB, patch)
2012-08-16 14:48 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (402.94 KB, application/zip)
2012-08-16 16:52 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-08 (366.88 KB, application/zip)
2012-08-16 18:00 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cq-04 (486.35 KB, application/zip)
2012-08-17 02:22 PDT, WebKit Review Bot
no flags Details
Rounding using lrint, handling halfway case, v6. (13.45 KB, patch)
2012-08-20 01:21 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Rounding using lrint, handling halfway case, v7. (13.47 KB, patch)
2012-08-20 01:27 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Rounding using lrint, handling halfway case, v8 (monday edition). (13.30 KB, patch)
2012-08-20 01:41 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (630.81 KB, application/zip)
2012-08-20 03:13 PDT, WebKit Review Bot
no flags Details
Rounding using lrint, handling halfway case, v9. (13.62 KB, patch)
2012-08-20 03:35 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Added tests, comments addressed. (11.73 KB, patch)
2012-08-24 01:36 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Added tests, condition shortened. (11.72 KB, patch)
2012-08-24 03:22 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Added tests, condition shortened, put back inline. (11.66 KB, patch)
2012-08-24 03:35 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Boundaries and zero tests. (12.00 KB, patch)
2012-08-27 08:59 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Boundaries and zero tests, Cr Android EWS build fix. (11.97 KB, patch)
2012-08-27 10:59 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Boundaries and zero tests, Assertion added. (12.19 KB, patch)
2012-08-28 00:55 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Final version, xcodeproj sort. (12.42 KB, patch)
2012-08-29 04:59 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Erik Hanssen 2010-06-07 17:39:07 PDT
The 2d.imageData.object.round and 2d.imageData.object.wrap canvas tests are failing, the values should be truncated and the wrap test assumes that values should wrap around.

Tests: 
http://philip.html5.org/tests/canvas/suite/tests/2d.imageData.object.round.html
http://philip.html5.org/tests/canvas/suite/tests/2d.imageData.object.wrap.html
Comment 1 Helder Correia 2010-07-19 19:16:22 PDT
Created attachment 62028 [details]
Proposed patch
Comment 2 Darin Adler 2010-07-19 20:01:49 PDT
Comment on attachment 62028 [details]
Proposed patch

> -            if (value & ~0xFF) {
> -                if (value < 0)
> -                    value = 0;
> -                else
> -                    value = 255;
> -            }
> +            if (value & ~0xFF)
> +                value %= 256;
>              m_storage->data()[i] = static_cast<unsigned char>(value);

To me this looks like an inefficient way to do what the static_cast will already do. For what values does this give a different result than if the %= was left out?

> -            if (!(value > 0)) // Clamp NaN to 0
> -                value = 0;
> -            else if (value > 255)
> -                value = 255;
> -            m_storage->data()[i] = static_cast<unsigned char>(value + 0.5);
> +            long long intValue = static_cast<long long>(value);
> +            if (intValue & ~0xFF)
> +                intValue %= 256;
> +            m_storage->data()[i] = static_cast<unsigned char>(intValue);

Why does this require the use of a 64-bit integer? I don’t understand why it’s important or helpful to convert the double into a long long. If 32 bits aren’t enough, then why are 64 bits enough? We need to code this in an efficient way, and converting a double to a 64-bit integer is a particularly slow operation. Perhaps this operation also can be done by simply doing a single static_cast.

Is there a test covering the behavior of NaN, Infinity, and -Infinity?

review- because I would like the questions above answered. If this is indeed the most efficient way to do the correct thing, then you can put it up for review again.
Comment 3 Andreas Kling 2010-07-19 20:19:55 PDT
Comment on attachment 62028 [details]
Proposed patch

>LayoutTests/ChangeLog:6
> +          Fix the 2d.imageData.object.round test to use the correct expected
> +          values for rounding according to the HTML5 specification.
Please make sure this change is upstreamed to http://dvcs.w3.org/hg/html/ via Philip Taylor.
Comment 4 Andreas Kling 2010-07-19 21:00:47 PDT
Comment on attachment 62028 [details]
Proposed patch

Tested this, no side effects AFAICT.

>LayoutTests/platform/qt/Skipped: 
> +  canvas/philip/tests/2d.imageData.object.wrap.html
This file needs updated expectations.
Comment 5 Helder Correia 2010-07-20 18:54:14 PDT
> To me this looks like an inefficient way to do what the static_cast will already do. For what values does this give a different result than if the %= was left out?

Indeed, the introduced "if" block is absolutely redundant (and the same applies to the other %= change).  The fix is even simpler and consists in leaving only the static cast there.

> Why does this require the use of a 64-bit integer? I don’t understand why it’s important or helpful to convert the double into a long long. If 32 bits aren’t enough, then why are 64 bits enough? We need to code this in an efficient way, and converting a double to a 64-bit integer is a particularly slow operation. Perhaps this operation also can be done by simply doing a single static_cast.

I used "long long" because a double can store integer values up to 2^53.  Using only the unsigned char static cast works for values up to 2^31-1.  I guess it all boils down to priorities: spec conformance or performance?  IMHO, performance should indeed be prioritized in this case, since we're talking about extremely high numbers that won't probably be used by anyone, but of course I'll let you and other WK reviewers decide what should be done in this situation.
 
> Is there a test covering the behavior of NaN, Infinity, and -Infinity?

Can't remember about NaN, but -/+Infinity are covered.

I will attach a new version of the patch right away.  Please note that it still contains the "long long" cast, which I can happily change according to the decision.
Comment 6 Helder Correia 2010-07-20 18:56:21 PDT
Created attachment 62139 [details]
Proposed patch v2
Comment 7 Helder Correia 2010-07-20 18:59:53 PDT
(In reply to comment #3)
> Please make sure this change is upstreamed to http://dvcs.w3.org/hg/html/ via Philip Taylor.

I did talk to Philip about this some weeks ago, and he confirmed the bug in the test and said he had fixed it in his local repo.  Not sure what happened after that.  I will talk to him again.
Comment 8 Darin Adler 2010-07-20 22:59:36 PDT
(In reply to comment #5)
> I used "long long" because a double can store integer values up to 2^53. Using only the unsigned char static cast works for values up to 2^31-1. I guess it all boils down to priorities: spec conformance or performance? IMHO, performance should indeed be prioritized in this case, since we're talking about extremely high numbers that won't probably be used by anyone, but of course I'll let you and other WK reviewers decide what should be done in this situation.

We should double check that we are optimizing real performance and real correctness.

Correctness: I don’t see any test cases covering the behavior of those super-large integers. Do those test cases exist?

Performance: Can we measure the performance?
Comment 9 Darin Adler 2010-07-20 23:00:33 PDT
If they two conflict, then I think in this case performance should win because the cases affected are extremely unusual edge cases, and it's not worth slowing down the normal case for the sake of those.
Comment 10 Helder Correia 2010-07-22 14:36:04 PDT
(In reply to comment #8)
> Correctness: I don’t see any test cases covering the behavior of those super-large integers. Do those test cases exist?

I don't know of any.

> Performance: Can we measure the performance?

I've now tried 2 different approaches to see if the patch slows the performance down:

1) I wrote http://pastebin.com/D70ukaf3 (which unfortunately will only work on Linux, and don't forget to link "-lrt" if you want to try it out). The I ran it using static_cast<unsigned char>(double) first, and then with static_cast<unsigned char>(static_cast<long long>(double)).

Results: after 10 iterations, each running a loop for 4e9 times, the averages in both cases were exactly the same in the ms magnitude.  This was obtained on 32- and 64-bit x86 Linux with "g++ -O2".  Then I tried without "-O2" and found that static_cast<unsigned char>(double) is slower than the proposed change.

I wasn't expecting this at all and so decided look at the generated ASM.  It turns out that the code dumped with "-O2" is exactly the same in both cases.  OTOH, with no optimizations, the compiler uses 1 MOVZWL instruction in the original case, and 2 MOVL instructions with the new approach.  So either 2 MOVL instructions are faster than 1 MOVZWL, or the CPU is able to pipeline the former.

2) I built release Qt + release WebKit and then ran "QtTestBrowser -graphicssystem raster" against this Canvas demo that I wrote: http://gitorious.org/ofi-labs/x2/trees/master/webkit/plasmaeffect

It changes every R, G, and B components of all pixels at a 16ms interval.  To test the differences even better, I maximized the window and pressed 'F' which will make the canvas be the size of the viewport.  Results: the frame rate indicated in the title bar is the same with both the original and the patched code.

My conclusion: not only the patch is completely spec conformant, it is also perfectly safe in terms of performance (at least on x86).
Comment 11 Darin Adler 2010-07-22 19:09:19 PDT
(In reply to comment #10)
> (In reply to comment #8)
> > Correctness: I don’t see any test cases covering the behavior of those super-large integers. Do those test cases exist?
> 
> I don't know of any.

Then we should create them. We need to test the behavior.

> My conclusion: not only the patch is completely spec conformant, it is also perfectly safe in terms of performance (at least on x86).

The cast to long long is not needed, then. Since it has no effect on either correctness or performance, I don’t any reason to leave it in
Comment 12 Helder Correia 2010-07-23 02:23:58 PDT
(In reply to comment #11)
> (In reply to comment #10)
> The cast to long long is not needed, then. Since it has no effect on either correctness or performance, I don’t any reason to leave it in

I don't understand why you say that.  In my previous post I tested the performance of an intermediate long long cast, not the correctness of a direct unsigned char cast.  If you want proof that the long long cast is needed for correctness, then please have a look at this simple test:

    http://pastebin.com/sY6med20

As a side note, g++ produces different results depending on the optimization flag:

With no flag:
-------------
Correct result is 41, 42, 43
  direct cast to unsigned char   : 41, 0, 0
  intermediate cast to int       : 41, 42, 0
  intermediate cast to long long : 41, 42, 43

With "-O2":
-----------
Correct result is 41, 42, 43
  direct cast to unsigned char   : 255, 255, 255
  intermediate cast to int       : 41, 42, 255
  intermediate cast to long long : 41, 42, 43

So the values are instead being clamped up or down, from a certain value on, when not using an intermediate long long cast. Unless this a bug in g++, this cast is needed.
Comment 13 Darin Adler 2010-07-23 08:46:33 PDT
(In reply to comment #12)
> In my previous post I tested the performance of an intermediate long long cast, not the correctness of a direct unsigned char cast.

You stated 'the code dumped with "-O2" is exactly the same in both cases'. How can that be, if the two cases produce different results?
Comment 14 Darin Adler 2010-07-23 08:52:03 PDT
(In reply to comment #12)
> As a side note, g++ produces different results depending on the optimization flag:

This means that your performance test is actually detecting a bug in g++. If it kept things correct, the opcodes would be different!

If we do land the code with "long long" in it, I think you’re going to need a comment explaining that long long is the smallest integral type long enough to give the correct values for certain large numbers in double, and that other integral types give incorrect results.
Comment 15 Darin Adler 2010-07-23 08:52:39 PDT
Comment on attachment 62139 [details]
Proposed patch v2

Looks good. As per our discussion, please add a regression test and a comment making clear why "long long" is needed in setIndex.
Comment 16 Helder Correia 2010-07-23 11:30:03 PDT
(In reply to comment #13)
> You stated 'the code dumped with "-O2" is exactly the same in both cases'.

My bad, I redid all steps and proved myself that statement is wrong, the code is not the same.  I must have made a wrong diff in the first time, sorry for the confusion.  But I've verified all other steps and my conclusions remain.  Thanks, I will add the test and the comment.  I should probably also file a bug for g++.
Comment 17 Helder Correia 2010-07-23 19:14:51 PDT
Created attachment 62483 [details]
Proposed patch v3

Added regression test and comment about long long.
Comment 18 WebKit Review Bot 2010-07-23 19:19:53 PDT
Attachment 62483 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 WebKit Commit Bot 2010-07-24 16:13:52 PDT
Comment on attachment 62483 [details]
Proposed patch v3

Rejecting patch 62483 from commit-queue.

Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
http://webkit.org/coding/contributing.html
Comment 20 Helder Correia 2010-07-25 19:05:26 PDT
(In reply to comment #19)
> Found no modified ChangeLogs, cannot create a commit message.

This patch has clearly hit some bug in the commit bot.
Comment 21 Eric Seidel (no email) 2010-07-26 04:48:39 PDT
Comment on attachment 62483 [details]
Proposed patch v3

I'm not sure why it tried so many times to land it and failed.

Oh, probably because this patch causes a full world rebuild and if anyone commits while it's rebuilding it will cause the commit to fail.
Comment 22 WebKit Commit Bot 2010-07-26 05:35:54 PDT
Comment on attachment 62483 [details]
Proposed patch v3

Rejecting patch 62483 from commit-queue.

Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
http://webkit.org/coding/contributing.html
Comment 23 Helder Correia 2010-07-26 16:59:11 PDT
Just for future reference, the GCC people said:

"If the value of the integral part cannot be represented by
the integer type, the behavior is undefined."

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45091
Comment 24 Darin Adler 2010-07-26 17:00:24 PDT
(In reply to comment #23)
> Just for future reference, the GCC people said:
> 
> "If the value of the integral part cannot be represented by
> the integer type, the behavior is undefined."
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45091

So to do this portably we'd have to do something like fmod?
Comment 25 Helder Correia 2010-07-26 18:52:07 PDT
(In reply to comment #24)
> So to do this portably we'd have to do something like fmod?

As long as "long long" is big enough to receive the integral part represented in the "double" value, we're good.  Of course double can apparently be bigger than 8 bytes, and the same happens with "long long".  So to be 100% safe, we would have to do something like:

    if (value > 255 || value < 0) // should be cheaper than fmod in most cases
        value = fmod(value, 256);
    m_storage->data()[i] = static_cast<unsigned char>(value);

I think it's not worth it compared to the proposed patch, because in nearly all cases the 2 comparisons will be performed in vain, and then there's the additional high cost of the function call in the abnormal cases.

IMHO, the canvas spec should probably say something like this instead:
  "If values outside of 0-255 are provided, the behavior is undefined."
or
  "Negatives values are clamped to 0, values bigger than 255 are clamped to 255."

Wrapping and truncation are in this case easily and cheaply achieved, but only when we stick to "small" values.  I might be missing some handy use case, but I really can't see why a developer would actually make use of the wrapping and negative values admitted in the specification, not to mention such huge values beyond the capacity of a "long long".
Comment 26 Andreas Kling 2010-07-27 14:12:08 PDT
Committed r64156: <http://trac.webkit.org/changeset/64156>
Comment 27 Oliver Hunt 2010-11-16 14:02:22 PST
Those tests were wrong, this patch regressed behaviour.

This patch should be rolled out.
Comment 28 Darin Adler 2010-11-17 12:29:47 PST
Oliver, are you going to roll it out?

I’m not sure that reopening the bug is the right way to say “we should roll this out”. Reopening the bug normally means “this bug is not fixed”, not “this bug was wrong and should not ever be fixed”.
Comment 29 Oliver Hunt 2010-11-17 12:33:14 PST
(In reply to comment #28)
> Oliver, are you going to roll it out?
> 
> I’m not sure that reopening the bug is the right way to say “we should roll this out”. Reopening the bug normally means “this bug is not fixed”, not “this bug was wrong and should not ever be fixed”.

The rounding change may be correct, the wrapping change was not.  I'm not 100% sure.
Comment 30 Eric Seidel (no email) 2010-12-14 01:41:59 PST
Looks like this is fixed, no?  Unclear if it was ever rolled out.  From the current status of the bug I'm guessing it was fixed.  Please re-open with more information if I'm reading incorrectly.
Comment 31 Oliver Hunt 2011-02-23 15:42:59 PST
This was not rolled out, why was this closed?

I just spotted another piece of code that was broken because of this.
Comment 32 Oliver Hunt 2011-02-23 16:05:59 PST
Rolled out in r79501

Retitled to reflect that rounding may need to be addressed.  Clamping behaviour is now correct again.
Comment 33 Ryosuke Niwa 2011-02-24 01:41:17 PST
The following tests have been failing on Snow Leopard since the patch was rolled out:
canvas/philip/tests/2d.imageData.object.round.html
canvas/philip/tests/2d.imageData.object.wrap.html

http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r79495%20(15088)/results.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r79501%20(15089)/results.html

We should either rebaseline or skip them.  What should we do?
Comment 34 Ryosuke Niwa 2011-02-24 02:01:25 PST
Ah, the problem was that r79501 didn't add them back to the skipped list.  Added them back in http://trac.webkit.org/changeset/79527.
Comment 35 Philippe Normand 2011-02-24 02:13:38 PST
Skipped those 2 tests in GTK as well. See http://trac.webkit.org/changeset/79528
Comment 36 Oliver Hunt 2011-02-24 09:06:19 PST
(In reply to comment #33)
> The following tests have been failing on Snow Leopard since the patch was rolled out:
> canvas/philip/tests/2d.imageData.object.round.html
> canvas/philip/tests/2d.imageData.object.wrap.html
> 
> http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r79495%20(15088)/results.html
> http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r79501%20(15089)/results.html
> 
> We should either rebaseline or skip them.  What should we do?

Ah, sorry about that :-(
Comment 37 Dominik Röttsches (drott) 2012-08-14 01:37:03 PDT
Helder, do you think you could give this a refresh? Thanks.
Comment 38 Dominik Röttsches (drott) 2012-08-14 05:25:46 PDT
(In reply to comment #37)
> Helder, do you think you could give this a refresh? Thanks.

Actually, I changed my mind - I think the 2d.imageData.object.round.html test case's expectations are wrong in the meantime since getImageData returns  Uint8ClampedArray which uses octet, which then needs to be rounded since it has Clamp extended attribute semantics, taken from WebIDL:
http://dev.w3.org/2006/webapi/WebIDL/#es-octet &
http://www.khronos.org/registry/typedarray/specs/latest/#7.1

Will file a bug for the test case in W3C.
Comment 39 Dominik Röttsches (drott) 2012-08-14 06:28:04 PDT
Test case is mostly correct already in W3C (except description), we need to update the local version:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=18556
Comment 40 Dominik Röttsches (drott) 2012-08-15 02:47:27 PDT
Tracking the wrap case in bug 94089.
Comment 41 Dominik Röttsches (drott) 2012-08-15 03:31:15 PDT
Created attachment 158538 [details]
Rounding using lrint, handling halfway case, v1.
Comment 42 Kenneth Rohde Christiansen 2012-08-15 03:55:40 PDT
Comment on attachment 158538 [details]
Rounding using lrint, handling halfway case, v1.

View in context: https://bugs.webkit.org/attachment.cgi?id=158538&action=review

> Source/WTF/ChangeLog:10
> +        we need to roud to nearest integer, and to the even one if exactly halfway in between.

round*
Comment 43 Build Bot 2012-08-15 04:00:56 PDT
Comment on attachment 158538 [details]
Rounding using lrint, handling halfway case, v1.

Attachment 158538 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13513048
Comment 44 Dominik Röttsches (drott) 2012-08-15 04:54:12 PDT
(In reply to comment #42)
> (From update of attachment 158538 [details])

> round*

Will fix the comment, but unfortunately the win-ews doesn't like lrint. 
"7>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Debug\include\private\wtf/Uint8ClampedArray.h(103) : error C3861: 'lrint': identifier not found"

Do the reviewers have any suggestions how to overcome this? Does anyone know an equivalent for lrint in Visual Studio? Or should we add our own implementation in WTF?
Comment 45 Dominik Röttsches (drott) 2012-08-15 05:28:57 PDT
I'll try an assembly implementation for X86, falling back to casting for non X86 on MSVC.
Comment 46 Dominik Röttsches (drott) 2012-08-15 06:02:22 PDT
Created attachment 158553 [details]
Rounding using lrint, handling halfway case, v2.
Comment 47 Kenneth Rohde Christiansen 2012-08-15 06:05:54 PDT
Comment on attachment 158553 [details]
Rounding using lrint, handling halfway case, v2.

View in context: https://bugs.webkit.org/attachment.cgi?id=158553&action=review

> Source/WTF/wtf/MathExtras.h:218
> +#else
> +    // Fall back to casting.
> +    intgr = static_cast<int>flt;
> +#endif
> +    return intgr;

what other platforms would that be for MSVC? ARM? Should we warn somehow?
Comment 48 Dominik Röttsches (drott) 2012-08-15 06:19:34 PDT
Created attachment 158557 [details]
Rounding using lrint, handling halfway case, v3.
Comment 49 Dominik Röttsches (drott) 2012-08-15 06:21:10 PDT
(In reply to comment #47)
> (From update of attachment 158553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158553&action=review
> 
> > Source/WTF/wtf/MathExtras.h:218
> > +#else
> > +    // Fall back to casting.
> > +    intgr = static_cast<int>flt;
> > +#endif
> > +    return intgr;
> 
> what other platforms would that be for MSVC? ARM? Should we warn somehow?

With all the Windows RT arm tablets news and rumours, I just didn't stand in the way of webkit windows on ARM happening :-). Yes we should warn, I put a "#pragma message". Let's hope the Apple Win EWS eats it.
Comment 50 Kenneth Rohde Christiansen 2012-08-15 06:23:53 PDT
Comment on attachment 158557 [details]
Rounding using lrint, handling halfway case, v3.

View in context: https://bugs.webkit.org/attachment.cgi?id=158557&action=review

> Source/WTF/wtf/MathExtras.h:215
> +#pragma message("Falling back to casting for lrint(), causes rounding inaccuracy in halfway case.")

I wonder if we should link to the bug report? but I guess it is fine
Comment 51 WebKit Review Bot 2012-08-15 10:41:16 PDT
Comment on attachment 158557 [details]
Rounding using lrint, handling halfway case, v3.

Attachment 158557 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13513180

New failing tests:
fast/canvas/canvas-ImageData-behaviour.html
svg/css/circle-in-mask-with-shadow.svg
platform/chromium/virtual/gpu/canvas/philip/tests/2d.imageData.object.round.html
platform/chromium/virtual/gpu/fast/canvas/canvas-ImageData-behaviour.html
canvas/philip/tests/2d.imageData.object.round.html
Comment 52 WebKit Review Bot 2012-08-15 10:41:24 PDT
Created attachment 158590 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 53 WebKit Review Bot 2012-08-15 11:43:04 PDT
Comment on attachment 158557 [details]
Rounding using lrint, handling halfway case, v3.

Attachment 158557 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13517065

New failing tests:
fast/canvas/canvas-ImageData-behaviour.html
svg/css/circle-in-mask-with-shadow.svg
platform/chromium/virtual/gpu/canvas/philip/tests/2d.imageData.object.round.html
platform/chromium/virtual/gpu/fast/canvas/canvas-ImageData-behaviour.html
canvas/philip/tests/2d.imageData.object.round.html
Comment 54 WebKit Review Bot 2012-08-15 11:43:12 PDT
Created attachment 158607 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 55 Dominik Röttsches (drott) 2012-08-16 02:38:43 PDT
Apparently, V8 sets the index differently, can the google guys help me find that part? I am looking at Source/WebCore/bindings/js/JSArrayBufferViewHelper.h - and I suspect it's somewhere there.
Comment 56 Dominik Röttsches (drott) 2012-08-16 05:22:18 PDT
As far as I can see, V8 handles the storage of the Uint8ClampedArray as an ExternalArray type, bypassing the Uint8ClampedArray set() function.
When accessing the imageData.data pixels using square bracket indexing syntax, incorrect rounding is used. This needs to be fixed in the Chromium V8 bindings. Maybe a new specific "clamped" array type is needed that does the correct rounding.
Comment 57 Dominik Röttsches (drott) 2012-08-16 05:28:50 PDT
Chromium issue: crbug.com/115240
Comment 58 Dominik Röttsches (drott) 2012-08-16 05:30:42 PDT
Would the Chromium people be okay if we land the WebKit side and mark these tests failing for Chromium for now?
Comment 59 Adam Barth 2012-08-16 09:42:30 PDT
> Would the Chromium people be okay if we land the WebKit side and mark these tests failing for Chromium for now?

Sure.  Would you be willing to file a bug about the failure?
Comment 60 Dominik Röttsches (drott) 2012-08-16 13:46:29 PDT
Created attachment 158891 [details]
Rounding using lrint, handling halfway case, v4.
Comment 61 Dominik Röttsches (drott) 2012-08-16 13:47:56 PDT
(In reply to comment #59)
> > Would the Chromium people be okay if we land the WebKit side and mark these tests failing for Chromium for now?
> 
> Sure.  Would you be willing to file a bug about the failure?

Sure, filed bug 94246 and updated chromium TestExpectations in my patch. Let's see what the Chromium EWS says now.
Comment 62 Dominik Röttsches (drott) 2012-08-16 14:48:29 PDT
Created attachment 158908 [details]
Rounding using lrint, handling halfway case, v5.
Comment 63 Kenneth Russell 2012-08-16 15:19:18 PDT
CC'ing Danno and Ulan from V8 team.

Both JSC and V8 implement the indexing operator for all typed arrays, including Uint8ClampedArray, inside their respective JITs, so it's surprising to me that this patch works with JSC and not with V8, assuming they both implement the correct semantics. It sounds like the typed array conformance tests (currently part of the WebGL conformance suite) need to be expanded. See https://www.khronos.org/registry/webgl/sdk/tests/conformance/typedarrays/array-unit-tests.html and http://www.khronos.org/webgl/wiki/Using_Github_To_Contribute .
Comment 64 WebKit Review Bot 2012-08-16 16:51:59 PDT
Comment on attachment 158908 [details]
Rounding using lrint, handling halfway case, v5.

Attachment 158908 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13516561

New failing tests:
svg/css/circle-in-mask-with-shadow.svg
Comment 65 WebKit Review Bot 2012-08-16 16:52:08 PDT
Created attachment 158944 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 66 WebKit Review Bot 2012-08-16 18:00:04 PDT
Comment on attachment 158908 [details]
Rounding using lrint, handling halfway case, v5.

Attachment 158908 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13511788

New failing tests:
svg/css/circle-in-mask-with-shadow.svg
Comment 67 WebKit Review Bot 2012-08-16 18:00:16 PDT
Created attachment 158961 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 68 WebKit Review Bot 2012-08-17 02:22:13 PDT
Comment on attachment 158908 [details]
Rounding using lrint, handling halfway case, v5.

Rejecting attachment 158908 [details] from commit-queue.

New failing tests:
svg/css/circle-in-mask-with-shadow.svg
Full output: http://queues.webkit.org/results/13514734
Comment 69 WebKit Review Bot 2012-08-17 02:22:22 PDT
Created attachment 159054 [details]
Archive of layout-test-results from gce-cq-04

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: gce-cq-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 70 Dominik Röttsches (drott) 2012-08-17 03:09:29 PDT
Adam, can you decide what to do with this SVG failure. I really can't see a link to my change. I don't see this test failing locally when I apply my patch and run webkit tests for chromium, also checking this image test, any difference that the scripts find is below my perceptive threshold. 
If needed, can you override the "commit-queue-"?
Comment 71 Adam Barth 2012-08-17 10:21:24 PDT
> Adam, can you decide what to do with this SVG failure.

I don't know enough about SVG to make an informed decision.  From your description, it sounds like we should land this patch and then update the expectation.  One way to do that is to add this test to the TestExpectations file and then to let the Chromium WebKit gardener know that you think it should be rebaselined.  (You can ask in #webkit who the Chromium WebKit gardener is.)
Comment 72 Dominik Röttsches (drott) 2012-08-20 01:21:21 PDT
Created attachment 159357 [details]
Rounding using lrint, handling halfway case, v6.
Comment 73 Dominik Röttsches (drott) 2012-08-20 01:27:21 PDT
Created attachment 159361 [details]
Rounding using lrint, handling halfway case, v7.
Comment 74 Dominik Röttsches (drott) 2012-08-20 01:41:16 PDT
Created attachment 159365 [details]
Rounding using lrint, handling halfway case, v8 (monday edition).
Comment 75 WebKit Review Bot 2012-08-20 03:13:03 PDT
Comment on attachment 159365 [details]
Rounding using lrint, handling halfway case, v8 (monday edition).

Attachment 159365 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13542269

New failing tests:
fast/canvas/canvas-ImageData-behaviour.html
platform/chromium/virtual/gpu/canvas/philip/tests/2d.imageData.object.round.html
platform/chromium/virtual/gpu/fast/canvas/canvas-ImageData-behaviour.html
canvas/philip/tests/2d.imageData.object.round.html
Comment 76 WebKit Review Bot 2012-08-20 03:13:12 PDT
Created attachment 159383 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 77 Dominik Röttsches (drott) 2012-08-20 03:35:26 PDT
Created attachment 159387 [details]
Rounding using lrint, handling halfway case, v9.
Comment 78 WebKit Review Bot 2012-08-20 06:25:06 PDT
Comment on attachment 159387 [details]
Rounding using lrint, handling halfway case, v9.

Clearing flags on attachment: 159387

Committed r126023: <http://trac.webkit.org/changeset/126023>
Comment 79 WebKit Review Bot 2012-08-20 06:25:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 80 Patrick R. Gansterer 2012-08-21 07:01:39 PDT
(In reply to comment #78)
> (From update of attachment 159387 [details])
> Clearing flags on attachment: 159387
> 
> Committed r126023: <http://trac.webkit.org/changeset/126023>

1) It broke COMPILER(MSVC) && !CPU(X86): http://trac.webkit.org/changeset/126153
2) There was a "+0.5" at http://trac.webkit.org/browser/trunk/Source/WTF/wtf/Uint8ClampedArray.h?rev=123935#L103, which I do not find at at http://trac.webkit.org/browser/trunk/Source/WTF/wtf/MathExtras.h?rev=126153#L216
3) It added a #pragma message at http://trac.webkit.org/browser/trunk/Source/WTF/wtf/MathExtras.h?rev=126153#L215. This adds many additonal output lines, when compiling. There is no other #pragma message in the code. So please replace it with the usual "// FIXME:".
Comment 81 Kenneth Rohde Christiansen 2012-08-22 11:40:32 PDT
Benjamin suggested that we implement a slow path for lrint so that it is always supported.

Something like the below should work, or at least works for me :-)

#include <math.h>

// Round to the nearest even value.
long lrint(double value)
{
    double rounded = round(value);
    long result = (long)rounded;

    // It the fraction part is exactly 0.5, we need to check whether
    // the rounded result is even, if it is not we need to add 1 to
    // negative values and substract one from positive values.
     if (fabs(result - value) == 0.5 && (result & 1))
         result -= ((result >> 31) | 1); // 1 with the sign of result. ie -1 or 1.
   
     return result;
}

int main() {
   printf("%ld\n", lrint(-7.5));
   printf("%ld\n", lrint(-8.5));
   printf("%ld\n", lrint(7.5));
   printf("%ld\n", lrint(8.5));

   return 0;
}
Comment 82 Benjamin Poulain 2012-08-22 12:32:25 PDT
Comment on attachment 159387 [details]
Rounding using lrint, handling halfway case, v9.

View in context: https://bugs.webkit.org/attachment.cgi?id=159387&action=review

I was planning to revert but people have already updated the tests expectations. Let's try not to create too much mess.
Please update this patch.

> Source/WTF/ChangeLog:11
> +        According to the Uint8ClampedArray spec (http://www.khronos.org/registry/typedarray/specs/latest/#7.1)
> +        which references WebIDL's clamping rules, with implications defined in http://www.w3.org/TR/WebIDL/#es-octet
> +        we need to round to nearest integer, and to the even one if exactly halfway in between.
> +        As a solution: applying C99's lrint which, in default rounding mode, does that.

Out of curiosity, what do the other browsers do?

> Source/WTF/wtf/MathExtras.h:206
> +inline long int lrint(double flt)

IMHO, that should not be inline by default.

If you can prove the inline make a significant difference, then we should have the inline fast version for every compiler and not just MSVC.

> Source/WTF/wtf/MathExtras.h:216
> +#pragma message("Falling back to casting for lrint(), causes rounding inaccuracy in halfway case.")
> +    intgr = static_cast<int>flt;

This should be a proper implementation.

MSVC supports x86_64 and ARM. By cutting corner, you will create subtle bugs for people using those platforms.

> Source/WTF/wtf/Uint8ClampedArray.h:36
> +#if COMPILER(MSVC)
> +#include <wtf/MathExtras.h>
> +#endif

The #if COMPILER(MSVC) should be in MathExtras.h, not here.
Comment 83 Oliver Hunt 2012-08-22 12:43:05 PDT
(In reply to comment #82)
> (From update of attachment 159387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159387&action=review
> 
> I was planning to revert but people have already updated the tests expectations. Let's try not to create too much mess.
> Please update this patch.
> 
> > Source/WTF/ChangeLog:11
> > +        According to the Uint8ClampedArray spec (http://www.khronos.org/registry/typedarray/specs/latest/#7.1)
> > +        which references WebIDL's clamping rules, with implications defined in http://www.w3.org/TR/WebIDL/#es-octet
> > +        we need to round to nearest integer, and to the even one if exactly halfway in between.
> > +        As a solution: applying C99's lrint which, in default rounding mode, does that.
> 
> Out of curiosity, what do the other browsers do?

I agree, I recall that Uint8Clamped is not expected to round, it is expected to truncate.  I believe all of the int-typed arrays are expected to behave in this way.  The correct specification for this should be the TypedArray spec, not WebIDL.

> 
> > Source/WTF/wtf/MathExtras.h:206
> > +inline long int lrint(double flt)
> 
> IMHO, that should not be inline by default.
> 
> If you can prove the inline make a significant difference, then we should have the inline fast version for every compiler and not just MSVC.

The inline keyword does not force inlining, it is in essence a linker directive.  It's necessary if you have a function body in a header but outside of a class definition.
Comment 84 Benjamin Poulain 2012-08-22 12:57:10 PDT
> > IMHO, that should not be inline by default.
> > 
> > If you can prove the inline make a significant difference, then we should have the inline fast version for every compiler and not just MSVC.
> 
> The inline keyword does not force inlining, it is in essence a linker directive.  It's necessary if you have a function body in a header but outside of a class definition.

I know that...

What is wrong with having the implementation in a cpp file unless needed?
Comment 85 Kenneth Russell 2012-08-22 13:04:14 PDT
(In reply to comment #83)
> (In reply to comment #82)
> > (From update of attachment 159387 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=159387&action=review
> > 
> > I was planning to revert but people have already updated the tests expectations. Let's try not to create too much mess.
> > Please update this patch.
> > 
> > > Source/WTF/ChangeLog:11
> > > +        According to the Uint8ClampedArray spec (http://www.khronos.org/registry/typedarray/specs/latest/#7.1)
> > > +        which references WebIDL's clamping rules, with implications defined in http://www.w3.org/TR/WebIDL/#es-octet
> > > +        we need to round to nearest integer, and to the even one if exactly halfway in between.
> > > +        As a solution: applying C99's lrint which, in default rounding mode, does that.
> > 
> > Out of curiosity, what do the other browsers do?
> 
> I agree, I recall that Uint8Clamped is not expected to round, it is expected to truncate.  I believe all of the int-typed arrays are expected to behave in this way.  The correct specification for this should be the TypedArray spec, not WebIDL.

The typed array spec necessarily refers both to the ECMA-262 and Web IDL specifications for its numeric conversion behavior. It would be really problematic if it duplicated all of that specification language.

Looking back, the typed array spec does use truncate, or round-to-zero, conversions per ECMA-262 for *most* floating-point to integer conversions. The outlier is Uint8ClampedArray, which refers to the reasonably new [Clamp] attribute in Web IDL (clearly added to encode the behavior of CanvasPixelArray). The behavior of this attribute is defined here: http://dev.w3.org/2006/webapi/WebIDL/#es-byte .
Comment 86 Dominik Röttsches (drott) 2012-08-23 05:30:36 PDT
(In reply to comment #85)
> (In reply to comment #83)
> > (In reply to comment #82)
> > > (From update of attachment 159387 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=159387&action=review
> > > 
> > > I was planning to revert but people have already updated the tests expectations. Let's try not to create too much mess.
> > > Please update this patch.
> > > 
> > > > Source/WTF/ChangeLog:11
> > > > +        According to the Uint8ClampedArray spec (http://www.khronos.org/registry/typedarray/specs/latest/#7.1)
> > > > +        which references WebIDL's clamping rules, with implications defined in http://www.w3.org/TR/WebIDL/#es-octet
> > > > +        we need to round to nearest integer, and to the even one if exactly halfway in between.
> > > > +        As a solution: applying C99's lrint which, in default rounding mode, does that.
> > > 
> > > Out of curiosity, what do the other browsers do?

Gecko implements the rounding as in WebIDL [Clamp]. So the test passes there.

> > I agree, I recall that Uint8Clamped is not expected to round, it is expected to truncate.  I believe all of the int-typed arrays are expected to behave in this way.  The correct specification for this should be the TypedArray spec, not WebIDL.
> 
> The typed array spec necessarily refers both to the ECMA-262 and Web IDL specifications for its numeric conversion behavior. It would be really problematic if it duplicated all of that specification language.
> 
> Looking back, the typed array spec does use truncate, or round-to-zero, conversions per ECMA-262 for *most* floating-point to integer conversions. The outlier is Uint8ClampedArray, which refers to the reasonably new [Clamp] attribute in Web IDL (clearly added to encode the behavior of CanvasPixelArray). The behavior of this attribute is defined here: http://dev.w3.org/2006/webapi/WebIDL/#es-byte .

I think for Uint8ClampedArray, it's http://dev.w3.org/2006/webapi/WebIDL/#es-octet.

Will update the patch.
Comment 87 Kenneth Rohde Christiansen 2012-08-23 06:32:12 PDT
>      if (fabs(result - value) == 0.5 && (result & 1))
>          result -= ((result >> 31) | 1); // 1 with the sign of result. ie -1 or 1.

I think this should work as well

     if (fabs(result - value) == 0.5 & result)
         result -= ((result >> 30) | 1);
Comment 88 Dominik Röttsches (drott) 2012-08-24 01:36:47 PDT
Created attachment 160355 [details]
Added tests, comments addressed.
Comment 89 WebKit Review Bot 2012-08-24 01:40:12 PDT
Attachment 160355 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ma..." exit_code: 1
Source/WTF/wtf/MathExtras.h:220:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 90 Dominik Röttsches (drott) 2012-08-24 01:41:32 PDT
> If any of these errors are false positives, please file a bug against check-webkit-style.

Filed bug 94913.
Comment 91 Build Bot 2012-08-24 02:06:22 PDT
Comment on attachment 160355 [details]
Added tests, comments addressed.

Attachment 160355 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13596046
Comment 92 Kenneth Rohde Christiansen 2012-08-24 02:09:23 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=160355&action=review

>> Source/WTF/wtf/MathExtras.h:220
>> +    if (fabs(result - value) == 0.5 && (result & 1))
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

the result of == will be an integer, so basically be the & 1 here can be substituted with it, like ... == 0.5 & result.

> Source/WTF/wtf/MathExtras.h:221
> +        result -= ((result >> 31) | 1); // 1 with the sign of result, i.e. -1 or 1.

As it is or'ed with 1 you only need to switch the signed result 30 steps
Comment 93 Dominik Röttsches (drott) 2012-08-24 03:22:32 PDT
Created attachment 160377 [details]
Added tests, condition shortened.
Comment 94 WebKit Review Bot 2012-08-24 03:26:49 PDT
Attachment 160377 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ma..." exit_code: 1
Source/WTF/wtf/MathExtras.h:220:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 95 Dominik Röttsches (drott) 2012-08-24 03:35:03 PDT
Created attachment 160382 [details]
Added tests, condition shortened, put back inline.
Comment 96 WebKit Review Bot 2012-08-24 03:38:52 PDT
Attachment 160382 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ma..." exit_code: 1
Source/WTF/wtf/MathExtras.h:220:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 97 Benjamin Poulain 2012-08-24 13:08:31 PDT
Comment on attachment 160382 [details]
Added tests, condition shortened, put back inline.

View in context: https://bugs.webkit.org/attachment.cgi?id=160382&action=review

Quick comments:

> Source/WTF/wtf/MathExtras.h:208
> -    int intgr;
> +    long int intgr;

This should be int64_t.
The type "long" is no necessarily 64 bits on all platforms.

> Tools/TestWebKitAPI/Tests/WTF/MathExtras.cpp:40
> +    EXPECT_EQ(lrint(-7.5), -8);
> +    EXPECT_EQ(lrint(-8.5), -8);
> +    EXPECT_EQ(lrint(-0.5), 0);
> +    EXPECT_EQ(lrint(0.5), 0);
> +    EXPECT_EQ(lrint(-0.5), 0);
> +    EXPECT_EQ(lrint(1.3), 1);
> +    EXPECT_EQ(lrint(1.7), 2);

You should have test for boundaries:
-Max double with 0.5 precision
-Min double with 0.5 precision
-Max double
-Min Double
etc
Comment 98 Dominik Röttsches (drott) 2012-08-25 10:44:31 PDT
(In reply to comment #97)

Thanks for taking a look, Benjamin. 

> (From update of attachment 160382 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160382&action=review
> 
> Quick comments:
> 
> > Source/WTF/wtf/MathExtras.h:208
> > -    int intgr;
> > +    long int intgr;
> 
> This should be int64_t.
> The type "long" is no necessarily 64 bits on all platforms.

Okay, will update this one.

> > Tools/TestWebKitAPI/Tests/WTF/MathExtras.cpp:40
> > +    EXPECT_EQ(lrint(-7.5), -8);
> > +    EXPECT_EQ(lrint(-8.5), -8);
> > +    EXPECT_EQ(lrint(-0.5), 0);
> > +    EXPECT_EQ(lrint(0.5), 0);
> > +    EXPECT_EQ(lrint(-0.5), 0);
> > +    EXPECT_EQ(lrint(1.3), 1);
> > +    EXPECT_EQ(lrint(1.7), 2);
> 
> You should have test for boundaries:
> -Max double with 0.5 precision
> -Min double with 0.5 precision
> -Max double
> -Min Double
> etc

Could you clarify or give an example what you mean by "with 0.5 precision"?

I understand your point from a testing methodology point of view - not that the function lrint is really used for max and min double in this case, since the UInt8ClampedArray values are clamped before they get passed to lrint().
Comment 99 Benjamin Poulain 2012-08-25 14:25:41 PDT
> > You should have test for boundaries:
> > -Max double with 0.5 precision
> > -Min double with 0.5 precision
> > -Max double
> > -Min Double
> > etc
> 
> Could you clarify or give an example what you mean by "with 0.5 precision"?

The boundaries. The biggest and smallest number that will be take the branch in lrint().

Thinking about boundaries, for double you should also have tests for infinity, -infinity, 0, -0, and NaN.

> I understand your point from a testing methodology point of view - not that the function lrint is really used for max and min double in this case, since the UInt8ClampedArray values are clamped before they get passed to lrint().

The current use of lrint() is pretty much irrelevant. Your addition should be working for any future use.

You replace a standard function in WTF. If your replacement does not behave the same as the original, that will introduce subtle bugs and potentially security issues.
Comment 100 Dominik Röttsches (drott) 2012-08-27 08:59:37 PDT
Created attachment 160730 [details]
Boundaries and zero tests.
Comment 101 WebKit Review Bot 2012-08-27 09:03:45 PDT
Attachment 160730 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ma..." exit_code: 1
Source/WTF/wtf/MathExtras.h:220:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 102 Dominik Röttsches (drott) 2012-08-27 09:07:29 PDT
(In reply to comment #99)
> > > You should have test for boundaries:
> > > -Max double with 0.5 precision
> > > -Min double with 0.5 precision

I added tests for these cases. 2^52-0.5 and -2^52+0.5 are the max/min values with 0.5 precision here.

> > > -Max double
> > > -Min Double

If I read the C99 spec as well as the IEC floating point rounding rules correctly, the results for these are not defined for lrint() since the result would not fit into the destination type.

> Thinking about boundaries, for double you should also have tests for infinity, -infinity, 0, -0, and NaN.

Except for 0, -0 (tests for these added), same here. The result is not defined for +/- infinity, and NaN. An exception should be raised.

I don't know of a platform and architecture independent way of raising the respective floating point exceptions. So I think, as a fallback, this is now as close as we can get.
Comment 103 Peter Beverloo (cr-android ews) 2012-08-27 10:14:52 PDT
Comment on attachment 160730 [details]
Boundaries and zero tests.

Attachment 160730 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13623080
Comment 104 Dominik Röttsches (drott) 2012-08-27 10:59:33 PDT
Created attachment 160747 [details]
Boundaries and zero tests, Cr Android EWS build fix.
Comment 105 WebKit Review Bot 2012-08-27 11:02:37 PDT
Attachment 160747 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ma..." exit_code: 1
Source/WTF/wtf/MathExtras.h:220:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 106 Benjamin Poulain 2012-08-27 18:22:54 PDT
Comment on attachment 160747 [details]
Boundaries and zero tests, Cr Android EWS build fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=160747&action=review

You addressed the spec above + extended the tests.

> Source/WTF/wtf/MathExtras.h:221
> +        intgr -= ((intgr >> 30) | 1); // 1 with the sign of result, i.e. -1 or 1.

The ">>30" seems incorrect here.
I would think the tests would have catch that. Did you run the test with this branch or only with the inline asm?
Comment 107 Benjamin Poulain 2012-08-27 18:24:19 PDT
> > > > -Max double
> > > > -Min Double
> 
> If I read the C99 spec as well as the IEC floating point rounding rules correctly, the results for these are not defined for lrint() since the result would not fit into the destination type.
> 
> > Thinking about boundaries, for double you should also have tests for infinity, -infinity, 0, -0, and NaN.
> 
> Except for 0, -0 (tests for these added), same here. The result is not defined for +/- infinity, and NaN. An exception should be raised.

Ok, I think it is fair to ignore this for now. But please add assertions for them in your lrint().
Comment 108 Dominik Röttsches (drott) 2012-08-28 00:55:32 PDT
Created attachment 160924 [details]
Boundaries and zero tests, Assertion added.
Comment 109 WebKit Review Bot 2012-08-28 00:58:22 PDT
Attachment 160924 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ma..." exit_code: 1
Source/WTF/wtf/MathExtras.h:221:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 110 Dominik Röttsches (drott) 2012-08-28 00:59:32 PDT
(In reply to comment #106)
> (From update of attachment 160747 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160747&action=review

> > Source/WTF/wtf/MathExtras.h:221
> > +        intgr -= ((intgr >> 30) | 1); // 1 with the sign of result, i.e. -1 or 1.
> 
> The ">>30" seems incorrect here.
> I would think the tests would have catch that. Did you run the test with this branch or only with the inline asm?

I did run them with both codepaths. The original boundary test doesn't catch the wrong shift - I added one that catches this particular case, and fixed the shift to 62.

(In reply to comment #107)
> > Except for 0, -0 (tests for these added), same here. The result is not defined for +/- infinity, and NaN. An exception should be raised.
> 
> Ok, I think it is fair to ignore this for now. But please add assertions for them in your lrint().

Added an assertion.
Comment 111 Benjamin Poulain 2012-08-28 22:37:50 PDT
Comment on attachment 160924 [details]
Boundaries and zero tests, Assertion added.

View in context: https://bugs.webkit.org/attachment.cgi?id=160924&action=review

This patch looks correct and well tested, r+.

Just a tiny change in TestWebKitAPI.xcodeproj to make the merges easier:

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:987
> +				B4039F9D15E6D8B3007255D6 /* MathExtras.cpp in Sources */,

This is the build section of the xcodeproject file. Please keep this region sorted (you can edit that part as text).
Comment 112 Dominik Röttsches (drott) 2012-08-29 04:59:26 PDT
Created attachment 161188 [details]
Final version, xcodeproj sort.
Comment 113 Dominik Röttsches (drott) 2012-08-29 05:00:23 PDT
Comment on attachment 161188 [details]
Final version, xcodeproj sort.

Thanks for your thorough review, Benjamin.
Comment 114 WebKit Review Bot 2012-08-29 08:32:46 PDT
Comment on attachment 161188 [details]
Final version, xcodeproj sort.

Clearing flags on attachment: 161188

Committed r127001: <http://trac.webkit.org/changeset/127001>
Comment 115 WebKit Review Bot 2012-08-29 08:32:57 PDT
All reviewed patches have been landed.  Closing bug.