Closed Bug 1014333 Opened 7 years ago Closed 6 years ago

Thick lines on page

Categories

(Core :: Graphics, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox29 --- unaffected
firefox30 - wontfix
firefox31 - wontfix
firefox32 - affected
firefox34 --- fixed
relnote-firefox --- 30+

People

(Reporter: kbrosnan, Unassigned)

References

()

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached image Screenshot showing the drawing issue (obsolete) —
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
(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?
Keywords: regression
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...))
Assignee: nobody → chrislord.net
tracking-fennec: ? → 30+
Not sure it's realistic to expect Chris to get to this during beta.
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.
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.
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.
Status: NEW → ASSIGNED
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)
Attachment #8428250 - Flags: feedback?(kbrosnan)
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.
Attachment #8426700 - Attachment is obsolete: true
Attachment #8428250 - Flags: feedback?(kbrosnan) → feedback-
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.
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).
tracking-fennec: 30+ → 31+
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.
Assignee: chrislord.net → nobody
Status: ASSIGNED → NEW
Duping this to bug 1009306 because it is.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1009306
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
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
tracking-fennec: 31+ → ?
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
Status: REOPENED → RESOLVED
tracking-fennec: ? → ---
Closed: 7 years ago6 years ago
Depends on: 1042423
No longer depends on: 1009306
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.