Implement subscription conflict handling #79

Glenn-1990 · 2015-07-26T21:32:46Z

ksooo · 2015-07-26T21:41:49Z

src/HTSPDemuxer.cpp

+        m_subscription.state = SUBSCRIPTION_SCRAMBLED;
+      else if (!strcmp("userLimit", error))
+        m_subscription.state = SUBSCRIPTION_USERLIMIT;
+      else if (!strcmp("noFreeAdapter", error))


Didn't we have some time ago a discussion regarding parsing error strings? My mind has not changed since then. Is it guaranteed that these strings are part of tvheadend's API and that nobody will at any time change these strings or that at one day localized strings will appear here?

Yes, but that was the 'status' message, which was a regular string.
Now we use 'subscriptionerror' which represents SM_CODE from tvh in string format, they are not supposed to change at some time in the future, at least that's not why I implemented that.

https://github.com/tvheadend/tvheadend/blob/master/src/htsp_server.c#L3630

Hmm. Another piece of code, same story, same design flaw, if you ask me.

Why do you not just return SM_CODE in the tvh API? This would be a much better approach, IMO. What you do here is convert the stuff you translated in tvh from numerical values into strings back into numerical values (maybe even the same numerically as used in tvh - not checked).

Anyhow, it is like it is now as the actual flaw is in tvh. Maybe you take into account what I said (if you can follow my arguments at all) when you add stuff to the tvh API next time.

Well, my first intention was to just sent SM_CODE as integer, but then it came to my mind that the SM_CODE values are just simple defines in tvheadend.h, so they might actually change somewhere in the future. That why I actually added those strings, these strings are not meant to be readable, but only parseble (It even are no sentences).

Only conversion is from int to string on the tvh side.

Seems that I still need this to the hstp specs wiki

I have write access to the tvheadend htsp spec in the wiki. Send me the text and I will update the spec. Then (and only then), the strings are really part of the API and I'm fine with this.

Well, my first intention was to just sent SM_CODE as integer, but then it came to my mind that the SM_CODE values are just simple defines in tvheadend.h, so they might actually change somewhere in the future

This is not a matter of data type, it's a matter of putting stuff into the htsp spec. If it's in the spec, it's bug to change it. If it is not in the spec, clients cannot rely on anything, strictly speaking. Again, no matter of data type. Using a string instead of a numeric value does not make things better.

@ksooo github seems to delete my tabs :s

subscriptionError str optional Subscription error code (Added in version 20).

Note: valid values for subscriptionError

noFreeAdapter No free adapter for this service
scrambled Service is scrambled
badSignal Bad signal status
tuningFailed Tuning of this service failed
subscriptionOverridden Subscription overridden by another one
muxNotEnabled No mux enabled for this service
invalidTarget Invalid target
userAccess User does not have acces rights for this service
userLimit User limit reached

What does"Invalid target" mean?
What does "User limit reached" mean?

Could you please extend the descriptions for these two a little to make things more clear?

User limit = each user has a number of maximum streaming connections set in it's user profile, defaulting to unlimited
invalid target = if tvh can not write the actual recording/livestream to the filesystem or when the recording or streaming config is incorrect

Okay. Thanks. Added to the spec now. https://tvheadend.org/projects/tvheadend/wiki/Htsp

Glenn-1990 · 2015-07-28T14:01:44Z

@ksooo reworked, might still need some work

I also think that my solution to the no response is not optimal?

Glenn-1990 · 2015-07-31T19:22:14Z

@ksooo I already have a better idea for the sendweight issue, we can do that in the dialog thread, so we won't block anything.

@Jalle19 Currently running out of time, so go ahead with re factoring (including SSubscription), I'll rebase later. Actually, if SSubscription has it's own class, things will get easier (i.e. better looking) :p

xhaggi · 2015-07-31T19:32:38Z

src/HTSPDemuxer.cpp

+void DialogLivestreamAborted( void )
+{
+  bDialogForceStart = GUI->Dialog_YesNo_ShowAndGetInput(
+      XBMC->GetLocalizedString(30461),XBMC->GetLocalizedString(30451),


spaces between parameter xx, xx

xhaggi · 2015-07-31T19:41:00Z

@Glenn-1990 only looked at formatting.

Glenn-1990 · 2015-09-06T11:40:15Z

@ksooo
I'm working on this at the moment, you suggests to move the functions with a lot of reads and writes to SSubsription to a member of it? Makes sense to me.

I already converted SSubsription from struct to a class, but it's still in htspTypes.cpp,
should I move it to a separate file instead?

ksooo · 2015-09-06T12:13:23Z

Leave it in HTSPTypes.cpp for now.

Glenn-1990 · 2015-09-06T20:57:54Z

src/HTSPTypes.cpp

+  m_state = state;
+  m_mutex.Unlock();
+}
+


@ksooo Need some advise here.
I want to make m_state thread safe, but I do not know much about multi threading, the only thing I know is that I need to protect variables accessed within multiple threads with mutex locks..

m_state is also accessed in GetState(), but that is an const function and const functions are thread safe?

Does this all make sense? :p

You could take a look at https://github.com/kodi-pvr/pvr.hts/blob/master/src/AsyncState.cpp and https://github.com/kodi-pvr/pvr.vbox/blob/master/src/vbox/StartupStateHandler.h. Your basic idea is correct, you need to use a mutex lock whenever you access the m_state variable. const functions are not thread-safe by definition since another thread could come in and overwrite the value of the variable while you're reading it. Since locking a mutex is a non-const operation it is common to mark the mutex variable as mutable so it can be used from const methods (see e.g. https://github.com/kodi-pvr/pvr.vbox/blob/master/src/vbox/StartupStateHandler.h#L123).

Glenn-1990 · 2015-09-10T17:47:34Z

src/HTSPTypes.cpp

+{
+  /* used in multiple threads */
+  CLockObject lock(m_mutex);
+  return m_weight;


@Jalle19 thanks for your help on this.

m_weght, m_state and m_subscriptionId are made threadsafe in the right way I guess? :p

Glenn-1990 · 2015-09-15T19:54:41Z

@Jalle19 @ksooo
Should be in working state now.

Any feedback would be welcome ;-)

The diff seems quite large, but that's just because of the reshuffle (subscription to own class instead of demuxer class)

Jalle19 · 2015-09-20T09:15:09Z

pvr.hts/resources/language/resource.language.en_gb/strings.po

+msgstr ""
+
+msgctxt "#30451"
+msgid "Livestream aborted, adapter stolen by other subscription"


Can you change "Livestream" to "Streaming" or "Live stream"?

Jalle19 · 2015-10-18T10:58:58Z

@Glenn-1990 any chance you could split the Subscription class refactoring to a separate PR?

Glenn-1990 · 2015-10-18T12:26:42Z

@Jalle19 where should it be placed? Root directory or entity?

Jalle19 · 2015-10-18T12:51:22Z

I'd place it either in tvheadend/, it's not really an entity since it doesn't require SetDirty().

Glenn-1990 · 2015-10-19T15:13:21Z

I'll rebase this when #119 is in

Glenn-1990 · 2015-10-21T15:12:29Z

Maybe we should have this as a default disabled setting?

Jalle19 · 2015-10-26T14:10:44Z

src/tvheadend/Subscription.cpp

+      {
+        /* If streaming conflict management enabled */
+        if (Settings::GetInstance().GetStreamingConflict())
+        {


I would factor out this block to a separate method, e.g. HandleSubscriptionConflict.

done, just added HandleConflict() as the class name makes it clear that it's about subscriptions

Glenn-1990 · 2015-10-26T19:05:20Z

@Jalle19 What do you think about the English strings in the dialog?

ksooo · 2015-10-26T19:15:48Z

If you ask me, this stuff is all far too tekky for a normal end user. As I said several times, why don't you just buy a server that fits your needs instead of adding loads of code just dealing with the situation that your server does not have the balls to serve your needs? Not 100% serious about this, but cannot understand your priorities... ;-)

I really do not like that you add some real GUI (dialog) to pvr.hts, because I thinks this is no good design.

But again, just my 2 Ct, I will not block this PR.

Glenn-1990 · 2015-10-26T23:10:20Z

Well, I'm not facing such a streaming conflict many times as I've a quad tuner, but when I face it, it seems unclear to the user what is happening...
So this dialog makes it clear that the channel he was watching was stopped because of a conflict with a starting recording or other livestream and gives him the ability to keep watching, which is a basic conflict feature most pvr apps have. (pvr.myth also does conflict handling)

I'm not adding a GUI dialog, I just call the kodi one, so no messing with xml files.
This is where the GUI api is for not? For addon specific stuff.

Maybe we can try to make it simpler?
i.e.
dialog title: live stream conflict
All adapters are in use.
To keep watching, interrupt an other client or recording.
buttons: interrupt, cancel

This is a very good example, page 2: http://www.connectctc.com/files/tv-support/Prime-User-Guide/Conflicts/How-to-Resolve-Conflicts.pdf

ksooo · 2015-10-27T06:06:59Z

Yeah. Simplify it. Especially untekkify the wording. Maybe like in the document you posted. Everything about"stolen adapters" and "priorities" and "hijack" has to go imo, because people will not understand, I'm afraid.

Regarding using a dialog. I still have the feeling that conflict handling belongs into Kodi core even though this might be much more work than just hacking this into one add-on.

Jalle19 · 2015-10-27T07:23:54Z

I agree about the techiness - the feature is nice though. I don't think it can be inplemented in core Kodi either since very few addons provide the necessary API to even determine if a conflict has taken place.

ksooo · 2015-10-27T07:41:51Z

I tend to disagree regarding implementing this not in Kodi core. Conflict management is an essential PVR feature and therefore should be handled by Kodi core in a consistent way for all PVR add-ons. The fact that not many servers (today!) offer a suitable API for this is imo unrelated.

But again, I'm not going to block this PR if the UI will be changed to be far less techy.

Glenn-1990 · 2015-10-27T21:48:31Z

@ksooo What do you think of the naming now?

ksooo · 2015-10-28T06:48:33Z

Is there any chance to present to the user what other conflicting subscriptions are active and let him choose from a list which one to interrupt in order to solve the conflict? This would be cool (and users will understand what's going on).

BTW, if I remember correctly, since Isengard we call it just "TV" everywhere in Kodi, not "Live TV", even in English.

Glenn-1990 · 2015-10-28T12:25:16Z

@ksooo Currently that's not possible, or we have to parse the active recordings and check the start time, but that's not a clean solution...

Keep in mind that conflict management will be disabled by default.

Glenn-1990 · 2015-11-20T18:42:24Z

@ksooo what if we just show a dialog like this?:
"All services are in use, please disable active recordings and/or other clients to resume watching"
-> OK button

And nothing else, so not changing weight etc, just make the user clear what just happened to him.

ksooo reviewed Jul 26, 2015
View reviewed changes

Glenn-1990 force-pushed the conflicting_subscriptions branch from 8b2cea2 to f55c1f9 Compare July 28, 2015 13:50

xhaggi reviewed Jul 31, 2015
View reviewed changes

Glenn-1990 changed the title ~~Implement subscription conflict handling~~ [WIP] Implement subscription conflict handling Sep 6, 2015

Glenn-1990 force-pushed the conflicting_subscriptions branch from f55c1f9 to 8b7e7b6 Compare September 6, 2015 20:45

Glenn-1990 reviewed Sep 6, 2015
View reviewed changes

Glenn-1990 force-pushed the conflicting_subscriptions branch from 8b7e7b6 to 81e1456 Compare September 10, 2015 17:44

Glenn-1990 reviewed Sep 10, 2015
View reviewed changes

Glenn-1990 force-pushed the conflicting_subscriptions branch 5 times, most recently from 773f880 to 86deb7e Compare September 15, 2015 19:49

Jalle19 reviewed Sep 20, 2015
View reviewed changes

Jalle19 added the Feature label Sep 20, 2015

Jalle19 mentioned this pull request Oct 18, 2015

Add support for streaming profiles #119

Merged

Glenn-1990 force-pushed the conflicting_subscriptions branch from 86deb7e to 41e4a1b Compare October 21, 2015 12:57

Glenn-1990 changed the title ~~[WIP] Implement subscription conflict handling~~ Implement subscription conflict handling Oct 21, 2015

Glenn-1990 force-pushed the conflicting_subscriptions branch 2 times, most recently from 00a6451 to 3382f47 Compare October 21, 2015 13:21

Glenn-1990 mentioned this pull request Oct 24, 2015

Settings rework v2 #126

Closed

Glenn-1990 force-pushed the conflicting_subscriptions branch from 3382f47 to f0e18f4 Compare October 26, 2015 13:15

Jalle19 reviewed Oct 26, 2015
View reviewed changes

Glenn-1990 force-pushed the conflicting_subscriptions branch from f0e18f4 to 6c4aa6a Compare October 26, 2015 19:01

Glenn-1990 force-pushed the conflicting_subscriptions branch 2 times, most recently from 83042fc to 021e77e Compare October 27, 2015 21:43

Add setting to enable streaming conflict management

e7eda9b

Glenn-1990 force-pushed the conflicting_subscriptions branch from 021e77e to e7eda9b Compare October 28, 2015 13:20

Glenn-1990 closed this Jan 22, 2017

Glenn-1990 deleted the conflicting_subscriptions branch January 22, 2017 17:10

Implement subscription conflict handling #79

Implement subscription conflict handling #79

Conversation

Glenn-1990 commented Jul 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glenn-1990 commented Jul 28, 2015

Glenn-1990 commented Jul 31, 2015

Choose a reason for hiding this comment

xhaggi commented Jul 31, 2015

Glenn-1990 commented Sep 6, 2015

ksooo commented Sep 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glenn-1990 commented Sep 15, 2015

Choose a reason for hiding this comment

Jalle19 commented Oct 18, 2015

Glenn-1990 commented Oct 18, 2015

Jalle19 commented Oct 18, 2015

Glenn-1990 commented Oct 19, 2015

Glenn-1990 commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glenn-1990 commented Oct 26, 2015

ksooo commented Oct 26, 2015

Glenn-1990 commented Oct 26, 2015

ksooo commented Oct 27, 2015

Jalle19 commented Oct 27, 2015

ksooo commented Oct 27, 2015

Glenn-1990 commented Oct 27, 2015

ksooo commented Oct 28, 2015

Glenn-1990 commented Oct 28, 2015

Glenn-1990 commented Nov 20, 2015