283225 - [BEOS] - semitransparent in-page "popups" rendering is broken

Status: RESOLVED FIXED

(Core Graveyard :: GFX: BeOS, defect)

Description

[Sergei Dolgov]

http://rezo.net
with mouseover events on text at left side of the page, semitransparent
text-frames do appear.
But in BeOS gecko browser, something is worng with those.
First, without "touching" that area with mouse text at leadt side is visible
partially, like being partially covered with opaque upper layer.

Second, if onmouse text-frame is raised, this is again visible partially.

Two screenshot attachments will follow.

Problem is that i don't know which part is guilty for that - some bug or
unimplemented thing in our gfx code, or 
some issues with visibility/z-order control in our widget implementation, or
some bugs in invalidating/clipping region handling between those two.

In Windows version all is OK.

Comment 1

[Sergei Dolgov]

Attachment: screenshot 1

bad page rendering without "popups"

Comment 2

[Sergei Dolgov]

Attachment: screenshot 2

partially-rendered "popups"

Comment 3

[Sergei Dolgov]

If i only understand XML and CSS and what visibility style tag should result
into in our code
<style>
div.introduction {
                font-size: 10px;
                padding: 5px;
                border: 1px solid #324C48;
                background-color: #e0EBE9;
                visibility: hidden;
                position: absolute;
                left: 25px;
                top: 0px;
                width: 300px;
                -moz-opacity: 0.9; filter: alpha(opacity=90);
        }
</style>

div style="position: absolute; visibility: hidden; z-index: 2000; top: 0px;
left: -150px;" id="lesrubs"

onMouseOver="changestyle('lesrubs','visible');
changestyle('lesthemes','hidden'); changestyle('lactu','hidden');

function changestyle(couche, style) {
if (!(layer = MM_findObj(couche))) return;
layer.style.visibility = style;
}

Comment 4

[Sergei Dolgov]

adding roc to cc as view and layout guru

Maybe single look of such man at the problem is enough:)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Does simple use of 'opacity' work in BeOS? You might want to check nsBlender.cpp
to see if it's doing the right thing on BeOS.

And does the problem depend on which screen depth you use?

Comment 6

[Sergei Dolgov]

There are group of "bugs" in nsBlender.cpp code - wrong order of parameters in
several places, and wrong check for  if (srcSpan == destSpan)
(must be if (srcRowBytes == destRowBytes) )
But those bugs compensate each other as far as i understood,
though, it will be nice if someone will fix all that mess, to avoid confusing
newcomer programmers.

Though, our problem seems being somewhere else, maybe in nsDrawingSurfaceBeOS
handling in nsRenderingContextBeOS code.

Some changes in nsBlender.cpp do improve situation, but do not fix it totally,
so i will continue investigations until at least March 27, 2005
(Leaving in NYC March 28)

Comment 7

[Sergei Dolgov]

Addition to previous comment.
No total "bug-compensation", though.
Color-depth value is calculated wrongly if stride/span != rowBytes,
and in generic case it shouldn't be equal, as locked rect may be only part of
surface pixmap

Comment 8

[Sergei Dolgov]

Problem happens only with two-source blending,
i will try if option to reuse backbuffer has some effect here.
Windows and GTK don't reuse backbuffer, while our port does it (with noreuse
option applied browser work is buggy)

Comment 9

[Sergei Dolgov]

Well, i found the reason for improper rezo.net rendering.

Problem is our nsDrawingSurfaceBeOS::FillRect()
which is asynchronous, as all other drawing utils.

Two-source blanding requires white color for one surface, black for another.
So view manager calls FillRect() when aquires those buffers.
But for the moment of blending due nature of BeOS app server, BBitmap isn't yet
colored.

I do think that in order to avoid similar issues in future in other places, i
will add Sync() to UnlockDrawable(), applying it in case of offscreen surface.

Though, i'm still wondering if you should file nsBlender.cpp bug myself.

Comment 10

[Sergei Dolgov]

Attachment: patch

review request.
I hope this one allows to avoid some other painting glitches, though, we should
revise gfx code to remove unnecessary Sync() calls, as we do it now in
UnlockDrawable()

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 175329 [details] [diff] [review]
patch

why do you now need to nullcheck mView when you didn't before?

Comment 12

[Sergei Dolgov]

heh, nullcheck landed here from version-in-work where i tried to create 3rd kind
of surface, pure BBitmap without view, for blending exclusively.

But i don't think it makes here any bad. Actually it is good manner for place
where we call LockLooper() and UnlockLooper() to check that. 

Comment 13

[Sergei Dolgov]

Attachment: patch - removing extra Sync() from nsImageBeOS

removing extra Sync() from nsImageBeOS
as we'll do it at next line in UnlockView()

Comment 14

[tqh]

Comment on attachment 175329 [details] [diff] [review]
patch

r=[e-mail]

Comment 15

[Sergei Dolgov]

checked in:
nsDrawingSurfaceBeOS.cpp
new revision: 1.20; previous revision: 1.19
nsImageBeOS.cpp
new revision: 1.33; previous revision: 1.32

Comment 16

[Simon]

I'm not sure we should call sync all that often - it more efficent to let the
app server get a full buffer before doing it's thing.

The only time I can think of when we need to have drawing commands carried out
*right now* are for caret flashing (there's a sync() there already) and just
before handing off the pointer to a BBitmap in nsDrawingSurface::Lock().

I think called sync on the BBitmap child BView in Lock() should be sufficient.

Comment 17

[Sergei Dolgov]

"out *right now* are for caret flashing (there's a sync() there already)"
Is it? So i missed it in my cleanup attempts. Should also to be removed, as
UnlockView follows that.
We also had sync before in nsImage, because i got artifacts without that at
pages with tiled backgrounds and moving over that animated gif. But now it is
done there also via UnlockDrawable.

Also i saw artifacts at rezo.net with text even when synced in ::Lock() before
providing pointer. 

Those artifact may depend on computer speed and some other coincidences -
remember how for some people your (and mine) Lock() worked sometimes, and
sometimes it didn't.

So i think too that putting it UnlockDrawable is reliable and will free us in
future from searching for blcak cat in dark room if again some glitches will happen.

Comment 18

[Sergei Dolgov]

Oops, sorry - about cursor/InvertRect() again.
Do you mean we need it even for on-screen drawing?
Can you test it? If it is still needed (probably it does) even with syncing
bitmap now?
I forgot testcases and sites for that problem.

Comment 19

[Simon]

I'll just make a new latest CVS build, then I will test this.

I accept your point on Sync in UnlockDrawable, it probably is safer. How often
is UnlockDrawable() called? Once per update or something?

Comment 20

[Sergei Dolgov]

UnlockDrawable is called once per each mozilla drawing method call.
(Which may have several calls for BeOS drawing inside).

Btw, i tested now cursor problem WITHOUT explicit Sync() call inside InvertRect,
and it looks ok. but i need your confirmation.

Comment 21

[tqh]

Seems like there may be an issue with this one. When you scroll a page it seems
images that are in a specific point of the page, 1 page and 1/4 down or so, may
get white horizontal lines across it. I'll provide a screenshot.

Comment 22

[tqh]

Attachment: Line artifacts

Comment 23

[tqh]

That is http://www.aftonbladet.se btw.

Comment 24

[Sergei Dolgov]

That's interesting.
Though, this bug/patch partially depends on 
https://bugzilla.mozilla.org/show_bug.cgi?id=283343

But maybe there is other reason, which get highlighted now.
Syncing is absolutely legal operation which itself cannot create any artifacts,
and actually, it is used in places for whose we call Invalidate(*, Synchronous
== PR_TRUE) in nsWindow.cpp (those calls are made for just that backbuffer
rendering in our case, asynchronous are for UI), so it makes synchronous drawing
REALLY synchronous.

Looks like some Draw()/Invalidate() call gets lost or provide insufficient
update rect for that part of scrolling, which renders prviously hodden part of page.

Or maybe CopyBits uses bit wrong rect?

Comment 25

[Simon]

Are you sure that is due to this transparency patch?

I have also noticed some slightly odd scrolling behaviour - sometimes a pixel 
line gets missed out just scrolling a normal text page (no transparency I 
thought). I thought these might be yet more rounding issues.

Comment 26

[tqh]

Yes I suspect that too. I also have my heavily modified nsWindow which I should
try without. As it seems to appear on specific images at specific places on page
and it seems to be just one line rounding errors is highly suspected though.

Comment 27

[Simon]

Attachment: More line artifacts - just on text

Here's a screenshot of the apparent rounding errors just on a normal page. See
the link at the bottom and the one near the top. It seems to suggest there is
something wrong in the blit as I can't even think how you would draw a string
with a line missing from the middle. It doesn't belong in this bug really,
IMHO, but this was where the previous discussion was happening.

Comment 28

[Simon]

Attachment: Better shot of text errors

This is a better shot. It's just from bbc news - definately no iframes or
transparency. I caused the artifacts by scrolling down to the bottom of a
long(ish) page and then scrolling up and down a little bit.

I'm reluctant to put this build on BeBits while we have this issue.

Comment 29

[Simon]

Done some bugzilla searching:

Check out bug 171282 and bug 152671

It seems these errors are caused by having a twips-to-pixels conversion factor
that is exactly divisible by 2. In other words, probably cause by the new
conversion factor stuff that Sergei included in his latest fonts patch.

We should move discussion to bug 189353

Comment 30

[Sergei Dolgov]

It seems our paranoid-safe solution with syncing at each drawable unlock produces very bad results at some too lame sites, like
http::/www.amdzone.com

To allow it work noticeably faster, we should roll back a bit, and add two(?) Sync()s - before providing bits in nsSurfaceBeOS::Lock() and after getting changed bits back in nsSurfaceBeOS::Unlock().

Will search a bit more for most effective but still reliable version

Comment 31

[Sergei Dolgov]

Looks like only Sync() in :: Lock() is really sufficient to solve http://rezo.net
 problem

Comment 32

[Sergei Dolgov]

Attachment: patch

returning to less generic but faster solution proposed by Simon.
review request

Comment 33

[tqh]

Comment on attachment 202897 [details] [diff] [review]
patch

r=[e-mail]

Comment 34

[Sergei Dolgov]

Landed in trunk
-----------------
Checking in mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp,v  <--  nsDrawingSurfaceBeOS.cpp
new revision: 1.21; previous revision: 1.20
done
Checking in mozilla/gfx/src/beos/nsImageBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsImageBeOS.cpp,v  <--  nsImageBeOS.cpp
new revision: 1.37; previous revision: 1.36
done 
-----------
Closing again

Comment 35

[Sergei Dolgov]

Maybe BBitmap::LockBits()/UnlockBits() to be removed.
Honestly we didn't understand well why we put bitlocks there and how those work in BeOS

Comment 36

[Sergei Dolgov]

tqh, can you reproduce problem described here:
http://www.livejournal.com/community/bezilla/147377.html?replyto=896177
?

Comment 37

[tqh]

I think it will be reproducable. I havn't compiled with these changes yet.

Comment 38

[Sergei Dolgov]

tqh, couldn't reproduce it at SeaMonkey at PIII 2*500 Mhz machine. Neither at Athlon XP 2400+ (both BeOS R5.3 + Bone7a). So your test is welcomed.
All that looks really strange, as we retrned to state which worked before with really small additions which should be safe according to BeBook.
Though, that previous state worked before tigerdog joined our team with his machine.

Also wondering if it may be Dano app_server specifics.

Comment 39

[Sergei Dolgov]

It seems that original version of UnlockDrawable had flaws in logic even before patch. Reopening

Comment 40

[Sergei Dolgov]

Attachment: patch

maybe this one is more correct.
still waiting for tqh's feedback, as i'm afraid don't see myself some stupid inconsistency in code anymore:(

Comment 41

[Sergei Dolgov]

seems working for tigerdog, but will wait for his further tests

Comment 42

[Doug Shelton]

latest patch (11-14) works well - fixes problems seen here.

Comment 43

[Sergei Dolgov]

Comment on attachment 203020 [details] [diff] [review]
patch

fixing unreliable lock/unlock code.
review request.

Comment 44

[tqh]

Comment on attachment 203020 [details] [diff] [review]
patch

r=[e-mail]

Comment 45

[Sergei Dolgov]

checked in trunk:
/cvsroot/mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp,v  <--  nsDrawingSurfaceBeOS.cpp
new revision: 1.22; previous revision: 1.21
done 

Comment 46

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 203020 [details] [diff] [review]
patch

er, why don't you need to unlock mBitmap?

Comment 47

[Sergei Dolgov]

"er, why don't you need to unlock mBitmap?"
And why this code is here at all, you can ask if investigate thing more?:)
https://bugzilla.mozilla.org/show_bug.cgi?id=200657

Comment 48

[Christian :Biesinger (don't email me, ping me on IRC)]

well indeed :-) But I don't understand why the lock in LockDrawable doesn't need a paired Unlock. Is it safe to assume that that won't cause problems?

Comment 49

[Sergei Dolgov]

Bahhh!!!! Sorry biesi
I was absolutely sure that there must is mBitmap->IsLocked() in LockDrawable!!!
Not mBitmap->Lock().

People, don't submit/checkin patches at day when you are dropping smoking! Abstinence affects your brain - look at results here...

Though, we need tigerdog again to test next version...

/me hides ashamed

Comment 50

[Doug Shelton]

Standing by and ready to test, fyysik!  Patch me up!

Comment 51

[Sergei Dolgov]

tigerdog - change line
http://lxr.mozilla.org/seamonkey/source/gfx/src/beos/nsDrawingSurfaceBeOS.cpp#259
rv = mBitmap->Lock();

to rv = mBitmap->IsLocked();

and see if those crashes will be back.

Comment 52

[Doug Shelton]

rebuilt (non-optimized, debug version) and have no problems with this change.  Compiled and tested in standard clock mode, then retested in OC mode.  No crash on startup.  I'll rebuild my optimized static version, too, but I believe we're OK with this.

Comment 53

[Doug Shelton]

confirmed latest change is OK with optimized build both standard and OC modes.

Comment 54

[Sergei Dolgov]

Attachment: patch

hope this time all things are at proper place.

Comment 55

[Sergei Dolgov]

Checking in mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp,v  <--  nsDrawingSurfaceBeOS.cpp
new revision: 1.23; previous revision: 1.22
done

Checked in trunk

Comment 56

[Niels Reedijk]

Going to land in the branch.

Comment 57

[Niels Reedijk]

Comment on attachment 202897 [details] [diff] [review]
patch

Requesting permission to land in the MOZILLA_1_8_BRANCH.

This is a BeOS-only change and does not affect other platforms.

Note: sergei_d: this is the first in the new patch queue

Comment 58

[Niels Reedijk]

Comment on attachment 203020 [details] [diff] [review]
patch

Requesting permission to land in the MOZILLA_1_8_BRANCH.

This is a BeOS-only change and does not affect other platforms.

Note: sergei_d: second patch of this bug

Comment 59

[Niels Reedijk]

Comment on attachment 203436 [details] [diff] [review]
patch

Requesting permission to land in the MOZILLA_1_8_BRANCH.

This is a BeOS-only change and does not affect other platforms.

Note: sergei_d: third and last patch on this bug.

Comment 60

[Sergei Dolgov]

Checking in mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsDrawingSurfaceBeOS.cpp,v  <--  nsDrawingSurfaceBeOS.cpp
new revision: 1.20.8.1; previous revision: 1.20
done
Checking in mozilla/gfx/src/beos/nsImageBeOS.cpp;
/cvsroot/mozilla/gfx/src/beos/nsImageBeOS.cpp,v  <--  nsImageBeOS.cpp
new revision: 1.34.8.2; previous revision: 1.34.8.1
done