Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Graphical corruption when omtc is enabled

RESOLVED FIXED in mozilla16

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dhylands, Assigned: ajuma)

Tracking

unspecified
mozilla16
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
This is a bug to followup bug 744238. The patch attached to 744238 forces tiled thebes layers to be used, which makes the problem go away.

However, it didn't determine the root cause of the problem.

This is to followup and see if there is still a problem once the gralloc code (bug 745137) lands.
(Reporter)

Updated

5 years ago
Blocks: 745137
(Reporter)

Comment 1

5 years ago
Created attachment 631090 [details]
Shows how things look (correctly).

This is an apitrace capture which shows how things are supposed to look. This was done with FORCE_BASICTILEDTHEBESLAYER set.
(Reporter)

Updated

5 years ago
Attachment #631090 - Attachment is obsolete: true
(Reporter)

Comment 2

5 years ago
My attachments were too large, so you can find them here instead:

Version which looks correct (OMTC enabled, FORCE_BASICTILEDTHEBESLAYER set)
http://people.mozilla.org/~dhylands/bug-762101/firefox-tiled.trace

Version which shows the artifact (OMTC enabled, FORCE_BASICTILEDTHEBESLAYER not set)
http://people.mozilla.org/~dhylands/bug-762101/firefox-artifact.trace

The artifact shows quite clearly in frame 12 and onward.

The last version of gaia which had the sliding lock screen is commit:

commit a2d84bef4e66e9f9dead2a55f40401e6ee3184dc
Author: Timothy Guan-tin Chien <timdream@gmail.com>
Date:   Thu May 31 13:50:38 2012 +0800

    Allow repaint before the screen is turned off, #1532
Blocks: 745143, 745135
(Reporter)

Comment 3

5 years ago
Additional steps required for reproducing:
1 - OMTC has to on
2 - gfx/thebes/gfxAndroidPlatform.cpp has to return an ImageFormat that doesn't have transparency in it (i.e. ImageFormatRGB16_565 or ImageFormatRGB24). If it returns ImageFormatARGB32 then the problem doesn't show up.
3 - FORCE_BASICTILEDTHEBESLAYER (gfx/layers/TiledLayerBuffer.h) has to NOT be set.
The bug causing (2) has been fixed --- CONTENT_COLOR gives RGB24 surfaces on phones with 24-bit displays.

Updated

5 years ago
blocking-basecamp: --- → +
It would be great if someone who knows the old shadow-layers pipeline could help out with this.
Assignee: nobody → joe
Component: General → Graphics
Product: Boot2Gecko → Core
QA Contact: general → thebes
blocking-kilimanjaro: --- → +

Comment 6

5 years ago
Turning on tiled thebes layers as per the original description is apparently not useful right now as it causes a host of other issues, so this is a key bug to fix atm.
The title of this bug is a little misleading.  When omtc is enabled, we see graphical corruption issues.  The issues reproduce on desktop and on device.  Simplest STR

 . Build b2g for desktop.  https://wiki.mozilla.org/Gaia/Hacking#Running_B2G .  Enable omtc.
 . Unlock lock screen.  (Slide the "lock" icon upwards.  Yeah, yeah, we know.)
 . Enter PIN code "0000".  Press "OK" and watch bottom of screen.
 . On my gtk2 build, I usually see flickering at the bottom of the screen.  I see similar symptoms on device.

If those STR don't work, clicking around through multiple apps, opening/closing, will easily produce artifacts.
Summary: Followup bug for the sliding lock screen sticking → Graphical corruption when omtc is enabled
Created attachment 637354 [details] [diff] [review]
fix

This is removing some dubious code that simply removed the x and y coordinate from the layer. I haven't run this through try yet, but it FEELS right, which is close enough. And it fixes the black line at the bottom of the screen, which is all that matters.
Attachment #637354 - Flags: review?(jones.chris.g)
Try run: https://tbpl.mozilla.org/?tree=Try&rev=20267b0e575e
Comment on attachment 637354 [details] [diff] [review]
fix

Fixes the corruption I can reproduce.  No idea why the coordinate space changed, but wfm!
Attachment #637354 - Flags: review?(jones.chris.g) → review+
Hm, seeing

[Parent 17222] ###!!! ASSERTION: Updated region lies across rotation boundaries!: '((destBounds.x % size.width) + destBounds.width <= size.width) && ((destBounds.y % size.height) + destBounds.height <= size.height)', file /home/cjones/mozilla/platform-demo-mc/gfx/layers/opengl/ThebesLayerOGL.cpp, line 935
Comment on attachment 637354 [details] [diff] [review]
fix

Yeah this is causing problems in my desktop build.
Attachment #637354 - Flags: review+
Note, this is landed on https://github.com/cgjones/platform-demo-mc/commit/20bed4db53a115cef4a1b496c3c57d5e0f1ebeab because it fixes issues on phones, but I don't feel comfortable landing it on m-c yet.

Comment 14

5 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #12)
> Comment on attachment 637354 [details] [diff] [review]
> fix
> 
> Yeah this is causing problems in my desktop build.

Chris, you mentioned this was on linux, is it easy to reproduce (may be we should confirm if it screws up on Mac too?)?
Yes, it shows up in all parts of the UI.  Note: testing on nvidia GPU, ubuntu 12.04, debug build.
I'm seeing another issue now where swiping down the "status bar tree" (grab status bar, pull down), results in an unrepainted area carrying down a "copy" of the quick settings while the tray is being pulled down.  May or may not be related.
s/tree/tray/

Comment 18

5 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> I'm seeing another issue now where swiping down the "status bar tree" (grab
> status bar, pull down), results in an unrepainted area carrying down a
> "copy" of the quick settings while the tray is being pulled down.  May or
> may not be related.

Only on desktop or on the device too?
Device.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> I'm seeing another issue now where swiping down the "status bar tree" (grab
> status bar, pull down), results in an unrepainted area carrying down a
> "copy" of the quick settings while the tray is being pulled down.  May or
> may not be related.

Unrelated. Happens with and without patch.
This breaks scrolling in the browser, presumably because it breaks rotation fundamentally. Rotation needs to be reimplemented in Upload correctly.
In fact, there is no buffer rotation going on. The visible rect gets changed correctly when we scroll in the browser.

This makes me think that we're just not uploading things to the right spot.
Er, drawing things to the right spot.
Created attachment 638048 [details] [diff] [review]
some failed attempts

I tried fixing this properly by actually tracking how much space the buffer needs, and using that to recreate the TextureImage, but it didn't work out.

I now suspect that the inputs into the calculation of GetOriginOffset() (specifically mBufferRect) is incorrect, but I don't have time to sort it out. :(
Attachment #637354 - Attachment is obsolete: true
Ali, this is reproducible on desktop by building B2G and Gaia <https://wiki.mozilla.org/Gaia/Hacking>, turning on OMTC (by putting the below prefs into gaia/profile/prefs.js), and then unlocking the lock screen (the 20px high black bar that appears at the bottom of the screen is the bug).

Can you take a look at this while I'm on vacation?

Sorry for dumping this on you :(

user_pref("layers.offmainthreadcomposition.enabled", true);
user_pref("gfx.xrender.enabled", false);
user_pref("dom.ipc.tabs.disabled", true);
Assignee: joe → ajuma
(Assignee)

Comment 26

5 years ago
I can reproduce this on an OS X desktop build.

I believe this is caused by the visible region changing (from (0,0,320,480) to (0,20,320,460)) between calls to ShadowBufferOGL::Upload. Since we are offsetting destRegion by the top-left corner of the visible region, this leaves the buffer in an inconsistent state (specifically, the latter upload is incorrectly offset vertically by 20 pixels). This explains why Joe's initial patch fixed this problem.

Interestingly, always doing a full upload in ShadowBufferOGL::Upload (that is, making the second argument passed to DirectUpdate always be nsIntRect(0,0,size.width,size.height)) fixes the problem without breaking scrolling. This strongly suggests that the problem is the calculation of destRegion in ShadowBufferOGL::Upload, and not the drawing code.
Backed out on platform-demo-mc for breaking async pan/zoom.
(Assignee)

Comment 28

5 years ago
Created attachment 639103 [details] [diff] [review]
Correctly convert from screen coordinates to buffer coordinates

The problem is that in ShadowBufferOGL::Upload, we're incorrectly converting aUpdated from screen coordinates to buffer coordinates. We're implicitly assuming that the top left corner of the buffer rect is the same as the top left corner of the visible region. This assumption is often correct, but not always; when it's wrong we compute destRegion incorrectly and upload to the wrong spot.

This patch fixes the conversion.
Attachment #639103 - Flags: review?(jones.chris.g)
Comment on attachment 639103 [details] [diff] [review]
Correctly convert from screen coordinates to buffer coordinates

Drat, I totally missed this patch!  Sorry for the review lag.

This looks reasonable to me.
Attachment #639103 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 30

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6bfdabd663
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/6e6bfdabd663
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.