1023190 - Fix extra horizontal line when draw background image with repeat-x mode

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[greatwall2686]

Attachment: 2014-06-10-19-39-45.png

* Description:
 SIM word display with a line and "..." under the "send" word when Insert two SIM card and select one to outgoing messages.

* Precondition:
  Two SIM cards are inserted in device.
* Reproduction steps:
  1.Open settings app ->"SIM manager".
  2.Select one SIM under "Outgoing messages".
  3.Open Messages app
  4.Check the message send button

* Expected result:
The horizontal line and "..." should be removed
* Actual result: 
There is a horizontal line on “SIM1/2” word and "..." under “Send” word.

* Reproduction build:(Buri - Firefox OS V1.4 inside)
 - Gaia      17b102ee8d9a724b62285547cc5f1c5d30bfb01c
 - Gecko     https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/95be84248033
 - BuildID   20140520000201
 - Version   30.0

* Reproduction Frequency
  - 100%

Comment 1

[Julien Wajsberg [:julienw]]

Yes, this looks to be a driver issue because it does not show on some device.

I don't know what to do from here.

Milan, maybe you can help? The horizontal line is at the border of a background-image (note that the image itself does not have this line at all).

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

This looks like a fairly old build - has it been reproduced on a more recent one?  In particular, today's 1.4 would have a fix to bug 948377 which may very well be the same thing.

Comment 3

[Julien Wajsberg [:julienw]]

Thanks Milan, this helped a lot.

QA, can you test this on a recent build?

Note that I don't know which device was used in comment 0; it's definitely not Buri because it needs a DSDS device. NI reporter to know which device it used? (is it a flame?)

Comment 4

[Yeojin Chung [:YeojinC]]

Attachment: Flame1.3BaseImage.png

This issue reproduces on the Flame build that seemed to be used in comment 0:

Environmental Variables:
Device: Flame 1.4
Build ID: 20140520000201
Gaia: 17b102ee8d9a724b62285547cc5f1c5d30bfb01c
Gecko: 95be84248033
Version: 30.0 (1.4) 
Firmware Version: v10G-2

------------------------------------------------
This issue reproduces on the latest Flame 1.4 build:

Environmental Variables:
Device: Flame 1.4
Build ID: 20140611000202
Gaia: d1cf95dc5e8b2f52148487291318542f1396608e
Gecko: a8bb6b76696b
Version: 30.0 (1.4) 
Firmware Version: v10G-2

-----------------------------------------------
This issue does NOT occur on the Flame base image. (screenshot attached):

Environmental Variables:
Device: Flame 1.3
Build ID: 20140520094859
Gaia: a73235d23685e9898f40647cebd83b3fcbfd0117
Gecko: b637b0677e15318dcce703f0358b397e09b018af
Version: 28.0 (1.3) 
Firmware Version: v10G-2
-----------------------------------------------
This issue does NOT occur on Flame 2.0. (screenshot attached):

Environmental Variables:
Device: Flame 2.0
Build ID: 20140611091244
Gaia: a0f9f1f41a436daad8a249ce85df80a81a5ba2d5
Gecko: 0c0effd600c4
Version: 32.0a2
Firmware Version: v10G-2

Comment 5

[Yeojin Chung [:YeojinC]]

Attachment: Flame2.0.png

Comment 6

[greatwall2686]

We get it, thanks for your help.

Comment 7

[Julien Wajsberg [:julienw]]

* 1.3 does not have the same style than 1.4, but we _do_ see the small horizontal line on the top of the button in attachment 8438818 [details] (note that we don't see it on a Buri for example).
* 2.0 is special because we removed the background image so we don't see the issue.

On 1.3, I think you can compare with a Buri: on a v1.3 Buri, there is no thin horizontal line on the top of the button. Could QA check this and add a screenshot (same page than on attachment 8438818 [details]) ?

Comment 8

[Yeojin Chung [:YeojinC]]

Attachment: Buri1.3.png

Here's the screenshot of Buri 1.3 as requested. Thanks.

Environmental Variables:
Device: Buri 1.3
Build ID: 20140602024006
Gaia: 5bd226b03a2d63dfe9df204f7c0afb9984e8fd42
Gecko: 14b755c65af8
Version: 28.0 (1.3) 
Firmware Version: v1.2device.cfg

Comment 9

[Joshua Mitchell (Inactive)]

QA-wanted request fulfilled, removing flag

Seems like this is not a regression but an issue due to feature changing.

Comment 10

[Julien Wajsberg [:julienw]]

So yes, we see clearly that Buri does not show the issue. Compare attachment 8438818 [details]  and attachment 8439294 [details].

Note that I've seen the issue on a Fugu and I don't see it on Tarako. Now we see that the Flame has the issue too.

Milan, can you help or at least point to someone who could help here?

Comment 11

[Jason Smith [:jsmith]]

This is ugly, but I'm not sure I'd hold a release on this.

Comment 12

[Julien Wajsberg [:julienw]]

Let's see what triage says.

I'm not sure either, but this looks broken.

Comment 13

[bhavana bajaj [:bajaj]]

Although, this looks looks minor it may negative perception to the user because of the strike, lets get this fixed.

Comment 14

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Julien, this bug only exist on dual sim device with v1.4 build only because:
1) We don't have sim indication wording on 1.3 build
2) v2.0 got totally different visual

The button height will be increase for sim indication if dual sim device set perfer sim in settings. It looks like the backgroung image got horizontal repeat in this scenario. I only change the background size in message app because we change button height. Please tell me if you prefer fix this in building block, thanks!

Comment 15

[Julien Wajsberg [:julienw]]

I'll look at your patch for sure, but it's weird because when I checked on a Buri (forcing a DSDS-like display) the issue was not appearing. That's what led me to believe it was a device-specific bug.

But if it's not I'm happy because it's easier like this.

Comment 16

[Julien Wajsberg [:julienw]]

Ok, I checked, the problem is not the repeat nor the background size. See https://bug947139.bugzilla.mozilla.org/attachment.cgi?id=8374272, it's perfectly implemented like it should.

As I said, the horizontal line comes from a driver issue. Milan I don't know if you can have a look on it?

Still, I would be happy enough with Steve's work around, if Victoria likes it as well. Let's see if Milan can do something about this before.

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8441880 [details] [review]
Link to github

r=me, but please don't land this until we have a ui-review from Victoria and a feedback about the root cause from Milan.

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

BenWa, can you take a quick look and advise if you see anything that can help here?

Comment 19

[Julien Wajsberg [:julienw]]

Note that we see the issue on Peak too, in case it's easier for you. It's a fine line on top of the send button. We see it more clearly when the send button is enabled (because the fine line is blue).

We don't see it on Buri.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Only showing up on some devices doesn't automatically mean graphics. The resolution is different, and isn't the css -> device pixels factor different as well?

Comment 21

[Benoit Girard (:BenWa)]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> As I said, the horizontal line comes from a driver issue.

I don't want to pile on here but we have no data to support this mentioned in this bug. A driver issue should be one of the last conclusion we come to after we've exhausted other option. We haven't even considered here that the problem could be the page layout doesn't scale properly.

I'm happy to look at this but lets avoiding jumping to conclusion without good data. From what I can tell we only checked repeat and background in comment 16. Why haven't we checked that the page layout resizes properly?

Comment 22

[Benoit Girard (:BenWa)]

I tried with some inactive sim card but I need valid simcard. Since it's not related to the problem we should come up with a reduced test case.

Julien can we get a standalone minimal test case? At the same time please check that layout.

Comment 23

[Steve Chung [:steveck]]

(In reply to Milan Sreckovic [:milan] from comment #20)
> Only showing up on some devices doesn't automatically mean graphics. The
> resolution is different, and isn't the css -> device pixels factor different
> as well?

I think device resolution is a reasonalble assumption, maybe graphic team could have better explanation for it. Hi Jerry, This bug seems reproducibale on the device with pixal ratio higher than 1X. When we repeat the backgroud image(with height 37px) to the button(with height 40px), a strange line dislayed at the top of the button. This line looks like coming from the bottom of the background image. Could it be rounding issue that cause the backgroung image mispositioned?

Comment 24

[Steve Chung [:steveck]]

Attachment: strange_line.png

The line is more clear when the button is enabled

Comment 25

[Steve Chung [:steveck]]

Attachment: active.png

Backgroung image which will be repeated horizontally to fill the button

Comment 26

[Julien Wajsberg [:julienw]]

I agree that this looks more like a rounding issue on HD devices than a driver issue.

I mispoke saying "driver issue" while I should have said "device-specific issue".

Steve said he'll try to do a reduced testcase.

Comment 27

[Steve Chung [:steveck]]

Attachment: 2014-06-19-15-39-39.png

After some discussion with Jerry, I made some small experiment with send button background image:
- Change the image hight to 35px and 36px: This line is visible when image height is 35px but not reproducible when heigh is 36px.
- Filled the image's bottom pixal with red color: The color of the line also changed to red(pleasee ref attachment). But if we set the background to no-repeat, this line would be disappear.

I'll try to create a test case for it.

Comment 28

[Steve Chung [:steveck]]

Attachment: index.html

Hi graphics team folks,
I create a test html that could reproduce this issue easily. Just replace index.html in test template app with this file, save attachment 8442577 [details] to style/icons and test it on high dpi devices(flame,fugu,peak...). Please note it's also reproducible with current master build.

Comment 29

[Julien Wajsberg [:julienw]]

Ahah (ok, it's a hollow laugh), I just noticed I filed bug 982712 with a testcase in the past.

In bug 982712, I said I didn't reproduce on Peak. At the time, I used v1.3 on the Peak, now I'm using v1.4. Fugu was using v1.3t. In case it helps.

So you can load http://everlong.org/mozilla/testcase-background/ in the browser and zoom in. But it seems more difficult to reproduce like this now. I'll put Steve's testcase on another URL.

Comment 30

[Julien Wajsberg [:julienw]]

On http://everlong.org/mozilla/testcase-background/index2.html you can see it very easily. Once every 2 button presses, we see the grey line. Once every 2 button presses, it disappears.

Comment 32

[Julien Wajsberg [:julienw]]

From bug 982712 comment 4, slightly modified with our new findings:

http://everlong.org/mozilla/testcase-background/index2.html shows it in a more easy manner. And I actually see slightly different behaviors on HD and non-HD devices

* on HD devices (eg: Fugu, Flame, Peak), you get the grey line when you click once on the button, then it disappears at the next click, then appears again at the next click, etc. Once every 2 clicks, there is the grey line.

* on non-HD devices (eg: Buri), you first need to zoom in (double click on the button does it) and then the grey line appears sometimes, but not in such a steady way. Sometimes it stays 2 clicks in a row, sometimes we don't see it for several clicks. And I couldn't make it appear without zooming in.

Comment 33

[Julien Wajsberg [:julienw]]

Does not happen on Firefox Desktop.

Comment 34

[Benoit Girard (:BenWa)]

(In reply to Julien Wajsberg [:julienw] (away June 21 to 30) from comment #30)
> On http://everlong.org/mozilla/testcase-background/index2.html you can see
> it very easily. Once every 2 button presses, we see the grey line. Once
> every 2 button presses, it disappears.

You're incrementing your CSS pixel count by 1 which means that's you're incrementing your device pixel count by 1.5 and hitting subpixel rendering. I can reproduce the bug by incrementing by 1.5 CSS pixel on desktop which causes the same device pixel increment that you see on hidpi b2g.

This isn't a platform bug, this is just a side effect of using a fractional content scale.

Comment 35

[Julien Wajsberg [:julienw]]

Benoit, why is subpixel rendering doing a line like this? The color is not at all similar to the color of close pixels...

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Julien Wajsberg [:julienw] (away June 21 to 30) from comment #30)
> On http://everlong.org/mozilla/testcase-background/index2.html you can see
> it very easily. Once every 2 button presses, we see the grey line. Once
> every 2 button presses, it disappears.

This does actually seem like a platform bug to me.

(It wouldn't even surprise me if it's fixed by a massive patch queue that seth is working on and trying to get green on all platforms, but that patch queue is not going to make it to any old branches.)

Comment 37

[Peter Chang[:pchang]]

I'm able to reproduce this issue one my flame.

I suspect this is a rounding issue when drawing the background with repeat property.
Checking the nsCSSRendering::PaintBackground first.

Comment 38

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #37)
> I'm able to reproduce this issue one my flame.
> 
> I suspect this is a rounding issue when drawing the background with repeat
> property.
> Checking the nsCSSRendering::PaintBackground first.

This issue is caused by the non integer translation of user-space-to-image-space transform[1] when paint background with repeat mode.

Disable the rounding of anchor point could fix issue from link http://everlong.org/mozilla/testcase-background/index2.html.
But it still has problem from below link (an extra line below the image).
http://people.mozilla.org/~pchang/line.html

Besides, cairo couldn't configure as repeat-x or repeat-y mode, it only accepts the repeat mode.
Dbaron, any suggestion?

[1]http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?from=nsLayoutUtils.cpp&case=true#4976
[2]http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?from=nsLayoutUtils.cpp&case=true#4959

Comment 39

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #38)
> (In reply to peter chang[:pchang][:peter] from comment #37)
> > I'm able to reproduce this issue one my flame.
> > 
> > I suspect this is a rounding issue when drawing the background with repeat
> > property.
> > Checking the nsCSSRendering::PaintBackground first.
> 
> This issue is caused by the non integer translation of
> user-space-to-image-space transform[1] when paint background with repeat
> mode.
> 
> Disable the rounding of anchor point could fix issue from link
> http://everlong.org/mozilla/testcase-background/index2.html.
> But it still has problem from below link (an extra line below the image).
> http://people.mozilla.org/~pchang/line.html
> 
> Besides, cairo couldn't configure as repeat-x or repeat-y mode, it only
> accepts the repeat mode.
> Dbaron, any suggestion?
> 
> [1]http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.
> cpp?from=nsLayoutUtils.cpp&case=true#4976
> [2]http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.
> cpp?from=nsLayoutUtils.cpp&case=true#4959

I just have one idea.

Since cairo can't configure as repeat-x or repeat-y, we could configure the width or height of dirty rectangle to image length under repeat-x or repeat-y mode. For example, align height of dirty rectangle to image height if only has repeat-x mode.

I will try to work on the WIP now.

Comment 40

[Benoit Girard (:BenWa)]

(In reply to Benoit Girard (:BenWa) from comment #34)
> This isn't a platform bug, this is just a side effect of using a fractional
> content scale.

Sorry. I take that back, layout should be snapping the rendering to pixels. This is indeed a layout bug.

Comment 41

[Peter Chang[:pchang]]

Attachment: v1

Flame is using 1.5 CSS pixel and gecko tries to snap the non-integer usersapce to device pixel space.

Attach patch to align the size of snapped rect to get proper user-space-to-image-space matrix and then map the edge pixels correctly from user-space to image-space.

Comment 42

[Benoit Girard (:BenWa)]

Comment on attachment 8450715 [details] [diff] [review]
v1

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

I'm not familiar with this code at all. Are you matt or can you suggest a better reviewer?

Comment 43

[Milan Sreckovic [:milan] (needinfo for best results)]

If not Matt, Rob should know this code.

Comment 44

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8450715 [details] [diff] [review]
v1

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

I don't really know this code sorry, roc has reviewed other patches to it recently.

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Seth has been working on some image drawing API changes one of whose goals is avoiding rounding issues related to repeated conversions between coordinate systems.  (Part of it is bug 1034345, but I think there are more bugs to be filed.)  I'm curious how related this is to those changes.

Comment 46

[Julien Wajsberg [:julienw]]

Hey, I just want to mention that this issue is blocking-b2g:1.4+ and as such will need to land in b2g30 and gecko-32 (aurora, I think?).

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8450715 [details] [diff] [review]
v1

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

::: layout/base/nsLayoutUtils.cpp
@@ +4942,5 @@
> +        devPixelDest.width = fill.width;
> +      }
> +      if (devPixelFill.height == devPixelDest.height) {
> +        devPixelDest.height = fill.height;
> +      }

devPixelFill.width == devPixelDest.width doesn't mean there's no repeat-x. The fill-rect could start in the middle of the dest-rect, so we could be rendering the right half of an image followed by the left half of the image.

I don't understand what bug this patch is trying to fix. Is it that the bottom edge of the image is not exactly hitting the top edge of the border? Can you give actual y-coordinate calculations that explain how this happens?

Comment 49

[Peter Chang[:pchang]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47)
> Comment on attachment 8450715 [details] [diff] [review]
> v1
> 
> Review of attachment 8450715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +4942,5 @@
> > +        devPixelDest.width = fill.width;
> > +      }
> > +      if (devPixelFill.height == devPixelDest.height) {
> > +        devPixelDest.height = fill.height;
> > +      }
> 
> devPixelFill.width == devPixelDest.width doesn't mean there's no repeat-x.
> The fill-rect could start in the middle of the dest-rect, so we could be
> rendering the right half of an image followed by the left half of the image.
> 
Do you mean that it is possible "the width of devPixelFill and devPixelDest are the same with repeat-x mode"?
If so, the condition here is fine because here is trying to take care about the rounding changes by UserToDevicePixelSnapped. And I could update the code comment for this.

> I don't understand what bug this patch is trying to fix. Is it that the
> bottom edge of the image is not exactly hitting the top edge of the border?
> Can you give actual y-coordinate calculations that explain how this happens?

Right now flame is using 1.5 CSS pixels.
And the following are the coordinate calculations from below the test link.
http://people.mozilla.org/~pchang/line.html
You can see the fill rect[1] changes to 56 but the devPixelDest still keeps as 55.5.
And it causes non-integer translation of user-space-to-image-space transform.

http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#4938

I/Gecko   (10222): before UserToDevicePixelSnapped devPixelDest [85.5 21 15 55.5] devPixelFill [12 21 162 55.5] devPixelDirty [12 21 162 55.5] anchorPoint 93 76.5
I/Gecko   (10222): afterUserToDevicePixelSnapped devPixelDest [85.5 21 15 55.5] fillRect [12 21 162 56] devPixelDirty [12 21 162 55.5] anchorPoint 93 76.5
I/Gecko   (10222): didSnap 1 transform [0.666667 0.666667 -57 -14.3333] fillRect [12 21 162 56] anchorPoint 93 77 imageSpaceAnchorPoint 5 37

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to peter chang[:pchang][:peter] from comment #49)
> Do you mean that it is possible "the width of devPixelFill and devPixelDest
> are the same with repeat-x mode"?
> If so, the condition here is fine because here is trying to take care about
> the rounding changes by UserToDevicePixelSnapped. And I could update the
> code comment for this.

You can have, for example dest-rect(x,y,w,h) = (0,0,100,100) and fill-rect = (50,0,100,100). The fill-rect spans two tiles.

> > I don't understand what bug this patch is trying to fix. Is it that the
> > bottom edge of the image is not exactly hitting the top edge of the border?
> > Can you give actual y-coordinate calculations that explain how this happens?
> 
> Right now flame is using 1.5 CSS pixels.
>
> And the following are the coordinate calculations from below the test link.
> http://people.mozilla.org/~pchang/line.html
> You can see the fill rect[1] changes to 56 but the devPixelDest still keeps
> as 55.5.
> And it causes non-integer translation of user-space-to-image-space transform.
> 
> http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.
> cpp#4938
> 
> I/Gecko   (10222): before UserToDevicePixelSnapped devPixelDest [85.5 21 15
> 55.5] devPixelFill [12 21 162 55.5] devPixelDirty [12 21 162 55.5]
> anchorPoint 93 76.5
> I/Gecko   (10222): afterUserToDevicePixelSnapped devPixelDest [85.5 21 15
> 55.5] fillRect [12 21 162 56] devPixelDirty [12 21 162 55.5] anchorPoint 93
> 76.5
> I/Gecko   (10222): didSnap 1 transform [0.666667 0.666667 -57 -14.3333]
> fillRect [12 21 162 56] anchorPoint 93 77 imageSpaceAnchorPoint 5 37

The transform maps the bottom of the fillRect (21+56=77) to the bottom of the image (37). So I wonder why a gray line is drawn.

I suspect it's the MOZ_GFX_OPTIMIZE_MOBILE here:
http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUtils.cpp#343
If you take that out, I'm pretty sure the bug will go away.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But there will also be performance problems in some cases, so we probably don't want to ship that.

One thing we should probably do is make this code controlled by a pref instead of an #ifdef so it's easy for people to toggle to see if that's causing a bug.

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One way to work around the bug would be to provide an image asset that's the correct device resolution. That will make the bug go away and also perform better.

We could also use a CSS gradient here instead of a bitmapped image.

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we can also take a patch similar to the one here that snaps the dest rect if we're really only using a single tile in that direction. It's a lot more risky though, this code is tricky.

Comment 54

[Matt Woodrow (:mattwoodrow)]

CreateSamplingRestrictedDrawable can sometimes avoid allocating a new surface by just creating a subimage around the existing pixels. That code should work on mobile, so we could/should enable that subset of CSRD for improved quality without a perf hit.

Comment 55

[Julien Wajsberg [:julienw]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #52)
> One way to work around the bug would be to provide an image asset that's the
> correct device resolution. That will make the bug go away and also perform
> better.

If this makes the bug go away I'm all for this simple solution.

But I really think there is an underlying subsampling issue here: why is this horizontal line appearing when we subsample pixels that don't have that color?

> 
> We could also use a CSS gradient here instead of a bitmapped image.

I think we used a gradient at one point (in the 1.1 development IIRC), and it had perf issues (I don't remember which ones, Vivien would probably if you want to know more). But maybe this improved.

Comment 56

[Peter Chang[:pchang]]

Attachment: v2

Re-write patch based on comment 53.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Julien Wajsberg [:julienw] from comment #55)
> But I really think there is an underlying subsampling issue here: why is
> this horizontal line appearing when we subsample pixels that don't have that
> color?

Because we're rendering with REPEAT (because we need to repeat horizontally), and we have to scale, so rendering the top row of pixels samples some contribution from the bottom row of image pixels.

These references may help:
http://robert.ocallahan.org/2008/10/hating-pixels_13.html
https://wiki.mozilla.org/Gecko:Image_Snapping_and_Rendering

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #54)
> CreateSamplingRestrictedDrawable can sometimes avoid allocating a new
> surface by just creating a subimage around the existing pixels. That code
> should work on mobile, so we could/should enable that subset of CSRD for
> improved quality without a perf hit.

Yes, although if I understand correctly, that won't help in this case.

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8452944 [details] [diff] [review]
v2

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

::: layout/base/nsLayoutUtils.cpp
@@ +4934,5 @@
> +
> +    // Snap the devPixelDest rect when the devPixelFill rect corresponds to
> +    // just a single tile in that direction
> +    if (fill.Size() != devPixelFill.Size() &&
> +        !currentMatrix.HasNonTranslation()) {

I don't think we should do these checks. The optimization you're trying to do here should not depend on them.

@@ +4937,5 @@
> +    if (fill.Size() != devPixelFill.Size() &&
> +        !currentMatrix.HasNonTranslation()) {
> +      if (devPixelDest.x == devPixelFill.x &&
> +          devPixelDest.XMost() == devPixelFill.XMost()) {
> +        devPixelDest.width = fill.width;

I don't think this is right. At least, I can't see a logical argument for its correctness.

I think a more correct-looking patch would modify just the calculation of scaleX to use fill.width when we have a single tiled and didSnap is true. So it would read better if you move this code down to where scaleX is adjusted under didSnap.

@@ +4941,5 @@
> +        devPixelDest.width = fill.width;
> +      }
> +      if (devPixelDest.y == devPixelFill.y &&
> +          devPixelDest.YMost() == devPixelFill.YMost()) {
> +        devPixelDest.height = fill.height;

Same here.

Comment 60

[Julien Wajsberg [:julienw]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #57)
> (In reply to Julien Wajsberg [:julienw] from comment #55)
> > But I really think there is an underlying subsampling issue here: why is
> > this horizontal line appearing when we subsample pixels that don't have that
> > color?
> 
> Because we're rendering with REPEAT (because we need to repeat
> horizontally), and we have to scale, so rendering the top row of pixels
> samples some contribution from the bottom row of image pixels.

That's where I miss it: we need to repeat horizontally only, so why do take pixels around in the vertical axe? I'm sorry, it's probably obvious, but I don't see the reason.

Comment 61

[Julien Wajsberg [:julienw]]

Hey Pau, as discussed on IRC with Arnau, looks like one of the cause for this issue is that the background.png and active.png images in [1] are not available in 1.5x or 2x resolutions (in v1.4). Could you help here? background-dark.png, if it's used somewhere, should probably be provided too.

Note that this bug is 1.4+ which means it's quite urgent :) Thanks a lot !

[1] https://github.com/mozilla-b2g/gaia/tree/v1.4/shared/style/input_areas/images/ui


Peter, Milan and others, thanks a lot for your work here, and I think it should move forward. As a 1.4+ fix though, I think providing the right images is the way to go as it's simpler and safer. We (I) definitely should have spot earlier that these files are missing.

Should we change the component for this bug and move the gecko work in a separate bug? Or should we do the contrary and file a separate bug for the image fix?

Comment 62

[Milan Sreckovic [:milan] (needinfo for best results)]

At the first glance, since this bug probably has more Gecko type information in it, it may make sense to leave it as the Gecko bug (remove 1.4+, I assume), and create a different Gaia 1.4+ for the image solution.

Comment 63

[Julien Wajsberg [:julienw]]

I agree, let's do this. Filed bug 1036532.

I hope it will fix the issue ;)

Removing NI for Pau here.

Comment 64

[Peter Chang[:pchang]]

Comment on attachment 8452944 [details] [diff] [review]
v2

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

::: layout/base/nsLayoutUtils.cpp
@@ +4934,5 @@
> +
> +    // Snap the devPixelDest rect when the devPixelFill rect corresponds to
> +    // just a single tile in that direction
> +    if (fill.Size() != devPixelFill.Size() &&
> +        !currentMatrix.HasNonTranslation()) {

It's fine to remove the fill.Size checking, but here needs to check currentMatrix is the straight translation or not.

If you zoom the page which contains this background image, the fill rect size changes based on zoom level(related to currentMatrix). With the new fill rect size, you will calculate the transform with incorrect scale factor.


The following log shows the fill rect size changed because of zoom level changes.

I/Gecko   (14254): before Snap devPixelDest [52.725 200.7 15 55.5] devPixelFill [-20.775 200.7 162 55.5] devPixelDirty [-20.775 200.7 162 55.5] anchorPoint 60.225 256.2
I/Gecko   (14254): after Snap devPixelDest [52.725 200.7 15 55.5] Fill [16 381 218 74] devPixelDirty [-20.775 200.7 162 55.5] anchorPoint 60.225 256.2
I/Gecko   (14254): pchang snap 1 transform [0.495793 0.371845 -56.9741 -132.189] fillRect [16 381 218 74] anchorPoint 125 455 imageSpaceAnchorPoint 5 37

@@ +4937,5 @@
> +    if (fill.Size() != devPixelFill.Size() &&
> +        !currentMatrix.HasNonTranslation()) {
> +      if (devPixelDest.x == devPixelFill.x &&
> +          devPixelDest.XMost() == devPixelFill.XMost()) {
> +        devPixelDest.width = fill.width;

For my point of view, the user-space-to-image-space transform is calculated with devPixelDest. If we meet the condition to snap the devPixelDest, I would prefer to change devPixelDest from the beginning. Then both of transform and imageSpaceAnchorPoint are calculated with expected devPixelDest.

How do you think?

Although it works if I move above logic to the area that calculates scaleX.

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Julien Wajsberg [:julienw] from comment #60)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #57)
> > (In reply to Julien Wajsberg [:julienw] from comment #55)
> > > But I really think there is an underlying subsampling issue here: why is
> > > this horizontal line appearing when we subsample pixels that don't have that
> > > color?
> > 
> > Because we're rendering with REPEAT (because we need to repeat
> > horizontally), and we have to scale, so rendering the top row of pixels
> > samples some contribution from the bottom row of image pixels.
> 
> That's where I miss it: we need to repeat horizontally only, so why do take
> pixels around in the vertical axe? I'm sorry, it's probably obvious, but I
> don't see the reason.

Because we're using cairo to render, and cairo doesn't let us repeat in one direction and clamp in the other direction. If we used Skia to render content (and exposed the right API in Moz2D) we could do that.

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to peter chang[:pchang][:peter] from comment #64)
> For my point of view, the user-space-to-image-space transform is calculated
> with devPixelDest. If we meet the condition to snap the devPixelDest, I
> would prefer to change devPixelDest from the beginning. Then both of
> transform and imageSpaceAnchorPoint are calculated with expected
> devPixelDest.

I still think it would be easier to understand if you move the logic down to scaleX/Y and don't mess with devPixelDest. It's not a good idea for devPixelDest to sometimes mean something else.

Comment 67

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #65)
>...
> Because we're using cairo to render, and cairo doesn't let us repeat in one
> direction and clamp in the other direction. If we used Skia to render
> content (and exposed the right API in Moz2D) we could do that.

Bas, if we don't have the Moz2D API for this, let's open a bug to track this requirement as we switch to Skia.

Comment 68

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #67)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #65)
> >...
> > Because we're using cairo to render, and cairo doesn't let us repeat in one
> > direction and clamp in the other direction. If we used Skia to render
> > content (and exposed the right API in Moz2D) we could do that.
> 
> Bas, if we don't have the Moz2D API for this, let's open a bug to track this
> requirement as we switch to Skia.

D2D and Skia both support this behavior natively. We chose not to expose it in the API because we had no firm requirements and because not all backends always support it.

Just to be clear, even once regular content drawing is switched to Skia we'll still be using Cairo at least for a little while for several other operation. I'm of the opinion if there's sufficient usecase for this we could expose this and implement it, and also implement it on cairo just in a slow performance workaround.

Comment 69

[Peter Chang[:pchang]]

Attachment: v3

Re-calculate scaleX/scaleY with fill rect if the fill rect corresponds to a single tile in one direction

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Bas Schouten (:bas.schouten) from comment #68)
> Just to be clear, even once regular content drawing is switched to Skia
> we'll still be using Cairo at least for a little while for several other
> operation. I'm of the opinion if there's sufficient usecase for this we
> could expose this and implement it, and also implement it on cairo just in a
> slow performance workaround.

We already have a workaround so if we could just detect the presence of this feature, we could use it. Would be a nice win for some pages, on Windows.

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8455113 [details] [diff] [review]
v3

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

::: layout/base/nsLayoutUtils.cpp
@@ +4982,5 @@
> +        devPixelDest.x == devPixelFill.x &&
> +        devPixelDest.XMost() == devPixelFill.XMost()) {
> +      scaleX = imageSize.width/fill.width;
> +    }
> +    if (!currentMatrix.HasNonTranslation() &&

I think you can take out the HasNonTranslation check here. Just do these changes *instead of* doing the scaleX and scaleY divisions below.

Comment 72

[Peter Chang[:pchang]]

Attachment: v4

Remove the redundant HasNonTranslation checking

Comment 73

[Xidorn Quan [:xidorn] UTC+11]

FYI, this bug might share the same intrinsic problem with bug 967532.

Comment 74

[Peter Chang[:pchang]]

https://tbpl.mozilla.org/?tree=Try&rev=77b1b56c8314

Waiting for try server result.

Comment 75

[Peter Chang[:pchang]]

Ref test failed on OSX, debugging...

https://tbpl.mozilla.org/?tree=Try&rev=77b1b56c8314&jobname=Rev4%20MacOSX%20Snow%20Leopard%2010.6%20try%20opt%20test%20reftest

Comment 76

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #75)
> Ref test failed on OSX, debugging...
> 
> https://tbpl.mozilla.org/
> ?tree=Try&rev=77b1b56c8314&jobname=Rev4%20MacOSX%20Snow%20Leopard%2010.
> 6%20try%20opt%20test%20reftest

roc, my patch is fixed the reftest image-seam-2.html on OSX which is disabled because of bug 567370[1]. Any concern if I enable that test case for OSX?

[1]http://dxr.mozilla.org/mozilla-central/source/layout/reftests/image/reftest.list#5

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No concern, please enable the test!

Comment 78

[Peter Chang[:pchang]]

Attachment: bug-1023190.patch

Enable the image-seam-2 for OSX.

And wait for the try server result.
https://tbpl.mozilla.org/?tree=Try&rev=54b708e7b4fd

Comment 79

[Peter Chang[:pchang]]

There is another Windows 7 Opt Ref test failed.
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/bugs/759036-2.html

Comment 80

[Xidorn Quan [:xidorn] UTC+11]

What's the progress on this bug? I think fixing it could hopefully also fix bug 967532 and bug 878023.

Comment 81

[Peter Chang[:pchang]]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #80)
> What's the progress on this bug? I think fixing it could hopefully also fix
> bug 967532 and bug 878023.
I couldn't apply the patch back to master, and the problem is still existed.
I will try to re-write the patch based on latest master.

Comment 82

[Peter Chang[:pchang]]

Attachment: bug-1023190.patch (rebased with latest master)

Rebase with latest master

Comment 83

[Peter Chang[:pchang]]

try is positive

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce4dd487419d

Comment 84

[Peter Chang[:pchang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/931e6cba2ac5

Comment 85

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/931e6cba2ac5

Comment 86

[Julien Wajsberg [:julienw]]

Peter, does it make sense to uplift this to Firefox OS v2.2 ? (b2g37 branch)

Comment 87

[Peter Chang[:pchang]]

(In reply to Julien Wajsberg [:julienw] from comment #86)
> Peter, does it make sense to uplift this to Firefox OS v2.2 ? (b2g37 branch)

The uplift to v2.2 requires v2.2 blocking flag now, do we have any blocker that require this fix?

Comment 88

[Julien Wajsberg [:julienw]]

Not that I know, but the issue can appear as soon as we use a gradient somewhere.

Bhavana, what do you think?

Comment 89

[Peter Chang[:pchang]]

(In reply to Julien Wajsberg [:julienw] from comment #88)
> Not that I know, but the issue can appear as soon as we use a gradient
> somewhere.
> 
> Bhavana, what do you think?

Agree, it may happen in somewhere and might take time to find out the root cause.

BTW, I just confirm attachment 8568390 [details] [diff] [review] could be applied to b2g37 branch.

Comment 90

[Peter Chang[:pchang]]

Open this bug to uplift this bug to b2g37.

Comment 91

[Peter Chang[:pchang]]

[Approval Request Comment]
Bug caused by (feature/regressing bug #):none
User impact if declined: horizontal line is possible to appear when draw image with repeat
Testing completed: locally tested and passed try in comment 83.
Risk to taking this patch (and alternatives if risky): relatively low risk.
String or UUID changes made by this patch: none.

Comment 92

[Julien Wajsberg [:julienw]]

It's "fixed" when it lands on central/master. Then we play with the "status-*" flags.

I also change the title to reflect your patch; the original issue was worked around in bug 1036532.

Comment 93

[bhavana bajaj [:bajaj]]

Comment on attachment 8568390 [details] [diff] [review]
bug-1023190.patch (rebased with latest master)

looks relatively low risk, so approving. If we have any fallouts/regressions we will backout on 2.2

Comment 94

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c7df45278750