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

RESOLVED FIXED in Firefox 39, Firefox OS v2.2

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: greatwall2686, Assigned: pchang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.0 affected, b2g-v2.1 affected, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: torch)

Attachments

(10 attachments, 5 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8437610 [details]
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%
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).
Flags: needinfo?(milan)
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.
Flags: needinfo?(milan)
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?)
Flags: needinfo?(greatwall2686)
Keywords: qawanted
Created attachment 8438818 [details]
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
Flags: needinfo?(jmitchell)
QA Whiteboard: [QAnalyst-Triage?]
Keywords: qawanted
QA Contact: ychung
Created attachment 8438819 [details]
Flame2.0.png
(Reporter)

Comment 6

4 years ago
We get it, thanks for your help.
Flags: needinfo?(greatwall2686)
* 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]) ?
Keywords: qawanted
Created attachment 8439294 [details]
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
QA-wanted request fulfilled, removing flag

Seems like this is not a regression but an issue due to feature changing.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Keywords: qawanted
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(milan)
This is ugly, but I'm not sure I'd hold a release on this.
Let's see what triage says.

I'm not sure either, but this looks broken.
blocking-b2g: --- → 1.4?
Although, this looks looks minor it may negative perception to the user because of the strike, lets get this fixed.
blocking-b2g: 1.4? → 1.4+

Updated

4 years ago
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]

Updated

4 years ago
Assignee: nobody → schung
Created attachment 8441880 [details] [review]
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!
Attachment #8441880 - Flags: review?(felash)
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.
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 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.
Attachment #8441880 - Flags: review?(felash) → review+
BenWa, can you take a quick look and advise if you see anything that can help here?
Flags: needinfo?(milan) → needinfo?(bgirard)
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.
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?
(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?
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(felash)
(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?
Flags: needinfo?(hshih)
Created attachment 8442575 [details]
strange_line.png

The line is more clear when the button is enabled
Created attachment 8442577 [details]
active.png

Backgroung image which will be repeated horizontally to fill the button
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.
Flags: needinfo?(felash)
Created attachment 8442694 [details]
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.
Created attachment 8442724 [details]
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.
Flags: needinfo?(bgirard)
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.
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.
Duplicate of this bug: 982712
Component: Gaia::SMS → Layout
Product: Firefox OS → Core
Assignee: schung → nobody
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.
Does not happen on Firefox Desktop.
(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.
Flags: needinfo?(bgirard)
Benoit, why is subpixel rendering doing a line like this? The color is not at all similar to the color of close pixels...
Flags: needinfo?(bgirard)
(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.)
Flags: needinfo?(hshih)
(Assignee)

Updated

4 years ago
Assignee: nobody → pchang
(Assignee)

Comment 37

4 years ago
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.
(Assignee)

Comment 38

4 years ago
(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
Flags: needinfo?(dbaron)

Updated

4 years ago
status-b2g-v2.0: --- → unaffected
Flags: needinfo?(dbaron) → needinfo?(seth)
(Assignee)

Comment 39

4 years ago
(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.

Updated

4 years ago
Flags: needinfo?(bgirard)
(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.
(Assignee)

Comment 41

4 years ago
Created attachment 8450715 [details] [diff] [review]
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.
Attachment #8450715 - Flags: review?(bgirard)
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?
Attachment #8450715 - Flags: review?(bgirard) → review?(matt.woodrow)
If not Matt, Rob should know this code.
Flags: needinfo?(seth)
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.
Attachment #8450715 - Flags: review?(matt.woodrow) → review?(roc)
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.
Flags: needinfo?(seth)
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 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?
Attachment #8450715 - Flags: review?(roc) → review-
Duplicate of this bug: 1035623
(Assignee)

Comment 49

4 years ago
(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
(Assignee)

Updated

4 years ago
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
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.
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.
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.
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.
(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.
(Assignee)

Comment 56

4 years ago
Created attachment 8452944 [details] [diff] [review]
v2

Re-write patch based on comment 53.
Attachment #8450715 - Attachment is obsolete: true
Attachment #8452944 - Flags: review?(roc)
(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
(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 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.
Attachment #8452944 - Flags: review?(roc) → review-
(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.
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?
Flags: needinfo?(b.pmm)
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.
I agree, let's do this. Filed bug 1036532.

I hope it will fix the issue ;)

Removing NI for Pau here.
blocking-b2g: 1.4+ → ---
Flags: needinfo?(b.pmm)
blocking-b2g: --- → backlog
(Assignee)

Comment 64

4 years ago
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.
(Assignee)

Updated

4 years ago
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
(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.
(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.
Flags: needinfo?(bas)
(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.
Flags: needinfo?(bas)
(Assignee)

Comment 69

4 years ago
Created attachment 8455113 [details] [diff] [review]
v3

Re-calculate scaleX/scaleY with fill rect if the fill rect corresponds to a single tile in one direction
Attachment #8452944 - Attachment is obsolete: true
Attachment #8455113 - Flags: review?(roc)
(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 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.
Attachment #8455113 - Flags: review?(roc) → review-
(Assignee)

Comment 72

4 years ago
Created attachment 8455131 [details] [diff] [review]
v4

Remove the redundant HasNonTranslation checking
Attachment #8455113 - Attachment is obsolete: true
Attachment #8455131 - Flags: review?(roc)
FYI, this bug might share the same intrinsic problem with bug 967532.
(Assignee)

Comment 74

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

Waiting for try server result.
(Assignee)

Comment 76

4 years ago
(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
Flags: needinfo?(roc)
No concern, please enable the test!
Flags: needinfo?(roc)
(Assignee)

Comment 78

4 years ago
Created attachment 8462266 [details] [diff] [review]
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
Attachment #8455131 - Attachment is obsolete: true
Attachment #8462266 - Flags: review+
Summary: [Flame][v1.4][Gaia::SMS] There is a horizontal line on “SIM1/2” word and "..." under “Send” word in the message send button when set a SIM card as default outgoing message. → [Flame][Dolphin][v1.4][Gaia::SMS] There is a horizontal line on “SIM1/2” word and "..." under “Send” word in the message send button when set a SIM card as default outgoing message.
(Assignee)

Comment 79

4 years ago
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
What's the progress on this bug? I think fixing it could hopefully also fix bug 967532 and bug 878023.
Flags: needinfo?(pchang)
(Assignee)

Comment 81

3 years ago
(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.
(Assignee)

Comment 82

3 years ago
Created attachment 8568390 [details] [diff] [review]
bug-1023190.patch (rebased with latest master)

Rebase with latest master
Attachment #8462266 - Attachment is obsolete: true
Flags: needinfo?(pchang)
Attachment #8568390 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/931e6cba2ac5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Peter, does it make sense to uplift this to Firefox OS v2.2 ? (b2g37 branch)
(Assignee)

Comment 87

3 years ago
(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?
Flags: needinfo?(seth)
Not that I know, but the issue can appear as soon as we use a gradient somewhere.

Bhavana, what do you think?
Flags: needinfo?(bbajaj)
(Assignee)

Comment 89

3 years ago
(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.
(Assignee)

Updated

3 years ago
blocking-b2g: backlog → 2.2?
(Assignee)

Updated

3 years ago
Attachment #8568390 - Flags: approval-mozilla-b2g37?
(Assignee)

Comment 90

3 years ago
Open this bug to uplift this bug to b2g37.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 91

3 years ago
[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.
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.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-b2g-v2.0: unaffected → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
Resolution: --- → FIXED
Summary: [Flame][Dolphin][v1.4][Gaia::SMS] There is a horizontal line on “SIM1/2” word and "..." under “Send” word in the message send button when set a SIM card as default outgoing message. → Fix extra horizontal line when draw background image with repeat-x mode
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
Flags: needinfo?(bbajaj)
Attachment #8568390 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
blocking-b2g: 2.2? → ---
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c7df45278750
status-b2g-v2.2: affected → fixed
status-firefox37: --- → wontfix
status-firefox38: --- → wontfix
You need to log in before you can comment on or make changes to this bug.