Plugins are broken with OGL on os x

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jrmuizel, Assigned: roc)

Tracking

unspecified
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(3 attachments, 16 obsolete attachments)

3.96 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
12.18 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.97 KB, patch
roc
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
blocking2.0: --- → beta7+
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.
Mouse click events seem to work. Mouse-over not so much.
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.
Where is the OGL patch, so I can try it out?
All you need to do is apply your popup window patch and then set the pref layers.accelerate-all to true.
Created attachment 475425 [details] [diff] [review]
fix
Assignee: nobody → roc
Attachment #475425 - Flags: review?(jmuizelaar)
Attachment #475425 - Flags: review?(jmuizelaar) → review?(joshmoz)
Created attachment 475433 [details] [diff] [review]
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).
Attachment #475433 - Flags: review?(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.
Attachment #475433 - Flags: review?(tnikkel) → review+
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.
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?
(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!
(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...
Created attachment 475451 [details] [diff] [review]
fix v2

Making isOpaque return false for plugin views.
Attachment #475451 - Flags: review?(joshmoz)
Attachment #475425 - Attachment is obsolete: true
Attachment #475425 - Flags: review?(joshmoz)
Hmm, fix v2 makes Quicktime *almost* work. It plays, but it keeps blacking out.
(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.
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.
Also keep in mind when testing this that there's bug 594635 that is affecting Quicktime on 10.6.
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.
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.
Created attachment 475757 [details] [diff] [review]
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.
Attachment #475757 - Flags: review?(roc)
Created attachment 475758 [details] [diff] [review]
don't subtract self-drawn widgets from our widget area
Attachment #475433 - Attachment is obsolete: true
Attachment #475758 - Flags: review?(roc)
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.
Created attachment 475789 [details] [diff] [review]
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.
Attachment #475789 - Flags: review?(roc)
Attachment #475789 - Flags: review?(jmuizelaar)
Created attachment 475793 [details] [diff] [review]
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.
Attachment #475451 - Attachment is obsolete: true
Attachment #475789 - Attachment is obsolete: true
Attachment #475793 - Flags: review?(roc)
Attachment #475793 - Flags: review?(joshmoz)
Attachment #475789 - Flags: review?(roc)
Attachment #475789 - Flags: review?(jmuizelaar)
Attachment #475451 - Flags: review?(joshmoz)
Created attachment 475794 [details] [diff] [review]
add nsIWidget::IsPaintedByGecko
Attachment #475757 - Attachment is obsolete: true
Attachment #475794 - Flags: review?(roc)
Attachment #475757 - Flags: review?(roc)
Created attachment 475795 [details] [diff] [review]
only subtract plugin widgets drawn by gecko from the region
Attachment #475758 - Attachment is obsolete: true
Attachment #475795 - Flags: review?(roc)
Attachment #475758 - Flags: review?(roc)
Attachment #475313 - Attachment is obsolete: true
Attachment #475793 - Flags: review?(roc) → review+
Attachment #475794 - Flags: review?(roc) → review+
Attachment #475795 - Flags: review?(roc) → review+
Flip4mac is quickdraw right? It seems strange that these patches would break it, since we're not changing behaviour for Quickdraw plugins, I think.
> Flip4mac is quickdraw right?

As I understand it, Flip4Mac uses the QuickTime framework.
I should have mentioned - the bad drawing happens with OpenGL on and flip4mac; i.e. this bug is not fixed for flip4mac.
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?
That looks to be the case, yes.

Comment 34

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

Discussed the updated necessary here with roc.
Attachment #475793 - Flags: review?(joshmoz) → review-
We need to handle Core Animation drawing model plugins here. The thing to do is give the drawing model directly to the widget.
Created attachment 475997 [details] [diff] [review]
Part 0: Tell ChildView the plugin drawing model
Attachment #475997 - Flags: review?(joshmoz)
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.
Created attachment 476064 [details] [diff] [review]
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.
Attachment #475793 - Attachment is obsolete: true
Attachment #476064 - Attachment description: don't paint any plugin views → part 1: don't paint any plugin views
Attachment #476064 - Flags: review?(roc)
Attachment #476064 - Flags: review?(joshmoz)
Created attachment 476065 [details] [diff] [review]
part 2: add nsIWidget::IsPaintedByGecko

Unbitrotted from other changes.
Attachment #475794 - Attachment is obsolete: true
Attachment #476065 - Flags: review?(roc)
Created attachment 476067 [details] [diff] [review]
part 3: only subtract plugin widgets drawn by gecko from the region

Unbitrotted.
Attachment #475795 - Attachment is obsolete: true
Attachment #476067 - Flags: review?(roc)

Comment 42

8 years ago
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.
(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.
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 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.
Attachment #475433 - Attachment is obsolete: false
Comment on attachment 476065 [details] [diff] [review]
part 2: add nsIWidget::IsPaintedByGecko

We don't need this with Jeff's patch in bug 597268.
Attachment #476065 - Attachment is obsolete: true
Attachment #476065 - Flags: review?(roc)
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.
Attachment #476067 - Attachment is obsolete: true
Attachment #476067 - Flags: review?(roc)
Created attachment 476139 [details] [diff] [review]
Part 1: don't paint any plugin views at all

This patch builds on bug 597268, and just stops painting for all plugin views.
Attachment #476064 - Attachment is obsolete: true
Attachment #476064 - Flags: review?(roc)
Attachment #476064 - Flags: review?(joshmoz)
Attachment #476139 - Flags: review?(roc)
Attachment #476139 - Flags: review?(joshmoz)
Attachment #476139 - Flags: review?(roc) → review+
Created attachment 476145 [details] [diff] [review]
Part 0 v2
Attachment #475997 - Attachment is obsolete: true
Attachment #476145 - Flags: review?(joshmoz)
Attachment #475997 - Flags: review?(joshmoz)
Created attachment 476146 [details] [diff] [review]
Part 0 v3

Oops, v2 didn't build
Attachment #476145 - Attachment is obsolete: true
Attachment #476146 - Flags: review?(joshmoz)
Attachment #476145 - Flags: review?(joshmoz)

Updated

8 years ago
Attachment #476146 - Flags: review?(joshmoz) → review+

Updated

8 years ago
Attachment #476139 - Flags: review?(joshmoz) → review+
Created attachment 476185 [details] [diff] [review]
part 0 v4

This makes the patch compile on Mac64.
Attachment #476146 - Attachment is obsolete: true
Attachment #476185 - Flags: review?(roc)
Comment on attachment 476185 [details] [diff] [review]
part 0 v4

I guess we should rev the IID in nsIPluginWidget as well
Attachment #476185 - Flags: review?(roc) → review+
Created attachment 476188 [details] [diff] [review]
Part 1: don't paint any plugin views at all, compile on mac64
Attachment #476139 - Attachment is obsolete: true
Attachment #476188 - Flags: review?(roc)
Attachment #476188 - Attachment is patch: true
Attachment #476188 - Attachment mime type: application/octet-stream → text/plain
Attachment #476188 - Flags: review?(roc) → review+
Depends on: 597268
Depends on: 600148
Depends on: 603731
You need to log in before you can comment on or make changes to this bug.