1014333 - Thick lines on page

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Screenshot showing the drawing issue

I found a similar issue on a different page on Reddit. Any r/AskScience discussion shows thicker than expected lines along part of the comment borders. Since this is different than bug 1009306 but seems similar I created a new bug.

For example load http://www.reddit.com/r/askscience/comments/25mmxp/in_a_loaded_bike_wheel_which_spokes_are_under_the/ in a current nightly on a Nexus 4 or 5. Scroll down the page till the incorrect drawing shows up.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c8355055899c&tochange=40b2ac413772

Comment 1

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kevin Brosnan [:kbrosnan] from comment #0)
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=c8355055899c&tochange=40b2ac413772

I assume this is a regression range?

Comment 2

[Chris Lord [:cwiiis]]

We used to have some code that, specifically on Android (but previously on Mobile), ignored the situation that we might need to resample when repainting due to sub-pixel scroll value differences. That said, although if you search for PAINT_WILL_RESAMPLE you can see some bits that are #ifdef'd out on Android, I can't actually see that doing what it used to do and there's no equivalent section for it in tiles.

I think it'd be interesting to see if the same issue reproduces on desktop (for example, by using the Metro browser, if that's still buildable - otherwise by enabling omtc, apzc and tiles on a platform that supports it (possibly all of them soon, but I'm not sure...))

Comment 3

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

Not sure it's realistic to expect Chris to get to this during beta.

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

Possibly but this is a regression caused by landing bug 963073 which is in 30. It is useful to track for 30 in case a fix that is upliftable is generated in time for beta or a 30.0.x release build is needed.

Comment 5

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

Sure, I don't mind tracking it, just setting expectations.  Not promising that Chris can't get to it, more explicitly not promising that he can. If that makes any sense.

Comment 6

[Chris Lord [:cwiiis]]

This problem disappears when you turn off progressive rendering - it suggests we have an accuracy problem when setting our rendering offset, but I'm not sure if that's fixable (I'll see if there are any places we're obviously losing accuracy though).

I'm not sure that this is easily fixable, or indeed that it's a regression - it may be seen as a regression because progressive rendering was so comprehensively broken for quite a while and we fixed it up at a similar time to getting tiling working on b2g.

Comment 7

[Chris Lord [:cwiiis]]

Attachment: maybe-help-progressive-tiled-update-rounding-issues

Sorry to foist this on you, but I really can't tell if this actually makes a difference or not... It feels like it makes the issue slightly better for me (it doesn't fix it, which may not be feasible), but I can't tell if it's just wishful thinking.

Could you try out this patch and see what you think?

Note, turning on layers.force-per-tile-drawing has a similarly slightly-better rendering output in my eyes (turning that on nullifies this patch, which only applies to the single-paint-buffer path)

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Screenshot showing Fx29 and 32 appearance

Patch does not help. Built http://people.mozilla.org/~kbrosnan/tmp/1014333/fennec-32.0a1.en-US.android-arm.apk and can still reproduce the thicker lines. Attaching a screenshot showing 29 and 32.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

We're definitely past the point for speculative, high risk uplifts to Beta 30. This looks like a more challenging fix than we should cram in at the last minute without time on Beta to try and shake out any potential regressions.  We'll have to target this for FF31.

Comment 10

[Chris Lord [:cwiiis]]

So played around and thought about this some more and not come to any decisive conclusions yet.

One worry is that this is a result of rendering elements that require sub-pixel precision on the borders of tiles. It could be that when sampling crosses a tile border, we end up with different rendering than if we rendered that whole region at once.

One way to fix this, at the cost of memory-use during a transaction, would be to allocate the entire dirty region on the first transaction and keep it in memory until the transaction ends. This would cause a temporary spike in memory use vs. the situation now, but would not differ to the memory used when progressive rendering is off.

The draw straight-to-tile path will always be broken if this is the cause of the error (well, without a cleverer fix than that).

I'm going to have a go at implementing that, as given we're ok with memory use with progressive rendering off, I don't think the memory hit is a concern and it may end up being a performance improvement (from not having to allocate/deallocate so much and reducing memory fragmentation), but I'm cc'ing mattwoodrow in case he has any brighter ideas (and anyone else already cc'd, please feel free to comment).

Comment 11

[Chris Lord [:cwiiis]]

I tried implementing what I suggested in comment #10 and it doesn't work - I guess somewhat unsurprisingly actually, I'm not sure what I was thinking. I'm just not certain this is easily fixable if we want to retain progressive rendering.

I'm unassigning, as I've switched teams - I don't have the time to look at this in down time and this falls outside of my remit now. I'll try to help however I can, but I can't just spend an indefinite amount of time on it and my initial investigation efforts haven't born fruit.

Cc'ing kats, BenWa and nical, as they all know the code at least as well as I do.

Comment 12

[Chris Lord [:cwiiis]]

Duping this to bug 1009306 because it is.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Updating the tracking flags.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

User reports here[0] (with a video) that this specific bug is not fixed in 34, though, so reopening this and unduping (not sure if bug 1009306 remains fixed or not). 

[0] https://bugzilla.mozilla.org/show_bug.cgi?id=1046017#c84

Comment 15

[Kevin Brosnan [Ex-Mozilla]]

This bug as listed in comment 0 is resolved. Copying details from bug 1009306 comment 55 I was using comment 0 of this bug for the test for the fix range.

I noticed that this is not reproducible in aurora 34 but is on beta 33. I found a fixed range which points to Bug 1042423

Last good revision: 326c91338df0
First bad revision: 06b540d3667a
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=326c91338df0&tochange=06b540d3667a