Skip to content
This repository has been archived by the owner on Apr 16, 2018. It is now read-only.

Add a 'missing-plugin' signal to player #11

Closed
wants to merge 2 commits into from

Conversation

jku
Copy link

@jku jku commented Mar 11, 2015

Also use the signal to notify user in both gst-play and gtk-play.

For gtk-play this means a GtkInfoBar that says e.g. Missing plugin 'H.264 decoder'. For gst-play it's just a g_warning() with same content.

@sdroege
Copy link
Owner

sdroege commented Mar 11, 2015

Thanks for the patch but I'm not sure how useful that is in general. Wouldn't an application prefer some API that directly lets you install the plugins?

@jku
Copy link
Author

jku commented Mar 11, 2015

Yeah, I should have talked about that... The gst API for this exists for two reasons :

  1. for concise error/problem reporting to the user mentioning what exactly is missing
  2. for initiating installation of missing plugins

and I only covered part 1 here because I wasn't going to do the actual installation myself anyway... but if you prefer that, I could include "installer_detail" string in the signal, so applications could then use gst_install_plugins_async () with that...

@sdroege
Copy link
Owner

sdroege commented Mar 11, 2015

IMHO ideally 1) should be solved by putting these information into the error or warning message that is posted after the missing-plugin messages, instead of providing a second API for a different kind of errors.

@jku
Copy link
Author

jku commented Mar 11, 2015

I'm afraid I don't understand what you mean by "a second API for different kind of errors". I'll try to explain my suggestion more clearly:

So the "installer_detail" I refer to would be from gst_missing_plugin_message_get_installer_detail() and is not really human readable (not for normal people anyway). So I think the best we can do for issue 1 is what I already provided. But the installer_detail might indeed be useful for applications that do want to start a plugin installation process to fix issue 2: I was proposing adding that string to the signal that is already in the patch.

@sdroege
Copy link
Owner

sdroege commented Mar 15, 2015

For 1), what we have already should be enough. There should be an error message that tells exactly what is missing, and if not that should be fixed 😄 With a second API I meant the "error" signal as the primary error API, and your new "missing-plugin" as a secondary API.

For the installer stuff, I would prefer to abstract that a bit more so that people wouldn't have to use magic GStreamer API but that it would just do the right thing easily, and install plugins if needed and if allowed by the application. Without leaking the non-human readable details about things to the application.

@jku
Copy link
Author

jku commented Mar 16, 2015

Oh, now I finally got what you meant. Let me try the error signal -- it should indeed be usable for this.

Jussi Kukkonen added 2 commits March 16, 2015 13:45
Note that this error does not necessarily mean the playback has
completely failed, but in practice the user experience will be bad
(think, e.g. of a mp4 file where H.264 codec is missing: AAC
playback still works...).
@jku
Copy link
Author

jku commented Mar 16, 2015

How about now? It uses "error" signal to achieve the same UX result.

If you don't think this is worthwhile, that's fine too: but I'm happy to keep iterating if I can make this into something you'd like to take into upstream gst-player...

@sdroege
Copy link
Owner

sdroege commented Mar 16, 2015

Thanks, I'll take a look later.

I definitely want to integrate something for this, but I'm also not completely sure yet about what API would be useful to have :)

emit_error (self, g_error_new (GST_PLAYER_ERROR,
GST_PLAYER_ERROR_MISSING_PLUGIN, "Missing plugin '%s'", desc));
g_free (desc);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost ok, but note that there can be multiple missing-plugin messages in theory. And only after the last one an error message is posted, or a warning message if only irrelevant plugins are missing and playback can continue.

I think you should accumulate all missing plugin messages and wait until the error or warning message (GST_CORE_ERROR_MISSING_PLUGIN or GST_STREAM_ERROR_CODEC_NOT_FOUND). And only if it's an error message, actually emit an error. As emitting an error will stop playback, which might not be needed.
In case of a warning message, it might make sense to add a warning signal that mirrors the error signal but has the big difference that it does not cause playback to stop.

@sdroege
Copy link
Owner

sdroege commented Mar 21, 2015

See comments inside the commit, this is definitely going into the right direction now. Thanks for working on it 😄

@sdroege
Copy link
Owner

sdroege commented Apr 15, 2015

Are you planning to continue working on this?

@jku
Copy link
Author

jku commented Apr 16, 2015

Yes, I've just been distracted -- I think I can look at it later next week... (but feel free to just take what's there and modify if you want)

@sdroege
Copy link
Owner

sdroege commented Apr 16, 2015

Thanks, it's not urgent :)

@rossburton
Copy link
Contributor

The commit that "closes" this didn't actually close it, but had a stack track in so closed all bugs 1 through 14. Reopen please!

@jku
Copy link
Author

jku commented May 3, 2016

I notice I've promised things in this bug that I've not delivered on :/

I won't do anything about it in the next weeks. I'll try to fit it in the calendar later but won't mind if someone else grabs it.

@sdroege
Copy link
Owner

sdroege commented May 3, 2016

@sdroege sdroege closed this May 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants