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

HTSP v25 additions #230

Merged
merged 4 commits into from Jun 19, 2016

Conversation

@Glenn-1990
Copy link
Contributor

commented Jun 14, 2016

Changes:

  1. bIsRadio -> channelType: channelType can have 'radio', 'tv' or 'other' assigned to it. By this way, we can hide all channels marked as 'other' from Kodi. With the current implementation, channels with data/download services where shown as tv channels in Kodi...
  2. Recordings with a deleted channel are now correctly identified as radio/television and the channel name will still be present.
    The current implementation just maps everything to tv recordings in this case.

@ksooo Part 2 is the one you requested some time ago.

else
rec.channelType = PVR_RECORDING_CHANNEL_TYPE_TV;
rec.channelType = static_cast<PVR_RECORDING_CHANNEL_TYPE>(cit->second.GetType());

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 14, 2016

Member

This is dangerous. If htsp "content" definition at some point in time gets extended by channel types with numerical values greater than 2, this cast will fail. Your implementation of SetType stores whatever numerical value is delivered via htsp.

This comment has been minimized.

Copy link
@Glenn-1990

Glenn-1990 Jun 14, 2016

Author Contributor

Yes, you're right, will change this.

@@ -1827,7 +1827,7 @@ void CTvheadend::ParseChannelAddOrUpdate ( htsmsg_t *msg, bool bAdd )
{
if (!strcmp(str, "Radio"))
channel.SetType(PVR_RECORDING_CHANNEL_TYPE_RADIO);
else
else if (!strcmp(str, "SDTV") || !strcmp(str, "HDTV"))

This comment has been minimized.

Copy link
@Glenn-1990

Glenn-1990 Jun 14, 2016

Author Contributor

"UHDTV" was added with htspv25, this code only gets called when htsp version < 25.

}

/* Channel name fallback (in case channel was deleted) */
if (rec.GetChannelName().empty() && m_conn.GetProtocol() >= 25)

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 14, 2016

Member

Please remove one of the two spaces between empty() and && m_conn.

if ((files = htsmsg_get_list(msg, "files")) != NULL)
{
htsmsg_field_t *file, *stream;
uint32_t hasAudio = 0, hasVideo = 0, u32;

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 14, 2016

Member

Please remove one of the two spaces between hasVideo and = 0.

@@ -1917,9 +1907,64 @@ void CTvheadend::ParseRecordingAddOrUpdate ( htsmsg_t *msg, bool bAdd )
rec.SetStop(stop);

/* Channel is optional, it may not exist anymore */
if (!htsmsg_get_u32(msg, "channel", &channel))
if (!htsmsg_get_u32(msg, "channel", &channel) && channel > 0)

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 14, 2016

Member

Why the additional condition? Is 0 an invalid channel id?

hasAudio = 1;
if (!htsmsg_get_u32(&stream->hmf_msg, "aspect_num", &u32)) // Only present for video streams
hasVideo = 1;
if (hasAudio && hasVideo) // No need to parse the rest

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 14, 2016

Member

Hmm. The above lines look somehow hacky to me. Might work, but still...

This comment has been minimized.

Copy link
@Glenn-1990

Glenn-1990 Jun 15, 2016

Author Contributor

Yes, it's not that clean but perexg preferred this solution as it doesn't need much additional logic in tvheadend. Remember that this only handles recordings with a missing channel, tested with radio and tv recording (without channel ofc).

Check the first comment here:
tvheadend/tvheadend#847

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 15, 2016

Member

Ah, okay then.

strncpy(rec.strIconPath, cit->second.GetIcon().c_str(),
sizeof(rec.strIconPath) - 1);
}

/* Channel name */
strncpy(rec.strChannelName, recording.GetChannelName().c_str(),
sizeof(rec.strChannelName) - 1);

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 14, 2016

Member

This is a bad idea if recording.GetChannelName returns an empty string. Was not good in original code, btw.

This comment has been minimized.

Copy link
@Glenn-1990

Glenn-1990 Jun 15, 2016

Author Contributor

@ksooo what's wrong with an empty string as long it's initialized?

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 15, 2016

Member

what's wrong with an empty string as long it's initialized?

Nothing. My fault, sorry. Mixed recording.GetChannelName() return value which can be empty with rec.strChannelName which cannot be zero lenght.

@Glenn-1990 Glenn-1990 force-pushed the Glenn-1990:radio-tv branch from fd486ed to 159a39a Jun 15, 2016
@Glenn-1990

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2016

@ksooo updated

@@ -44,7 +44,7 @@ namespace tvheadend
Channel() :
m_num(0),
m_numMinor(0),
m_radio(false),
m_type(0),

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 15, 2016

Member

CHANNEL_TYPE_OTHER ?

@@ -90,7 +90,7 @@ bool Tag::ContainsChannelType(bool bRadio) const
{
if ((cit = channels.find(*it)) != channels.end())
{
if (bRadio == cit->second.IsRadio())
if (cit->second.GetType() == (bRadio ? CHANNEL_TYPE_RADIO : CHANNEL_TYPE_TV))

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 15, 2016

Member

Shouldn't Tag::ContainsChannelType(bool bRadio) consequently be changed to Tag::ContainsChannelType(channel_type_t eType)? Currently implmentation returns true for "other" channels if bRadio is false which is not correct.

This comment has been minimized.

Copy link
@Glenn-1990

Glenn-1990 Jun 15, 2016

Author Contributor

If bRadio is false, the function returns true for tv channels only..
But changing the function to what you suggest makes sense.

case CHANNEL_TYPE_RADIO:
rec.channelType = PVR_RECORDING_CHANNEL_TYPE_RADIO;
break;
default:

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 15, 2016

Member

For the sake of completeness I would also handle CHANNEL_TYPE_OTHER explicitely here.

@Glenn-1990 Glenn-1990 force-pushed the Glenn-1990:radio-tv branch from 159a39a to 3ca57df Jun 15, 2016
@Glenn-1990

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2016

@ksooo updated

@ksooo

This comment has been minimized.

Copy link
Member

commented Jun 15, 2016

@Glenn-1990 many thanks. Could you please add changelog and version bump? Then we are ready to merge this.

@ksooo ksooo added the Improvement label Jun 17, 2016
@Glenn-1990

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2016

@ksooo version bump done

@ksooo

This comment has been minimized.

Copy link
Member

commented Jun 19, 2016

@Glenn-1990 please rebase. 3.4.2 already exists ;-)

@Glenn-1990 Glenn-1990 force-pushed the Glenn-1990:radio-tv branch from 130967a to 3444a52 Jun 19, 2016
@Glenn-1990

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2016

@ksoo, oops my bad, rebased now

@ksooo ksooo merged commit ac2e3be into kodi-pvr:master Jun 19, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ksooo

This comment has been minimized.

Copy link
Member

commented Jun 19, 2016

@Glenn-1990 unrelated, but i just stumbled over the fact that DVR_RET_SPACE and DVR_RET_FOREVER are both defined as INT32_MAX-1. This feels wrong.

https://github.com/kodi-pvr/pvr.hts/pull/230/files#diff-2db6f295170a206772114b26ca4174a0R79

Seems also be buggy in tvheadend.

https://github.com/tvheadend/tvheadend/blob/master/src/dvr/dvr.h#L146

???

@Glenn-1990

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2016

@ksooo
This is correct as dvr_retention_t and dvr_removal_t are combined to one.
retention INT32_MAX-1 = remove log file when dvr file gets removed.
removal INT32_MAX-1 = remove dvr file when space is needed.
For all other cases, remove and retention are similar.

@ksooo

This comment has been minimized.

Copy link
Member

commented Jun 19, 2016

okay, then. thanks for the clarification

@Glenn-1990 Glenn-1990 deleted the Glenn-1990:radio-tv branch Jan 22, 2017
Jalle19 added a commit to Jalle19/pvr.hts that referenced this pull request Feb 5, 2017
HTSP v25 additions
Conflicts:
	pvr.hts/addon.xml.in
	pvr.hts/changelog.txt
	src/Tvheadend.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.