Closed Bug 762101 Opened 12 years ago Closed 12 years ago

Graphical corruption when omtc is enabled

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16
Project Flags:
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: dhylands, Assigned: ajuma)

References

Details

Attachments

(2 files, 2 obsolete files)

some failed attempts
(deleted), patch
Details | Diff | Splinter Review
Correctly convert from screen coordinates to buffer coordinates
(deleted), patch
cjones
: review+
Details | Diff | Splinter Review 
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.
Blocks: 745137
Attached file Shows how things look (correctly). (obsolete) (deleted) — 
This is an apitrace capture which shows how things are supposed to look. This was done with FORCE_BASICTILEDTHEBESLAYER set.
Attachment #631090 - Attachment is obsolete: true
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
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.
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: --- → +
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
Attached patch fix (obsolete) (deleted) — — Splinter Review 
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)
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.
(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.
(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?
(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.
Attached patch some failed attempts (deleted) — — Splinter Review 
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
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.
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+
https://hg.mozilla.org/mozilla-central/rev/6e6bfdabd663
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: