Add support for streaming profiles #119

Jalle19 · 2015-10-18T10:40:01Z

@ksooo for review. I've split the feature into three commits for easier review, we can squash it before merging if you want to.

I'm thinking whether we should add another boolean option to disable predictive tuning if a streaming profile is set. Having multiple transcoded subscriptions can easily overwhelm a low-powered server.

Since this touches settings I'd like to get #116 merged before this one. #79 would be nice to get in before as well since it refactors Subscription into a class (if the feature is not ready we could opt for just merging that particular part).

@TheTroll can you test this?

ksooo · 2015-10-18T15:48:31Z

Yeah, looks good. Obviously the new setting heavily interferes with #116...

ghost · 2015-10-18T17:35:53Z

Hi,

I ll take a look thx!

Jalle19 · 2015-10-18T18:49:48Z

@ksooo yeah I'll rebase once it's in.
@TheTroll thanks

Jalle19 · 2015-10-20T07:41:50Z

@TheTroll can you test?

ghost · 2015-10-20T07:45:04Z

Hi, I will before the end of the week.
Sorry for the delay!

Jalle19 · 2015-10-20T07:54:02Z

No problem, I'll test again when I get home (just checking that the rebase went well).

@ksooo @Glenn-1990 we should refactor SendSubscribe so that there are two separate methods for "restart" and "normal subscribe". That way we don't have to make all the parameters optional and it improves readability a bit. Currently you have to use e.g. GetProfile() inside the method instead of just using the parameter directly which may be a bit confusing and prone to errors.

Glenn-1990 · 2015-10-20T14:14:46Z

src/tvheadend/Subscription.h

+    void SendSubscribe(uint32_t channelId = 0,
+                       uint32_t weight = SUBSCRIPTION_WEIGHT_NORMAL,
+                       const std::string &profile = "",
+                       bool restart = false);



Is it possible to change the profile on run-time?
Otherwise we could just pass the profile with the constructor, that will make all this profile passing unnecessary?

Technically it should be but currently no since an addon restart is triggered if you change the setting. I'm perfectly fine with requiring a restart and feeding the desired profile through the constructor instead, anything to keep the parameter count on this method down is good in by book. @ksooo thoughts?

Jalle19 · 2015-10-20T14:15:16Z

@ksooo I had to move the logic that queries for available profiles to CTvheadend::SyncCompleted(). The initial sync is done on a separate thread than client.cpp which meant that when connecting to the server over WAN (which is slower) the getProfiles call timed out because tvheadend was busy pushing all the initial sync data over the wire. Now that I moved the logic the issue disappears, plus we can keep the streaming profile related methods private in CTvheadend.

Will do some final runtime testing when I get home, then I'll merge and update the changelog etc.

Glenn-1990 · 2015-10-20T14:19:49Z

src/Tvheadend.cpp

+  {
+    XBMC->QueueNotification(
+        QUEUE_ERROR,
+        XBMC->GetLocalizedString(30502), streamingProfile.c_str());


This error is shown when I didn't set any streaming profile?

Haha could be that I didn't test that scenario yet :-D

Glenn-1990 · 2015-10-20T14:43:00Z

It's probably a silly question, but why do wee need this, we can set the desired streaming profile in the tvheadend config right?

sorano · 2015-10-20T14:47:47Z

I'm thinking that we can provide different profiles for end-users, and they can choose the profile that fits their current scenario?

For example, if the user is in a place with low bandwidth, he can select a profile with heavy transcoding to save bandwidth.

Jalle19 · 2015-10-20T15:47:21Z

@Glenn-1990 see #40, there are a few reasonable use cases for this (especially the one about recordings)

Jalle19 · 2015-10-21T20:35:57Z

@Glenn-1990 you mind giving this a final spin (mainly to check that untouched configurations still work)? @ksooo I squashed the latest fixups, mind doing a final review before merging?

ksooo · 2015-10-21T22:22:21Z

pvr.hts/resources/settings.xml

@@ -15,6 +15,9 @@
    <setting id="autorec_approxtime" type="enum" label="30051" lvalues="30052|30053" default="0"/>
    <setting id="autorec_maxdiff" enable="eq(-1,1)"  type="slider" option="int" range="0,5,120" label="30054" default="15" />

+    <setting label="30500" type="lsep"/>
+    <setting id="streaming_profile" type="text" label="30501" default="" />
+


I have the feeling that our "Advanced settings" page becomes to big. Is it just me or should we do a settings pages redesign soon?

@Glenn-1990 already refactored it, but yeah, we may need an additional tab. I'd leave that to a separate PR though.

Sure, nothing for this PR. Maybe you can file an issue for that so we don't forget.

Glenn-1990 · 2015-10-22T11:24:51Z

@Jalle19 working now with default config ;-)

Jalle19 · 2015-10-22T11:29:41Z

Good, I'll just have to plug the leaks then and we should be good to go.

from the server

display an error notification during startup if the specified profile name is not valid

(closes kodi-pvr#40)

Add support for streaming profiles

ghost · 2015-11-02T15:37:38Z

Hi,

I have moved to 15.2 and pvr.hts master, is it the right combination to test the new feature ?
Where can I select the profile ?

Thanks

Jalle19 · 2015-11-03T10:21:56Z

You can't use pvr.hts master with 15.2, if you want to test this you should grab a nightly build.

ghost · 2015-11-03T10:23:13Z

So master is meant to be for kodi master ?

Jalle19 · 2015-11-03T10:23:29Z

Yes, exactly.

ghost · 2015-11-03T10:24:30Z

Let me check again :)

ghost · 2015-11-03T10:26:58Z

Ok my bad, I was using the the isengard branch!
Let me test with the right branch

ghost · 2015-11-03T10:52:15Z

Now it works thanks :)
Why not adding this settings in "client specific" instead of the addon's settings ?

Jalle19 · 2015-11-03T13:56:46Z

Because none of us have any idea how to use that and users are less likely to find it.

ghost · 2015-11-03T13:57:46Z

Ok :)
Last question, I see that the profile is entered manually. Any way to retrieve it and let the user choose from a list ?

Jalle19 · 2015-11-03T13:58:24Z

Nope.

Jalle19 added the Feature label Oct 18, 2015

Glenn-1990 mentioned this pull request Oct 19, 2015

Implement subscription conflict handling #79

Closed

Jalle19 force-pushed the selectable-profile branch from ea4f625 to e268f7b Compare October 20, 2015 07:41

Jalle19 force-pushed the selectable-profile branch from a7d3ff3 to 36973b4 Compare October 20, 2015 14:12

Glenn-1990 reviewed Oct 20, 2015
View reviewed changes

Jalle19 force-pushed the selectable-profile branch from 41b586e to 3b7782b Compare October 21, 2015 20:35

ksooo reviewed Oct 21, 2015
View reviewed changes

Glenn-1990 mentioned this pull request Oct 24, 2015

Settings rework v2 #126

Closed

Sam Stenvall added 3 commits October 26, 2015 11:18

add support for retrieving the list of available stremaing profiles

313b95a

from the server

add a setting to select which streaming profile should be used, and

e30cbdc

display an error notification during startup if the specified profile name is not valid

pass the streaming profile to use to the demuxer so it gets used

a850428

(closes kodi-pvr#40)

Jalle19 force-pushed the selectable-profile branch from 3b7782b to a850428 Compare October 26, 2015 09:18

Jalle19 added a commit that referenced this pull request Oct 26, 2015

Merge pull request #119 from Jalle19/selectable-profile

e14678a

Add support for streaming profiles

Jalle19 merged commit e14678a into kodi-pvr:master Oct 26, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for streaming profiles #119

Add support for streaming profiles #119

Jalle19 commented Oct 18, 2015

ksooo commented Oct 18, 2015

ghost commented Oct 18, 2015

Jalle19 commented Oct 18, 2015

Jalle19 commented Oct 20, 2015

ghost commented Oct 20, 2015

Jalle19 commented Oct 20, 2015

Glenn-1990 Oct 20, 2015

Jalle19 Oct 20, 2015

Jalle19 commented Oct 20, 2015

Glenn-1990 Oct 20, 2015

Jalle19 Oct 20, 2015

Glenn-1990 commented Oct 20, 2015

sorano commented Oct 20, 2015

Jalle19 commented Oct 20, 2015

Jalle19 commented Oct 21, 2015

ksooo Oct 21, 2015

Jalle19 Oct 22, 2015

ksooo Oct 22, 2015

Glenn-1990 commented Oct 22, 2015

Jalle19 commented Oct 22, 2015

ghost commented Nov 2, 2015

Jalle19 commented Nov 3, 2015

ghost commented Nov 3, 2015

Jalle19 commented Nov 3, 2015

ghost commented Nov 3, 2015

ghost commented Nov 3, 2015

ghost commented Nov 3, 2015

Jalle19 commented Nov 3, 2015

ghost commented Nov 3, 2015

Jalle19 commented Nov 3, 2015

Add support for streaming profiles #119

Add support for streaming profiles #119

Conversation

Jalle19 commented Oct 18, 2015

ksooo commented Oct 18, 2015

ghost commented Oct 18, 2015

Jalle19 commented Oct 18, 2015

Jalle19 commented Oct 20, 2015

ghost commented Oct 20, 2015

Jalle19 commented Oct 20, 2015

Glenn-1990 Oct 20, 2015

Choose a reason for hiding this comment

Jalle19 Oct 20, 2015

Choose a reason for hiding this comment

Jalle19 commented Oct 20, 2015

Glenn-1990 Oct 20, 2015

Choose a reason for hiding this comment

Jalle19 Oct 20, 2015

Choose a reason for hiding this comment

Glenn-1990 commented Oct 20, 2015

sorano commented Oct 20, 2015

Jalle19 commented Oct 20, 2015

Jalle19 commented Oct 21, 2015

ksooo Oct 21, 2015

Choose a reason for hiding this comment

Jalle19 Oct 22, 2015

Choose a reason for hiding this comment

ksooo Oct 22, 2015

Choose a reason for hiding this comment

Glenn-1990 commented Oct 22, 2015

Jalle19 commented Oct 22, 2015

ghost commented Nov 2, 2015

Jalle19 commented Nov 3, 2015

ghost commented Nov 3, 2015

Jalle19 commented Nov 3, 2015

ghost commented Nov 3, 2015

ghost commented Nov 3, 2015

ghost commented Nov 3, 2015

Jalle19 commented Nov 3, 2015

ghost commented Nov 3, 2015

Jalle19 commented Nov 3, 2015