addons: deprecate plugin ability to set player. #1427

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
Member

theuni commented Sep 15, 2012

Now that we have multiple players in mainline, specifying a player in an addon should be considered a bug, and is deprecated for Frodo.

Instead, we always use the standard player detection logic.

@jimfcarroll please let me know if any other diddling is necessary with the code-generation. it appears as though this is all that's needed.

Member

elupus commented Sep 15, 2012

Hmm.. this seem strange to me. We allow specifying player in xbmc, why shouldn't a plugin be able?

Member

elupus commented Sep 15, 2012

If a player doesn't exist, have it fall back to default then instead. You may still want to force paplayer instead of dvdplayer for some streams, or gstreamer instead of dvdplayer whenever that shows up in linux builds.

Contributor

huceke commented Sep 15, 2012

You might force paplayer, doesn't mean the platform supports paplayer.

Member

arnova commented Sep 15, 2012

Like Elupus says, I think this is the wrong approach. Perhaps this needs a little more thought?

Contributor

huceke commented Sep 15, 2012

I think the plugin shouldn't know anything about the player selection. XBMC should be the one in charge to select the right player. Since we are getting more internal players and supported platforms, plugin writers can't know what the right player is to choose.
For example, on the PI we only have OMXPlayer, no PAPLayer or DVDPlayer. Plugin developers on platforms with DVDPlayer/PAPlayer have no clue that their plugin might not work on other platforms.

Member

elupus commented Sep 15, 2012

Then I assume you want to remove the play using option too? Users likely
know even less.

Imho it would be better to just fall back to default if requested player
doesn't exist.

Owner

Montellese commented Sep 15, 2012

IMO "play using" in the GUI is another use case. It only shows the players suitable for a certain file and if there's no dvdplayer it won't show as an option. So users can't really make a completely wrong decision whereas a plugin can do whatever it wants.Joakim Plate notifications@github.com hat geschrieben:Then I assume you want to remove the play using option too? Users likely
know even less.

Imho it would be better to just fall back to default if requested player
doesn't exist.

Reply to this email directly or view it on GitHub.

Member

elupus commented Sep 15, 2012

Right, so then let plug ins do the same. Also since upnp renderers on the
network will be exposed this way too, it make sense for plug in to set a
target player.

Contributor

amet commented Sep 15, 2012

most of the time addon authors will not spec the player needed, in case they do and its not available on the platform we should gracefully fallback to AUTO

Contributor

davilla commented Sep 15, 2012

that would fail for pivos as both dvdplayer and amlplayer are present.

Member

elupus commented Sep 15, 2012

What would fail? Auto?!

Member

theuni commented Sep 15, 2012

@elupus the issue is that addon author might specify dvdplayer, because that's where it plays best. but on pivos/rpi, it needs to be routed to the hardware player.

We have tons of logic in playercorefactory for deciding the right player to use. It's senseless to leave it up to the addon.

Contributor

amet commented Sep 15, 2012

@davilla if AUTO will fail then we are doing something wrong, addon developers will almost never spec the player but we have to be ready for it.

@theuni if author specs for DVDPLAYER and that shouldn't be the case for a specific platform than we should overrule it in that platform only.

Member

theuni commented Sep 15, 2012

@amet: of course, that's exactly what this commit does. The player it set to auto, platform uses the default player to play it. on desktop, that'd be dvdplayer. on embedded, it'd be the hardware player.

I really don't understand the controversy here. All we're doing is ignoring the setting, and using AUTO instead. Desktop users will see zero change.

Contributor

amet commented Sep 16, 2012

@theuni what happens if PAPLAYER is chosen by the addon as it performs better? on desktop it will go to DVD player by default and that was not what the addon author wanted.

all I am saying is that if these changes are needed for embeded or specific platforms they should apply to those platforms and not on Desktop as well

@ghost

ghost commented Sep 16, 2012

then there is something wrong with our playercore rules.

Member

arnova commented Sep 16, 2012

I think we can consider this functionality an Xbox remnant where we had both mplayer & dvdplayer on the same platform and some stuff would only work with one of them. Since this is no longer the case on any platform the only thing the remains is either default video or default audio player...

Owner

Montellese commented Sep 16, 2012

Which and how many use cases are there were it makes sense for a plugin to dictate which player to use? IMO if there is such a case it means that our player selection rules in core and not optimal and could be improved. DVDPlayer should handle anything that contains video and PAPlayer anything that only contains audio and if there are other players available there should be specific rules that prefer those players over DVDPlayer/PAPlayer. IMO letting plugins specify the player will only lead to problems and confusion.

@amet: So you want to have different plugins for embedded and non-embedded XBMC installations? Good luck with that.

Contributor

amet commented Sep 16, 2012

@Montellese I am not sure how you got that out of what I said :)

if addon developer specs PAPlayer and we default it to AUTO with this PR desktop users will use DVDplayer and that is wrong I think. if PAPlayer is needed by the addon and it doesnt exist for the platform then we need to default to AUTO, if DVD player is needed and user is on Pivos, again default to AUTO but if user is on desktop let it be DVDplayer

Contributor

huceke commented Sep 16, 2012

@amet: But you know on Desktop default would be DVDPlayer/PAPLayer.

Member

elupus commented Sep 16, 2012

Again. Why shouldn't a addon be able to target an upnp renderer on the
network. Or gstreamer instead of dvdplayer should we add that.

I se no point in hiding functionality we have in xbmc (and are not going to
remove) from addons.

The addon should just be marked broken until it is just optional on the
platform.

Member

arnova commented Sep 16, 2012

An alternative could be specifying multiple players using bitmaks, eg. (DVDPlayer | OMXPlayer | AMPlayer). Obviously this does require us to modify the defines for those.

Contributor

huceke commented Sep 16, 2012

@elupus: Draw me a strong example why it is that important for plugins to be able to set the player ?

Member

elupus commented Sep 16, 2012

Say you have 4 xbmc systems on your network. Plugin should select which one
to play the selected item on.

Mp3 stream works badly in Paplayer plug in ask to play it in dvdplayer.

Seeking doesn't work in omxplayer, until solved in xbmc, addon forces
dvdplayer since it know stream doesn't need hw acceleration.

Contributor

huceke commented Sep 16, 2012

@elupus: here we come to the point you oversee. On RaspberryPI only OMXPlayer is usable. You can't select DVDPlayer or PAPLayer. There might come other platforms where it is the same.

Member

opdenkamp commented Sep 16, 2012

that's really something that should be decided in xbmc, not in the plugin imho.

i'm not 100% familiar with how it's done now, but if it's not done already, then the plugin should provide some metadata and let xbmc decide what the best player is instead of trying to figure it out in the plugin. the best player for the file can be different for each platform the same plugin runs on, can be different when something changes in xbmc, etc. why would we want to have logic in plugin when xbmc already contains a selection mechanism?

Member

elupus commented Sep 16, 2012

If player isn't supported it should use default.... So that is imho a mute
problem. Only issue is if player is available but known worse.

We should not remove functionality from addon api due to add on devs being
stupid. Then it's better to remove the addon.

Also how should xbmc know if user want to playback in living room or
kitchen? This user must decide, and if Plugin control player startup,
Plugin must decide what player to use.

Contributor

huceke commented Sep 16, 2012

Plugin should only tell the source to play. XBMC should decide which player to use.

Member

elupus commented Sep 16, 2012

Do you even read what I write?

Contributor

huceke commented Sep 16, 2012

@elupus: yes i do. We just have a different point of view on player selection and plugins. I could ask you the same ;)

Member

elupus commented Sep 16, 2012

Then how do you solve the multi room playback if addon can't specify what
room to start playback? It is a fullscreen addon, thus xbmc is in no way in
control. It will only react to a play media command from addon.

Member

opdenkamp commented Sep 16, 2012

I'd rather see this solved by adding some "room to play in" param to the play media command, than selecting the player type to use.

Member

elupus commented Sep 16, 2012

Perhaps yes. But why make a difference?

As I see it, the only issue we have with current system is on platforms
where one player is suboptimal. Then mark the addon that miss uses the
feature as broken.

Addons should be able to control every aspect of xbmc imho.

Member

opdenkamp commented Sep 16, 2012

Because we already got a player selection mechanism in xbmc. If it doesn't select the correct player for a file that a plugin requests to be played, or if a feature is missing there (e.g. to select the correct player for a specific room to play in), then that's the thing that needs to be fixed imo. I can't really think of any good reason why we should allow plugins to do this. But again, I'm not 100% familiar with the code and how it's being used now, so ignore me if you like :)

Contributor

huceke commented Sep 16, 2012

I think i understand your point ( a bit ).

How i see it, our main problem with the API is that hardcoded player names are posible. The proper solution would be, the plugin is requesting a player based on a type ( video / audio / .... ). Based on that type XBMC creates, based on it's internal player selection, a player for the plugin.

Member

elupus commented Sep 16, 2012

@huceke , most of the time there are multiple players for file type. There could be external players, internal player, players on other network. All these can match a certain file type. All could be fully valid choices that a user want for playback (especially external and on network). If we don't allow plugin to specify a player, xbmc would always have to pop up a dialog allowing the user to select a player every time a addon ask xbmc to play something. That would be incredible annoying.

But yes, we do allow hardcoded players currently which I agree can make it easy for an addon dev to always select dvdplayer. But then that is a bug in the addon, not in our API.

What however may be a "bug"/lack in our API is that I don't think there even is a way to request a list of available players for a certain item. We should expose the same logic as the Play Using context menu uses to populate a list of available players. That is a sorted list in order of preference of available players for an item. From that list the addon could select a player.

Owner

Memphiz commented Sep 16, 2012

The question for multiroom is - how is an xbmc instance announced? Is it like a seperate "player" for each instance like "upnp@xbmcinlivingroom", "upnp@kitchen" and so on? Or is the room some sort of parameter to the upnpplayer (like after selecting upnp you need to query a list of found upnp targets?

Member

elupus commented Sep 16, 2012

Well.. there is no such thing as room in this regard. It is just going to be a upnp frendly name. So there is technically no way other than the name of the remote player to know what room it is in.

Member

elupus commented Sep 16, 2012

Ie all upnp renderers on you network will show up in play using context menu with their friendly name.

Owner

Memphiz commented Sep 16, 2012

Right - so there needs to be an API call for getting that list definitly.

Member

jmarshallnz commented Sep 17, 2012

I agree that having the list exposed is essential as is validating the
requested player against that list.
On 16 Sep 2012 15:35, "Memphiz" notifications@github.com wrote:

Right - so there needs to be an API call for getting that list definitly.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/1427#issuecomment-8594856.

Member

theuni commented Oct 10, 2012

Hesitant bump. We need some decision on this. Imo upnp devices and other future things should be left out of the argument and hash them out as they come in.

@jmarshallnz how would you propose validating?

Member

jmarshallnz commented Oct 10, 2012

In the magical future, xbmc.getplayers(media_item) would return a sorted list of allowed players, with preferred on top, and the plugin would select one of the players from that list. Validating, therefore, would simply check that the chosen player was in the allowed list. If it wasn't, it'd be ignored and the default player for that item would be used. We could even comment in the log that this player is suboptimal if it's not the optimal choice.

I presume that this validation is already done in XBMC now anyway, otherwise what happens if the player specifies that they want mplayer?

The only problem then is with add-ons that specify a player that is available on the platform but known to be worse than another player. The add-on is at fault for choosing a bad player for the particular item, but I see no reason why XBMC should ignore their choice. If the player is so bad on a platform that it should never be used, then don't allow it in the allowed player list.

Thus, on playback where a player is specified, I'd:

  1. Retrieve allowed player list, sorted so the optimal player is on top.
  2. Check the given player is in the list - if not, choose the top one.
  3. If it is in the list and is non-optimal, log the fact.
  4. Remove any players that are completely inappropriate from each platform.
Contributor

davilla commented Oct 10, 2012

The problem is in the past, addons could pick paplayer (for audio), dvdplayer (for video) and mplayer (not a clue what happens). This worked fine on desktop for picking the audio player (paplayer) or the video player (dvdplayer).

Note the common theme, audio or video. That's the choice that addons want, not to explicitly pick a player but to vector to the default audio or default video player. They really don't care about which internal video player is used, they only had one choice before and that is what some have hardcoded.

Now we have embedded and two new internal players, amlplayer and omxplayer. These new internal players are the only way to play HD video content on embedded as there is just not enough ponies to use software decode and dvdplayer.

So rather than complicate things by exposing internal video player names (which is only going to grow), just limit the addon to being able to pick one, either the default audio, or default video and XBMC core will make the choice as to which internal video player is best for that platform. This simplifies the solution and still gives addon their audio/video player choice.

Member

jmarshallnz commented Oct 10, 2012

Consider the happy world of only 2 players: paplayer (audio-only) and dvdplayer (either). In this world, the add-on author doesn't have a choice between audio and video player, as there's only one video player anyway.

Thus, the only reason that the add-on author has to choose between the players is in the case of audio-only playback. One presumes, then, that they're choosing dvdplayer as it works better for an audio-only item than paplayer does. This assumption should ofcourse be checked with data - I'm sure that a quick grep through the addons repo will identify the add-ons that use this feature and the authors can then be queried.

If there is a reason for this to be there, then we really need to leave it there. If there's not, then you can kill it.

In the meantime, if platform A really doesn't want anything other than player B available, then that is all it returns in the list of allowed players.

The first step either way is validating the choice the add-on makes against the list of allowed players. This provides a mechanism for the platforms that don't want to use dvdplayer or paplayer to cross them off the list.

Member

theuni commented Oct 10, 2012

@jmarshallnz sight unseen, I'll buy you a sixer for any addon you can find in the repo with a legitimate need for specifying any player that it's specifying.

Imo you're asssuming a need here when there's no need. As I see it currently, it's an option that's exposed that can only break things.

Member

jmarshallnz commented Oct 10, 2012

I wouldn't be surprised if that were the case, though equally, I wouldn't be surprised if there are some audio streams that work nicer in dvdplayer than in paplayer, so the potential for the choice being available is there. Whether it's enough to warrant having it available for abuse, I have no idea.

Assuming there's no reason for it being there, then this would be a bugfix, so I don't see the need for a hurry. Perhaps the best way to convince @elupus would be to show there's no existing use case for it by checking the audio add-ons (no need to check video add-ons as there's only one player there in the old world).

Member

theuni commented Oct 10, 2012

Reasonable as always, I'll accept that as a fair criteria and report back.

Member

jmarshallnz commented Apr 8, 2013

@theuni: At the risk of re-poking the hornets nest, what was the verdict?

Owner

MartijnKaijser commented Sep 29, 2013

I would like to resurrect this PR
I went through the plugins in repo and the audio ones either use:

  • xbmc.PLAYER_CORE_PAPLAYER
  • xbmc.PLAYER_CORE_AUTO
  • xbmc.PLAYER_CORE_MPLAYER which obviously doesn't work any more
  • only one using xbmc.PLAYER_CORE_DVDPLAYER but judging from the addon this playinly wrong and should not being using DVD_PLAYER

So i would say drop the ability to choose players.
@theuni care to rebase thise PR or should i open a new one?

rivy commented Jan 18, 2014

@theuni , @MartijnKaijser , @jmarshallnz

Sorry to interpose myself in this conversation, but I have a use case (the XBMC Pandora plugin @ https://github.com/rivy/xbmc-pandora), in which I must specify xbmc.Player( xbmc.PLAYER_CORE_MPLAYER ).play( url, item) because the AUTO logic logic fails to find an appropriate codec to play the stream (specifically, for .mp4 audio streams).

I assume that .mp4 audio streams must be unusual. But I'm sure some similar issues may arise where the basic info about a file or stream doesn't lead to correct AUTO selection.

I see in http://forum.xbmc.org/showthread.php?tid=173887&pid=1516662#pid1516662 that xbmc.Player([core]) is already deprecated. But, if you actually remove this functionality, how do you propose suggesting to the XBMC player that it needs to search for in a specific realm (eg, audio) for a codec when the AUTO logic fails?

Or, is this use case a bug, or an incorrect usage of the player API?

@rivy rivy added a commit to rivy/xbmc-script.audio.pandora that referenced this pull request Jan 18, 2014

@rivy rivy UPDATE: minimize use of xbmc.Player( xbmc.PLAYER_CORE_MPLAYER ); add …
…deprecation commentary

* xbmc.Player( xbmc.PLAYER_CORE_MPLAYER ) is deprecated [ see URLref: http://forum.xbmc.org/showthread.php?tid=173887&pid=1516662#pid1516662 ]
* using the CORE_MPLAYER is currently necessary to play .mp4 files (low/medium quality from Pandora)
* PENDING investigation about a bugfix for playing the stream or leaving the current API as is in Gotham [ see URLref: xbmc/xbmc#1427 ]
e22dbdc
Member

da-anda commented Jan 22, 2014

@rivy the inability to detect the correct player for mp4 is no reason to drop this feature. It's a bug that has to be fixed.

@elupus - I agree that plugins should be able to specify the target device XBMC should play to, but no longer the internal player. In doubt abstract it and use xbmc.PLAYER_CORE_AUDIO and xbmc.PLAYER_CORE_VIDEO which internally resolves the correct player for the specific media type.

Member

popcornmix commented Jan 22, 2014

I'm in favour of this PR. On Pi I'd like to choose between dvdplayer and omxplayer, but currently they are hacked to both resolve as omxplayer.

If I remove that hack I'm hit by this problem that iPlayer (and other plugins) request dvdplayer, even though the default video player is set to omxplayer.

I agree with @da-anda that a plugin should not be allowed to choose a specific internal player.
xbmc should choose the default player, and if there are cases where xbmc is not able to choose the desired player, then perhaps abstract hints could be allowed. e.g.
(PLAYER_SUPPORTS_VIDEO | PLAYER_SUPPORTS_STREAMING).

Member

elupus commented Jan 23, 2014

Why should a addon not be able to specify a player over UPNP?
Again.. I've said it before. Add a new (optional) parameter to the playfile function. Ignore the currently passed parameter, let the new parameter work.

Member

elupus commented Jan 23, 2014

What popcornmix also make sense. Ie filter based on features instead, but that should not remove the ability to specify player.

Owner

MartijnKaijser commented May 25, 2014

@popcornmix
chances are good if the last comments are taken care of.

@LongChair LongChair added a commit to plexinc/plex-home-theater-public that referenced this pull request Feb 5, 2015

@LongChair LongChair Fix Playlist addition , part of #1427
Looks like unprocessed_rating key is not available anymore ....
8da83de

@LongChair LongChair added a commit to plexinc/plex-home-theater-public that referenced this pull request Feb 5, 2015

@LongChair LongChair Fix PL EditMode , part of #1427 8807e76

@LongChair LongChair added a commit to plexinc/plex-home-theater-public that referenced this pull request Apr 30, 2015

@LongChair LongChair Dont Do Play Systematically on PL Window upon selection, only if we'r…
…e in the list, fixes #1427
8133afc
Owner

MartijnKaijser commented Jul 12, 2015

@popcornmix i'm still in favour of getting this in. Any chance of you picking this up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment