1216396 - Remove gfxSize

Status: NEW

(Core :: Graphics, defect)

Description

[Nicholas Nethercote [inactive]]

As part of Moz2Dification, gfxSize should be replaced with gfx::Size. The former has double-precision FP values and the latter has single-precision. Other than that, they're very similar, both being subclasses of gfx::BaseSize.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Make gfxSize use single-precision floats instead of double-precision floats

This is the part that has the potential to cause noticeable behavioural
changes.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Make gfxSize an alias for gfx::Size

The most notable consequence of this is that we no longer have an implicit
constructor for gfxSize that takes a gfx::IntSize. Instead the new explicit
ToFloat() converter can be used.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove the gfxSize typedef

This allows ToSize() and ThebesSize() to be removed as well.

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8676005 [details] [diff] [review]
(part 2) - Make gfxSize an alias for gfx::Size

Review of attachment 8676005 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Point.h
@@ +222,5 @@
>                               int32_t(floorf(aSize.height + 0.5f)));
>  }
>  
> +template<class units>
> +SizeTyped<units> ToFloat(const IntSizeTyped<units>& aSize) {

I'm not sure I'm thrilled about this just being called 'ToFloat'.. but it seems to be consistent with what other things in this file do, so oh well.

Comment 5

[Nicholas Nethercote [inactive]]

Part 1 (changing gfxSize from double-precision to single-precision) is causing some reftest failures on Windows. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=f36289f926c4 for examples.

dholbert, heycam suggested I ask you about the layout/reftests/svg/sizing ones. They already use |fuzzy-if(d2d,1,400)|. Would it be reasonable to change the 400 to 979?

Bas, the layout/reftests/writing-mode/tables ones are slightly stranger. There's a strip along the right-hand side of the green box that is a very light grey instead of white. Oddly enough, it's not 100% reproducible -- sometimes these tests pass. Any suggestions here?

Comment 6

[Nicholas Nethercote [inactive]]

(See comment 5.)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> dholbert, heycam suggested I ask you about the layout/reftests/svg/sizing
> ones. They already use |fuzzy-if(d2d,1,400)|. Would it be reasonable to
> change the 400 to 979?

Sounds good to me. (not sure what the "400" corresponds to; I'm assuming it's just one of the edges was oh-so-slightly fuzzy. Now it's 2 edges, based on your Try run. Not a huge deal, with a difference of 1 in the color channel.)

> Bas, the layout/reftests/writing-mode/tables ones are slightly stranger.
> There's a strip along the right-hand side of the green box that is a very
> light grey instead of white. Oddly enough, it's not 100% reproducible --
> sometimes these tests pass. Any suggestions here?

It's a light green color, actually -- rgb(239,247,239) -- not gray.

This seems reasonable to mark as fuzzy-if to me, too, FWIW.  I'm assuming at some step in the pipeline we're creating a green surface that's *slightly* bigger than it's supposed to be (due to decreased float precision from your patch), and when we composite the green surface onto the background, we get a teensy bit of green being composited over the white background for this particular column of pixels.  So we end up with an weighted average of dark-green surface -- rgb(0,128,0) -- and white background -- rgb(255,255,255) -- which produces this greenish near-white color, rgb(239,247,239).

So, I'm happy to sign off on marking both groups of tests as fuzzy-if(d2d) with appropriate numeric values.

Comment 8

[Daniel Holbert [:dholbert]]

Just noticed that it's really the *reference* that's rendering incorrectly, on the writing-mode/tables failures. And the reference is just an <img> with an explicit 100px width and 200px height.  It's a little concerning that we're letting that bleed outside its borders a bit...

I'll bet part of the issue there is that the image is 15x15, and scaling that up to 100px wide is non-exact (100/15 = 6.66666 repeating). That scaling operation may be the source of the float-rounding issue.

Anyway, absent any concerns from Bas, I think this still seems like the type of scenario where we've been fine with conditional fuzzy-if() annotations in the past.  (Not human-noticeable really, at edges of things that are composited together, with some believable reason for the rounding error.)

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> Part 1 (changing gfxSize from double-precision to single-precision) is
> causing some reftest failures on Windows. See
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f36289f926c4 for
> examples.
> 
> dholbert, heycam suggested I ask you about the layout/reftests/svg/sizing
> ones. They already use |fuzzy-if(d2d,1,400)|. Would it be reasonable to
> change the 400 to 979?
> 
> Bas, the layout/reftests/writing-mode/tables ones are slightly stranger.
> There's a strip along the right-hand side of the green box that is a very
> light grey instead of white. Oddly enough, it's not 100% reproducible --
> sometimes these tests pass. Any suggestions here?

It's not very light grey, it's exactly 1/16th coverage with dark green we're seeing here. I've seen D2D have bugs before where it resulted in 1/16th coverage errors (this seems to be the precision within which D2D does coverage of rectangular paths. Note also that as Daniel already remarked, this is in the -reference- and not the test.

I suspect Daniel is right about the cause. I'm somewhat surprised we don't clip to the bounds of the image though. Might be good to ask Seth for his opinion.

Comment 10

[Daniel Holbert [:dholbert]]

(Side note: I bet we would render the s72-border-spacing reference cases correctly if the pref "image.single-color-optimization.enabled" were turned on when it ran. That pref is on by default for our users, but it's disabled in reftests, for predictability/consistency[1]. It makes us take a special-case for solid-color images, to just directly fill the target region with the color, IIRC, which seems like it should avoid this issue if it's scaling/rounding-related.)

[1] http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-preferences.js?rev=2b6a3ec30a14#28

Comment 11

[Daniel Holbert [:dholbert]]

(To be clear, I'm not suggesting we should enable that pref for this reftest; I'm saying we shouldn't hit this issue in the real world, at least for stretched solid-color images like this.)

Comment 12

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> (To be clear, I'm not suggesting we should enable that pref for this
> reftest; I'm saying we shouldn't hit this issue in the real world, at least
> for stretched solid-color images like this.)

Yeah, but we'd hit it for any non-solid-color image, and those are the common case. That's exactly why I turned the pref off for reftests - we want to catch these issues.

(In reply to Bas Schouten (:bas.schouten) from comment #9)
> I suspect Daniel is right about the cause. I'm somewhat surprised we don't
> clip to the bounds of the image though. Might be good to ask Seth for his
> opinion.

We don't normally clip when drawing images, at least at the level of DrawImageInternal in nsLayoutUtils. Instead we try to snap to integer pixels in ComputeSnappedImageDrawingParameters. Given the topic of this bug, I'd guess that some intermediate computation in that function no longer has enough precision once this change has been made. It might be a good idea to update that code to use doubles explicitly instead of implicitly via gfxSize. My experience with these kinds of issues suggests to me that people *will* hit this if we regress it.

Comment 13

[Nicholas Nethercote [inactive]]

> We don't normally clip when drawing images, at least at the level of
> DrawImageInternal in nsLayoutUtils. Instead we try to snap to integer pixels
> in ComputeSnappedImageDrawingParameters. Given the topic of this bug, I'd
> guess that some intermediate computation in that function no longer has
> enough precision once this change has been made. It might be a good idea to
> update that code to use doubles explicitly instead of implicitly via
> gfxSize. My experience with these kinds of issues suggests to me that people
> *will* hit this if we regress it.

I took a stab at fixing this here but it didn't work:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6be05e9a80c

I also tried fiddling with layout.css.devPixelsPerPx to try and reproduce on my Windows machine, but failed.

So now I'm stuck.

Comment 14

[Nicholas Nethercote [inactive]]

I'm giving up on this. There doesn't seem to be a good way to safely mass-converting double-precision gfx types to single-precision.