Fix rounding of phase for background image

Fix rounding of phase for background image

In the BackgroundImageGeometry::calculate methods we were previously
computing the full image tiling phase value and then rounding to int
at the end. But frequently the phase is computed as the negative of a
background-position value, which is itself frequently negative. The
result of negating a value then rounding is not the same as rounding
then negating, and this difference matters when trying to, for
instance, match the end of a linear gradient with a solid background
color.

This patch changes phase rounding to round before negating. The test
case is fixed, and no other test results change.

R=pdr@chromium.org,fmalita@chromium.org
BUG=622294
Committed: https://crrev.com/689f41251eab6e3812c09296a14cb1edc6db3b0e
Cr-Commit-Position: refs/heads/master@{#403458}

[Stephen Chennney, 2016-06-27 20:36:10]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[Stephen Chennney, 2016-06-27 20:36:50]

Thankfully, this was an easy one to fix.

[commit-bot: I haz the power, 2016-06-27 20:36:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2016-06-27 21:10:41]

On 2016/06/27 20:36:50, Stephen Chennney wrote:
> The result of negating a value then rounding is
> not the same as rounding then negating

Hmm, I thought round/roundf behave in this case - is this something specific to
LayoutUnit?

[pdr., 2016-06-27 21:46:07]

I spent a couple days last week fighting with pixel snapping so I'd like to
understand this change too, to make sure I can make it work in spv2.

"The result of negating a value then rounding is not the same as rounding then
negating"
Is this true, or is it more about the min(value, 0) part?

It seems wrong to round the xOffset before calling setX, but not when calling
setWidth. I'd expect those to both be rounded or both not be.

In this change you've rounded before calling setX. Can you remove the rounding
that used to occur after calling setX?

[commit-bot: I haz the power, 2016-06-27 22:07:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-27 22:07:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[f(malita), 2016-06-27 22:10:56]

On 2016/06/27 21:10:41, f(malita) wrote:
> On 2016/06/27 20:36:50, Stephen Chennney wrote:
> > The result of negating a value then rounding is
> > not the same as rounding then negating
> 
> Hmm, I thought round/roundf behave in this case - is this something specific
to
> LayoutUnit?

To answer my own question, yup, this seems to be a problem with LayoutUnit
rounding of halfway values in particular: unlike round/roundf,
LayoutUnit::round() doesn't round halfway negative values away from zero.

So

  roundf(37.5) == 38 && roundf(-37.5) == -38

but

  LayoutUnit(37.5).round() == 38 && LayoutUnit(-37.5).round() == -37

ugh :(

Also, have you tested the fix against the original bug report?  I can still
repro, so I suspect there's more going on there.

[Stephen Chennney, 2016-06-28 13:11:34]

On 2016/06/27 22:10:56, f(malita) wrote:
> On 2016/06/27 21:10:41, f(malita) wrote:
> > On 2016/06/27 20:36:50, Stephen Chennney wrote:
> > > The result of negating a value then rounding is
> > > not the same as rounding then negating
> > 
> > Hmm, I thought round/roundf behave in this case - is this something specific
> to
> > LayoutUnit?
> 
> To answer my own question, yup, this seems to be a problem with LayoutUnit
> rounding of halfway values in particular: unlike round/roundf,
> LayoutUnit::round() doesn't round halfway negative values away from zero.
> 
> So
> 
>   roundf(37.5) == 38 && roundf(-37.5) == -38
> 
> but
> 
>   LayoutUnit(37.5).round() == 38 && LayoutUnit(-37.5).round() == -37
> 
> ugh :(
> 
> Also, have you tested the fix against the original bug report?  I can still
> repro, so I suspect there's more going on there.

Yeah, the original bug still repros. Odd that the isolated test case doesn't.
Back to the drawing board.

[Stephen Chennney, 2016-06-28 13:15:11]

On 2016/06/27 21:46:07, pdr. wrote:
> I spent a couple days last week fighting with pixel snapping so I'd like to
> understand this change too, to make sure I can make it work in spv2.
> 
> "The result of negating a value then rounding is not the same as rounding then
> negating"
> Is this true, or is it more about the min(value, 0) part?

As fmalita explains, it's a problem with LayoutUnit.

> It seems wrong to round the xOffset before calling setX, but not when calling
> setWidth. I'd expect those to both be rounded or both not be.

The width is always positive, so we don't get into these issues. All the values
get rounded at some point, we just round them at different times depending on
when we need rounded or unrounded for the computation of other values. It's a
mess, and can probably do with an entire rewrite, but I'm not sure hat
ultimately we can avoid the complexity.
> 
> In this change you've rounded before calling setX. Can you remove the rounding
> that used to occur after calling setX?

The values only get rounded once. Not sure what you mean here.

[f(malita), 2016-06-28 16:46:30]

On 2016/06/28 13:11:34, Stephen Chennney wrote:
> On 2016/06/27 22:10:56, f(malita) wrote:
> > On 2016/06/27 21:10:41, f(malita) wrote:
> > > On 2016/06/27 20:36:50, Stephen Chennney wrote:
> > > > The result of negating a value then rounding is
> > > > not the same as rounding then negating
> > > 
> > > Hmm, I thought round/roundf behave in this case - is this something
specific
> > to
> > > LayoutUnit?
> > 
> > To answer my own question, yup, this seems to be a problem with LayoutUnit
> > rounding of halfway values in particular: unlike round/roundf,
> > LayoutUnit::round() doesn't round halfway negative values away from zero.
> > 
> > So
> > 
> >   roundf(37.5) == 38 && roundf(-37.5) == -38
> > 
> > but
> > 
> >   LayoutUnit(37.5).round() == 38 && LayoutUnit(-37.5).round() == -37
> > 
> > ugh :(
> > 
> > Also, have you tested the fix against the original bug report?  I can still
> > repro, so I suspect there's more going on there.
> 
> Yeah, the original bug still repros. Odd that the isolated test case doesn't.
> Back to the drawing board.

So I've been poking at the CL test case, and I think I finally grok what's going
on:

* the gradient image (64x74 scaled) is shifted up by -15px * 2.5 == -37.5
* so the top edge is at -37.5 and the bottom edge is at 36.5
* the phase is also -37.5 (== top edge)
* these are values in dest space, and if we were to round them using LayoutUnit
rules we get

  top/phase: -37
  bottom: 37

  ^^ looks good, they all snap "down", the height is still 74 so we only need to
draw one tile

* but the GC code downstream expects the phase to be in src space, so we need to
flip its sign
* if we round now, we get

  top: -37
  bottom: 37
  phase: 38

  ^^ edges still look good, but the phase snapped "up" => which means we're
pulling the gradient up one-too-many pixels => the bottom row wraps around and
draws the top color (we draw more than one tile)

I think the root cause is that we switch the phase coordinate system too early,
before rounding.  For consistency, we need to round all values in the same
coordinate space (dest), and only convert the phase to src space at the very
end.

Which basically boils down to rounding before negating the phase - so yeah, the
CL seems correct for this test :) (not sure what else is going on with the bug)

But rather than rounding explicitly whenever we call setPhaseX/Y, can you try
the following:

* leave the phase in dest space (invert the sign at all setPhaseXY call sites)
* switch to src space at the very end of calculate() (invert the phase sign
after rounding)

If this works, I think it'd look cleaner.

[f(malita), 2016-06-28 17:17:11]

On 2016/06/28 13:15:11, Stephen Chennney wrote:
> On 2016/06/27 21:46:07, pdr. wrote:
> > I spent a couple days last week fighting with pixel snapping so I'd like to
> > understand this change too, to make sure I can make it work in spv2.
> > 
> > "The result of negating a value then rounding is not the same as rounding
then
> > negating"
> > Is this true, or is it more about the min(value, 0) part?
> 
> As fmalita explains, it's a problem with LayoutUnit.

I am now convinced that LayoutUnit rounding semantics are desirable: having a
consistent bias for positive/negative half-values ensures they all snap in the
same direction (and the size doesn't magically grow by one when edges have
different signs, for example).  Skia also uses an equivalent approach for
SkScalar rounding: #define sk_float_round2int(x)   (int)sk_float_floor((x) +
0.5f)

I guess this means we should be careful with round/roundf in Blink, and possibly
use floorf(x + .5f) instead.

[Stephen Chennney, 2016-06-28 20:32:45]

On 2016/06/28 17:17:11, f(malita) wrote:
> On 2016/06/28 13:15:11, Stephen Chennney wrote:
> > On 2016/06/27 21:46:07, pdr. wrote:
> > > I spent a couple days last week fighting with pixel snapping so I'd like
to
> > > understand this change too, to make sure I can make it work in spv2.
> > > 
> > > "The result of negating a value then rounding is not the same as rounding
> then
> > > negating"
> > > Is this true, or is it more about the min(value, 0) part?
> > 
> > As fmalita explains, it's a problem with LayoutUnit.
> 
> I am now convinced that LayoutUnit rounding semantics are desirable: having a
> consistent bias for positive/negative half-values ensures they all snap in the
> same direction (and the size doesn't magically grow by one when edges have
> different signs, for example).  Skia also uses an equivalent approach for
> SkScalar rounding: #define sk_float_round2int(x)   (int)sk_float_floor((x) +
> 0.5f)
> 
> I guess this means we should be careful with round/roundf in Blink, and
possibly
> use floorf(x + .5f) instead.

The original bug report page can be reproduced with this, and it does still
fail:

      .background1 {
      	background-color: rgb(0,68,204);
      	background-image: linear-gradient(rgb(0,136,204), rgb(0,68,204));
      	background-position: 0px -18.75px;
      	background-repeat: repeat no-repeat;
      	width: 50px;
      	height: 35px;
        position: fixed;
        top: 10.25px;
        left: 10px;
      }

Debugging now.

[Stephen Chennney, 2016-06-29 20:26:23]

This resolves the bug.

I verified that the original bug report for the
background-with-sub-pixel-offset-positioning.html test is still correct with my
change. The test was nonsensical.

The ietestcenter test was wrong before and is wrong now. I think to get the
right result we need non-integer tile space, which I'm thinking of adding
anyway.

See inline comments for more info.

https://codereview.chromium.org/2103563002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/backgrounds/background-with-sub-pixel-offset-positioning.html
(left):

https://codereview.chromium.org/2103563002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/backgrounds/background-with-sub-pixel-offset-positioning.html:4:
background: url('resources/bgimg1x50.png') 100% 0px no-repeat #FFF;
This test was wrong. With 100% position, the entire image should be outside the
div and not visible. But previous rounding behavior magically made it work, I
think by rounding down the dest rect and position so that something rendered
inside the div.

https://codereview.chromium.org/2103563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/2103563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:164:
m_destRect.move(std::max(roundedOffset, 0), LayoutUnit());
If we round the dest rect differently to the phase we can end up with a dest
rect that is bigger than the tile and re-hit the bug. So whatever we do to the
phase we also need to do it to the dest rect.

[Stephen Chennney, 2016-06-29 20:27:15]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[Stephen Chennney, 2016-06-29 20:27:51]

On 2016/06/29 20:26:23, Stephen Chennney wrote:
> This resolves the bug.
> 
> I verified that the original bug report for the
> background-with-sub-pixel-offset-positioning.html test is still correct with
my
> change. The test was nonsensical.

That's https://bugs.chromium.org/p/chromium/issues/detail?id=570611

[commit-bot: I haz the power, 2016-06-29 20:28:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 20:37:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 20:37:38]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Stephen Chennney, 2016-06-29 20:58:39]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 20:59:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 22:35:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 22:35:22]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-06-30 06:01:02]

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:251: m_phase =
LayoutPoint(roundedIntPoint(m_phase));
Can m_phase be refactored to be an IntPoint?

[f(malita), 2016-06-30 15:03:05]

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:165:
m_phase.setX(LayoutUnit(-std::min(roundedOffset, 0)));
Nit: use setDestRect()/setPhaseX() for consistency?

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:174:
m_phase.setY(LayoutUnit(-std::min(roundedOffset, 0)));
Ditto (setDestRect/setPhaseY).

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:193:
setPhaseX(LayoutUnit(round(fractionalPositionWithinTile * tileSize().width())));
Nit: (here and throughout) roundf

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:236:
setPhaseX(actualWidth ? LayoutUnit(round(actualWidth - fmodf((computedXPosition
+ extraOffset), actualWidth))) : LayoutUnit());
Sice round/roundf has a different bias vs. LayoutUnit::round() for negative
half-values, I'm wary of mixing the two.  But I guess it's provable that the arg
is positive in all these cases?

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:424:
setDestRect(LayoutRect(pixelSnappedIntRect(m_destRect)));
So we round the dest rect offset, then we round the dest rect, then we intersect
and round the dest rect again.  Methinks we round too much :)

I think all this logic needs to be rethought at some point - perform all
computations in layout units, in a common coordinate space, and only snap at the
end?

Also possible regression for background-repeat-space-padding-box-expected (moar
red with the patch), but it's minor and since the change fixes some obvious bugs
I'm fine with it.

[Stephen Chennney, 2016-06-30 18:50:56]

See inline comments. I'll be looking at fixing the bad ietestcenter result next
(likely https://bugs.chromium.org/p/chromium/issues/detail?id=449600)

Patch up very shortly.

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:193:
setPhaseX(LayoutUnit(round(fractionalPositionWithinTile * tileSize().width())));
On 2016/06/30 15:03:05, f(malita) wrote:
> Nit: (here and throughout) roundf

Done.

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:236:
setPhaseX(actualWidth ? LayoutUnit(round(actualWidth - fmodf((computedXPosition
+ extraOffset), actualWidth))) : LayoutUnit());
On 2016/06/30 15:03:05, f(malita) wrote:
> Sice round/roundf has a different bias vs. LayoutUnit::round() for negative
> half-values, I'm wary of mixing the two.  But I guess it's provable that the
arg
> is positive in all these cases?

Rounding with LayoutUnit will cause all sorts of type cast issues. I think we're
dealing with LayoutUnits and floats in this line. It would take
LayoutUnit(rountToInt(LayoutUnit(float thing))). It can't be negative though,
because fmodf(blah, actualWidth) < actualWidth.

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:251: m_phase =
LayoutPoint(roundedIntPoint(m_phase));
On 2016/06/30 06:01:02, pdr. wrote:
> Can m_phase be refactored to be an IntPoint?

Yes, and so can lots of other things. But it's a big change that should be done
separately to avoid obfuscating the actual change here.

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:424:
setDestRect(LayoutRect(pixelSnappedIntRect(m_destRect)));
On 2016/06/30 15:03:05, f(malita) wrote:
> So we round the dest rect offset, then we round the dest rect, then we
intersect
> and round the dest rect again.  Methinks we round too much :)
> 
> I think all this logic needs to be rethought at some point - perform all
> computations in layout units, in a common coordinate space, and only snap at
the
> end?

That's the way snapping was originally done before I got into this, and it
causes a whole set of other problems. Basically the changes have been toward
snapping as early as possible so that subsequent calculations are done with the
input values that will actually be used when painting, allowing for the most
accurate sizing and rounding for what appears on the screen.

Areas for improving are cases where downstream code can do a better job rounding
some things, in particular the space between images, to avoid nasty
discretization issues.

[Stephen Chennney, 2016-06-30 18:53:57]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 18:54:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2016-06-30 19:01:54]

LGTM

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:236:
setPhaseX(actualWidth ? LayoutUnit(round(actualWidth - fmodf((computedXPosition
+ extraOffset), actualWidth))) : LayoutUnit());
On 2016/06/30 18:50:55, Stephen Chennney wrote:
> On 2016/06/30 15:03:05, f(malita) wrote:
> > Sice round/roundf has a different bias vs. LayoutUnit::round() for negative
> > half-values, I'm wary of mixing the two.  But I guess it's provable that the
> arg
> > is positive in all these cases?
> 
> Rounding with LayoutUnit will cause all sorts of type cast issues. I think
we're
> dealing with LayoutUnits and floats in this line. It would take
> LayoutUnit(rountToInt(LayoutUnit(float thing))). It can't be negative though,
> because fmodf(blah, actualWidth) < actualWidth.

I'm mostly worried about inconsistencies vs. other code paths - say phase
rounding - which use LayoutUnit rounding rules.  But yeah, I think these are all
safe because the args are positive.

(if we had to, floorf(x + 0.5f) should give us equivalent results without
LayoutUnit gymnastics)

[pdr., 2016-06-30 20:14:36]

LGTM

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/2103563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp:251: m_phase =
LayoutPoint(roundedIntPoint(m_phase));
On 2016/06/30 at 18:50:55, Stephen Chennney wrote:
> On 2016/06/30 06:01:02, pdr. wrote:
> > Can m_phase be refactored to be an IntPoint?
> 
> Yes, and so can lots of other things. But it's a big change that should be
done separately to avoid obfuscating the actual change here.

SG, can you add a TODO above m_phase for this (and any other useful refactorings
this patch makes possible)?

[commit-bot: I haz the power, 2016-06-30 20:31:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-30 20:31:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Stephen Chennney, 2016-07-01 13:14:07]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2016-07-01 13:14:08]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2103563002/#ps80001
(title: "Add todo")

[commit-bot: I haz the power, 2016-07-01 13:14:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stephen Chennney, 2016-07-01 14:13:52]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2016-07-01 14:13:53]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2103563002/#ps100001
(title: "Re-add expectation.")

[commit-bot: I haz the power, 2016-07-01 14:14:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 15:34:31]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-01 15:34:41]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-01 15:36:09]

Description was changed from

==========
Fix rounding of phase for background image

In the BackgroundImageGeometry::calculate methods we were previously
computing the full image tiling phase value and then rounding to int
at the end. But frequently the phase is computed as the negative of a
background-position value, which is itself frequently negative. The
result of negating a value then rounding is not the same as rounding
then negating, and this difference matters when trying to, for
instance, match the end of a linear gradient with a solid background
color.

This patch changes phase rounding to round before negating. The test
case is fixed, and no other test results change.

R=pdr@chromium.org,fmalita@chromium.org
BUG=622294
==========

to

==========
Fix rounding of phase for background image

In the BackgroundImageGeometry::calculate methods we were previously
computing the full image tiling phase value and then rounding to int
at the end. But frequently the phase is computed as the negative of a
background-position value, which is itself frequently negative. The
result of negating a value then rounding is not the same as rounding
then negating, and this difference matters when trying to, for
instance, match the end of a linear gradient with a solid background
color.

This patch changes phase rounding to round before negating. The test
case is fixed, and no other test results change.

R=pdr@chromium.org,fmalita@chromium.org
BUG=622294

Committed: https://crrev.com/689f41251eab6e3812c09296a14cb1edc6db3b0e
Cr-Commit-Position: refs/heads/master@{#403458}
==========

[commit-bot: I haz the power, 2016-07-01 15:36:10]

Patchset 6 (id:??) landed as
https://crrev.com/689f41251eab6e3812c09296a14cb1edc6db3b0e
Cr-Commit-Position: refs/heads/master@{#403458}