comskip on remote server

[bilkusg]

I believe it's already well-known that mediaportal and comskip only work with kodi if the kodi instance is running on the same machine as mediaportal itself. I've found a reasonably simple code fix for this which works remotely with android/linux/windows ( and probably others but I can't test them ) in the common use-case where the remote access uses smb.

I'm using this myself at home, and would be happy to contribute my changes, but before I do the necesssary work I am looking for a sanity check as to whether this change is of sufficient interest, and how to synchronise the change here with the corresponding changes on the kodi side. ( My fix is safe in the sense that changed kodi still works with vanilla pvr.mediaportal.tvserver and vice versa, but obviously both need updates for anything good to happen ).

Any advice welcome.

[phunkyfish]

The addons operate under different rules than kodi core does. Their contract is the API between them. Do you need to change the API. If so, then an addon specific change to the API would not be allowed.

Can you give a sense what the changes entail?

[bilkusg]

Essentially, in GetRecordingStreamProperties I've added:

const char* blixFilePath = myrecording->FilePath(); const string &pvrSmbPath = m_tsreader->TranslatePath(blixFilePath); properties.emplace_back("smb_filepath", pvrSmbPath.c_str());

which adds an extra property to what's passed back to kodi, which kodi is free to ignore or use as it wishes.

My change to kodi simply looks for that property, adds it to the kodi file object, and uses that as the base for searching for edl files.

Whether that counts as 'changing the contract' is a matter of interpretation but as far as I can tell it's a backward compatible change in both directions since my kodi code change does nothing new if the property is not found, and the mediaportal addin doesn't care if it's not used.

There are a few other minor changes like making TranslatePath a public method, but those are internal to the addon and don't affect the contract in any way

[phunkyfish]

The addon has an API for EDL files already. Why not just use that?

See GetRecordingEdl()

[bilkusg]

If it's the right thing to do I can look at doing that instead, especially if it removes the need to change Kodi itself. to get this mainstream. But I think that means I need to duplicate a lot of the code in VideoPlayer/Edl.cpp in Kodi into the addin so I can find and parse the files myself, whereas my current approach allows me to get Kodi to do all the hard work, and presumably track any future Edl changes. I can do that, but it will take a bit of time..... Have I missed something?

[phunkyfish]

If it's the right thing to do I can look at doing that instead, especially if it removes the need to change Kodi itself. to get this mainstream. But I think that means I need to duplicate a lot of the code in VideoPlayer/Edl.cpp in Kodi into the addin so I can find and parse the files myself, whereas my current approach allows me to get Kodi to do all the hard work, and presumably track any future Edl changes. I can do that, but it will take a bit of time..... Have I missed something?

Just have a look at the recordings class in pvr.vuplus. You can use the code from there.

The kodi way would not be accepted as the API already provides a way to do this cleanly. In your case and that of pvr.vuplus it just uses standards EDL files (I think). But many backends use unusual customisations of these files hence the expressiveness available in the addon API.

[bilkusg]

Thanks for the pointer. I'll take a look as soon as I have time. Presumably if I can get this working with standard kodi, I should do a fork and pull request in due course?

By the way, is there a clever way of compiling the addin for android from my modified source code cleanly. The only way I've done it so far is to fork the kodi-addons repo and point it at my own repo, since every time I make a change and recompile the addin it redownloads the 'offficial' source. I've tried and failed to use the ADDON_SRC_PREFIX or ADDONS_DEFINITION_DIR referred to in the cmake/addons README in kodi, but I'm not entirely sure what to set them to or at what point in the build instructions.

Thanks

[phunkyfish]

I would fork the addon first and then clone and work off that. Then once you upload the branch to your fork it’s easy to create a PR.

I’m afraid I don’t know much about cross compiling for android. The three Dev platforms are Mac/windows and Linux.

You could build on a Dev platform and create a wip PR. Then Jenkins will build the android binary for you.

[bilkusg]

OK Thanks. I'll give it a go and see what happens.

[bilkusg]

Hmm, I'm trying a fresh build before I start work, and the latest version of the repo won't compile on Windows using VS19 - I get

C:\dev\progsrc\kodiwin\kodi\cmake\addons\build\pvr.mediaportal.tvserver\src\windows\FileUtils.cpp(25): error C2039: 'make_unique': is not a member of 'std

I'm following the standard instructions to build kodi plugin on windows which worked before yesterday......

[phunkyfish]

That seems like it’s not using the newer c++ standard. Did you follow all the setup instructions for windows for kodi first?

@emveepee you use windows right? Maybe you can provide some guidance on how to build a pvr addon on windows? I would think that VS2019 should be ok.

[bilkusg]

Well it certainly thinks it's using the newer standard! I've tried both compiling directly inside the kodi tree using the addons build as described in the windows readme, and using the kodi-addons.sln which cmake creates in a standalone copy of the repo and neither works. So a bit stuck. I could I suppose go back to the master release rather than the Matrix one and work on that in the meantime, but I assume I need to get this working against the Matrix branch before you can accept it.

[emveepee]

That seems like it’s not using the newer c++ standard. Did you follow all the setup instructions for windows for kodi first?

@emveepee you use windows right? Maybe you can provide some guidance on how to build a pvr addon on windows? I would think that VS2019 should be ok.

Windows determines which standard to compile for based on the code. I used VS2019 for pvr.nextpvr windows builds and cmake did eveything for me based on step 6 in https://github.com/xbmc/xbmc/blob/master/docs/README.Windows.md Matrix and master will be different 20N is going to VS2019, If you want want a Matrix build on 64bit VS2019 instead of 2017 I would use cmake -G "Visual Studio 16 2019" -Tv141 %userprofile%\kodi

[phunkyfish]

Well it certainly thinks it's using the newer standard! I've tried both compiling directly inside the kodi tree using the addons build as described in the windows readme, and using the kodi-addons.sln which cmake creates in a standalone copy of the repo and neither works. So a bit stuck. I could I suppose go back to the master release rather than the Matrix one and work on that in the meantime, but I assume I need to get this working against the Matrix branch before you can accept it.

Actually working against master is fine right now as we have not branched binary addons for kodi N yet. So if that works it's fine once you don't need any changes in kodi. Otherwise hopefully @emveepee's suggestion helps.

[bilkusg]

Hmm. I've now established that if I explicitly checkout tag 4ac67a3e8985c129a1b8f9b85d42a7c6a88cb4e3 which is the one which the kodi repo used by default until a few days ago, then I can at least now compile again using the standard instructions. But the default for the kodi auto build of addons uses Matrix latest, and that just doesn't work for me. So I can try making progress with my original issue thanks, but you may want to look into the other issue, as anyone else coming to Kodi builds on windows will have problems following the default instructions. Happy to provide more info if you want, but I know next to nothing about cmake so I don't know what might be useful.

[phunkyfish]

That's the minimum commit required as it's from the same API version for Matrix. Note that you may run into a conflict when you PR your fix it as the dependency on P8-platform was removed in the next release. But let's cross that bridge when we come to it. I reckon you'll be ok.

[emveepee]

I just tried with the raw instructions and it failed with many errors, including make_ptr, and then added the -Tv141 parameter from cmake I suggested (to generate VS2017 code) and it worked

6>  CPack: -   Install component: pvr.mediaportal.tvserver-8.1.2-windows-x86_64
6>  CPack: Create package
6>  CPack: - package: C:/Users/Martin/AppData/Local/Temp/addon-pvr.mediaportal.tvserver-8.1.2-windows-x86_64.zip generated.
6>  Building Custom Rule D:/v5/builds/workspace/pvr.mediaportal.tvserver/CMakeLists.txt
6>Building Custom Rule D:/v5/builds/workspace/xbmc/cmake/addons/CMakeLists.txt
8>------ Skipped Build: Project: package-addons, Configuration: Debug x64 ------
8>Project not selected to build for this solution configuration 
========== Build: 4 succeeded, 0 failed, 0 up-to-date, 4 skipped ==========

[emveepee]

I had another go trying to get it to work on VS2019 directly and I had to update FileUtils.cpp with

#include <memory> 

[bilkusg]

Thanks. So just to avoid any confusion. If I add in the 'memory.h' reference to FileUtils and then compile with VS 2019 without the -Tv141 flag it should now work? (If I try to use -Tv141 I get told I need to install a whole load of VS2017 stuff alongside VS22019 which I consider a last resort)

More importantly, when I'm ready to contribute back, do I create a new branch in my fork or do I work in the Matrix branch and add my commits to that branch?

And from a substantive point of view, is it OK in the code to make the TranslatePath method in TSReader.h public instead of private, so I can call it from GetRecordingEdl, since as far as I can tell its the only valid way to get the smb pathname from the recording object. If you would prefer TranslatePath stays private, what's your preferred coding approach to get round this - add a new public method to TSReader instead?

Thanks

[phunkyfish]

Thanks. So just to avoid any confusion. If I add in the 'memory.h' reference to FileUtils and then compile with VS 2019 without the -Tv141 flag it should now work? (If I try to use -Tv141 I get told I need to install a whole load of VS2017 stuff alongside VS22019 which I consider a last resort)

You should use the latest version from this repo. The memory include has now been added.

More importantly, when I'm ready to contribute back, do I create a new branch in my fork or do I work in the Matrix branch and add my commits to that branch?

You should always be working from your fork. So update you fork to the latest version from this repo and then create your PR fork from that.

For example here is how I setup my pvr addon repos. First step is setup:

Clone your fork and map only the Matrix branch to the upstream Matrix branch git clone https://github.com/phunkfish/pvr.mediaportal.tvserver git remote add upstream https://github.com/kodi-pvr/pvr.mediaportal.tvserver git fetch upstream git branch -u upstream/Matrix Matrix

Create a new dev branch but update to latest upstream first git checkout Matrix git pull git checkout -b my-dev-branch

Rebase dev branch to latest version from upstream git checkout Matrix git pull git checkout my-dev-branch git rebase Matrix

Push dev branch to your fork git push origin my-dev-branch

Force push dev branch to your fork git push origin -ff my-dev-branch

And from a substantive point of view, is it OK in the code to make the TranslatePath method in TSReader.h public instead of private, so I can call it from GetRecordingEdl, since as far as I can tell its the only valid way to get the smb pathname from the recording object. If you would prefer TranslatePath stays private, what's your preferred coding approach to get round this - add a new public method to TSReader instead?

Thanks

It's a library so you shouldn't but I don't see any other way.

[emveepee]

Right now I still recommend -Tv141 because Matrix is using VS2017 and if and when you submit the PR the compiler acts differently (as we just saw) I found cases where I had clean compiles that failed or generated unexpected warning in CI.

[phunkyfish]

Yes, fair and agreed. Another issue is that media portal uses long file names and one of the build slaves is not configure correctly. Even though the registry change was made on it there is some other reason why it’s not working like the others.

[bilkusg]

OK. Many thanks for these pointers. I will try to get this working and tested on Windows, Linux and Android and submit a pull request when I'm done

[bilkusg]

I've done the pull request and everything has succeeded except for one error which I think is the build slave @phunkyfish mentioned above. Please advise if there's anything more I need to do.

[bilkusg]

Thanks for the rapid code review. I assume that I incorporate these changes and issue a new pull request, or is there something cleverer I'm meant to do?

[phunkyfish]

Thanks for the rapid code review. I assume that I incorporate these changes and issue a new pull request, or is there something cleverer I'm meant to do?

No, instead just make your changes. Then

git add -u git commit —amend

Then force push it with:

git push origin -ff your-branch-name

[bilkusg]

OK. By the way, is there a 'best place' to put a new function to read the file into a string. My choices seem to be:

• just do it inside the current function
• add a new method in pvrclient-mediaportal.cpp
• add a new method in tsreader which takes the pvr filename and returns the contents of the edl file if any as a string. I think that is probably the best way to do it as it then allows TranslatePath to remain private, but I don't know if there's an even better alternative?

[phunkyfish]

You can put it in an empty namespace above the function you want to use it in. For C++ this lets you define a function that is only visible in the current compilation unit. Like this:

namespace
{

bool myFunc()
{
  return true;
}

}

[phunkyfish]

I've done the pull request and everything has succeeded except for one error which I think is the build slave @phunkyfish mentioned above. Please advise if there's anything more I need to do.

Don't mind the build error. Once one of the windows builds passes on Jenkins it should be ok.

[bilkusg]

OK. If I use the 'empty namespace' approach, I still need TranslatePath to be public. Is that OK?

[phunkyfish]

OK. If I use the 'empty namespace' approach, I still need TranslatePath to be public. Is that OK?

I don't see why not. Strange that there is not a better way to get the path but it appears to be Kodi specific so should be fine.

[bilkusg]

OK I've made what I hope are the required changes and pushed them up

[bilkusg]

I've requested re-review. Do I need to do anything else?

[phunkyfish]

That’s it. I’ll take a look in a while.

[bilkusg]

All done and now live. Thanks.