Gnash - 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: |
Sandro Santilli <strk> |
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> |
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> |
Thu 21 Jul 2016 02:55:28 PM UTC, comment #29: Ok with the kids.discovery.com test file I get:
|
Sandro Santilli <strk> |
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?
|
Dag Hovland <daghovland> |
Thu 21 Jul 2016 01:16:23 PM UTC, comment #27: This URL seems to trigger the plugin install:
|
Sandro Santilli <strk> |
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> |
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)
|
Dag Hovland <daghovland> |
Wed 20 Jul 2016 07:28:23 PM UTC, comment #24: strk: He did. In the latest patch it looks like:
|
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> |
Wed 20 Jul 2016 06:55:17 PM UTC, comment #22: Looks good from a visual perspective.
|
Richard Wilbur <richard_wilbur> |
Wed 20 Jul 2016 04:48:29 PM UTC, comment #21: New patch with spelling errors in comments fixed. |
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
|
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.
|
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> |
Mon 18 Jul 2016 07:18:01 AM UTC, comment #17: My build still fails after ./autogen.sh and ./configure.
|
Sandro Santilli <strk> |
Mon 18 Jul 2016 07:15:37 AM UTC, comment #16: Example of "other style changes":
|
Sandro Santilli <strk> |
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. |
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. |
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:
|
Sandro Santilli <strk> |
Sun 17 Jul 2016 01:16:58 PM UTC, comment #11: Ok made a new patch with these changes:
|
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> |
Sun 17 Jul 2016 11:47:12 AM UTC, comment #8: Removed a wrong g_free as suggested by Richard Wilbur on email. |
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> |
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.)
|
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 ?
|
Sandro Santilli <strk> |
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.
|
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.
|
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).
|
Sandro Santilli <strk> |
Thu 02 Jun 2016 10:52:12 AM UTC, original submission:
This is a fix for bug bug #48087
|
Dag Hovland <daghovland> |
Depends on the following items: None found
Items that depend on this one: None found
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 | Status | None | Done | |
Open/Closed | Open | 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 to | None | 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 |
Powered by Savane 3.13-f8d8.
Corresponding source code
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 !