Closed
Bug 1014333
Opened 11 years ago
Closed 10 years ago
Thick lines on page
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: kbrosnan, Unassigned)
References
()
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
kbrosnan
:
feedback-
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details |
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
Reporter | ||
Updated•11 years ago
|
Comment 1•11 years ago
|
||
(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
Comment 2•11 years ago
|
||
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...))
Updated•11 years ago
|
Assignee: nobody → chrislord.net
tracking-fennec: ? → 30+
Comment 3•11 years ago
|
||
Not sure it's realistic to expect Chris to get to this during beta.
Reporter | ||
Comment 4•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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
Comment 7•11 years ago
|
||
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)
Reporter | ||
Comment 8•11 years ago
|
||
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
Reporter | ||
Updated•11 years ago
|
Attachment #8428250 -
Flags: feedback?(kbrosnan) → feedback-
Comment 9•11 years ago
|
||
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•11 years ago
|
||
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).
Updated•10 years ago
|
tracking-fennec: 30+ → 31+
Updated•10 years ago
|
relnote-firefox:
--- → 30+
Comment 11•10 years ago
|
||
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
Comment 12•10 years ago
|
||
Duping this to bug 1009306 because it is.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Comment 13•10 years ago
|
||
Updating the tracking flags.
Comment 14•10 years ago
|
||
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
Updated•10 years ago
|
tracking-fennec: 31+ → ?
Reporter | ||
Comment 15•10 years ago
|
||
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: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•