Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#6383 closed Patches (Committed)

Extension of ExternalPlayer (file extension)

Reported by: tiben20 Owned by:
Priority: 4 - Normal Milestone: 9.11 "Camelot"
Component: Video playback (inc. audio in video and codecs) Version: GIT
Severity: Normal Keywords: ExternalPlayer
Cc: jmarshall, WiSo, yuvalt, vulkanr, elupus, davilla, MaestroDD, malloc, spiff, Jester, Blocked By:
Blocking: Platform: All
Revision:

Description (last modified by Gamester17)

This patch make possible to have multiple external players based on the media file extension:

externalplayersettings.xml

<externalplayersettings>

<avi>

<filename>c:\apps\smplayer\smplayer.exe</filename> <args>-fullscreen -close-at-end</args> <forceontop>false</forceontop> <hidexbmc>true</hidexbmc> <hideconsole>false</hideconsole> <hidecursor>false</hidecursor> <startuptime>2000</startuptime>

</avi> <divx>

...

</divx>

</externalplayersettings>

PS! For the original ExternalPlayer patch please see: http://trac.xbmc.org/ticket/5300 and more importantly this forum discussion: http://forum.xbmc.org/showthread.php?t=40136

Attachments (9)

myexternalplayerextension.patch (13.1 KB) - added by tiben20 6 years ago.
externalplayerdefaultfor.patch (3.2 KB) - added by tiben20 6 years ago.
New complety different version
playercorefactory_xml_config.diff (45.1 KB) - added by moby-uk 6 years ago.
Updated (and against SVN 19873)
playercorefactory_xml_config.2.diff (47.1 KB) - added by moby-uk 6 years ago.
playercorefactory_xml_config.3.diff (47.0 KB) - added by moby-uk 6 years ago.
playercorefactory_xml_config.4.diff (46.7 KB) - added by moby-uk 6 years ago.
with player name in attribute instead of element
playercorefactory.xml (823 bytes) - added by moby-uk 6 years ago.
Example userdata configuration file
playercorefactory_xml_config.5.diff (46.7 KB) - added by moby-uk 6 years ago.
s/StdAfx.h/stdafx.h/
playercorefactory_xml_config.6.diff (47.1 KB) - added by moby-uk 6 years ago.

Download all attachments as: .zip

Change History (46)

Changed 6 years ago by tiben20

Changed 6 years ago by tiben20

New complety different version

comment:1 Changed 6 years ago by tiben20

the new one allow to add a default player for an extension extension need to be seperated by ; like this ex: <externalplayer>

<filename>C:\Apps\mplayerww\mplayer.exe</filename> <args>-slave -quiet -fs</args> <forceontop>false</forceontop> <hidexbmc>true</hidexbmc> <hideconsole>false</hideconsole> <hidecursor>false</hidecursor> <defaultfor>avi;mkv</defaultfor>

</externalplayer>

comment:2 Changed 6 years ago by moby-uk

Following spiff's comments in the forum, I'm thinking of an XML description of the rules currently codified in PlayerCoreFactory, something like this:

<playercorefactory>
  <players>
    <!-- Standard players, from an xml file in the installation dir? -->
    <player name="PAPlayer" />
    <player name="DVDPlayer" />
    <!-- Just because PCF still refers to it, replace with DVDPlayer in the rules? -->
    <player name="MPlayer" type="DVDPlayer" />

    <!-- My players, merge from a new file in profile or a node in advancedsettings -->
    <player name="MPC-HC" type="ExternalPlayer">
      <config>
        <filename>C:\mpc-hc\mplayerc.exe</filename>
        <args>-fs {0}</args>
      </config>
    </player>
  </players>

  <rules>
    <!-- My rules; maybe merge these in from a node in advancedsettings? -->
    <rule filetypes="mkv" name=".*720p.*">
      <player-ref name="MPC-HC"/>
    </rule>

    <!-- Standard rules; from the installation dir? -->
    <rule protocols="rtv">
      <player-ref name="MPlayer"/>
    </rule>
    <rule protocols="hdhomerun|myth|cmyth|rtmp">
      <player-ref name="DVDPlayer"/>
    </rule>
    <rule protocols="lastfm|shout">
      <player-ref name="PAPlayer"/>
    </rule>
    <rule protocols="rtsp" filetypes="!(rm|ra)" >
      <player-ref name="PAPlayer"/>
    </rule>
    <rule internetstream="true" mimetypes="video/x-flv|video-flv|audio/aacp|application/sdp">
      <player-ref name="DVDPlayer"/>
    </rule>
    <rule internetstream="true" filetypes="mp2">
      <player-ref name="PAPlayer"/>
    </rule>

    <!-- Some specific DVD rules in PCF here, need some extra attributes? -->

    <rule filetypes="sdp|asf">
      <player-ref name="DVDPlayer"/>
    </rule>

    <rule video="true" audio="false">
      <player-ref name="DVDPlayer"/>
    </rule>

    <!-- Some more special stuff in PCF here  -->

    <rule audio="true">
      <player-ref name="PAPlayer"/>
      <player-ref name="DVDPlayer"/>
      <player-ref name="MPC-HC"/>
    </rule>

  </rules>
</playercorefactory>

I have a partial implementation, still working on it. The type attribute on the player nodes (defaults to the name) is because I'm currently constructing the IPlayers based on the current well-known names. Ideally I guess this could be a plugin-style dll/so load which could allow binary-only core distributions???

Any comments welcome....

comment:3 Changed 6 years ago by moby-uk

Attached is an initial pass at an XML-driven PlayerCoreFactory along with an example userdata externalplayer config. There's still some work to do on this:

  • there's a section of the current PlayerCoreFactory::GetPlayers that doesn't fit well (it init's a codec to make a decision)
  • I haven't looked at backward compatibility with the current externalplayer config
  • I haven't added the user-defined "replacers" that rewrite the filename for the externalplayer

comment:4 Changed 6 years ago by Gamester17

  • Cc jmarshall WiSo yuvalt vulkanr elupus davilla MaestroDD malloc spiff added
  • Description modified (diff)

Also see #6380 which is for Extension of ExternalPlayer, a patch by hannisen.

comment:5 Changed 6 years ago by spiff

hi,

i'll give a review tomorrow, a bit too intoxicated atm :)

comment:6 Changed 6 years ago by spiff

nits;

1) tabs -> 2 spaces 2) if(..) { -> if (..) \n { 3) you obviously need a player rule for audio codec. this is interesting now to handle the ac3/dts stuff, and maybe interesting in general once we get the hdflagging branch merged (full codec info and such about files will be available) 4) since those vars you moved are now members of the externalplayer (woho!), no need for the prefix in the variable names 5) don't comment code, remove it (code you don't need any longer such as stuff with the EPLAYERCORE) 6) very much like what i had in mind - cheers for working on this!

Changed 6 years ago by moby-uk

Updated (and against SVN 19873)

comment:7 Changed 6 years ago by moby-uk

I've updated the patch. Changes since the last version:

  • Dealt with spiff's nits
  • Moved the "filename replacers" config into ExternalPlayer.cpp
  • Added backward-compatibility for existing externalplayer configs
  • Added support for using "audiodefaultplayer", "videodefaultplayer" and "videodefaultdvdplayer" as core-names; you'll get the core defined in the appropriate advancedsetting (mostly this is there for backward compatibility)

I've put some of the rules: selection of the default audio & video players and the stuff that init's the DVDPlayerCodec back in as code (rather than xml-driven rules). I've done this becuase

  • I can't think of a elegant (i.e. won't look like a kludge) way to express this in the xml
  • because it seems to me like there's 3 places you may wish to add to the default rules:

1 At the beginning; in the attached example userdata playercorefactory.xml

I wan't 720p mkv to go MPC-HC

2 At the end of the special-case rules but before the default players 3 After the default players; because they shouldn't be the default for

anything, you just want to be able to select them from the "Play using..." menu.

however, putting the defaults back in the code means that in this current impl, you can't do 3 (prepend places the rules at 1, append places them at 2). My only idea at this point is to add a couple of attributes to the <player> element to indicate whether the player handles audio and/or video; this would remove the need to add a rule just to get the player into the "Play using..." list as it would drive the default players. Any other suggestions?

Changed 6 years ago by moby-uk

comment:8 Changed 6 years ago by moby-uk

(Sorry, formatting went a bit odd on my last post) I've added "audio" and "video" attributes to the <player/> element which drive whether that player is added to the list after all the rules are processed. It obviates the need to define rules for new players just to get them into the "Play using..." menu and makes the GetPlayers() code a bit cleaner.

comment:9 Changed 6 years ago by spiff

playercoreconfig leaks if it fails to init the player.

some stray .c_str()'s, http://www.cppreference.com/wiki/string/append

missing .c_str() in the log in CExternalPlayer::Initialize()

Changed 6 years ago by moby-uk

comment:10 Changed 6 years ago by moby-uk

Thanks spiff, took care of those. I also added caching of the playerName-to-playerCoreId lookup in a new CPlayerSelectionRule::GetPlayerCore()

Question: Should I ditch the <player-ref/> nodes and just have a "player" attribute in the <rule/> nodes? I added the <player-ref/> nodes initially because I anticipated having multiple players defined for a rule, but I don't currently need or support it; and an attribute is more readable?

comment:11 Changed 6 years ago by spiff

agreed, attributes are preferable

comment:12 Changed 6 years ago by spiff

also mind the case of stdafx.h in PlayerSelectionRule.cpp

Changed 6 years ago by moby-uk

with player name in attribute instead of element

Changed 6 years ago by moby-uk

Example userdata configuration file

comment:13 Changed 6 years ago by moby-uk

last question: a separate userdata/playercorefactory.xml for user-defined rules or a playercorefactory node in advancedsettings.xml?

comment:14 Changed 6 years ago by spiff

separate file

Changed 6 years ago by moby-uk

s/StdAfx.h/stdafx.h/

comment:15 Changed 6 years ago by spiff

excellent. will get this in asap :)

comment:16 Changed 6 years ago by elupus

There is something odd here. Why do you need that new function for default players? Why doesn't the xml syntax add the default players as last available player for a file?

I think the rules should allow a hiararcy..

<rule audio = true internet=true >
 <rule filetypes="(rm|ra)" player=DVDPlayer></rule>
 <rule player=PAPlayer></rule>
</rule>

ie for a audio file on internet, DVDPlayer is the first choice player for (rm|ra) and PAPlayer is the next possible one and allowed for all types.

comment:17 follow-up: Changed 6 years ago by moby-uk

I like the hierarchy idea, but I'm not sure I understand the semantics. At the moment I have a single, linear list of rules, first one that matches defines the default player for the file, is the above effectively shorthand for having

<rule audio="true" internet="true" filetypes="(rm|ra)" player="DVDPlayer" />
<rule audio="true" internet="true" player="PAPlayer" />

in that linear list or would there be some other sematics to a nested rule?

Re the default players:

Do you mean the "audiodefaultplayer", "videodefaultplayer" and "videodefaultdvdplayer" aliases? For backward compatibility.

Or, do you mean adding all the players with the "audio"/"video" attribute set as the very last step? I did originally have rules at the end of the system playercorefactory.xml to catch any file that hadn't matched one of the special-case rules, but, if a user wanted to add a <rule/> to define a default player for some files then they only had two choices

  1. "prepend" the rule, so it took precedence over all the built-in rules
  2. Copy the entire playercorefactory.xml from system and insert the rule where they wanted it

Also, it means a user defining a <player/> doesn't have to remember to define an explicit <rule/> as well just to get the player to appear in the "Play using..." menu.

That said, I'm not precious about it, discussion is good ...

comment:18 in reply to: ↑ 17 Changed 6 years ago by elupus

Replying to moby-uk:

I like the hierarchy idea, but I'm not sure I understand the semantics. At the moment I have a single, linear list of rules, first one that matches defines the default player for the file, is the above effectively shorthand for having

<rule audio="true" internet="true" filetypes="(rm|ra)" player="DVDPlayer" />
<rule audio="true" internet="true" player="PAPlayer" />

in that linear list or would there be some other sematics to a nested rule?

Exactly so.

Re the default players:

Do you mean the "audiodefaultplayer", "videodefaultplayer" and "videodefaultdvdplayer" aliases? For backward compatibility.

Skip backword compatibility in this case. It's pointless for this type of feature.

Or, do you mean adding all the players with the "audio"/"video" attribute set as the very last step?

Yea I meant adding rule at the end of the rules list that is the default rules. something like.

<rule audio=true>
  <rule player=PAPlayer/>
  <rule player=DVDPlayer/>
</rule>
<rule video=true>
  <rule player=DVDPlayer/>
</rule>

I did originally have rules at the end of the system playercorefactory.xml to catch any file that hadn't matched one of the special-case rules, but, if a user wanted to add a <rule/> to define a default player for some files then they only had two choices

  1. "prepend" the rule, so it took precedence over all the built-in rules

Which I think makes sence. If they want to overide our rules, they should prepend the data.

  1. Copy the entire playercorefactory.xml from system and insert the rule where they wanted it

To me that makes sence. If they want to customize xbmc, they are allowed to. But the they have to keep track of the changes. Another option would be to add a placeholder rule in the file where a userdata xml could prepend ammend, rules.

<include id="userprepend">
.. our rules ...
<include id="userammend">
.. defaults  ...

Also, it means a user defining a <player/> doesn't have to remember to define an explicit <rule/> as well just to get the player to appear in the "Play using..." menu.

I suppose that makes sence, but I sort of like that to be expressed in the xml file. Perhaps like.

<rule audio=true>
  <include id="audioplayers"/>
</rule>

But maybe my ideas are overkill :). Handling those includes are probably easiest to do by preparting the xml structure (using tinyxml) and replacing the <include> nodes with what they include.

Changed 6 years ago by moby-uk

comment:19 Changed 6 years ago by moby-uk

Haven't had much time to work on this but I've implemented the hierarchical-rules suggestion.

wrt having explicit rules for default players, instead of implicit ones derived from attributes on the player definitions, I'm not convinced it makes the configuration significantly more readable/flexible. The tradeoff of then maybe having to introduce <include/> flags doesn't seem worth it to me. Plus there's the fact that there's still one hardcoded "rule" (because it has to init the codecs and I can't think of a good way of expressing this in the XML; open to suggestions...) which has to be processed between the explicit rules and the default/implicit ones.

comment:20 follow-up: Changed 6 years ago by plight

Hi there. like what I see here so far. One thing I did not see (perhaps because of the fact that reading too much code makes my eyes bleed) is the ability to automatically discern which player to launch based on the directory structure of a iso.

An example would be launching POWERDVD when the iso contains a directory called BDMV and use the embedded player when VIDEO_TS is present.

comment:21 in reply to: ↑ 20 Changed 6 years ago by moby-uk

Replying to plight:

Hi there. like what I see here so far. One thing I did not see (perhaps because of the fact that reading too much code makes my eyes bleed) is the ability to automatically discern which player to launch based on the directory structure of a iso.

It's not your eyes, you didn't see it because it's not there :) In general I don't know what the impact would be of trying to do that; e.g. for an iso (or other archive format like zip/rar, which would be the next request) in a remote location like smb (or ftp/http??) how much would have to be dragged over the network to match every contained path against some regexp.

spiff: once the hdflagging branch lands would we have this kind of info available, i.e. whether an iso is hddvd/bluray format??

comment:22 follow-up: Changed 6 years ago by spiff

yes that should be available.

in the meantime i suggest adding a rule for specific tokens in the filename. that should do it until we get it in properly through flagging.

comment:23 in reply to: ↑ 22 Changed 6 years ago by moby-uk

Replying to spiff:

yes that should be available.

great.

in the meantime i suggest adding a rule for specific tokens in the filename. that should do it until we get it in properly through flagging.

already there (filename="<regexp>") :)

comment:24 Changed 6 years ago by ashlar

moby-uk, is part of your work already available on SVN or is this still "work in progress" material?

comment:25 Changed 6 years ago by spiff

the ticket isn't closed with an attached revision.

extrapolate from there

comment:26 Changed 6 years ago by spiff

  • Resolution set to Commited to SVN
  • Status changed from new to closed

r20983.

please make sure this is documented where it should be etc :)

cheers

comment:27 Changed 6 years ago by tiben20

you forgot to add the .sln from the last diff. Withtout the the .sln its not compiling on windows

comment:28 Changed 6 years ago by WiSo

no need to edit the solution file but I fixed the project file in r20990.

comment:29 Changed 6 years ago by Gamester17

@tiben20 and others, please make sure that you update the XBMC Online Manual (wiki): http://wiki.xbmc.org/index.php?title=Advancedsettings.xml and http://wiki.xbmc.org/?title=XBMC_Online_Manual and http://wiki.xbmc.org/?title=Frequently_Asked_Questions

Right now it does not look like any externalplayer option is documented at all :(

comment:30 Changed 6 years ago by tiben20

i would love to write a wiki explaining everything but moby-uk is the one who written the patch not not me

comment:31 Changed 6 years ago by Gamester17

Anyone can help maintain our wiki, you do not need to the the author (nor even a programmer for that matter) to help with updating the wiki.

Anyone can just register an account then send us a PM and we will give you write access to the wiki: http://wiki.xbmc.org/index.php?title=Special:Userlogin&type=signup

...in fact, most articles in the wiki are not written by the developers but by you, the users :)

comment:32 Changed 6 years ago by moby-uk

I will write some documentation for this, when I get a few spare minutes

comment:33 Changed 6 years ago by Gamester17

  • Cc Jester added

We are still looking for people to help update the wiki this, so PM me for access.

comment:34 Changed 6 years ago by Gamester17

Anyone willing to help with the documentation of the External Player functions? http://wiki.xbmc.org/index.php?title=Advancedsettings.xml

@tiben20, how about you? You seem to know it like the back of your hand :) @moby-uk, you too please? This feature really need to get documented ASAP

@everyone, please register an account on the wiki with your forum username: http://wiki.xbmc.org/index.php?title=Special:Userlogin&type=signup Then PM me to let me know you created it as I will give you write access: http://forum.xbmc.org/member.php?u=14188

comment:35 Changed 6 years ago by davilla

Gamester17, I think we need to threaten to remote said feature is no one steps up to document it. That might motivate someone.

comment:36 Changed 6 years ago by moby-uk

davilla, I've told Gamester17 a couple of times in the forum that ashlar volunteered to write up the documentation and I've sent some information to him which he is working on.

comment:37 Changed 6 years ago by Gamester17

ashlar has now created this article in the wiki as a beginning: http://wiki.xbmc.org/?title=HOW-TO_use_an_External_Player_for_media_playback

@moby-uk and tiben20, please register wiki accounts you too so that you can help update it and maintain it.

Can you please also look into updating the advancedsettings.xml article as well if needed? http://wiki.xbmc.org/?title=advancedsettings.xml

Thanks!

Note: See TracTickets for help on using tickets.