Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pvr] add PVR and EPG support to XBMC #1357

Merged
merged 2 commits into from Sep 4, 2012
Merged

[pvr] add PVR and EPG support to XBMC #1357

merged 2 commits into from Sep 4, 2012

Conversation

opdenkamp
Copy link
Member

add-ons are not included, but can be found here: https://github.com/opdenkamp/xbmc-pvr-addons

when the pvr manager is enabled without any pvr add-ons installed, then the following warning text will be displayed: "No PVR add-ons could be found. You need a tuner, backend software, and an add-on for the backend to be able to use PVR. Please visit xbmc.org/PVR to learn more."
this page needs to be created (just forward to the wiki page about pvr and update that one?)

when the pvrmanager is enabled, and there are add-ons found, but none is enabled, then the user will see a warning message that he needs to enable at least one add-on, and is taken to the add-ons directory with the disabled add-ons.

pvr related issues that i assigned to the frodo milestone (that i'd like to fix or see fixed before frodo final) can be found here: https://github.com/opdenkamp/xbmc/issues?milestone=3&state=open
i haven't checked and cleaned up trac tickets lately.

@Memphiz
Copy link
Member

Memphiz commented Sep 2, 2012

Great work Lars - will fixup the Xcode projects for iOS and atv2 tomorrow in the train :D

@opdenkamp
Copy link
Member Author

cheers! you got r/w access to the repos now

@sofakng
Copy link

sofakng commented Sep 3, 2012

Very exited about this!

@garbear
Copy link
Member

garbear commented Sep 3, 2012

It looks like we have unused variables at here and here

Would it be possible to split up the big commit so that lib/ additions and xbmc/pvr/ additions are in separate commits? I know it breaks compilation but it's too big for github to load the commit diff

edit, nevermind just realized I can do it myself

@ghost
Copy link

ghost commented Sep 3, 2012

there are d in the big commit. it needs explaion. just a gut reaction with just a phone

@opdenkamp
Copy link
Member Author

@cptspiff "there are d in the big commit. it needs explaion. just a gut reaction with just a phone"
sorry, "d"? i'll be happy to add more explanation, just not sure what you'd like to see explained :)

@opdenkamp
Copy link
Member Author

@garbear those left overs from earlier. have been removed again in a later commit. i'll push a squashed commit

</entry>

</epg>
</demo>

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@bennykill
Copy link

I dont't think that the raspberry is powerfull enough to navigate through the channel list while watching a channel! Tried it with raspbmc R4 connected to a tvheadend server witch ran on an ubuntu pc and got crashes, picture bugs and so on! Main problem is the CPU performance! :(

@opdenkamp
Copy link
Member Author

@bennykill you need to wait until both pvr and rpi support are in mainline and i can't help you with quirks in someone's pvr+rpi merge attempt

@Memphiz
Copy link
Member

Memphiz commented Sep 3, 2012

Beside that we are working here and this information was totally useless to us for this pull request.

@opdenkamp - with "D" spiff means the deleted files in that big commit.

@opdenkamp
Copy link
Member Author

@Memphiz @cptspiff right thanks, i'll add an explanation for those

@bennykill
Copy link

@Memphiz With this information I wanted to help by telling you about my experiences. Because implementing PVR support isnt necessary when the hardware isnt strong enough! ;) Someone who wants to use XBMC as a receiver also wants to use a channel list! ;)

@opdenkamp
Copy link
Member Author

@bennykill the information is useless for this PR, like memphiz already said. you need to wait. and every comment on this PR will send mails to 100+ people so please leave it at this for now, unless you have some valid comment about the code changes in this PR.

@wsoltys
Copy link

wsoltys commented Sep 3, 2012

compiles and works fine on windows (tested with the vnsi plugin and my vdr). I don't have nav sounds with directsound but other audio is fine (mp3, live tv, etc). Maybe @DDDamian could have a look at it. Otherwise its fine and I'm okay with the merge.

@wsoltys
Copy link

wsoltys commented Sep 3, 2012

Another thing I noticed is that the description for settings->server is the same like settings->live tv. I use German but since its English I assume it uses the default values.

@DDDamian
Copy link
Contributor

DDDamian commented Sep 3, 2012

@wsoltys - no TV tuner card here (thx to opdenkamp & others I now have an incentive) so not much help in testing. For nav sounds enable <streamsilence under <audio heading in as.xml so engine doesn't sleep. Since adding suspend / resume I'm adding some other functionality to restore nav sounds without re-opening sink from suspend mode with every GUI beep ;)

@wsoltys
Copy link

wsoltys commented Sep 3, 2012

you don't need the hw for testing the navsounds for this branch :)

@opdenkamp
Copy link
Member Author

@wsoltys thanks, I'll check that out. probably my mistake when merging that in earlier when that category was added

@DDDamian there's a demo add-on now that you can use that simulates a pvr backend. just plays back an internet stream, so it's not the same as playing back from hts or vdr, but that shouldn't matter for navsounds

@DDDamian
Copy link
Contributor

DDDamian commented Sep 3, 2012

Sorry tags lost to parser lol - tag is streamsilence under audio heading.

@opdenkamp - then test I shall! Congrats btw.

@ghost ghost assigned opdenkamp Sep 3, 2012
@wsoltys
Copy link

wsoltys commented Sep 3, 2012

The file win32/stdbool.h is added but I can't find where its used right now. Maybe a leftover?

@elupus
Copy link
Contributor

elupus commented Sep 3, 2012

Stdbool.h us good to have broken systems like Windows :-). so leave it.

@Fneufneu
Copy link
Member

Fneufneu commented Sep 3, 2012

build fine on FreeBSD

@dersphere
Copy link
Contributor

Not sure if this is a surprise but builds (and works so far) fine on ubuntu 12.04 ;-)

@wsoltys
Copy link

wsoltys commented Sep 3, 2012

@DDDamian current master is also missing the nav sounds with direct sound so not related to pvr.

@Montellese
Copy link
Member

Nice work on this. Congrats.
Would it be possible to take out the PVR-specific additions to JSON-RPC and do them in a seperate PR. I currently don't have the possibility to test them and I don't want to stall the whole PVR stuff from going in just because of things I'd like to do differently in the JSON-RPC API. But I also don't want to add a new set of methods and then change it again a week or two later.

If it's too much hassle to take it out and do a seperate PR afterwards I'll just fix it up later when I got time. AFAIK you didn't write that code yourself either so you won't be the right person to answer my questions on it.

@RobertMe
Copy link
Contributor

RobertMe commented Sep 3, 2012

@Montellese
Unsure about what you would like to do on the JSON side of things (only adding/extending the PVR API, or internally changing how JSON-RPC works?) But I'm already doing some work on the JSON API for PVR (current code isn't mine). @opdenkamp already told me he wouldn't like to include my work right now (in this PR). But I'm continuing development on it and already did a few things differently (most PVR methods which are in this PR should be part of Player IMHO, for one). One of the changes I already did was adding Player integration (opdenkamp#626) and currently I'm working on "read" support, so things like PVR.GetChannels etc.

}


};

This comment was marked as spam.

This comment was marked as spam.

@opdenkamp
Copy link
Member Author

@Montellese @RobertMe shouldn't be too hard to undo the json-rpc changes. i'll have a look at it and will update this PR

@@ -183,6 +184,7 @@ void CURL::Parse(const CStdString& strURL1)
if(strProtocol2.Equals("http")
|| strProtocol2.Equals("https")
|| strProtocol2.Equals("plugin")
|| m_strProtocol.Equals("addon")

This comment was marked as spam.

This comment was marked as spam.

@elupus
Copy link
Contributor

elupus commented Sep 3, 2012

Annoying github not showing diffs on files further down. Anyway:
Key.h changes look scary, it's changing old window id's.

There is clearly some stuff i still don't like in the code. Mainly in dvdplayer.cpp and the interface to the addons for providing data, but I only have myself for blame regarding that since i've not looked at it. But it's nothing blocking a merge.

Some sami subtitle support has snuck in that could be separate.

The key.h header stuff i think needs fixing. Feel like it could break stuff. It's a huge amount of code, but my first scan through the patch is good so I have nothing blocking a merge.

@opdenkamp
Copy link
Member Author

@Montellese json-rpc changes have been removed from the PR
@elupus moved the window ids in Key.h around a bit so we don't change existing ids

everything will be squashed up into 1 commit when done

@Montellese
Copy link
Member

@opdenkamp Thanks for the reverts. The project files (VS and XCode) will need updating (i.e. remove the previously added xbmc/interfaces/json-rpc/PVROperations.h/cpp from the project files) but that shouldn't be a big deal.

@opdenkamp
Copy link
Member Author

@Montellese indeed. will fix it

@opdenkamp
Copy link
Member Author

@cptspiff was checking where the deletions were coming from, and noticed that this slipped in: following files were deleted in the pvr branch when i was cleaning things up, which should have been done in mainline:
lib/libhts/Win32/include/stdint.h
lib/libhts/Win32/libhts_2003.sln
lib/libhts/Win32/libhts_2003.vcproj
lib/libhts/Win32/libhts_2008.sln
lib/libhts/Win32/libhts_2008.vcproj

since this is completely unrelated to this PR, i'll re-add the files here and put that in a separate PR (those not being used anymore)

@opdenkamp
Copy link
Member Author

@cptspiff re-added project files for libhts in 36825e5. saves a few d's ;-) scrolled through the patch quickly, and most other deletions are things like changed if statements, cases added to switches, etc that hook up the various pvr bits. i'll be happy to explain certain parts you'd like to see explained.

@Montellese removed the refs to the files from the project files. just a quick find and replace, not sure whether the apple stuff needs more than just this.

@ghost
Copy link

ghost commented Sep 3, 2012

thanks lars, it was the deleted files i wondered about. that some hunks remove code is less of a concern.

@opdenkamp
Copy link
Member Author

@elupus the samitagconvertor stuff was merged in from pvr-testing2 earlier, but doesn't seem to be hooked up to anything. it's not even hooked up in the makefiles/project files. so i'll just remove it from the PR.

@Memphiz
Copy link
Member

Memphiz commented Sep 4, 2012

Just to confirm. Your changes in removing the PVR JSON stuff from the xcode projects are fine.

@opdenkamp
Copy link
Member Author

@pieh included the two fixes from irc, thanks

let me know when everybody finished this review, and i'll rebase & squash

@elupus
Copy link
Contributor

elupus commented Sep 4, 2012

When you rebase, would be nice if non english translations where added as a separate commit. (they are not a core requirement)

@alanwww1
Copy link
Member

alanwww1 commented Sep 4, 2012

@elupus , @opdenkamp

It is not a problem for me to handle the translations. This is why a held back my bulk language file update PR, so that it wouldn't interfere with this PR. If this is merged, I can update Transifex with the new English strings and also the non-English translations as well. Translation groups can than review, or change the as they decide.

So from my side, they can stay in, if there is no other reason to separate.

@opdenkamp
Copy link
Member Author

@alanwww1 great, thanks for holding back your PR. i can just push in the english strings, or push them all. whichever you prefer. it's an easy change for me

unless someone wants to review the latest changes separately, i'll squash&rebase now. the removals will make github show some more changes without complaining that the pr is too big ;-)

@alanwww1
Copy link
Member

alanwww1 commented Sep 4, 2012

@opdenkamp For me, it would be better to have them all in, so I can merge back the new translations right after it, if that's ok for everyone.

@elupus
Copy link
Contributor

elupus commented Sep 4, 2012

I never meant leave the translations out. I just meant as a separate commit. This is one huge commit as it is. If it can easily be split that makes it more readable.

@opdenkamp
Copy link
Member Author

@elupus done

opdenkamp pushed a commit that referenced this pull request Sep 4, 2012
[pvr] add PVR and EPG support to XBMC
@opdenkamp opdenkamp merged commit d9d303f into xbmc:master Sep 4, 2012
@jmarshallnz
Copy link
Contributor

I haven't reviewed, but if others are happy with it I'm happy to review the
guilib changes after the fact and fix up anything I don't like in master.
On Sep 5, 2012 12:18 AM, "Lars Op den Kamp" notifications@github.com
wrote:

@elupus https://github.com/elupus done


Reply to this email directly or view it on GitHubhttps://github.com//pull/1357#issuecomment-8281126.

@ghost
Copy link

ghost commented Sep 5, 2012

On Wed, Sep 5, 2012 at 8:51 PM, jmarshallnz notifications@github.comwrote:

I haven't reviewed, but if others are happy with it I'm happy to review
the
guilib changes after the fact and fix up anything I don't like in master.

good. we merged it 24h ago ;)

@jmarshallnz
Copy link
Contributor

Heh, interwebs in Spain haven't caught up.
On Sep 5, 2012 8:56 PM, "Arne Morten Kvarving" notifications@github.com
wrote:

On Wed, Sep 5, 2012 at 8:51 PM, jmarshallnz <notifications@github.com

wrote:

I haven't reviewed, but if others are happy with it I'm happy to review
the
guilib changes after the fact and fix up anything I don't like in master.

good. we merged it 24h ago ;)


Reply to this email directly or view it on GitHubhttps://github.com//pull/1357#issuecomment-8309887.

@opdenkamp
Copy link
Member Author

@jmarshallnz i'm pretty sure there will be plenty to review and fix up when you get back ;-) have a nice trip :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC PR submitted for gathering feedback Type: Feature non-breaking change which adds functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet