Bug 40272 – The 2d.imageData.object.round canvas test is failing

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Jan Erik Hanssen]

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

[Helder Correia]

Created attachment 62028 [details]
Proposed patch

[Darin Adler]

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.

[Andreas Kling]

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.

[Andreas Kling]

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.

[Helder Correia]

> 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.

[Helder Correia]

Created attachment 62139 [details]
Proposed patch v2

[Helder Correia]

(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.

[Darin Adler]

(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?

[Darin Adler]

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.

[Helder Correia]

(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).

[Darin Adler]

(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

[Helder Correia]

(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.

[Darin Adler]

(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?

[Darin Adler]

(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.

[Darin Adler]

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.

[Helder Correia]

(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++.

[Helder Correia]

Created attachment 62483 [details]
Proposed patch v3

Added regression test and comment about long long.

[WebKit Review Bot]

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.

[WebKit Commit Bot]

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

[Helder Correia]

(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.

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

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

[Helder Correia]

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

[Darin Adler]

(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?

[Helder Correia]

(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".

[Andreas Kling]

Committed r64156: <http://trac.webkit.org/changeset/64156>

[Oliver Hunt]

Those tests were wrong, this patch regressed behaviour.

This patch should be rolled out.

[Darin Adler]

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”.

[Oliver Hunt]

(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.

[Eric Seidel (no email)]

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.

[Oliver Hunt]

This was not rolled out, why was this closed?

I just spotted another piece of code that was broken because of this.

[Oliver Hunt]

Rolled out in r79501

Retitled to reflect that rounding may need to be addressed.  Clamping behaviour is now correct again.

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

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.

[Philippe Normand]

Skipped those 2 tests in GTK as well. See http://trac.webkit.org/changeset/79528

[Oliver Hunt]

(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 :-(

[Dominik Röttsches (drott)]

Helder, do you think you could give this a refresh? Thanks.

[Dominik Röttsches (drott)]

(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.

[Dominik Röttsches (drott)]

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

[Dominik Röttsches (drott)]

Tracking the wrap case in bug 94089.

[Dominik Röttsches (drott)]

Created attachment 158538 [details]
Rounding using lrint, handling halfway case, v1.

[Kenneth Rohde Christiansen]

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*

[Build Bot]

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

[Dominik Röttsches (drott)]

(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?

[Dominik Röttsches (drott)]

I'll try an assembly implementation for X86, falling back to casting for non X86 on MSVC.

[Dominik Röttsches (drott)]

Created attachment 158553 [details]
Rounding using lrint, handling halfway case, v2.

[Kenneth Rohde Christiansen]

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?

[Dominik Röttsches (drott)]

Created attachment 158557 [details]
Rounding using lrint, handling halfway case, v3.

[Dominik Röttsches (drott)]

(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.

[Kenneth Rohde Christiansen]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

Chromium issue: crbug.com/115240

[Dominik Röttsches (drott)]

Would the Chromium people be okay if we land the WebKit side and mark these tests failing for Chromium for now?

[Adam Barth]

> 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?

[Dominik Röttsches (drott)]

Created attachment 158891 [details]
Rounding using lrint, handling halfway case, v4.

[Dominik Röttsches (drott)]

(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.

[Dominik Röttsches (drott)]

Created attachment 158908 [details]
Rounding using lrint, handling halfway case, v5.

[Kenneth Russell]

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 .

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Dominik Röttsches (drott)]

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-"?

[Adam Barth]

> 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.)

[Dominik Röttsches (drott)]

Created attachment 159357 [details]
Rounding using lrint, handling halfway case, v6.

[Dominik Röttsches (drott)]

Created attachment 159361 [details]
Rounding using lrint, handling halfway case, v7.

[Dominik Röttsches (drott)]

Created attachment 159365 [details]
Rounding using lrint, handling halfway case, v8 (monday edition).

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Dominik Röttsches (drott)]

Created attachment 159387 [details]
Rounding using lrint, handling halfway case, v9.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Patrick R. Gansterer]

(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:".

[Kenneth Rohde Christiansen]

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;
}

[Benjamin Poulain]

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.

[Oliver Hunt]

(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.

[Benjamin Poulain]

> > 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?

[Kenneth Russell]

(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 .

[Dominik Röttsches (drott)]

(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.

[Kenneth Rohde Christiansen]

>      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);

[Dominik Röttsches (drott)]

Created attachment 160355 [details]
Added tests, comments addressed.

[WebKit Review Bot]

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.

[Dominik Röttsches (drott)]

> If any of these errors are false positives, please file a bug against check-webkit-style.

Filed bug 94913.

[Build Bot]

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

[Kenneth Rohde Christiansen]

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

[Dominik Röttsches (drott)]

Created attachment 160377 [details]
Added tests, condition shortened.

[WebKit Review Bot]

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.

[Dominik Röttsches (drott)]

Created attachment 160382 [details]
Added tests, condition shortened, put back inline.

[WebKit Review Bot]

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.

[Benjamin Poulain]

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

[Dominik Röttsches (drott)]

(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().

[Benjamin Poulain]

> > 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.

[Dominik Röttsches (drott)]

Created attachment 160730 [details]
Boundaries and zero tests.

[WebKit Review Bot]

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.

[Dominik Röttsches (drott)]

(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.

[Peter Beverloo (cr-android ews)]

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

[Dominik Röttsches (drott)]

Created attachment 160747 [details]
Boundaries and zero tests, Cr Android EWS build fix.

[WebKit Review Bot]

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.

[Benjamin Poulain]

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?

[Benjamin Poulain]

> > > > -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().

[Dominik Röttsches (drott)]

Created attachment 160924 [details]
Boundaries and zero tests, Assertion added.

[WebKit Review Bot]

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.

[Dominik Röttsches (drott)]

(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.

[Benjamin Poulain]

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).

[Dominik Röttsches (drott)]

Created attachment 161188 [details]
Final version, xcodeproj sort.

[Dominik Röttsches (drott)]

Comment on attachment 161188 [details]
Final version, xcodeproj sort.

Thanks for your thorough review, Benjamin.

[WebKit Review Bot]

Comment on attachment 161188 [details]
Final version, xcodeproj sort.

Clearing flags on attachment: 161188

Committed r127001: <http://trac.webkit.org/changeset/127001>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.