Closed Bug 442109 Opened 12 years ago Closed 11 years ago

youtube videos don't play.

Categories

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

x86
Maemo
defect

Tracking

(fennec1.0b1+)

VERIFIED FIXED
fennec1.0b1
Tracking Status
fennec 1.0b1+ ---

People

(Reporter: dougt, Assigned: jrmuizel)

References

Details

Attachments

(2 files, 7 obsolete files)

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.
do you have flash installed?
yeah, to works in microb.  I didn't move the np* files around.
Duplicate of this bug: 449239
depended of bug 313462 ?
Duplicate of this bug: 443945
Flags: blocking-fennec1.0+
Target Milestone: --- → Fennec A2
Depends on: 458928
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.
OS: Mac OS X → Linux (embedded)
Priority: -- → P1
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.
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...
Stuart, did you try any prototyping, with the xdamage option of yours?
My concern is that performance degradation might be quite severe
BTW, why was the approach of forcing all Flash objects to be windowless rejected? Unavailability of a Flash 10 player?
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.
Blocks: 456091
Target Milestone: Fennec A2 → Fennec A3
Attached patch Composited plugins prototye (obsolete) — Splinter Review
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.
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.
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?
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?
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.
It looks like we'll be able to use XSendEvent to redirect input.
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.
The updated adds mouse and keyboard input and improves drawing performance.
Attachment #357705 - Attachment is obsolete: true
Attached patch fennec plugins v2 (obsolete) — Splinter Review
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.
Attachment #360146 - Attachment is obsolete: true
tracking-fennec: --- → 1.0b1+
Flags: blocking-fennec1.0+
Duplicate of this bug: 478616
Assignee: nobody → jmuizelaar
Attached patch fennec plugins v3 (obsolete) — Splinter Review
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.
Attachment #360808 - Attachment is obsolete: true
Attachment #363786 - Flags: review?(vladimir)
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. */
Attachment #363786 - Flags: review?(vladimir) → review-
(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.
Attached patch fennec plugins v4 (obsolete) — Splinter Review
Fixes all the comments except unifying 'if !defined(XP_MACOSX) && !defined(MOZ_COMPOSITED_PLUGINS)'
Attachment #363786 - Attachment is obsolete: true
Attachment #363836 - Flags: review?(vladimir)
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
Attached patch fennec plugins v5 (obsolete) — Splinter Review
Updated to deal with Vlad's comments.
Attachment #363836 - Attachment is obsolete: true
Further fixes, this should be ready to land provided it passes the currently running tryserver builds.
Attachment #363915 - Attachment is obsolete: true
(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.
Keywords: checkin-needed
Comment on attachment 363965 [details] [diff] [review]
fennec plugins v6
[Checkin: Comment 31+36]


http://hg.mozilla.org/mozilla-central/rev/4f3614d130da
Attachment #363965 - Attachment description: fennec plugins v6 → fennec plugins v6 [Checkin: Comment 31]
Attachment #352635 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 480125
The arm linux tinderboxes don't have the xdamage and xcomposite headers needed for this to build on them. See bug 480125
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(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.
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 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 ? :-/
Attachment #363965 - Attachment description: fennec plugins v6 [Checkin: Comment 31] → fennec plugins v6 [Checkin: Comment 31+36]
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"
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 489294
Depends on: 491241
verified with maemo nightly from 20090818
Status: RESOLVED → VERIFIED
http://hg.mozilla.org/mozilla-central/rev/0c265fa5acc1
Remove debugging code that slipped in with changeset 4f3614d130da
You need to log in before you can comment on or make changes to this bug.