patchGnash - The GNU Flash player - Patches: patch #9016, Improved handling of gstreamer...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #9016: Improved handling of gstreamer plugin installation returns

Submitter:  Dag Hovland <daghovland>
Submitted:  Thu 02 Jun 2016 10:52:12 AM UTC
   
 
Category:  gui Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  strk Open/Closed:  Closed

Jump to the original submission

Thu 21 Jul 2016 09:46:39 PM UTC, 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 !

Sandro Santilli <strk>
Group Member
Thu 21 Jul 2016 09:43:25 PM UTC, comment #31: 

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)

Sandro Santilli <strk>
Group Member
Thu 21 Jul 2016 02:56:48 PM UTC, comment #30: 

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.

Sandro Santilli <strk>
Group Member
Thu 21 Jul 2016 02:55:28 PM UTC, comment #29: 

Ok with the kids.discovery.com test file I get:

In the GUI:
{{{
Required plugin could not be found

lt-gtk-gnash requires to install plugins to play media files of the following type: Sorenson Spark Video decoder
}}}

In the logs:
{{{
[21034:1] 2614 ERROR: Could not create Video Decoder: VideoDecoderGst: initialisation failed for video type video/x-flash-video!
}}}

But once again the second run (new process) does not popup the GUI. It sounds like the operating system itself is caching the information about the required video plugin being impossible to find, could it be ?


Sandro Santilli <strk>
Group Member
Thu 21 Jul 2016 02:17:39 PM UTC, comment #28: 

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)

Dag Hovland <daghovland>
Thu 21 Jul 2016 01:16:23 PM UTC, comment #27: 

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

Sandro Santilli <strk>
Group Member
Thu 21 Jul 2016 01:07:39 PM UTC, comment #26: 

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

Sandro Santilli <strk>
Group Member
Thu 21 Jul 2016 10:55:59 AM UTC, comment #25: 

#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)

Dag Hovland <daghovland>
Wed 20 Jul 2016 07:28:23 PM UTC, comment #24: 

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

Richard Wilbur <richard_wilbur>
Wed 20 Jul 2016 07:03:04 PM UTC, comment #23: 

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

Sandro Santilli <strk>
Group Member
Wed 20 Jul 2016 06:55:17 PM UTC, comment #22: 

Looks good from a visual perspective.

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

Richard Wilbur <richard_wilbur>
Wed 20 Jul 2016 04:48:29 PM UTC, comment #21: 

New patch with spelling errors in comments fixed.

(file #37957)

Dag Hovland <daghovland>
Mon 18 Jul 2016 09:17:24 PM UTC, comment #20: 

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

  "diaplog" -> "dialog"

Richard Wilbur <richard_wilbur>
Mon 18 Jul 2016 05:29:23 PM UTC, comment #19: 

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)

Dag Hovland <daghovland>
Mon 18 Jul 2016 07:19:22 AM UTC, comment #18: 

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

Sandro Santilli <strk>
Group Member
Mon 18 Jul 2016 07:18:01 AM UTC, comment #17: 

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

Sandro Santilli <strk>
Group Member
Mon 18 Jul 2016 07:15:37 AM UTC, comment #16: 

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.

Sandro Santilli <strk>
Group Member
Sun 17 Jul 2016 07:06:50 PM UTC, comment #15: 

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)

Dag Hovland <daghovland>
Sun 17 Jul 2016 06:51:52 PM UTC, comment #14: 

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

(file #37926)

Dag Hovland <daghovland>
Sun 17 Jul 2016 06:44:03 PM UTC, comment #13: 

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)

Dag Hovland <daghovland>
Sun 17 Jul 2016 02:46:46 PM UTC, comment #12: 

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.

Sandro Santilli <strk>
Group Member
Sun 17 Jul 2016 01:16:58 PM UTC, comment #11: 

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)

Dag Hovland <daghovland>
Sun 17 Jul 2016 12:48:07 PM UTC, comment #10: 

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?

Dag Hovland <daghovland>
Sun 17 Jul 2016 11:53:03 AM UTC, comment #9: 

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. ;)

Bastiaan Jacques <bjacques>
Group Member
Sun 17 Jul 2016 11:47:12 AM UTC, comment #8: 

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

(file #37921)

Dag Hovland <daghovland>
Sun 17 Jul 2016 11:42:30 AM UTC, comment #7: 

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?

Dag Hovland <daghovland>
Sun 17 Jul 2016 10:20:39 AM UTC, comment #6: 

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?

Bastiaan Jacques <bjacques>
Group Member
Sat 16 Jul 2016 02:22:53 PM UTC, comment #5: 

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

Cheers,

Dag

(file #37911)

Dag Hovland <daghovland>
Mon 27 Jun 2016 11:23:37 AM UTC, 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 ?

Sandro Santilli <strk>
Group Member
Fri 17 Jun 2016 11:09:15 AM UTC, comment #3: 

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)

Dag Hovland <daghovland>
Fri 03 Jun 2016 11:26:20 AM UTC, comment #2: 

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

Cheers,

Dag

(file #37374)

Dag Hovland <daghovland>
Thu 02 Jun 2016 08:51:37 PM UTC, comment #1: 

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).



Sandro Santilli <strk>
Group Member
Thu 02 Jun 2016 10:52:12 AM UTC, original submission:  

This is a fix for bug bug #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?

Dag Hovland <daghovland>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #37970:  0001-Changes-to-the-gst-plugin-installation.patch added by daghovland (18KiB - text/x-patch - Removed inclusion of mutex.h from GstUtil.h)
file #37957:  0001-Changes-to-the-gst-plugin-installation.patch added by daghovland (18KiB - text/x-patch - spelling errors)
file #37934:  0001-Changes-to-the-gst-plugin-installation.patch added by daghovland (18KiB - text/x-patch - Patch without style changes. Hopefully now correct Makefile.am etc.)
file #37928:  0001-Changes-to-the-gst-plugin-installation.patch added by daghovland (19KiB - text/x-patch - Fixed lacking line in Makefile.am )
file #37927:  0001-Changes-to-the-gst-plugin-installation.patch added by daghovland (18KiB - text/x-patch - Fixed spelling errors )
file #37926:  0001-Changes-to-the-gst-plugin-installation.patch added by daghovland (18KiB - text/x-patch - Patch with removed unrelated whitespace)
file #37923:  0001-Changes-to-the-gst-plugin-installation.patch added by daghovland (18KiB - text/x-patch - patch with no mutexes, and no double-dialogs over each other)
file #37921:  0001-Changes-to-the-gst-plugin-installation.patch added by daghovland (19KiB - text/x-patch - patch for async gst plugin install - improved with Richards bugfix)
file #37911:  0001-Changes-to-the-gst-plugin-installation.patch added by daghovland (19KiB - text/x-patch - patch made with git format-patch after a rebase)
file #37374:  asyncPluginInstall2.patch added by daghovland (271KiB - text/x-patch)
file #37353:  pluginasync.patch added by daghovland (31KiB - text/x-patch - patch for async gst plugin install)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by richard_wilbur (Posted a comment)
  • -email is unavailable- added by bjacques (Posted a comment)
  • -email is unavailable- added by strk (Posted a comment)
  • -email is unavailable- added by daghovland (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-21 strk StatusNone Done
        Open/ClosedOpen Closed
    2016-07-21 daghovland Attached File- Added 0001-Changes-to-the-gst-plugin-installation.patch, #37970
    2016-07-20 daghovland Attached File- Added 0001-Changes-to-the-gst-plugin-installation.patch, #37957
    2016-07-18 daghovland Attached File- Added 0001-Changes-to-the-gst-plugin-installation.patch, #37934
    2016-07-17 daghovland Attached File- Added 0001-Changes-to-the-gst-plugin-installation.patch, #37928
    2016-07-17 daghovland Attached File- Added 0001-Changes-to-the-gst-plugin-installation.patch, #37927
    2016-07-17 daghovland Attached File- Added 0001-Changes-to-the-gst-plugin-installation.patch, #37926
    2016-07-17 daghovland Attached File- Added 0001-Changes-to-the-gst-plugin-installation.patch, #37923
    2016-07-17 daghovland Attached File- Added 0001-Changes-to-the-gst-plugin-installation.patch, #37921
    2016-07-16 daghovland Attached File- Added 0001-Changes-to-the-gst-plugin-installation.patch, #37911
    2016-06-27 strk Assigned toNone strk
    2016-06-17 daghovland Attached File- Added asyncPluginInstall3.patch, #37516
    2016-06-03 daghovland Attached File- Added asyncPluginInstall2.patch, #37374
    2016-06-02 daghovland Attached File- Added pluginasync.patch, #37353

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code