[Gnash-commit] [patch #9016] Improved handling of gstreamer plugin installation returns

[Dag Hovland]

URL:
<http://savannah.gnu.org/patch/?9016>

Summary: Improved handling of gstreamer plugin installation
returns
Project: Gnash - The GNU Flash player
Submitted by: daghovland
Submitted on: Thu 02 Jun 2016 10:52:12 AM GMT
Category: gui
Priority: 5 - Normal
Status: None
Privacy: Public
Assigned to: None
Originator Email:
Open/Closed: Open
Discussion Lock: Any

_______________________________________________________

Details:

This is a fix for bug bug #48087 <https://savannah.gnu.org/bugs/?48087>

Problems:
- The call to gst_install_plugins_sync in libmedia/gst/GstUtil.cpp will not
return if the plugin installer crashes.
- If the plugin installation is unsuccessful or the user cancels,
gst_install_plugins_sync will be called repeatedly, making the movie
unusable.

Changes:
- Changed the call to gst_install_plugins_async, as recommended by the
gstremaer docs.
- Made the movie pause during plugin installation, and resume afterwards. The
user may click "Play" on the screen at any time, e.g. if the installer
crashes.
- Prevented any further install attempts after the user has once clicked
cancel.
- Added dialogs for some of the different return values from the gst plugin
installer: Informing the user about the consequence of plugin installation
errors, and asking the user whether to restart the movie in case of successful
installation.

To do the last part, I had to get hold of a movie_root object, which was not
available in libmedia. I passed the object down all the calls from the classes
in libcore/asobj, where it is available.

Questions:
- Is there a better way to get access to movie_root or the gui? (Sandro
Santili suggests using RunResources instead, but that would also have to be
passed down the same path. I think.)
- Do the automatic pausing of the movie during plugin installation, and the
added dialogs fit in the general user experience?





_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Additional Item Attachment, patch #9016 (project gnash):

File name: pluginasync.patch Size:31 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #1, patch #9016 (project gnash):

About movie_root: the only reason why you need movie_root is to get to call
the event callback. The movie_root instance knows about the
"_callbacksHandler" because it was passed to it by gui/Player.cpp (see
root.registerEventCallback).

All media handling classes are created by a factory which is initialized by
the same Player class that passed the EventHandler to movie_root, so I suggest
you pass the same EventHandler _also_ to the MediaHandler, and let
MediaHandler pass it further to the objets it create.

See if doing so makes the patch cleaner.

(looking at the code now I see that RunResource would be already holding an
handler to the MediaHandler so it's not appropriate to create a circular link
there).




_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #2, patch #9016 (project gnash):

Hi! Thanks for the tip. Yes, that was much less messy.

Cheers,

Dag

(file #37374)
_______________________________________________________

Additional Item Attachment:

File name: asyncPluginInstall2.patch Size:271 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #3, patch #9016 (project gnash):

I attach a new version of the patch with some attempts at improving the user
interaction after installation. Please ask if you want more info or comments
on the code of any of these patch versions.

Here is a kind of flow-diagram of the user interaction:


Lacking plugin detected:

Has user previously cancelled, and then answered no whether to restart
the plugin installation?

- If yes, then abort

Is another plugin installation in progress?

- If yes, then abort


Has plugin installation been cancelled by the user before?

- If yes then ask whether to retry?

- If no, then abort.

Start async plugin installation and pause movie.

User can anytime during installation click the lower bar on the movie to
continue playing.

After installation finished (which includes two-three dialogs from gstreamer)

If successful:
- User is informed and asked whether to restart movie

If no plugin found
- User is informed

If plugin installation error
- User is informed

If partial success
- User is informed and asked whether to restart movie

If installation was aborted
- No dialog, but the cancellation is registered and will lead to a dialog if
there is a new

If update of the gst registry fails:
- User is advised to restart gnash

Movie is set to play.


Cheers,

Dag

(file #37516)
_______________________________________________________

Additional Item Attachment:

File name: asyncPluginInstall3.patch Size:17 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Update of patch #9016 (project gnash):

Assigned to: None => strk

_______________________________________________________

Follow-up Comment #4:

I can't seem to be able to apply the patch against commit
f6a0b382f752e919a6c53f5c619e1eaf2189be74 -- does it need a rebase ?

Could you please squash-rebase and attach a git-format-patch ?

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #5, patch #9016 (project gnash):

Ok did a rebase and squash and then format-patch against current master.
(Sorry about the delay was on holidays.)

Cheers,

Dag

(file #37911)
_______________________________________________________

Additional Item Attachment:

File name: 0001-Changes-to-the-gst-plugin-installation.patch Size:19 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Richard Wilbur]

A few comments:

gui/Player.cpp: Player::run() in comment
s/gstremer/gstreamer/

libmedia/gst/GstUtil.cpp:
@@ -121,31 +143,118 @@ GstUtil::check_missing_plugins(GstCaps* caps)

I've got a question here: Doesn't "if (!detail)" imply that detail is
NULL? If so, does it seem appropriate to call "g_free(detail)"?

Later in same hunk in comment above GstUtil::plugin_success_dialog():
replace "where successully installed" with "were successfully installed."

libmedia/gst/GstUtil.h:
@@ -73,12 +78,35 @@ public:
in comments
s/poping/popping/
s/Protcts/Protects/
s/diaplog/dialog/

[Dag Hovland]

Yes, you're right. Thanks. The free should have been in one of the other
error handling blocks. I will upload a new patch.

Dag

Den 17. juli 2016 03:18, skrev Richard Wilbur:
> A few comments:
>
> gui/Player.cpp: Player::run() in comment
> s/gstremer/gstreamer/
>
> libmedia/gst/GstUtil.cpp:
> @@ -121,31 +143,118 @@ GstUtil::check_missing_plugins(GstCaps* caps)
>
> I've got a question here: Doesn't "if (!detail)" imply that detail is
> NULL? If so, does it seem appropriate to call "g_free(detail)"?
>
> Later in same hunk in comment above GstUtil::plugin_success_dialog():
> replace "where successully installed" with "were successfully installed."
>
> libmedia/gst/GstUtil.h:
> @@ -73,12 +78,35 @@ public:
> in comments
> s/poping/popping/
> s/Protcts/Protects/
> s/diaplog/dialog/

[Bastiaan Jacques]

Follow-up Comment #6, patch #9016 (project gnash):

According to the docs, gst_install_plugins_async, requires a Glib mainloop,
from which I gather that results from any threaded code (assuming one is
created) are marshalled to gst_install_plugins_async in a synchronous manner.
So that would obviate the need for mutex protection. Right?

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #7, patch #9016 (project gnash):

It seems you are right. I tried a few times now without the mutex and it seems
ok. But I still think the advantage of having the function thread-safe, just
in case, is important enough. What's the advantage of removing the mutex? Are
you afraid of some deadlock?

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #8, patch #9016 (project gnash):

Removed a wrong g_free as suggested by Richard Wilbur on email.

(file #37921)
_______________________________________________________

Additional Item Attachment:

File name: 0001-Changes-to-the-gst-plugin-installation.patch Size:19 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Bastiaan Jacques]

Follow-up Comment #9, patch #9016 (project gnash):

Less code is generally better (easier to maintain and/or extend; KISS). In
this case it is also misleading the reader that there is some thread here that
requires locking. And not doing something that serves no purpose is a good
thing in my book. ;)

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #10, patch #9016 (project gnash):

Good arguments. Just one more thing: to be completely safe it must then also
be true that check_missing_plugins is never called concurrently from multiple
threads in gnash. It seems to be true from the single example I have been
using, but I don't know how to check for this. Can you confirm this?

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #11, patch #9016 (project gnash):

Ok made a new patch with these changes:

- No more mutexes and blocking, since this is unnecessary as Bastiaan Jacques
has pointed out

- A change in the dialogs: In the same dialog that reports an error with
plugin installation, the user is asked whether to block further installation
attempts. This removes an issue where two dialog windows could appear over
each other in the previous patches.

(file #37923)
_______________________________________________________

Additional Item Attachment:

File name: 0001-Changes-to-the-gst-plugin-installation.patch Size:18 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #12, patch #9016 (project gnash):

Does not cleanly build here:


In file included from
/usr/src/gnash/gnash-head/libsound/StreamingSoundData.cpp:27:0:
/usr/src/gnash/gnash-head/libmedia/MediaHandler.h:32:27: fatal error:
HostInterface.h: No such file or directory
#include "HostInterface.h"
^
compilation terminated.
In file included from /usr/src/gnash/gnash-head/libsound/EmbedSound.cpp:28:0:
/usr/src/gnash/gnash-head/libmedia/MediaHandler.h:32:27: fatal error:
HostInterface.h: No such file or directory
#include "HostInterface.h"
^
compilation terminated.
In file included from
/usr/src/gnash/gnash-head/libsound/EmbedSoundInst.cpp:26:0:
/usr/src/gnash/gnash-head/libmedia/MediaHandler.h:32:27: fatal error:
HostInterface.h: No such file or directory
#include "HostInterface.h"
^
compilation terminated.
In file included from /usr/src/gnash/gnash-head/libsound/LiveSound.cpp:28:0:
/usr/src/gnash/gnash-head/libmedia/MediaHandler.h:32:27: fatal error:
HostInterface.h: No such file or directory
#include "HostInterface.h"
^
compilation terminated.
In file included from
/usr/src/gnash/gnash-head/libsound/sound_handler.cpp:33:0:
/usr/src/gnash/gnash-head/libmedia/MediaHandler.h:32:27: fatal error:
HostInterface.h: No such file or directory
#include "HostInterface.h"
^
compilation terminated.
make[2]: *** [libgnashsound_la-EmbedSound.lo] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [libgnashsound_la-EmbedSoundInst.lo] Error 1
make[2]: *** [libgnashsound_la-StreamingSoundData.lo] Error 1
make[2]: *** [libgnashsound_la-LiveSound.lo] Error 1
make[2]: *** [libgnashsound_la-sound_handler.lo] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2


Also, I've found a few cases of style changes, please avoid those ones (adding
spaces in unrelated code).

Finally, please check with "git diff --check" and "git diff --cached --check"
and remove the trailing spaces reported from it.

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Richard Wilbur]

A few inconvenient misspellings:

gui/Player.cpp: Player::run() in comment (patch line 39)
"gstremer" -> "gstreamer"


libmedia/gst/GstUtil.cpp: In comment above
GstUtil::plugin_success_dialog() (patch line 281)
"where" -> "were"


libmedia/gst/GstUtil.h: In comment above mutex declaration (patch line
397)
"Protcts" -> "Protects"

libmedia/gst/GstUtil.h: In comments above plugin_success_dialog() and
plugin_fail_dialog() (patch lines 402, 406)
"diaplog" -> "dialog"

[Dag Hovland]

Follow-up Comment #13, patch #9016 (project gnash):

Re. the HostInterface errors: I think you need to run autogen.sh. (I had to
change libmedia/Makefile.am to be able to include HostInterface.h)

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #14, patch #9016 (project gnash):

Ok I ran git diff --check and removed all unrelated whitespace. Not sure what
you meant about the other style changes, though.

(file #37926)
_______________________________________________________

Additional Item Attachment:

File name: 0001-Changes-to-the-gst-plugin-installation.patch Size:18 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #15, patch #9016 (project gnash):

Re. Hostinterface: Seems it was me that had not reran autogen.sh. Sorry about
that. Let me rechek it and I will upload a new patch.

(file #37927)
_______________________________________________________

Additional Item Attachment:

File name: 0001-Changes-to-the-gst-plugin-installation.patch Size:18 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Additional Item Attachment, patch #9016 (project gnash):

File name: 0001-Changes-to-the-gst-plugin-installation.patch Size:18 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #16, patch #9016 (project gnash):

Example of "other style changes":

/// This is an abstract base class, so not instantiable anyway.
- MediaHandler() {}
+ MediaHandler(){};


Not a big deal, don't bother uploading a new patch.

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #17, patch #9016 (project gnash):

My build still fails after ./autogen.sh and ./configure.
My configure switches:

--enable-maintainer-mode --prefix=/extra/gnash-dev --disable-jemalloc
--enable-fps-debug --enable-extensions=fileio,mysql
--with-swfdec-testsuite=/usr/src/swfdec/swfdec/test
--enable-http-testsuite=http://localhost/ --enable-python --enable-doublebuf
--enable-red5-testing --enable-docbook --with-plugins-install=prefix
--disable-static --enable-gui=gtk,qt4,sdl,fb,dump --enable-media=ffmpeg,gst

The error is due to -I../libcore not being present in libsound/Makefile

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #18, patch #9016 (project gnash):

If you're going to upload a new patch, make sure to keep commit log within 70
columns, and leave a blank line after the first one

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #19, patch #9016 (project gnash):

I reran with strk's configure (except the --enable-media=ffmpeg part, since
then I cannot test the plugin installation). To make it compile I had to add
an <iostream> include to gui.cpp, but otherwise it worked fine. Maybe I did
something wrong with the previous patch, here is at least a new one, also with
the style changes removed and narrower commit message.


(file #37934)
_______________________________________________________

Additional Item Attachment:

File name: 0001-Changes-to-the-gst-plugin-installation.patch Size:18 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Richard Wilbur]

Follow-up Comment #20, patch #9016 (project gnash):

libmedia/gst/GstUtil.h: In comments above plugin_success_dialog() and
plugin_fail_dialog() (patch lines 408, 412):

"diaplog" -> "dialog"

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #21, patch #9016 (project gnash):

New patch with spelling errors in comments fixed.

(file #37957)
_______________________________________________________

Additional Item Attachment:

File name: 0001-Changes-to-the-gst-plugin-installation.patch Size:18 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Richard Wilbur]

Follow-up Comment #22, patch #9016 (project gnash):

Looks good from a visual perspective.

Did you remove the last reason for libmedia/gst/GstUtil.h to "#include
<mutex>"?

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #23, patch #9016 (project gnash):

And did you add -I../libcore in libsound/Makefile.am ?

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Richard Wilbur]

Follow-up Comment #24, patch #9016 (project gnash):

strk: He did. In the latest patch it looks like:
+++ b/libsound/Makefile.am
libgnashsound_la_CPPFLAGS = \
[...]
+ -I$(top_srcdir)/libcore \

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #25, patch #9016 (project gnash):

#include <mutex.h> : This line was (unnecessarily) still in my patch of
GstUtil.h, but I have removed it in the attached patch. (No other changes)

libsound/Makefile.am: Yes, this is done. Also tested with strk's configure
switches.

Reproduction: To reproduce (on my Ubuntu, at least) I removed the packages
listed below, and then reran configure without ffmpeg and then rebuilt.

gstreamer0.10-ffmpeg
libavcodec-dev
libavformat-dev
libswscale-dev
libavutil-dev
libaacs0
libavformat-ffmpeg56
libbdplus0
libbluray1
libgme0
libmodplug1
libswscale-ffmpeg3

(file #37970)
_______________________________________________________

Additional Item Attachment:

File name: 0001-Changes-to-the-gst-plugin-installation.patch Size:18 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #26, patch #9016 (project gnash):

Can you please add an URL to an example SWF that should trigger the user
prompt for installing the plugin ?

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #27, patch #9016 (project gnash):

This URL seems to trigger the plugin install:
http://elegans.imbb.forth.gr/games/WOT_II_SpecOps.swf

I did get the popup after accepting the mission (in the game), the popup
failed to find an appropriate plugin, I asked it not to ask me again and
continued. Now after restarting I don't get the popup anymore. Where was my
choice of not being asked again stored ? It may be useful to report it in the
user propmt message, so as a user I know how to get the popup again

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Dag Hovland]

Follow-up Comment #28, patch #9016 (project gnash):

The choice is only stored in memory (in GstUtil::no_plugin_install). If you
restart from the gnash menu, I guess this variable is not reset?

In my opinion this is a good solution: you never run the danger of a user not
knowing she is lacking a plugin, but the rest of the movie is still usable,
since you can block the repeated installation attempts.

For testing I have used the file in the original bugreport:

http://kids.discovery.com/files/games/pompeii/media/global.swf

(with the libavcodec etc. uninstalled as mentioned below)

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #30, patch #9016 (project gnash):

Note that the movie gets in "stop" mode on subsequent gnash invocations, as if
Gnash knows it needs to install a plugin (but the popup not showing up) --
sounds like something that needs to be fixed before applying the patch.

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #31, patch #9016 (project gnash):

I just found out the popup _did_ come out on subsequent calls, just the window
was hidden by other windows. As the popup is not considered a real window
(does not appear in the windows list) I just failed to find it. Any chance to
improve that behaviour ? By either giving the popup window first-class status
OR ensuring the popup is always shown when the main window is (this is not the
case for me)

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Update of patch #9016 (project gnash):

Status: None => Done
Open/Closed: Open => Closed

_______________________________________________________

Follow-up Comment #32:

Actually, I've just realized that the popup window is not coming from Gnash
but from the system, so I dubt there's anything we can do here.

I've applied the patch and pushed it as commit
2b3bdede0305c4fc3ad21a0a4197330606c9b880


Thanks !

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9016>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/