442109 - youtube videos don't play.

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Doug Turner (:dougt)]

go to youtube.com and try to play a video  I see the text:

Hello, you either have JavaScript turned off or an old version of Adobe's Flash Player.  Get the lasted Flash player.

Comment 1

[Stuart Parmenter]

do you have flash installed?

Comment 2

[Doug Turner (:dougt)]

yeah, to works in microb.  I didn't move the np* files around.

Comment 4

[Antonio Gomes (tonikitoo)]

depended of bug 313462 ?

Comment 6

[Stuart Parmenter]

For this, we basically need to make windowed plugins render like windowless ones.  The best way to do this is probably to host windowed plugins in an external window and use the X Composite and/or Damage extensions to render the contents in to an offscreen buffer and treat the plugin as windowless.  We'll have to do some work to pass events through to the plugin like we would in windowless mode, even if the plugin doesn't support that mode.

Comment 7

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

Who's supposed to do this, and when?

When I talked to Benjamin Otte about this, he mentioned that transforming input coordinates was not really supported currently with Composite/Damage. I don't know the details, though.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

I'm taking a look at this now. I haven't looked at how to do input yet, but I think some sort of input redirection is possible...

Comment 9

[Andrey Petrov]

Stuart, did you try any prototyping, with the xdamage option of yours?
My concern is that performance degradation might be quite severe

Comment 10

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

BTW, why was the approach of forcing all Flash objects to be windowless rejected? Unavailability of a Flash 10 player?

Comment 11

[Stuart Parmenter]

Andrey: Jeff was just looking at it, but we haven't had a chance to do much prototyping.  Performance should be OK, but it is certainly not as optimal as having plugins that natively support windowless mode.

roc: We don't know when Flash 10 for ARM Linux will be out and we need to have plugins working by beta.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Attachment: Composited plugins prototye

Here's a really rough prototype that enables composited plugins. It works in firefox, (the plugins are drawn with a yellow tinge). However, I haven't been able to get it to work with fennec yet, and am not sure why. (Perhaps some eventy things are wrong...)

The patch also depends on gdk_window_set_composited() which only exists with gtk 2.12 (the n810 has gtk 2.10). I think we'll probably be able to get somesort of hacky solution working for gtk 2.10.

Comment 13

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

How's performance?

Comment 14

[Jeff Muizelaar [:jrmuizel]]

youtube works fine when painting using cairo_paint_with_alpha(.5) in my linux vm. (plain cairo_paint() is broken because it does a XCopyArea without IncludeInferiors). It sits at about 70% cpu, but this is with gDumpPaintList = PR_TRUE; but piped to /dev/null. The same video the regular ubuntu firefox3 uses about 50% cpu.

Comment 15

[Andrey Petrov]

guys, as I understand you just composite window elsewhere.
but what happens if there is an edit form created by flash and other mouse/key event of interactions?

Comment 16

[Jeff Muizelaar [:jrmuizel]]

I've looked into why it works in firefox and not in fennec. The reason is that the window we paint into on fennec has an empty clip whereas in firefox it has a reasonable clip. I'm don't know why this is. Anyone have any ideas?

Comment 17

[Jeff Muizelaar [:jrmuizel]]

I've been able to get plugins painting in fennec by parenting them in a top level window. I don't have input working yet though.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

It looks like we'll be able to use XSendEvent to redirect input.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Attachment: Prototype composited plugins for fennec

This patch is enough to make youtube basically work (drawing and mouse clicks). It is still very rough and currently breaks windowless plugins.

I've yet to try it out on an actual n810.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Attachment: Updated version of composited plugins for fennec

The updated adds mouse and keyboard input and improves drawing performance.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

Attachment: fennec plugins v2

I believe the last patch was broken. This patch should be better and also forces a link with libesd so flash player doesn't cause us to exit when it tries to play sound.

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Attachment: fennec plugins v3

notes:
- should have no affect on the current code. i.e. everything is ifdef MOZ_COMPOSITED_PLUGINS
- MOZ_COMPOSITED_PLUGINS is currently defined for debugging purposes. It will conditional on MOZ_PLATFORM_HILDON in the final patch.
- There's a fair amount of uglyness. Any suggestions on how to clean it up are welcome.

known issues:
- currently breaks windowless plugins. This shouldn't be that big a deal on the n810 because I don't think there are any windowless plugins.

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 363786 [details] [diff] [review]
fennec plugins v3

Overall, this looks really good.. just a bit more cleanup/build config stuff is needed, I think.  Also, can you add the option to your hgrc to get 8 lines of diff context?

>diff --git a/configure.in b/configure.in
>index 62a6178..637f594 100644
>--- a/configure.in
>+++ b/configure.in
>@@ -3121,7 +3121,7 @@ else
>     LDFLAGS="$XLDFLAGS $LDFLAGS"
>     AC_CHECK_LIB(X11, XDrawLines, [X11_LIBS="-lX11"],
>         [MISSING_X="$MISSING_X -lX11"], $XLIBS)
>-    AC_CHECK_LIB(Xext, XextAddDisplay, [XEXT_LIBS="-lXext"],
>+    AC_CHECK_LIB(Xext, XextAddDisplay, [XEXT_LIBS="-lXext -lXcomposite -lXdamage -lXfixes"],
>         [MISSING_X="$MISSING_X -lXext"], $XLIBS)

Cheater ;)

Test for Xcomposite/Xdamage/Xfixes separately, and possibly set some flag like HAS_X11_COMPOSITE; otherwise we'll probably break Sun and anyone with an ancient X server.


>diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp
>index 516ef5f..64aebed 100644
>--- a/layout/generic/nsObjectFrame.cpp
>+++ b/layout/generic/nsObjectFrame.cpp
>@@ -167,9 +167,23 @@ enum { XKeyPress = KeyPress };
> #ifdef KeyPress
> #undef KeyPress
> #endif
>+
>+#define MOZ_COMPOSITED_PLUGINS 1
>+#ifdef  MOZ_COMPOSITED_PLUGINS

This won't really work, now :)  I'd suggest using MOZ_HILDON or whatever we're calling that thing these days... or perhaps adding a new configure option, --enable-x11-composited-plugins?

>+extern "C" __attribute__ ((visibility ("default"))) void
>+cairo_paint (cairo_t *cr);
>+extern "C" __attribute__ ((visibility ("default"))) void
>+cairo_set_source_rgb (cairo_t *cr, double b , double n, double a);
>+extern "C" __attribute__ ((visibility ("default"))) void
>+cairo_set_source_rgba (cairo_t *cr, double n, double b , double n, double a);
>+extern "C" __attribute__ ((visibility ("default"))) void
>+cairo_paint_with_alpha (cairo_t *cr, double a);

Are these for the system cairo versions?  I'd be more comfortable if you were to dlsym them instead of letting the linker find them (also probably needs some #ifdef cairo_paint #undef cairo_paint action here like below).

>-#ifndef XP_MACOSX
>+#if !defined(XP_MACOSX) && !defined(MOZ_COMPOSITED_PLUGINS)

What's different about OSX and Composited plugins?  Maybe have a:

#if !defined(XP_MACOSX) && !defined(MOZ_COM...)
#define MAGIC_THING_THATS_DIFFERENT
#endif

and then use #ifndef MAGIC_THING so it's clearer why those two are different.

> 
>+static void find_dest_id(XID top, XID *root, XID *dest, int target_x, int target_y)

This should just be defined if COMPOSITED_PLUGINS, right?  (Especially given that it doesn't seem to be #ifdef MOZ_X11, given that that's the next block...)

>+#ifdef MOZ_X11
>+nsEventStatus nsPluginInstanceOwner::ProcessEventX11Composited(const nsGUIEvent& anEvent)

Why MOZ_X11 and not X11_COMPOSITED_PLUGINS?  Do we need this in the non-composited case?


>@@ -4386,7 +4708,56 @@ nsPluginInstanceOwner::Renderer::NativeDraw(QWidget * drawable,
>   PRBool eventHandled = PR_FALSE;
>   mInstance->HandleEvent(&pluginEvent, &eventHandled);
> #endif
>+#endif
>+
>+#ifdef MOZ_COMPOSITED_PLUGINS
>+// make sure we're using system cairo
>+#undef cairo_set_source_rgb
>+#undef cairo_set_source_rgba
>+#undef cairo_paint
>+#undef cairo_paint_with_alpha

This needs to be earlier, so you get the prototype (in case something brought in the cairo remap stuff), but should also be later.. hence getting this from dlsym probably begin clearer.  You should be able to get these via dlsym(RTLD_DEFAULT, ...) without needing to know the location of the system cairo shlib. 

>+  /* XXX: this is very nasty. We need a better way of getting at mPlugWindow */
>+  //GdkWindow *plug = ((GtkWidget*)*(((nsPluginNativeWindow*)mWindow)->mPlugWindow))->window;
>+  GtkWidget *plug = (GtkWidget*)(((nsPluginNativeWindow*)mWindow)->mPlugWindow);
>+  //GtkWidget *plug = (GtkWidget*)(((nsPluginNativeWindowGtk2*)mWindow)->mSocketWidget);

Youch.  Probably no better way, though.

Though...

>+#if 0

If you're using XCopyArea anyway, why not just get rid of the system cairo goop?

> [...]
>+#else
>+  /* Cairo has bugs with IncludeInferiors when using paint
>+   * so we use XCopyArea directly instead. */

Comment 25

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #24)
> (From update of attachment 363786 [details] [diff] [review])
> Overall, this looks really good.. just a bit more cleanup/build config stuff is
> needed, I think.  Also, can you add the option to your hgrc to get 8 lines of
> diff context?
> 
> >diff --git a/configure.in b/configure.in
> >index 62a6178..637f594 100644
> >--- a/configure.in
> >+++ b/configure.in
> >@@ -3121,7 +3121,7 @@ else
> >     LDFLAGS="$XLDFLAGS $LDFLAGS"
> >     AC_CHECK_LIB(X11, XDrawLines, [X11_LIBS="-lX11"],
> >         [MISSING_X="$MISSING_X -lX11"], $XLIBS)
> >-    AC_CHECK_LIB(Xext, XextAddDisplay, [XEXT_LIBS="-lXext"],
> >+    AC_CHECK_LIB(Xext, XextAddDisplay, [XEXT_LIBS="-lXext -lXcomposite -lXdamage -lXfixes"],
> >         [MISSING_X="$MISSING_X -lXext"], $XLIBS)
> 
> Cheater ;)
> 
> Test for Xcomposite/Xdamage/Xfixes separately, and possibly set some flag like
> HAS_X11_COMPOSITE; otherwise we'll probably break Sun and anyone with an
> ancient X server.

Done.

> >diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp
> >index 516ef5f..64aebed 100644
> >--- a/layout/generic/nsObjectFrame.cpp
> >+++ b/layout/generic/nsObjectFrame.cpp
> >@@ -167,9 +167,23 @@ enum { XKeyPress = KeyPress };
> > #ifdef KeyPress
> > #undef KeyPress
> > #endif
> >+
> >+#define MOZ_COMPOSITED_PLUGINS 1
> >+#ifdef  MOZ_COMPOSITED_PLUGINS
> 
> This won't really work, now :)  I'd suggest using MOZ_HILDON or whatever we're
> calling that thing these days... or perhaps adding a new configure option,
> --enable-x11-composited-plugins?

Done.
 
> >+extern "C" __attribute__ ((visibility ("default"))) void
> >+cairo_paint (cairo_t *cr);
> >+extern "C" __attribute__ ((visibility ("default"))) void
> >+cairo_set_source_rgb (cairo_t *cr, double b , double n, double a);
> >+extern "C" __attribute__ ((visibility ("default"))) void
> >+cairo_set_source_rgba (cairo_t *cr, double n, double b , double n, double a);
> >+extern "C" __attribute__ ((visibility ("default"))) void
> >+cairo_paint_with_alpha (cairo_t *cr, double a);
> 
> Are these for the system cairo versions?  I'd be more comfortable if you were
> to dlsym them instead of letting the linker find them (also probably needs some
> #ifdef cairo_paint #undef cairo_paint action here like below).

I just took out the cairo stuff.

> >-#ifndef XP_MACOSX
> >+#if !defined(XP_MACOSX) && !defined(MOZ_COMPOSITED_PLUGINS)
> 
> What's different about OSX and Composited plugins?  Maybe have a:
> 
> #if !defined(XP_MACOSX) && !defined(MOZ_COM...)
> #define MAGIC_THING_THATS_DIFFERENT
> #endif
> 
> and then use #ifndef MAGIC_THING so it's clearer why those two are different.


I agree that this is a good idea, however I'm not really sure why XP_MACOSX does what it does. It looks like MACOSX plugins have the window type set to windowed instead of drawable, but mostly act like they're drawables. Composited plugins have windows but act sort of like drawables. So the only real thing they share is that neither really fits nicely into the (nsPluginWindowType_Window|nsPluginWindowType_Drawable) categories.

Tomorrow, I'll look into whether we can have a type (or store some additional information) that describes how we interact with the plugin instead of whether the plugin thinks it's windowless or not. This will hopefully make it easier to support composited plugins along side windowless ones as well.


> > 
> >+static void find_dest_id(XID top, XID *root, XID *dest, int target_x, int target_y)
> 
> This should just be defined if COMPOSITED_PLUGINS, right?  (Especially given
> that it doesn't seem to be #ifdef MOZ_X11, given that that's the next block...)

Put into ifdef MOZ_COMPOSITED_PLUGINS

> 
> >+#ifdef MOZ_X11
> >+nsEventStatus nsPluginInstanceOwner::ProcessEventX11Composited(const nsGUIEvent& anEvent)
> 
> Why MOZ_X11 and not X11_COMPOSITED_PLUGINS?  Do we need this in the
> non-composited case?

Changed to MOZ_COMPOSITED_PLUGINS

> 
> >@@ -4386,7 +4708,56 @@ nsPluginInstanceOwner::Renderer::NativeDraw(QWidget * drawable,
> >   PRBool eventHandled = PR_FALSE;
> >   mInstance->HandleEvent(&pluginEvent, &eventHandled);
> > #endif
> >+#endif
> >+
> >+#ifdef MOZ_COMPOSITED_PLUGINS
> >+// make sure we're using system cairo
> >+#undef cairo_set_source_rgb
> >+#undef cairo_set_source_rgba
> >+#undef cairo_paint
> >+#undef cairo_paint_with_alpha
> 
> This needs to be earlier, so you get the prototype (in case something brought
> in the cairo remap stuff), but should also be later.. hence getting this from
> dlsym probably begin clearer.  You should be able to get these via
> dlsym(RTLD_DEFAULT, ...) without needing to know the location of the system
> cairo shlib. 

Removed instead.

> >+  /* XXX: this is very nasty. We need a better way of getting at mPlugWindow */
> >+  //GdkWindow *plug = ((GtkWidget*)*(((nsPluginNativeWindow*)mWindow)->mPlugWindow))->window;
> >+  GtkWidget *plug = (GtkWidget*)(((nsPluginNativeWindow*)mWindow)->mPlugWindow);
> >+  //GtkWidget *plug = (GtkWidget*)(((nsPluginNativeWindowGtk2*)mWindow)->mSocketWidget);
> 
> Youch.  Probably no better way, though.
> 
> Though...
> 
> >+#if 0
> 
> If you're using XCopyArea anyway, why not just get rid of the system cairo
> goop?

Agreed.

Comment 26

[Jeff Muizelaar [:jrmuizel]]

Attachment: fennec plugins v4

Fixes all the comments except unifying 'if !defined(XP_MACOSX) && !defined(MOZ_COMPOSITED_PLUGINS)'

Comment 27

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 363836 [details] [diff] [review]
fennec plugins v4

r+ as long as you..

- only check for Xcomposite lib if we're on hildon, as per irc conversation

- do a try server run

- either figure out or file a bug on the XCopyArea coordinate wonkiness

Comment 28

[Jeff Muizelaar [:jrmuizel]]

Attachment: fennec plugins v5

Updated to deal with Vlad's comments.

Comment 29

[Jeff Muizelaar [:jrmuizel]]

Attachment: fennec plugins v6 [Checkin: Comment 31+36]

Further fixes, this should be ready to land provided it passes the currently running tryserver builds.

Comment 30

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #29)
> Created an attachment (id=363965) [details]
> fennec plugins v6
> 
> Further fixes, this should be ready to land provided it passes the currently
> running tryserver builds.

It looks like it builds fine on the try server.

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 363965 [details] [diff] [review]
fennec plugins v6
[Checkin: Comment 31+36]


http://hg.mozilla.org/mozilla-central/rev/4f3614d130da

Comment 32

[Jeff Muizelaar [:jrmuizel]]

The arm linux tinderboxes don't have the xdamage and xcomposite headers needed for this to build on them. See bug 480125

Comment 33

[Chris Lord [:cwiiis]]

Hi there, I've been following this bug as we have similar issues in the headless mozilla backend. The event synthesis that his check-in adds, is it reliable? Are there any issues with it? It would save us some effort and allow us to have input transformation for free if that's the case.

Comment 34

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #33)
> Hi there, I've been following this bug as we have similar issues in the
> headless mozilla backend. The event synthesis that his check-in adds, is it
> reliable? Are there any issues with it? It would save us some effort and allow
> us to have input transformation for free if that's the case.

Calling it reliable is probably a stretch. However, it does seem to work well enough and I don't know about any issues with it, though it's only been lightly tested in fennec. It currently doesn't deal with all the different event types but there's no reason that I can see why that can't be fixed.

Comment 35

[Robert Bragg]

One thing I wonder about is plugins that X grab the pointer and bypass the send event forwarding. This might e.g. be done for menus.

Maybe this wont turn out to be a big issue, but seems worth mentioning.

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 363965 [details] [diff] [review]
fennec plugins v6
[Checkin: Comment 31+36]


http://hg.mozilla.org/mozilla-central/rev/5354b94d250f
missed to sync this file

PS: I wonder why you added theses files in the reverse-alpha order ? :-/

Comment 37

[Oleg Romashin (:romaxa)]

Attachment: Compilation error, missing include

I've got compilation error on X86 sbox 2007...
Seems there are some missing includes

@@ -58,6 +58,8 @@ extern "C" { 
 #include <X11/extensions/Xdamage.h> 
 #include <X11/extensions/Xcomposite.h> 
 } 
+#include "nsIPluginInstancePeer2.h" 
+#include "nsPluginInstancePeer.h" 
 #endif 
. 
 #include "gtk2xtbin.h"

Comment 38

[Joel Maher ( :jmaher ) (UTC -8)]

verified with maemo nightly from 20090818

Comment 39

[Serge Gautherie (:sgautherie)]

http://hg.mozilla.org/mozilla-central/rev/0c265fa5acc1
Remove debugging code that slipped in with changeset 4f3614d130da