239813 - BeOS Transparency: nsDrawingSurface::Lock() needs implementing

Status: RESOLVED FIXED

(Core Graveyard :: GFX: BeOS, defect)

Description

[Simon]

Bug 206561 also affects BeOS:
http://bugzilla.mozilla.org/show_bug.cgi?id=206561

Worse, not freeing offscreen bitmaps soon brings down the whole system, as each
offscreen bitmap that is drawable uses it's own thread in BeOS, and the
app_server runs out of threads quite quickly. The fix is the same as in the GTK
code.

Also, BeOS has never had an implementation of nsDrawingSurface::Lock() and
Unlock(), which means sites with transparency (and the new firefox download
manager) don't render correctly as well as bringing the system down as described
above. I have an implementation for this too.

Patch to follow...

Comment 1

[Simon]

Attachment: patch (diff -up4)

Fixes leak of offscreen bitmaps (and the whole-system crash that caused), and
adds an implementation for nsDrawingSurface::Lock() and Unlock() so sites that
use -opacity and the Firefox download manager render properly.

Comment 2

[Simon]

Comment on attachment 145553 [details] [diff] [review]
patch (diff -up4)

Reveiw request - BeOS-specific code in BeOS-specific folder.
Sergei can you review this?

Comment 3

[Sergei Dolgov]

Seems good so far, though, before reviewing and checkin i wish to ask you opinion
on problems i'm unsure myself in.

1)Shouldn't we in destructor in if(mBitmap) case also check for for mView!=0
and also remove it, not only detach from mBitmap?
As in case of mBitmap-ped surface this is temporary view belonging to that
BBitmap only - so cleanup for safety.

2)Looper and Bits locking. I'm unsure myself at moment about locking policy.
Should we lock looper and bits at all?  And also, if it is reuired, should we
lock/Unlock those objetcs in both nsSurface::Lock and nsSurface::Unlock.
Or, just lock looper in nsSurfaceLock and unlock in nsSurface::Unlock?
And maybe bits unlocking should depend on flags NS_LOCK_SURFACE_READ_ONLY and
NS_LOCK_SURFACE_WRITE_ONLY ?

Honestly, i don't know:(

3)I noticed that gtk implementation has special care about deleting mLockBitmap
in nsSurface::Init. What do you think about it?

Comment 4

[Sergei Dolgov]

patch is outdated anyway, as leak fix happened in other bug already

Comment 5

[Simon]

Comment on attachment 145553 [details] [diff] [review]
patch (diff -up4)

Obseleting patch

Comment 6

[Simon]

Leak fixed in another bug, no longer critical

Comment 7

[Niels Reedijk]

Attachment: Patch for 1.0.0-d4 (tangobravo

This is the patch taht will be included in 1.0.0-d4

Comment 8

[Simon]

Changed summary to just be about transparency implementation

Comment 9

[Sergei Dolgov]

Comment on attachment 165203 [details] [diff] [review]
Patch for 1.0.0-d4 (tangobravo

obsoleting

Comment 10

[Sergei Dolgov]

Attachment: patch

Slightly cleaned previous version.
Time to checkin

Comment 11

[tqh]

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

Some mix of tabs and spaces, but I can't see anything else wrong.

Comment 12

[Sergei Dolgov]

checked in 2005-02-20 09:15
"Bug 239813 - implementing nsDrawingSurface::Lock()/Unlock() for BeOS."
with little formatting cleanup
Marking as fixed

Comment 13

[Sergei Dolgov]

It seems we don't need that mess with temporary bitmap,
providing proper pointer and stride for existing backbuffer bitmap looks totally
sufficient to perform required task.

Comment 14

[Sergei Dolgov]

Attachment: patch for simplified solution

It works here as good as previous version.
Only issue is that we lack in BeOS any method to really "lock" certain part of
BBitmap, so if something tries to write to "lock rect" at backbuffer, nothing
prevents from that action. 
(With additional bitmap we preserved content of that rect).
But actually backbuffer bitmap is always locked for use by certain thread only
for lifetime, so i doubt that it may be issue

Comment 15

[tqh]

Comment on attachment 175172 [details] [diff] [review]
patch for simplified solution

r=[e-mail]

Comment 16

[Sergei Dolgov]

simpler version checked in.
also removed unused lock-variables from nsDrawingSurfaceBeOS.h

Comment 17

[Simon]

Another quick checkin!

Hope it works properly, I'll be testing soon.