787513 - The update rectangle for NPAPI plugins does not work on Mac

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Bill Appleton]

Attachment: This is a simple xcode project that shows the error

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

When an NPAPI plugin receives the draw message under the Cocoa event model the plugin is supposed to paint the supplied "update" rectangle. But if you do this then the rest of the CGContext turns white. If you draw the entire CGContext every time than it works OK but this is very slow. This bug started happening one year ago. This does NOT happen on Safari or Chrome. I have attached a simple xcode project that shows this problem, this is from the "basic" plugin sample.


Actual results:

When I draw the update area for the CGContext the rest of the context turned white.


Expected results:

On Safari or Chrome this works correctly. The background of the CGContext stays the same when you draw just the update portion of the rectangle.

Comment 1

[Benoit Girard (:BenWa)]

I'll take a look next week. The plugin should only paint the area requested by the browser in the paint event.

Comment 2

[Bill Appleton]

(In reply to Benoit Girard (:BenWa) from comment #1)
> I'll take a look next week. The plugin should only paint the area requested
> by the browser in the paint event.

In the provided example code the plugin only paints the area requested by the browser in the paint event. However when we do that the area outside the paint rectangle turns white.

Comment 3

[Bill Appleton]

Can someone please take a look at this?

Comment 4

[Matthias Versen [:Matti]]

Benoit: Do you had time to look at this ?

Comment 5

[Bill Appleton]

An easy way to see this problem is to resize the browser window -- while you drag the corner everything in the plugin will look fine. But when you let go the rectangle will start updating, and everything outside the rectangle will turn white.

Comment 6

[Benoit Girard (:BenWa)]

(In reply to Matthias Versen (Matti) from comment #4)
> Benoit: Do you had time to look at this ?

I wish I did but I don't at the moment. I'm already look at too many issues. This is a serious problem that we need to fix.

Comment 7

[Benjamin Smedberg]

smichaud said he'll take a look at this next week unless somebody else steals it first.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

I can reproduce this (testing on OS X 10.7.5), in FF 17 and today's mozilla-central nightly.  I'll look for a regression range.

Comment 9

[Benoit Girard (:BenWa)]

My guess for the regression range is async plugin drawing which puts CoreGraphics over CoreAnimation.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #9)

You seem to be right.

Here's the regression range:
firefox-2011-07-13-03-07-41-mozilla-central
firefox-2011-07-14-03-07-48-mozilla-central
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=931f06b80727&tochange=34b0b3bc6984

Which includes the patch for bug 663259:
http://hg.mozilla.org/mozilla-central/rev/5cea57e451dd

This is further confirmed by the fact that turning off plugins.use_layers stops the problem from happening in the 2011-07-14 mozilla-central nightly.

(By the way, the plugins.use_layers setting seems to have disappeared in current code.  Is it possible any longer to turn off async plugin drawing?)

Comment 11

[Benoit Girard (:BenWa)]

I have more time then I did two week ago. Let me take this bug off your hands.

turning off async plugin drawing isn't something we want to support, even just for testings. I'd like for us to remove plugin code paths as we drop support for 10.5 and soon for in process plugins.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

> Let me take this bug off your hands.

Thank you! :-)

Comment 13

[Benoit Girard (:BenWa)]

Alright what's going on is that we overwrite CALayer drawInContext. Using this method we need to handle the drawing ourselves. CALayer provides no facility to retain what was drawn between frames.

We have two solutions:
1) Have the plugin draw everything. Low effort, low perf.
2) Keep a texture on our side, draw and move it to a texture using glTexSubImage2D. Mid effort, high perf.

Both options are viable. The CoreGraphics isn't a high performance paths. Plugins can just provide their own CALayer and get the best performance.

Comment 14

[Benoit Girard (:BenWa)]

Attachment: patch1

Comment 15

[Benoit Girard (:BenWa)]

Comment on attachment 687907 [details] [diff] [review]
patch1

Actually option 2 wont work as I had in-visioned. We're got double buffering happening here so a plugin will need to draw its dirty rect twice. I think the right thing to do here is let the plugin find it's optimal way to retain contain for double buffering rather then provide that by default and penalize the performance of plugins that handle this efficiently.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 687907 [details] [diff] [review]
patch1

> 1) Have the plugin draw everything. Low effort, low perf.

So you're choosing option 1?

Doesn't this patch obsolete CGBridgeLayer.mUpdateRect and -[CGBridgeLayer updateRect:]?  Why not just get rid of them and make mUpdateRect a local variable in -[CGBridgeLayer drawInContext:]?

(I haven't yet tested your patch with this bug's testcase, but will do so shortly.)

Comment 17

[Benoit Girard (:BenWa)]

(In reply to Steven Michaud from comment #16)
> Comment on attachment 687907 [details] [diff] [review]
> patch1
> 
> > 1) Have the plugin draw everything. Low effort, low perf.
> 
> So you're choosing option 1?
> 
> Doesn't this patch obsolete CGBridgeLayer.mUpdateRect and -[CGBridgeLayer
> updateRect:]?  Why not just get rid of them and make mUpdateRect a local
> variable in -[CGBridgeLayer drawInContext:]?
>
 
We could kill it.

> (I haven't yet tested your patch with this bug's testcase, but will do so
> shortly.)

Keep in mind that the testcase doesn't comply to the behavior it claims. If asked to paint everything it will only paint one color. It is not complying with the paint event rect.

Comment 18

[Benoit Girard (:BenWa)]

Note: If you don't have a universal build make sure to clean the project and select the right platform in xcode.

Comment 19

[Benjamin Smedberg]

On non-mac, we read back the previously painted buffer (which is now the active buffer) because they are both just shared-memory bitmaps:

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginInstanceChild.cpp#3682

Comment 20

[Benoit Girard (:BenWa)]

We double buffer using GPU memory on mac. The overhead of a lock, copy, unlock isn't worth it. Rolling out our own code here to avoid this copy is just going to make Flash slower since it's going to paint to the whole buffer anyways. We could maybe avoid this by clipping the surface to the paint rect.

I'm not terribly interested in optimizing this (but I could be convinced) since it's something plugins can easily do themselves or better yet they can provide their own CALayer and implement the exact semantic they want without any overhead on our side.

Comment 21

[Bill Appleton]

I'm hoping FireFox will

1) follow the NPAPI spec

2) be compatible with how the windows version works

3) be like the other browsers and support NPAPI

Chrome and Safari handle updates correctly on OSX

Thanks

Comment 22

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #17)

> Keep in mind that the testcase doesn't comply to the behavior it
> claims. If asked to paint everything it will only paint one
> color. It is not complying with the paint event rect.

I don't really understand what you're saying here.

But it's true enough that the testcase is so strange that it's only a
matter of luck that it "works" properly in other browsers.  I *don't*
think its behavior in FF (with or without your patch) fails to conform
to the NPAPI.

Here's the problem with the testcase in a nutshell:

It calls CGContextFillRect() to set the fill color for the entire
rectangle that has been passed to the plugin's NPP_HandleEvent()
handler (NPCocoaEvent.data.draw.x,y,width,height), then tries to use
NPN_InvalidateRect() to limit the range of the update to a
different/smaller rectangle.

I think this amounts to a misunderstanding of what
NPN_InvalidateRect() is for.  It invalidates a particular rectangle,
but doesn't limit the invalidation to that particular rectangle.  (See
https://developer.mozilla.org/en-US/docs/NPN_InvalidateRect.)

That said, Bill Appleton may be able to come up with a better
testcase.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 687907 [details] [diff] [review]
patch1

In principle this is fine with me.  And I don't think it would adversely effect many plugins (at least ones that behave properly).  Most of the ones we really care about already use CoreAnimation, where the problem doesn't arise.

But if you're going to do it you should also get rid of CGBridgeLayer.mUpdateRect and -[CGBridgeLayer updateRect:].

Comment 24

[Bill Appleton]

I dont see what CGContextFillRect has to do with it, that was just a simple example. Our engine maintains an off screen bitmap. When we get a draw event we copy that rectangle to the screen.

On FF the rest of the screen turns white when we do that. So if you drag something you can see the object but everything else is white. The only work around is to copy the entire screen every update event.

Comment 25

[Bill Appleton]

I probably should clarify: we ALWAYS paint the rectangle that is received in the update event. We paint that entire rectangle every time. But if you do that the area OUTSIDE this rectangle turns white.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

> We paint that entire rectangle every time. But if you do that the
> area OUTSIDE this rectangle turns white.

Benoit's patch from comment #15 should fix this.

In my attempt to figure out your testcase (which is *very* confusing)
I'd forgotten that this was why you'd originally filed this bug.
Benoit's patch still "breaks" your testcase, but doesn't cause the
area outside what's updated to turn white.

I'll start a tryserver build with Benoit's patch, which should be
available in a few hours.  When that happens I'll post a link here,
and you can test with it.

Comment 27

[Bill Appleton]

Thank you.

I was hoping the test case was simple but I see there is some confusion. Please think about it this way:

Make an NPAPI plugin that draws a bit mapped picture on the screen.

For some reason part of the picture is invalidated... what happens next?

You get a draw event with that rectangle, and then the plugin draws that area.

When this happens the rectangle is updated properly but the REST of the screen turns white.

Really. This is a very fundamental problem. I'm not sure why this doesn't effect every plugin vendor.

Thanks again.

Comment 28

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with Benoit's patch from comment #15:
http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-8ea0ebb5ec6e/try-macosx64/firefox-20.0a1.en-US.mac.dmg

Please try it out, Bill, and let us know your results.

(There were no test failures.)

Comment 29

[Bill Appleton]

Hi all, the new build definitely fixed the white screen issue over here. I tried out our main plugin with smart rectangle updates turned on and everything worked as expected. The same plugin was also tested with the current version of FF and that still did have the white screen problem as expected. Let me know if there is anything else that can be done to help.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Benoit, how about landing your patch :-)

Comment 31

[Benoit Girard (:BenWa)]

Opps:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5844e951d2

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2b5844e951d2