STR: 1. View a twitter page with a small Emoji smiley, like: https://twitter.com/saredelman (has a smiley in the bio text at the top) ACTUAL RESULS: The smiley is visibly cropped on the right & the bottom. EXPECTED RESULTS: No cropping. Chrome gives EXPECTED RESULTS. So does a nightly Firefox from 2013-01-01. But current firefox release & nightly yield ACTUAL RESULTS. 32.0a1 (2014-05-04) Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0

Also visible at e.g. https://twitter.com/GetEmoji/status/461762329701801984 -- the right edges of the hearts & orange are visibly cropped, and so are the bottoms bottom of the eggplant/apple/orange.

(Narrowing in on a regression range w/ mozregression, BTW)

m-c nightly regression window: { Last good revision: 8b5875dc7e31 (2013-12-13) First bad revision: 9d593727eb94 (2013-12-14) Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8b5875dc7e31&tochange=9d593727eb94 }

That pushlog includes some IntSize type changes, and this does look like a rounding error of some sort. That'd be my first guess for the culprit... (mozregression is narrowing further with inbound builds, BTW. Will post tighter regression range soon.)

mozregression bailed partway through with "socket reset by peer", but at that point it'd gotten me to this tighter regression range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6aad311883cc&tochange=b3d1f5ab7889 In that range, the size changes (Bug 929513) look most likely to be connected. Marking as blocking that bug.

gfxSize uses doubles, gfx::Size uses float. So http://hg.mozilla.org/integration/mozilla-inbound/rev/526236089c17 reduced the precision here and could have conceivably caused this.

Spot on -- confirmed with targeted builds that that's where this bug was introduced. (I can repro w/ a build from 526236089c17; can't repro w/ a build from its parent, 8ef7a2f35d7b.)

That means we just shipped this bug to release users in Firefox 29. Setting status flags; if possible, it'd be great to fix this in Firefox 30. I suspect we want to just revert all of the RasterImage changes from that changeset. (Tor, do you remember why those RasterImage changes were there? bug 929513 seems to have been targeted at layers code, and these RasterImage changes seem to be modifying imagelib internal things (private methods/data) which don't interact with layers code at all.)

Comment on attachment 8417714 [details] [diff] [review] fix v1: revert RasterImage changes from regressing cset I was initially worried that maybe these RasterImage changes were (a) accommodate a layers client of an image API, or (b) to accommodate a change to a layers API. If one of those were the case, then we'd need to tread a bit lightly in backing out, to avoid introducing implicit type conversions at API boundaries. However, I don't think either of these are the case. The regressing cset http://hg.mozilla.org/integration/mozilla-inbound/rev/526236089c17 only changed local variables in layers code -- not any APIs -- and it doesn't look like any of those variables are passed to imagelib. (and moreover, the RasterImage changes are in imagelib-internal stuff, in code that's all rooted at the private method DrawWithPreDownscaleIfNeeded, AFAICT). So I'm pretty sure the RasterImage changes were unnecessary and should be reverted. Nical/Tor, please correct me if I'm wrong.

I'd like to hear from Nical or Tor before taking any further action here.

Hi, Daniel. I don't remember *exactly* why I made changes to RasterImage, but generally the work on bug 929513 consisted of a lot of search/replace work when changing the layers APIs from Thebes to Moz2D. So typically, I updated the layers API, and then continued to search/replace usages until it compiled.. In that process I might have a build error on a usage of, say, "gfxMatrix.ScaleFactors(bool)" and then maybe replaced some parts outside of the layers directories in an attempt to make the compiler happy. (ScaleFactors is used inside RasterImage) I could reproduce this issue on FF29 on linux and windows, but not on Mac OS X Mavericks (I tested both on a retina and a non-retina display). I'm a little curious as to why that is... But in any case, your change to the RasterImage class indeed seems fine, at least as far as bug 929513 is concerned ;) So I have no problem with your patch.

Comment on attachment 8417714 [details] [diff] [review] fix v1: revert RasterImage changes from regressing cset Review of attachment 8417714 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks safe. I am a bit worried about the this bug. I hope we didn't subtly break other things in the moz2dification effort.

You should also add a warning comment about this in RasterImage. Long term, all or most of the code under gfx/thebes/ will disappear so we need to make sure we won't make this mistake another time as we keep moving way from thebes.

(In reply to Nicolas Silva [:nical] from comment #14) <snip> > I am a bit worried about the this bug. I hope we > didn't subtly break other things in the moz2dification effort. Yeah I agree, but that also sounds to me like it would just be "unmasking" an underlying problem.. Like - if this was not thought out when gfx::Size was designed to be float (and not double like gfxSize) that sounds a tad surprising, no? :) I don't really know a lot about how comprehensive the reftest suite is, but it would probably be good to add one for this case (which is scaling images... So find a scale factor that makes it round one way with float, and the other with double)

Comment on attachment 8417714 [details] [diff] [review] fix v1: revert RasterImage changes from regressing cset Review of attachment 8417714 [details] [diff] [review]: ----------------------------------------------------------------- Hrm, so this seems fine as a bandaid for beta. But let's please fix this properly, there's no reason we should need double precision here and therefor we should avoid using double precision :).

(In reply to Bas Schouten (:bas.schouten) from comment #17) > Hrm, so this seems fine as a bandaid for beta. But let's please fix this > properly, there's no reason we should need double precision here and > therefor we should avoid using double precision :). Unfortunately I don't have cycles to dig into this too much at the moment to figure out a better way to "fix this properly". (And we clearly don't have enough regression testing in this area to tell us if the "proper" fix breaks something else. :-/ ) I agree with you in principle that we should only use double if we need it. However, in this case, this code (in RasterImage) has existed using double for a while now, and there may be subtle assumptions that rely on that, and AFAICT the switch to float here was accidental (part of a layers bug, bleeding into some non-layers code). So I'd think that reverting RasterImage to gfxSize even on Nightly is appropriate (not just "as a bandaid for beta"). Unless we're trying to get rid of gfxSize entirely ASAP (?) Moreover, given the poor regression testing in this area, I think that anything we ship to Beta probably needs Nightly/Aurora testing to increase the confidence that we're not breaking something.

(I do have a reftest for this specific bug, comparing a downscaled circle vs. a circle. I'll post it soon. (It has to be marked as fuzzy, due to the imperfection of downscaling & differences between platforms. The idea is, bugs like this one make the mismatch worse than the normal fuzzy threshold, so the test will fail if this regresses. I'm using Try to figure out the required fuzziness threshold on various platforms at the moment.)

(In reply to Tor Arvid Lund [:torarvid] (Telenor) from comment #13) > In that process I might have a build error on a usage of, say, > "gfxMatrix.ScaleFactors(bool)" and then maybe replaced some parts outside of > the layers directories in an attempt to make the compiler happy. > (ScaleFactors is used inside RasterImage) Yeah, but ScaleFactors returns gfxSize (not gfx::Size). So gfxSize is more correct here. > I could reproduce this issue on FF29 on linux and windows, but not on Mac OS > X Mavericks (I tested both on a retina and a non-retina display). I'm a > little curious as to why that is... This bug only reproduces if we return true from RasterImage::CanScale(). Presumably one of the various conditions in that function (or in CanQualityScale) are failing on your mac platform. They might also succeed on retina, but we have enough pixels that the issue is less visible. (I couldn't initially reproduce this in the reftest harness, because we set the pref "image.high_quality_downscaling.enabled" to false there (by default). Maybe check that pref in your profile? Anyway, I'm not going to worry about this platform difference too much.) > But in any case, your change to the RasterImage class indeed seems fine, at > least as far as bug 929513 is concerned ;) Thanks!

(In reply to Daniel Holbert [:dholbert] from comment #18) <snip> > Unless we're trying to get rid of gfxSize entirely ASAP (?) Well, we are, as per bug 882109 :)

(Yeah -- sorry for not being clearer -- I meant meant more like, "unless gfxSize is disappearing imminently & it would throw a monkey wrench into the plan if we reintroduced these usages." Based on bug 882109 comment 0, and based on a MXR search for 'gfxSize', it looks like that's not the case.)

Tracking as it's a new regression in FF29 and yes it would be nice to have a fix. However it sounds like we might hit this again in the future? Can we get an assignee for for a proper fix?

To (re)state the obvious, we can always hit problems when reducing precision of the computation. I like Daniel's immediate patch and we should just finish reviewing that and land it, uplifting as much as we can. Looks safe to me - we're just improving the precision, and I don't see us running into performance problems due to that. Once we do that, we'll be left with a "we should change RasterImage to use single precision/gfx::Size instead" which should probably be a separate bug and I imagine would be something we'd just land on trunk and let it ride.

(In reply to Milan Sreckovic [:milan] from comment #24) > we'll be left with a "we should change RasterImage to use > single precision/gfx::Size instead" which should probably be a separate bug Agreed. I filed bug 1006749 on that.

(In reply to Nicolas Silva [:nical] from comment #15) > You should also add a warning comment about this in RasterImage. Long term, > all or most of the code under gfx/thebes/ will disappear so we need to make > sure we won't make this mistake another time as we keep moving way from > thebes. (sorry for not replying to this earlier) Any warning comment that I put in the code here would potentially need to be next to every gfxSize variable, which feels messy. Moreover, this sort of rounding-error issue is just as likely to happen for *any* s/gfxSize/gfx::Size/ conversion, and it feels too special-casey to just warn about gfxSize variables in this file. (I'd rather that we spread the word that this particular conversion is lossy & potentially regression-prone, and to tread lightly & test thoroughly.) I think a regression test will be better than a warning-comment at preventing this bug from being re-introduced, and I should have one of those up here later today.

(In reply to Daniel Holbert [:dholbert] from comment #20) > > I could reproduce this issue on FF29 on linux and windows, but not on Mac OS > > X Mavericks (I tested both on a retina and a non-retina display). I'm a > > little curious as to why that is... > > This bug only reproduces if we return true from RasterImage::CanScale(). Yup - more specifically, "image.high_quality_downscaling.enabled" is set to false on OS X because OS X has its own native high-quality image scaling. This means that gHQDownscaling is set to false and so CanScale always returns false.

(In reply to Daniel Holbert [:dholbert] from comment #18) > (In reply to Bas Schouten (:bas.schouten) from comment #17) > > Hrm, so this seems fine as a bandaid for beta. But let's please fix this > > properly, there's no reason we should need double precision here and > > therefor we should avoid using double precision :). I agree with the view that this is a bandaid. (Though I'd say we should just land it on Nightly/Aurora/Beta; I don't want future uplifts to be painful to merge because of this divergence.) > Unfortunately I don't have cycles to dig into this too much at the moment to > figure out a better way to "fix this properly". (And we clearly don't have > enough regression testing in this area to tell us if the "proper" fix breaks > something else. :-/ ) Yup, but I am actually working on fixing this right now. I have a massive patch cooking that will hopefully fix this issue and several other related ones. (Though I haven't tried this specific testcase yet.) > Moreover, given the poor regression testing in this area, I think that > anything we ship to Beta probably needs Nightly/Aurora testing to increase > the confidence that we're not breaking something. Things are already super busted right now, TBH, and we did after all use doubles here until the recent past. I think the risk of uplifting to Beta is limited. I'm going to go ahead and r+ this patch. Can we land the new test you mentioned at the same and get both uplifted?

Here's the reftest patch. As discussed on IRC, this unfortunately requires a setTimeout to reliably exercise this bug (until we fix bug 1006883). Try run with just the reftest, with no patch (expected to be orange): https://tbpl.mozilla.org/?tree=Try&rev=016a016ffc1d Try run with the fix + reftest (expected to be green): https://tbpl.mozilla.org/?tree=Try&rev=de57925953cf

Broader Try run w/ all platforms & more testsuites that could conceivably be affected by this change: https://tbpl.mozilla.org/?tree=Try&rev=94b409a1da51

I'm changing the images in the reftest to let them match more closely while still exposing this bug when the fix isn't applied. (The previous reftest patch compared 10px-wide stripes in a 75x75 image vs. 2px-wide stripes in a 14x14 image (w/ those image sizes taken from the twitter smiley). Note that 2/14 isn't quite equal to 10/75 (and in fact 14 and 75 are relatively prime), so the test can't be perfect (using solid black & white) except by accident when we round favorably. In my updated version of the reftest, I instead compare 15px-wide stripes in 75x75 image vs. 4px-wide stripes in a 20x20 image. This is better because 15/75 == 4/20, which makes it more justifiable to assert that the images should match.) I'm running this patch through Try right now, after which point I can adjust the fuzziness levels to make TBPL happy (particularly for Mac OS). With the fuzzy annotation in the patch right now, the test behaves as expected on my local machine. (passes w/ fix, fails w/o fix)

If you're curious, the images in the reftest patch are currently visible courtesy of Try-hg-server hosting: https://hg.mozilla.org/try/raw-file/b3c4eb4874db/image/test/reftest/downscaling/downscale-1-smallimage.png https://hg.mozilla.org/try/raw-file/b3c4eb4874db/image/test/reftest/downscaling/downscale-1-bigimage.png (They don't display very well on the dark-gray standalone-image background, but they're clearer in the context of the reftest with a white background.)

[updated reftest manifest w/ fuzzy-if(cocoaWidget) annotation, to handle the mac-specific downscaling stuff.]

Are we waiting until we have reftests before we land this? Is there anything preventing us from landing this now to we can uplift ASAP?

I was just waiting on the r+ on the reftest, but I can go ahead and land the patch. (Try runs w/ the test have already shown that the fix is effective, so that makes me more willing to go ahead and land the fix ahead of the test.)

Landed the fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/cc363e379b2a

Also, FWIW, here are the latest Try runs, just lacking the fuzzy-if(cocoaWidget,...) annotation from comment 33 (hence the mac oranges in the run with the fix): Test w/out fix (should be orange): https://tbpl.mozilla.org/?tree=Try&rev=a89d6021e78f Test w/ fix (should be green except on mac): https://tbpl.mozilla.org/?tree=Try&rev=12d75bca947e

Comment on attachment 8419816 [details] [diff] [review] reftest patch v2a Review of attachment 8419816 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

(In reply to Daniel Holbert [:dholbert] from comment #36) > Landed the fix: > https://hg.mozilla.org/integration/mozilla-inbound/rev/cc363e379b2a Looks like this did get merged to central but no comment was left here and the bug status was left untouched. Odd. https://hg.mozilla.org/mozilla-central/rev/cc363e379b2a

Pre-landing try run (reftests only) as a sanity check in case something's changed to make this test fail since my last try run: https://tbpl.mozilla.org/?tree=Try&rev=133cd90250d5

Landed test: https://hg.mozilla.org/integration/mozilla-inbound/rev/0958c46e35a1

Comment on attachment 8417714 [details] [diff] [review] fix v1: revert RasterImage changes from regressing cset Requesting Aurora/Beta approval. [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 929513 User impact if declined: Cropped images on sites like twitter (see screenshot) Testing completed (on m-c, etc.): The fix has been on mozilla-central for over a week. Testing locally to verify that it fixes the bug, and Try runs to prove that it passes automated test (which has just now landed). Risk to taking this patch (and alternatives if risky): Low. The fix just reverts part of bug 929513, converting some variables from floats back to doubles (giving them more precision & fixing the rounding errors that caused this bug). String or IDL/UUID changes made by this patch: None.

https://hg.mozilla.org/releases/mozilla-aurora/rev/f812379f00c0 https://hg.mozilla.org/releases/mozilla-aurora/rev/14b67350d0e9 https://hg.mozilla.org/releases/mozilla-beta/rev/c4eead99c95b https://hg.mozilla.org/releases/mozilla-beta/rev/4241ff080afa

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c4eead99c95b https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4241ff080afa

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 I can still the cropped images on latest Aurora 31.0a2 (20140525004002) and latest Nightly 32.0a1 (20140522030204) under Win 7 64-bit if the screen is zoomed in / out (Ctrl +/-).

(In reply to Petruta Rasa [QA] [:petruta] from comment #46) > I can still the cropped images on latest Aurora 31.0a2 (20140525004002) and > latest Nightly 32.0a1 (20140522030204) under Win 7 64-bit if the screen is > zoomed in / out (Ctrl +/-). Sorry, the Nightly 32 buildID is 20140525030203.

(In reply to Petruta Rasa [QA] [:petruta] from comment #46) > Created attachment 8428660 [details] > TwitterEmoji.png > > I can still [see] the cropped images [...] if the screen is > zoomed in / out (Ctrl +/-). I see that as well, but that's a separate bug / separate regression. (I can reproduce it with the attached testcase in Nightly 27.0a1 (2013-10-27), which is before this bug regressed, per comment 4.) Let's spin off a separate bug for that, and keep this bug here tracking the clipping at the default zoom-level.

I filed bug 1016018 on the clipping-when-zoomed.

Thanks, Daniel! Verified as fixed on Aurora 31.0a1 (20140526004004) and Nightly 32.0a1 (20140526030202) under Win 7 64-bit and Ubuntu 12.10 32-bit keeping the browser at the default zoom default. The fix is not yet in beta and will be verified later this week.

Verified fixed on Firefox 30 beta 8 (20140527133511) under Win 7 64-bit and Ubuntu 12.10 32-bit.