Last Comment Bug 762101 - Graphical corruption when omtc is enabled
: Graphical corruption when omtc is enabled
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla16
Assigned To: Ali Juma [:ajuma]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: b2g-layers-work 745137 b2g-e10s-work
  Show dependency treegraph
 
Reported: 2012-06-06 09:55 PDT by Dave Hylands [:dhylands]
Modified: 2012-07-10 15:45 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
Shows how things look (correctly). (3.88 MB, text/plain)
2012-06-07 12:51 PDT, Dave Hylands [:dhylands]
no flags Details
fix (1.11 KB, patch)
2012-06-27 20:06 PDT, Joe Drew (not getting mail)
no flags Details | Diff | Splinter Review
some failed attempts (5.77 KB, patch)
2012-06-29 16:19 PDT, Joe Drew (not getting mail)
no flags Details | Diff | Splinter Review
Correctly convert from screen coordinates to buffer coordinates (1.36 KB, patch)
2012-07-04 08:11 PDT, Ali Juma [:ajuma]
cjones.bugs: review+
Details | Diff | Splinter Review

Description Dave Hylands [:dhylands] 2012-06-06 09:55:14 PDT
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.
Comment 1 Dave Hylands [:dhylands] 2012-06-07 12:51:53 PDT
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.
Comment 2 Dave Hylands [:dhylands] 2012-06-07 13:08:28 PDT
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
Comment 3 Dave Hylands [:dhylands] 2012-06-15 14:49:21 PDT
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.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-15 15:14:51 PDT
The bug causing (2) has been fixed --- CONTENT_COLOR gives RGB24 surfaces on phones with 24-bit displays.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-21 14:58:51 PDT
It would be great if someone who knows the old shadow-layers pipeline could help out with this.
Comment 6 JP Rosevear [:jpr] 2012-06-27 01:56:47 PDT
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.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-27 03:38:41 PDT
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.
Comment 8 Joe Drew (not getting mail) 2012-06-27 20:06:23 PDT
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.
Comment 9 Joe Drew (not getting mail) 2012-06-27 20:09:57 PDT
Try run: https://tbpl.mozilla.org/?tree=Try&rev=20267b0e575e
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-27 23:28:04 PDT
Comment on attachment 637354 [details] [diff] [review]
fix

Fixes the corruption I can reproduce.  No idea why the coordinate space changed, but wfm!
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-27 23:40:04 PDT
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 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-27 23:40:51 PDT
Comment on attachment 637354 [details] [diff] [review]
fix

Yeah this is causing problems in my desktop build.
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-28 00:50:54 PDT
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 JP Rosevear [:jpr] 2012-06-28 01:19:47 PDT
(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?)?
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-28 02:00:13 PDT
Yes, it shows up in all parts of the UI.  Note: testing on nvidia GPU, ubuntu 12.04, debug build.
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-28 02:14:22 PDT
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.
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-28 02:15:27 PDT
s/tree/tray/
Comment 18 JP Rosevear [:jpr] 2012-06-28 05:44:59 PDT
(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?
Comment 19 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-28 05:47:35 PDT
Device.
Comment 20 Joe Drew (not getting mail) 2012-06-28 09:44:26 PDT
(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.
Comment 21 Joe Drew (not getting mail) 2012-06-28 15:41:30 PDT
This breaks scrolling in the browser, presumably because it breaks rotation fundamentally. Rotation needs to be reimplemented in Upload correctly.
Comment 22 Joe Drew (not getting mail) 2012-06-29 15:13:20 PDT
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.
Comment 23 Joe Drew (not getting mail) 2012-06-29 15:18:39 PDT
Er, drawing things to the right spot.
Comment 24 Joe Drew (not getting mail) 2012-06-29 16:19:51 PDT
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. :(
Comment 25 Joe Drew (not getting mail) 2012-06-29 16:23:02 PDT
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);
Comment 26 Ali Juma [:ajuma] 2012-07-03 14:51:52 PDT
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.
Comment 27 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-04 01:13:41 PDT
Backed out on platform-demo-mc for breaking async pan/zoom.
Comment 28 Ali Juma [:ajuma] 2012-07-04 08:11:51 PDT
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.
Comment 29 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-09 19:13:32 PDT
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.
Comment 31 Ryan VanderMeulen [:RyanVM] 2012-07-10 15:45:52 PDT
https://hg.mozilla.org/mozilla-central/rev/6e6bfdabd663

Note You need to log in before you can comment on or make changes to this bug.