596414 - Plugins are broken with OGL on os x

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

This seems to be related to the NSViews we have for plugins. Hiding the NSViews makes things better and get's things mostly drawing but events don't seem to work entirely and invalidation is finicky.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Get things working a little bit

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Mouse click events seem to work. Mouse-over not so much.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

If you make the plugin's ChildView/NSView always hidden, it should *never* get any events.  I'll bet that what events you appear to be getting are in fact coming from the plugin view's superview.

*Please* don't plan on keeping this change for FF 4.  It's more or less equivalent to doing away with plugin widgets altogether, and is too big a change to make at this late stage for FF 4.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Where is the OGL patch, so I can try it out?

Comment 6

[Joe Drew (not getting mail)]

All you need to do is apply your popup window patch and then set the pref layers.accelerate-all to true.

Comment 7

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

Attachment: fix

Comment 8

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

Attachment: Part 2: fix nsViewManager::UpdateWidgetArea

A few things here:

The check "Don't mess with views that are in completely different view manager trees" can never fail anymore, so remove it.

The only child widgets we can reach by traversing child views can be plugins or popups. Skip popups and the rest are plugins.

We do not need to invalidate in plugin widgets on any platform, because we don't paint plugin widgets in any platform (including Mac, with the previous patch here). So just take out the recursive call to UpdateWidgetArea.

We should not remove the area of the child widget from the area to invalidate, on Mac, because with the previous patch the plugin widget is completely invisible on Mac and we still need to paint the content underneath it (which will typically, but not always, be the plugin itself).

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 475433 [details] [diff] [review]
Part 2: fix nsViewManager::UpdateWidgetArea

>+        NS_ASSERTION(type == eWindowType_plugin,
>+                     "Only plugin or popup widgets can be children!");

We can't reach dialogs here?

>+        // We do not need to invalidate in plugin widgets, but we should
>+        // exclude them from the invalidation region IF we're not on
>+        // Mac. On Mac we need to draw under plugin widgets, because
>+        // plugin widgets are basically invisible

Make it clear in this comment that we are actually drawing the contents of the plugin under the plugin widget, not some other random content.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 475425 [details] [diff] [review]
fix

I think we also need to make isOpaque return NO for plugin widgets. Otherwise drawRect might not be called on the plugin's superview when the invalid region is completely contained in the plugin's rect.

Comment 11

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

With these patches, Flash works, Java works (Carbon plugin), but Quicktime doesn't (also Carbon plugin). With Quicktime, the controls render but the movie doesn't display. I guess Quicktime is doing something clever that our use of GL interferes with.Maybe if we enable the Core Animation drawing model, Quicktime would work? Anyone know how to do that?

Comment 12

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

(In reply to comment #9)
> We can't reach dialogs here?

No, dialogs are toplevel.

> >+        // We do not need to invalidate in plugin widgets, but we should
> >+        // exclude them from the invalidation region IF we're not on
> >+        // Mac. On Mac we need to draw under plugin widgets, because
> >+        // plugin widgets are basically invisible
> 
> Make it clear in this comment that we are actually drawing the contents of the
> plugin under the plugin widget, not some other random content.

We could be drawing a mixture of the plugin and random content, all into the toplevel window. I'll adjust the comment to reflect that.

(In reply to comment #10)
> I think we also need to make isOpaque return NO for plugin widgets. Otherwise
> drawRect might not be called on the plugin's superview when the invalid region
> is completely contained in the plugin's rect.

OK!

Comment 13

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

(In reply to comment #11)
> Maybe if we enable the Core Animation drawing model, Quicktime
> would work? Anyone know how to do that?

Oh, I guess Core Animation drawing is 10.6 only. I've only got 10.5, so someone else should test this on 10.6...

Comment 14

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

Attachment: fix v2

Making isOpaque return false for plugin views.

Comment 15

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

Hmm, fix v2 makes Quicktime *almost* work. It plays, but it keeps blacking out.

Comment 16

[Markus Stange [:mstange]]

(In reply to comment #11)
> Maybe if we enable the Core Animation drawing model, Quicktime
> would work? Anyone know how to do that?

If somebody knows, Benoit will.

Comment 17

[Benoit Girard (:BenWa)]

If I recall correctly Quicktime will look if Core Animation is supported. If it is not it will used Quickdraw. I always make sure by checking what's being negotiated on NPAPI to be 100% sure.

How is the plug-in being drawn with OGL? Are we still using an image surface? I think there's the potential to create a layer backed by a Core Animation layer instead that could give us better performance.

Comment 18

[Benoit Girard (:BenWa)]

Also keep in mind when testing this that there's bug 594635 that is affecting Quicktime on 10.6.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

I've been testing this bug's patches plus my patch for bug 594796,
with layers.accelerate-all set to 'true'.

For what it's worth, the following QuickTime videos (which aren't
effected by bug	594635)	play very well on OS X 10.6.4, but only very
choppily on OS X 10.5.8.

http://trailers.apple.com/trailers/dreamworks/megamind/

I'm not sure what the crucial difference is.

Comment 20

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

Joe is working on new patches here to ensure that we continue painting NSViews for Carbon plugins so they don't break when OGL is disabled.

Comment 21

[Joe Drew (not getting mail)]

Attachment: add nsIWidget::IsSelfDrawn

Add a new virtual to nsIWidget, IsSelfDrawn. This returns whether the widget is going to draw itself from outside of Gecko, and thus we should draw ourselves underneath it. This only ever returns true if the widget represents a QuickDraw plugin.

Comment 22

[Joe Drew (not getting mail)]

Attachment: don't subtract self-drawn widgets from our widget area

Comment 23

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

Comment on attachment 475757 [details] [diff] [review]
add nsIWidget::IsSelfDrawn

Cocoa plugin windows don't really paint themselves "independently of us". They don't get painted at all.

How about calling the method IsPaintedByGecko? In nsBaseWidget, have it return true for all non-plugin widgets, and in nsChildView return true for Carbon plugin widgets as well.

The comment would be something like
 * Return true when this widget paints by dispatching paint events into Gecko.
 * If true, we need to invalidate this widget when content changes.

Comment 24

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

I mean Quickdraw!

Comment 25

[Joe Drew (not getting mail)]

Attachment: don't paint the plugin views for quickdraw plugins

This patch causes flash not to draw on 10.6. I don't remember, and haven't checked currently, whether Rob's "fix v2" patch has the same effect.

Comment 26

[Joe Drew (not getting mail)]

Attachment: don't paint the plugin views for quickdraw plugins v2

I made a dumb mistake in the past patch by subtracting all child view rects from the region, not only quickdraw view rects. This patch fixes that and restores drawing to plugins on 10.6. Unfortunately flip4mac still causes a lot of bad drawing, but that might be something we need to fix subsequent to this bug.

Comment 27

[Joe Drew (not getting mail)]

Attachment: add nsIWidget::IsPaintedByGecko

Comment 28

[Joe Drew (not getting mail)]

Attachment: only subtract plugin widgets drawn by gecko from the region

Comment 29

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

Flip4mac is quickdraw right? It seems strange that these patches would break it, since we're not changing behaviour for Quickdraw plugins, I think.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

> Flip4mac is quickdraw right?

As I understand it, Flip4Mac uses the QuickTime framework.

Comment 31

[Joe Drew (not getting mail)]

I should have mentioned - the bad drawing happens with OpenGL on and flip4mac; i.e. this bug is not fixed for flip4mac.

Comment 32

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

So flip4Mac works without OpenGL, and it is safe to land these patches on trunk? And it would also be safe to enable OpenGL for 64-bit builds only, where we don't support Quickdraw anyway?

Comment 33

[Joe Drew (not getting mail)]

That looks to be the case, yes.

Comment 34

[Josh Aas]

Comment on attachment 475793 [details] [diff] [review]
don't paint the plugin views for quickdraw plugins v2

Discussed the updated necessary here with roc.

Comment 35

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

We need to handle Core Animation drawing model plugins here. The thing to do is give the drawing model directly to the widget.

Comment 36

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

Attachment: Part 0: Tell ChildView the plugin drawing model

Comment 37

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

I just noticed that we seem to repaint CoreGraphics plugins on every paint, no matter what is being repainted, which is terrible! We need to fix that, but probably as a separate bug.

Comment 38

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

That'll be bug 596491.

Comment 39

[Joe Drew (not getting mail)]

Attachment: part 1: don't paint any plugin views

This is built on top of roc's patch. We now don't draw any plugin views, but we *do* clip ourselves out of quickdraw plugin regions.

Comment 40

[Joe Drew (not getting mail)]

Attachment: part 2: add nsIWidget::IsPaintedByGecko

Unbitrotted from other changes.

Comment 41

[Joe Drew (not getting mail)]

Attachment: part 3: only subtract plugin widgets drawn by gecko from the region

Unbitrotted.

Comment 42

[Josh Aas]

Comment on attachment 475997 [details] [diff] [review]
Part 0: Tell ChildView the plugin drawing model

>-  if (mPluginInstanceOwner && !mPluginIsCG) {
>+  if (mPluginInstanceOwner && mView &&
>+      [(ChildView*)mView pluginDrawingModel] == NPDrawingModelQuickDraw) {

Does this change the behavior for Core Animation plugins in a way we care about?

> #ifndef NP_NO_CARBON
>     mPluginEventModel = NPEventModelCarbon;
> #else
>     mPluginEventModel = NPEventModelCocoa;
> #endif
>+    mPluginDrawingModel = NPDrawingModelCoreGraphics;

We probably need an NP_NO_QUICKDRAW condition here, much like the event model code above. Where that is true the default is core graphics, where it is false the default is Quickdraw. When the Quickdraw model is available it is the default, plugins don't have to set a drawing model to select it.

Comment 43

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

(In reply to comment #42)
> Comment on attachment 475997 [details] [diff] [review]
> Part 0: Tell ChildView the plugin drawing model
> 
> >-  if (mPluginInstanceOwner && !mPluginIsCG) {
> >+  if (mPluginInstanceOwner && mView &&
> >+      [(ChildView*)mView pluginDrawingModel] == NPDrawingModelQuickDraw) {
> 
> Does this change the behavior for Core Animation plugins in a way we care
> about?

I think mPluginIsCG was actually being set to true for Core Animation plugins, since nsObjectFrame::CallSetWindow calls nsPluginInstanceOwner::GetPluginPortFromWidget which does
    if (GetDrawingModel() == NPDrawingModelCoreGraphics || 
        GetDrawingModel() == NPDrawingModelCoreAnimation ||
        GetDrawingModel() == NPDrawingModelInvalidatingCoreAnimation)
      result = mWidget->GetNativeData(NS_NATIVE_PLUGIN_PORT_CG); 

So, no.

> > #ifndef NP_NO_CARBON
> >     mPluginEventModel = NPEventModelCarbon;
> > #else
> >     mPluginEventModel = NPEventModelCocoa;
> > #endif
> >+    mPluginDrawingModel = NPDrawingModelCoreGraphics;
> 
> We probably need an NP_NO_QUICKDRAW condition here, much like the event model
> code above. Where that is true the default is core graphics, where it is false
> the default is Quickdraw. When the Quickdraw model is available it is the
> default, plugins don't have to set a drawing model to select it.

OK.

Comment 44

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

Joe, Jeff patch for Quickdraw plugins makes this much easier. With Jeff's Quickdraw plugin patch (assuming it works), we don't need IsPaintedByGecko anymore (because we won't paint Quickdraw plugins by entering Gecko frame painting anymore), we can just check type == eWindowType_plugin.

Comment 45

[Joe Drew (not getting mail)]

Comment on attachment 475433 [details] [diff] [review]
Part 2: fix nsViewManager::UpdateWidgetArea

With bug 597268, we can do this for all plugins on OS X, and so roc's earlier patch works nicely.

Comment 46

[Joe Drew (not getting mail)]

Comment on attachment 476065 [details] [diff] [review]
part 2: add nsIWidget::IsPaintedByGecko

We don't need this with Jeff's patch in bug 597268.

Comment 47

[Joe Drew (not getting mail)]

Comment on attachment 476067 [details] [diff] [review]
part 3: only subtract plugin widgets drawn by gecko from the region

Without IsPaintedByGecko, we can just use roc's patch earlier.

Comment 48

[Joe Drew (not getting mail)]

Attachment: Part 1: don't paint any plugin views at all

This patch builds on bug 597268, and just stops painting for all plugin views.

Comment 49

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

Attachment: Part 0 v2

Comment 50

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

Attachment: Part 0 v3

Oops, v2 didn't build

Comment 51

[Joe Drew (not getting mail)]

Attachment: part 0 v4

This makes the patch compile on Mac64.

Comment 52

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

Comment on attachment 476185 [details] [diff] [review]
part 0 v4

I guess we should rev the IID in nsIPluginWidget as well

Comment 53

[Joe Drew (not getting mail)]

Attachment: Part 1: don't paint any plugin views at all, compile on mac64

Comment 54

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/f005b3142706
http://hg.mozilla.org/mozilla-central/rev/5cb88a72483e
http://hg.mozilla.org/mozilla-central/rev/9c33280093f8