Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Resubmit pvr.wmc addon to official pvr addons repo (squashed) #247

Merged
merged 4 commits into from

5 participants

@ryangribble

Introduction

pvr.wmc is an XBMC PVR Addon that talks to a custom backend service, that ultimately utilises the Microsoft WMC engines for tuner hardware, recording, EPG and so on.

As per this XBMC forum post, the addon was granted it's own subforum on the condition it was submitted to the official repostory. This PR is the official submission. We have tested xbmc_pvr_addons builds on Windows, Linux and OSX platforms which are all compiling OK with the new addition. We just released version 0.1.92 (Frodo) and 0.2.92 (Gotham) of this addon, from the github repo source in the krustyreturns fork of xbmc_pvr_addons. This PR contains all code for the recently released Gotham 0.2.92 build of this addon.

Below is more detailed information on the addon. Please reply if you require any changes to be made on our side, in order to be incorporated into the official repo

Forum/Wiki Pages

Forum: http://forum.xbmc.org/forumdisplay.php?fid=205
Wiki: http://wiki.xbmc.org/index.php?title=Add-on:PVR.WMC

Clients Supported

Windows, Linux, OSX and Raspberry Pi

Addon Architecture

Backend service running on windows, hooking into Microsoft WMC recording and EPG services
LiveTV is remuxing from inprogress WMC live recording into TS files which XBMC plays
Recordings are WMC WTV format files that are played by XBMC

Live TV Functionality

Viewing channels live, including "timeshift buffer" and pause/seek support
Multi client aware sharing time shift buffer on the same channel (2nd client can rewind to when first client was watching channel if desired)
Signal Strength/Status information provided to XBMC
Importing channel icons configured in WMC

Recordings Functionality

Instant record, including timeshift data from the moment the user started watching the channel
Scheduled recordings, including Series recording options via a custom SBMC gui popup (confluence plus several skins now have these dialogs)
Extensive customisation for pre/post padding and conflict resolution options
Delete/rename recordings
Watching in progress recordings
Watching completed recordings (including a setting to help raspberry pi's that cant play large WTV files)
Recordings displayed in stacked/heirarchical view in XBMC
Multi client aware - resume status shared between multiple clients

EPG Functionality

Using WMC EPG service means EPG in many countries/technology is supported, from internet sources or from OTA broadcast scans etc

Hardware Support

Using WMC recording service means all tuners/tv technologies supported by Microsoft are able to be supported by this addon

Future Plans

Support for favourites and channel groups being imported from WMC
Implement cleaner instant record that starts from last show boundary rather than beginning of timeshift buffer
Implement rolling timeshift window rather than ever growing
Auto resume/restart live TV if failures occur
Android development (if we can get/gain required skill in this area)

Not Supported

Subtitles on live TV or in progress recordings. (subtitles on completed recordings are present, but an XBMC issue with subtitles in PVR section currently preventing those from working too)

@opdenkamp
Owner

you need to rebase and force push. this PR contains a merge commit.

@ryangribble

Lars, ive rebased and forced pushed, there is now only 1 commit. But it does still contain the other files that came from the merge commit. The problem seems to be that I pulled the latest master from your repo into ours, then merged that into our devel branch (thinking it was a good thing to update our codebase with the latest from your repo, prior to pushing our changes back). So even after rebasing to master and ensuring only 1 commit, I cant figure out how to only get my PR to include just the changes we made (pvr.wmc files, project/make files) and not these other files from the other projects (which are already in your repo anyway). Or is this how it's meant to work (changes pulled from your repo are included in our push back, eventhough they are already there)?

@da-anda

no, other changes should not be part of your commit. Seems you squashed them into your commit while rebasing or chose the wrong base for your rebase (add opdenkamp/xbmc-pvr-addons to your sources and use this for rebase, or fastforward your local/git master). You should never just pull from a foreign repo into your branch as it will mess things up if you don't know what you do. Git can be a real bitch in the beginning :) If you are on freenode, feel free to ping me in #xbmc channel for help.

@ryangribble

OK I think I have it all fixed up! I am bringing 15 years of centralised version control baggage along (perforce mainly) so please forgive my git indiscretions :)

The push should now include only the pvr.wmc addon changes, and also those strings.po files for unimplemented languages have been removed.

Thanks for the help, please let me know what else I need to do!

@ryangribble

@opdenkamp @da-anda is there anything else we need to do to this PR to get it in for the merge window? Please let me know!

@opdenkamp
Owner

the changes to things that are not in your new add-on, like the changes to the project file of the platform lib, must split up into separate commits.

could you please either remove the commented out code or document why it's been commented out. you've got whole methods commented out. that doesn't belong in this repos really.

then for the socket code, i strongly recommend you to use the socket code from the platform lib. it's being used by multiple add-ons on each platform we support, and being maintained for those platforms. i won't block this PR because of that now, but I urge you to look into replacing that code in your next version

@ryangribble

OK I will split out those platform files into separate commits.

Regarding the socket code, we will look at that in the next version but thanks for not blocking the PR over that, as obviously it will require re-testing

@ryangribble

@opdenkamp - the PR now has 1 commit for the pvr.wmc code, and separate commits for changes made to common pvr_addons (a gitignore change, adding pvr.wmc to make/build solution, and that platform.vcxproj change).

We will look at the socket stuff for the next release.

Regarding the comment about commented out code, I scanned through the pvr.wmc source files and couldnt see any excessive blocks of commented code. There are a couple of commented lines that could be removed but nothing excessive, unless you can point out an example that is serious enough to warrant doing all the commit squashing again! Would be happier to address in the next release along with the socket stuff?

Please let me know if we are good now!

@opdenkamp
Owner

re commented out code: https://github.com/krustyreturns/xbmc-pvr-addons/blob/a5e9534b0d93c706ea701967c50d5b4f43a30c28/addons/pvr.wmc/src/webclient.cpp is an example

i'd also like to ask you to remove things like this from the next version and use branches instead: https://github.com/opdenkamp/xbmc-pvr-addons/pull/247/files#diff-d53cb805bb3fa07dbde98d4ba305106eR25

same goes for things like this: https://github.com/opdenkamp/xbmc-pvr-addons/pull/247/files#diff-4e06f1cbdc76f0168fc7746c2d92d3c4R420 which results in different behaviour on windows based on ifdefs spread out through the code.

@ryangribble

1) It appears that webclient.cpp and .h aren't actually part of the project! I had already tidied up commented code and looked again today after your remarks, but I was doing it via Visual Studio so didnt see those files. They will be removed from the project. These files are from before my time!

2) Regarding the #define GOTHAM stuff, yes we can remove them and use the branches, this was just to allow less git experienced users to have an easier time developing on the project.

3) Im not sure about the ifdef TARGET WINDOWS. Will need to refer to other devs.

@ryangribble

Ive updated the PR with corrections to 1 and 2, but I cant change 3 without talking to original devs and probably re-testing. It appears to be related to the socket code at least in some instances anyway, so I propose to leave the ifdef TARGET WINDOWS stuff until the next release along with the sockets. Hopefully you are OK with that, given that Ive removed the GOTHAM #define and removed the unused class that had commented out blocks in it

@opdenkamp
Owner

let's see what jenkins thinks of it

@opdenkamp opdenkamp merged commit a32a0bd into opdenkamp:master
@stefansaraev

on clean install. unlike other pvr clients - this is enabled by default and can not be disabled. with some luck you can try disable disable disable and then restart xbmc to get it disabled.. IMO ALL pvr clients are supposed to be disabled by default. no excceptions.

EDIT: ref OpenELEC/OpenELEC.tv#2848

@stefansaraev

please ignore the above comment. just re-tested - it happens on xbmc-pvr-addons update when a new pvr client addon is introduced (probably with ALL new pvr clients and related to Addon*.db). and is NOT specific to this addon.

here is a log for @opdenkamp http://sprunge.us/MSgG

to reproduce:

  • disable live tv.
  • move pvr.wmc away from /usr/share/xbmc/addons.
  • delete userdata/Database/Addons16.db.
  • wait a bit as xbmc does not handle SIGTERM. restart xbmc. wait a bit.. restart xbmc agian to be sure :)
  • enable live tv.
  • enable at least one pvr client (in my case vnsi).
  • move pvr.wmc to /usr/share/xbmc/addons
  • wait a bit as xbmc does not handle SIGTERM. restart xbmc
  • try disabling pvr.wmc via xbmc's addonmanager.
@opdenkamp
Owner

please report this bug on http://issues.xbmc.org/
thanks

@stefansaraev stefansaraev referenced this pull request from a commit in OpenELEC/OpenELEC.tv
@stefansaraev stefansaraev xbmc-pvr-addons: update to xbmc-pvr-addons-9021115 e559d7d
@opdenkamp

warning: format not a string literal and no format arguments [-Wformat-security]

@scarecrow420 these should be written like this:

snprintf(signalStatus.strAdapterName, sizeof(signalStatus.strAdapterName), "%s", results[0].c_str());

Thanks, will fix in next release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.