[JSON-RPC] Adding support for plugin based video source inside json-rpc command GetFileDetails

[bigretromike]

Description

My code change only touch one function of JSON-RPC, it check if the item that we asked for is based on plugin. Next it try to access that plugin and get items from it, like the old way but I adapted it to the way the creator did with video source (using PluginDirectory). I rearange old code so if its non plugin content it works exactly the same as it did and if its plugin source it try to extract the proper url for it because without extracting the proper url this would end up with playing the while trying to match it. DB saves the url for file and we check against plugin, thats why It tries to get the url thats ends with / because we need to execute the plugin to get the ListItems for that file.

Motivation and Context

From Kodi 18.2 you can use video plugins as video library source, this is awesome feature, but not everything support this source. For example the JSON-RPC that could return information about file and ID from db. With current code the function return INVALID Parameters which is other way of saying Not supported instead of you used wrong parameters.
This fix issue that I posted some time ago: #15897
Also I would love this being backported to 18.x - this code should be compatible with it because I started on 18.x and then upgraded to 19.x

How Has This Been Tested?

This code only touch one function of JSON-RPC call for item inside db, as before plugin:// wan't avaiable to be inside video library this code does not break any compatibility.
I coded/compiled/run this code on Win10Pro with VisualStudio2017, I added few normal files to video library and I added plugin.video.nakamori as source test, I then executed JSON-RPC GetFileDetails with full url from DB to get ID to verify it working - it did return proper ids. I run JSOn-RPC GetFileDetails on normal (file) objects from db and the results was proper ids.

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Clean up (non-breaking change which removes non-working, unmaintained functionality)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that will cause existing functionality to change)
• Cosmetic change (non-breaking change that doesn't touch code)
• None of the above (please explain below)

Checklist:

• My code follows the Code Guidelines of this project
• My change requires a change to the documentation, either Doxygen or wiki
• I have updated the documentation accordingly
• I have read the Contributing document
• I have added tests to cover my change
• All new and existing tests passed

[thebrid]

The indentation seems a little off here.

[bigretromike]

I replaces tabs with spaces;
Now it should look better;

[bigretromike]

@thebrid any chance you could approve/review this ? I would love to start using it

[thebrid]

@bigretromike I don't have merge access.

[bigretromike]

oh, thanks anyway. Will wait then.

[rmrector]

This should not be in the JSON-RPC layer.

It looks like just skipping the CFile::Exists check should get the basics working, but the better solution is a new CPluginFile from CFileFactory to match CPluginDirectory, and implement the "existence" check described in #14210.

[rmrector]

I do still feel strongly that this shouldn't be an extra code path in the JSON-RPC layer, though.

I tried a patch that just skipped the "exists" check for plugins and a path like plugin://plugin.video.testlib/lib/tvshows/121361/3254641 worked for me with AkariDN's test plugin.

This particular function works by listing a directory and then picking the requested item out of that list, which means plugins need to present like a basic file system: listing the parent directory should include the requested path.

[bigretromike]

@rmrector Strange because when adding testlib plugin as a tvshows source, the "files" added to DataBase are in form of plugin://plugin.video.testlib/tvshows/?sid=121361&eid=3436421 and I'm unable to retrive that item uniqueID with GetFileDetails with path: plugin://plugin.video.testlib/tvshows/?sid=121361&eid=3436421 which is the issue here.

also with this code skipped:

  if (!CFile::Exists(file))
    return InvalidParams;

I get InvalidParams error because this code throw it:

if (path.empty() || !CDirectory::GetDirectory(path, items, "", DIR_FLAG_DEFAULTS) || !items.Contains(file))
    return InvalidParams;

But yes, when asking for plugin://plugin.video.testlib/lib/tvshows/121361/3254641 which is not in MyVideosXXX.db -> files I get basic info, but from my understanding the proper file to ask should be:
plugin://plugin.video.testlib/tvshows/?sid=121361&eid=3254641 which is in files table and with my path you get proper idFile for it and you can interact with it as with normal files.

[bigretromike]

Also the issue is you are unable to ask "database" about additional information:

{"jsonrpc":"2.0","id":5,"method":"Files.GetFileDetails","params":{"file":"plugin://plugin.video.testlib/lib/tvshows/121361/3254641","media":"video","properties":["episode","title","uniqueid","tvshowid","playcount"]}}

return:

{
    "id": 5,
    "jsonrpc": "2.0",
    "result": {
        "filedetails": {
            "episode": 1,
            "label": "Winter Is Coming",
            "playcount": 0,
            "title": "Winter Is Coming",
            "tvshowid": -1,
            "type": "episode",
            "uniqueid": {
                "tvdb": "3254641"
            }
        }
    }
}

Even when
files have this info:

1236	120	plugin://plugin.video.testlib/tvshows/?sid=121361&eid=3254641	1	2019-05-23 09:22:22	2018-01-15 05:45:43

And the fact that I 'watched' that one file is also on Kodi main window 'In progress TV shows'

Which shows that you cannot ask database for file instead you execute plugin and get information from there which make it a bit strange because when you watch file added via plugin source your playcount is saved to database but you are unable to retrieve that information, and can't do anything about it.

Also when you adding content from Plugin Source you add it to db, when you play it you save metadata in db so it is logical that I want to access that data somehow and read it from DB not from plugin.

[AkariDN]

@bigretromike that's all you need: AkariDN@6d5d048
Information can be obtained directly from video database, no need to call CPluginDirectory::GetDirectory

[rmrector]

Wait, no, this doesn't sound right. This method is about file details first and I think it should only return info if the file can be accessed; it's not just a way to find database info by file path.

Plugins get to determine what "can be accessed" means. Or maybe more accurately the plugin layer: CPluginDirectory::Exists just returns true and such a thing for CPluginFile sounds well enough for me. But I don't see a good reason to treat plugins special here in the JSON-RPC layer.

[bigretromike]

@rmrector I don't know where you came to that conclusion but GetFileDetails return details about file from database right ? the metadata that scrapper helped to get, right ? so why would I have to ask scrapper (this time its a plugin) about file that is already in database? I understand that I could ask scrapper (plugin) for update info when upgrading VideoLibrary. Especialy when Kodi VideoLibrary hold the data (about playcount, lastplaytime, id, etc).
Maybe Im wrong, because Im not part of the Team, but while having great feature added by @AkariDN I would like to have same accessibility as with normal files, the MINIMUM i would like to get is a dbid so I can update or query info from/in database.

@AkariDN Would you mind pushing your fix as PR? As the author/creator of this awesome feature you already pave the way. I just tried to make this work as best I was able to without any experience while trying my best to not break ANY legacy code - looks like it was not enough, and being not able to get dbid from already added items without hacks like mapping service/hooking to player is a waste.

I don't mind if my PR will be closed without merge, the important part is to make the feature usable so people can start using it without waiting next 2 version to anything to be added.

[AkariDN]

it's not just a way to find database info by file path

Based on function name I think the main goal is to return file's information, especially when "media=video". In this case call to plugin is a bad idea. If this function should check plugin url "existence" it can be done in "media=file" mode via plugin call. If this function should always call a plugin, then it is not suitable for author's purposes. Could you please point to json-rpc function that can return dbid by plugin url (path) without calling a plugin?

Would you mind pushing your fix as PR?

I can make a PR, but I have no time to support it. Sorry. I'm not a json-rpc expert.

[rmrector]

I don't know where you came to that conclusion

It's right in the name, GetFileDetails, and the code was specifically designed to do exactly that. This is about the file first. There's nothing special about plugins.

Could you please point to json-rpc function that can return dbid by plugin url (path) without calling a plugin?

There isn't such thing exactly. The closest we've got now is VideoLibrary.GetMovies and the like filtered by path. Not the best solution because you need to know what the media type is first. That's still not a good reason to hack this method up to do something else sometimes treating plugins differently.

I appreciate that you need something more, but it needs to be designed differently. Plugins are not special here. Maybe a new method to look up library info by path like Akari's patch, but not limited to plugins. This functionality could be just as useful to other file sources.

[AkariDN]

@bigretromike If rmrector insist that GetFileDetails should check "existence" by calling a plugin - you can implement new method VideoLibrary.GetFileId (or GetEpisodeId, GetMovieId - I don't know what kind of "dbid" you need) and use appropriate CVideoDatabase functions. Or use VideoLibrary.GetMovies or VideoLibrary.GetEpisodes as rmrector said. Especially if you want to sync watched state for all Nakamori videos, then methods like GetEpisodes would be better because you can get all your tvshow's episodes in one call.

{"jsonrpc": "2.0", "id": 1, "method": "VideoLibrary.GetEpisodes", "params": {"tvshowid": 1, "properties": ["uniqueid", "playcount"]}}

[DaveTBlake]

A general point - when changing the internal functionality of JSON methods a bump of patch version is also needed (so |API consumers know that something has changed).

But before we get to that I have to say I have hesitations about this modifcation to GetFileDetails. GetDirectory should be able to return the same info as GetFilesDetails, so just changing this one method seems wrong.

Also if the items is scraped into the library why are you using GetFileDetails at all (that is for files not in the library)?

That's still not a good reason to hack this method up to do something else sometimes treating plugins differently.

Yeap, I think I agree with @rmrector. No doubt there is some new fucntionality you need, and happy to help work that out, but not convinced this is the right approach. Far too much is already done at JSON level that should be down in core (and thus common to GUI, Python and JSON intefaces) and results in different implementation of things depending on route through the code which is not good.

[bigretromike]

But before we get to that I have to say I have hesitations about this modifcation to GetFileDetails. GetDirectory should be able to return the same info as GetFilesDetails, so just changing this one method seems wrong.
So you want to change bot GetFilesDetails and GetDirectory ?

Also if the items is scraped into the library why are you using GetFileDetails at all (that is for files not in the library)?

Like I mention before, when adding item to VideoLibrary via new feature which is Source=Plugin currently there is no way to retrieve DBID of that item, the only JSON-RPC I found in Kodi Documentation was GetFileDetails, as there was error about protocol plugin:// not supported I tried best I could add that plugin:// support into it without touching any existance code - so I wouldn't break anything (while hoping for quick merge maybe even in 18.x). If GetFilesDetails is for files outside VideoLibrary (strage?!) what command should be used to retrive information about file (playcount, playdate) that was added via Source=Plugin ?

That's still not a good reason to hack this method up to do something else sometimes treating plugins differently.

The issue here is that without modification Kodi code treat plugin source diffrently - it does not see any added file, while VideoLibrary is proper populated. So doing nothing would make tread it diffrently. Also without support for plugin:// in those commands the new feature which IMO is one of the awesome stuff that happened lately is half usable because you cannot do anything with files added that way, while you can do almost anything via json-rpc with files added normally again, which make different.

Yeap, I think I agree with @rmrector. No doubt there is some new fucntionality you need, and happy to help work that out, but not convinced this is the right approach. Far too much is already done at JSON level that should be down in core (and thus common to GUI, Python and JSON intefaces) and results in different implementation of things depending on route through the code which is not good.

As I would be super happy I also know how this could end, you would like to have this in core - maybe on python level, and while its super super no one will be willing to do this right now. Quick fix for one function is a no no. I would be super happy if there were new commands GetPluginFilesDetails or something like that but then again, why would we want treat files added via plugin source any different that those added with simply folder.

Probably adding anything exclusive to Plugins in json-rpc would be also a no no.

TL;DR; I wanted to fix the only function that needed to be fix GetFileDetails so anyone could retrive DBID of item and do what he want with anyother function because they work on ID, which currently you are unable to retrieve in any way.

Maybe this is all not needed, maybe there is a way? Just need to know this:

• While knowing file plugin path (that was added to VideoLibrary) how do I retrieve id of the row that he sits in (or playcount/playdate from that db, because solution that @rmrector would retrieve information from inside plugin and not database which miss the point)?

[bigretromike]

I appreciate that you need something more, but it needs to be designed differently. Plugins are not special here. Maybe a new method to look up library info by path like Akari's patch, but not limited to plugins. This functionality could be just as useful to other file sources.

I didn't saw your response there.
From my point of view you are the one that differents the cross between files added by plugin source and files added normally.
GetFileDetails for normal files retrieve information from kodi database while your proposal (by bypassing File::Check) execute plugin and retrieve information from inside plugin and not database.
Sorry but I have no files that include playcount/playdate/title that are updated inside file, that metadata is saved for kodi in database, so why would you insist in executing plugin to get that data from outside of kodi database?

[bigretromike]

Ok - I understand that noone want to approve this - nothing strange, it just a "quick way to fix new problem". It took me few days to be able to make this work, and I don't think it was a waste of time because it did worked in the end - maybe not by standards that this open source project needed by in fact did work and did not break anything is enough for me.

I understand that it would be even better if there would be a different commands/json-rpc/python/etc endpoint that would support this (somehow)... and I hope that some day it will be needed ... but Im unable to help with it, because the point of complication in this project is our of my league and I admit it.

So if there is nothing more I can do please close this PR, because I have not clue how to make a new function in other place or use pointers - it was miracle that I was manage to copy-past those there.

And I just wish that someone, someday will need this function and will be willing to write code that will be worthy being merged.

TL;DR; I was hoping that lack my poor quality code would be fixed by someone that knew what to do and merge in the end, maybe next time...

[rmrector]

The only thing I insist on here is that plugins don't get special treatment in the JSON-RPC layer. I suppose I am presenting other ideas, but that is the one thing I'd like now.

From my point of view you are the one that differents the cross between files added by plugin source and files added normally. GetFileDetails for normal files retrieve information from kodi database while your proposal (by bypassing File::Check) execute plugin and retrieve information from inside plugin and not database.

Both will get as much info as possible from the file system and the database - the plugin file system is just designed to return library-level data in the initial listing.

While knowing file plugin path (that was added to VideoLibrary) how do I retrieve id of the row that he sits in (or playcount/playdate from that db, because solution that @rmrector would retrieve information from inside plugin and not database which miss the point)?

@bigretromike The IDs are available in many library methods, and the library list methods can be filtered on several fields. If you want to sync libraries use VideoLibrary.GetEpisodes and work with the list. You can also set a uniqueid for each episode in the plugin and then filter on that - you can also filter by path. Nope, plugins are special, but you've found filtering by file works. There are JSON-RPC methods designed specifically to get library information, consider them for your needs. Further discussion on options available now for your specific use case should be taken to the forums.

[bigretromike]

@rmrector you are totally right about VideoLibrary.GetEpisodes and uniqueid.

The only unknown for me is still why you think I wan't to treat plugins special ? From my perspective I tried to make it same for files and plugin, because when you do json-rpc for file info you don't play that file, why would you do that for plugin.

Do you assume I treat them differently because I used code do build PluginDirectory ?

Edit1: Also going back to your recommendation about using path, no you cannot, because Kodi treat plugin:// differently :-) you can only look by name of plugin, and not by full path, just so you know - because Kodi save file differently than files (it save strPath as plugin name, and files as full url with plugin name) and it looks like path filter look in Path Table.

Edit2: Looks like its not possible to Filter Episode by its uniqueid https://kodi.wiki/view/JSON-RPC_API/v9#List.Filter.Fields.Episodes

[bigretromike]

@AkariDN Could the issue with JSON-RPC is result of a bug when VideoLibrary is populated with plugin-source based files?

Because for some reasons in Path I see root path (which is name of plugin) and sub-directories that contains series, while Files are linked only to root path (the plugin name) - shouldn't they be linked to folder that they came from ?

Because when you look for them using path filter, you see all of them when you type name of plugin and none when you filter by pluginname/tvshows

[bigretromike]

using filename as filter give proper results.

[AkariDN]

Files are linked only to root path

This is Kodi plugin parenting rules (see URIUtils::GetParentPath)

plugin://my.favorite.plugin/fld1/fld2/?k1=v1&k2=v2 -> plugin://my.favorite.plugin/fld1/fld2/
plugin://my.favorite.plugin/fld1/fld2/ -> plugin://my.favorite.plugin/
plugin://my.favorite.plugin/?k1=v1&k2=v2 -> plugin://my.favorite.plugin/

[rmrector]

The only unknown for me is still why you think I wan't to treat plugins special ?

It's not about what you want, the code you are adding literally treats plugins differently from all other paths in the JSON-RPC layer. Their behavior should be adjusted where it differs in the first place, don't add another place where their behavior differs.

[rmrector]

I appreciate your effort here but the fix for #15897 will need to work a different way. As you've noted you won't be able to help code-wise I will go ahead and close this PR, but this is useful information for future changes.

[dagwieers]

That Files.GetFileDetails() and Files.SetFileDetails() are only about (real) files is a semantic discussion, and one that is misleading some people here. A file in Kodi is used for virtual objects as well, just like path also reflects virtual paths.

What about Kodi logging this:

2019-09-28 02:53:29.693 T:1937239584  NOTICE: VideoPlayer::OpenFile: plugin://plugin.video.vrt.nu/play/id/vid-79b4c336-3374-4a59-92b7-ea9baa6ec28f/pbs-pub-b5105259-8d2d-4601-93e8-48b80222b705

So Kodi has a call VideoPlayer::OpenFile() and lo and behold, it plays a plugin:// URI. The fact that Files.GetFileDetails returns not file information, but media information should have been the first clue that this was not about real files only. The fact that it returns information from a database, should have been the second clue.

I am the first person to cheer for internal consistency and good naming standards when designing a framework, but if the naming of internal functions was too limited from the start, please don't use it as a design decision, because you are dragging everything down the wrong path with it.

IMO SetFileDetails and GetFileDetails simply ought to work on the same set of data, and the fact it doesn't support all paths was an oversight. It is easy to support it with backward compatibility in mind by making a clear distinction between real files and "virtual" files and handling them differently (because of backward compatibility). At least the database backend doesn't really make a distinction between both but for historic reasons we probably have to.

[michaelarnauts]

@rmrector can this be reconsidered? The only workaround that is possible now is to manually modify the database of Kodi directly, and that's something that should be avoided IMO.

[rmrector]

Nope. Merging incorrect (and mostly unnecessary) code changes is not healthy for the project.

It looks like just skipping the CFile::Exists check should get the basics working

In my testing works just as well as this PR, but the author declined.

Yes, a change is needed to support plugin paths, which has been noted with #15897, but this PR is not the solution. Discussion on the forum about a more thorough implementation is welcome, or submit another PR with a more thorough implementation.

[mediaminister]

I submitted a new PR with a fix that hopefully meets all Kodi team requirements. #17202