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

Fix upnp iso resume exporting and resume from upnp #11045

Merged
merged 6 commits into from Dec 14, 2016

Conversation

@phate89
Copy link
Contributor

commented Dec 3, 2016

Description

Add exporting of playerstate to fix importing of resume point in isos and adds playerstate to upnp to fix resuming of iso via upnp.
@Montellese for review

Motivation and Context

Right now if we export/import a iso resume point it doesn't work anymore. Same for resume point of a iso via upnp

How Has This Been Tested?

Tested with 2 local upnp libraries and exporting/importing them once.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

@phate89 phate89 changed the title Fix upnp iso resume Fix upnp iso resume exporting and resume from upnp Dec 3, 2016

@phate89 phate89 force-pushed the phate89:fix_upnp_iso_resume branch 2 times, most recently from 99b6900 to 86657a2 Dec 3, 2016

@Montellese
Copy link
Member

left a comment

What's the format of playerstate? Is it a pure string or is it XML or JSON stored in a string?

You certainly need to change the XML namespace of the newly introduced XML tags from upnp to xbmc.

@@ -1187,6 +1197,13 @@ void CVideoInfoTag::ParseNative(const TiXmlElement* movie, bool prioritise)
{
XMLUtils::GetDouble(resume, "position", m_resumePoint.timeInSeconds);
XMLUtils::GetDouble(resume, "total", m_resumePoint.totalTimeInSeconds);
const TiXmlElement *playerstate = epbookmark->FirstChildElement("playerstate");

This comment has been minimized.

Copy link
@Montellese

Montellese Dec 4, 2016

Member

Something looks wrong here. Shouldn't it be resume instead of epbookmark?

{
const TiXmlElement *value = playerstate->FirstChildElement();
if (value)
m_EpBookmark.playerState << *value;

This comment has been minimized.

Copy link
@Montellese

Montellese Dec 4, 2016

Member

Shouldn't this be m_resumePoint instead of m_EpBookmark to match the Save() implementation?

@@ -249,7 +249,10 @@ class CMediaBrowser : public PLT_SyncMediaBrowser,
if (time < 0) time = 0;
curr_value.Append(NPT_String::Format("<upnp:lastPlaybackPosition>%ld</upnp:lastPlaybackPosition>",
(long)item.GetVideoInfoTag()->m_resumePoint.timeInSeconds));
curr_value.Append(NPT_String::Format("<upnp:lastPlayerState>%s</upnp:lastPlayerState>",

This comment has been minimized.

Copy link
@Montellese

Montellese Dec 4, 2016

Member

Is upnp:lastPlayerState defined in the UPnP specs? If not you need to use another XML namespace like xbmc: (which we use in otherplaces).

@@ -123,6 +124,7 @@
#define PLT_FILTER_FIELD_SERIESTITLE "upnp:seriesTitle"
#define PLT_FILTER_FIELD_EPISODE "upnp:episodeNumber"
#define PLT_FILTER_FIELD_LASTPOSITION "upnp:lastPlaybackPosition"
#define PLT_FILTER_FIELD_LASTPLAYERSTATE "upnp:lastPlayerState"

This comment has been minimized.

Copy link
@Montellese

Montellese Dec 4, 2016

Member

See my other comment on the XML namespace.

@@ -131,6 +131,7 @@ typedef struct {
NPT_String toc;
NPT_String user_annotation; //TODO: can be multiple
NPT_UInt32 last_position;
NPT_String last_playerstate;

This comment has been minimized.

Copy link
@Montellese

Montellese Dec 4, 2016

Member

Please put all Kodi-specific properties into the PLT_XbmcInfo structure because the other ones in PLT_MiscInfo are defined in the UPnP spec.

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2016

It's an xml stored in a string.. I missed there was the upnp/xbmc distinction.. I'll move them to xbmc

@phate89 phate89 force-pushed the phate89:fix_upnp_iso_resume branch 2 times, most recently from eefd7f5 to 1e63e9d Dec 5, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2016

@Montellese I updated the pr.. thanks for the review.
looked at the code and it seems it's always xml the playerstate but I'm not 100% sure..
@FernetMenta maybe you know something more? is playerstate always xml or empty in our videoplayer?

@Montellese

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

@phate89 If it's XML I have to consult the UPnP specs again. IIRC any XML payload inside the DIDL-Lite elements needs to be encoded for transport but I'm not 100% sure.

@@ -256,6 +256,7 @@ PLT_MediaObject::Reset()

m_Resources.Clear();

m_XbmcInfo.last_playerstate = "";

This comment has been minimized.

Copy link
@Montellese

Montellese Dec 5, 2016

Member

Unnecessary spaces.

@@ -756,7 +764,7 @@ PLT_MediaObject::FromDidl(NPT_XmlElementNode* entry)
PLT_XmlHelper::GetChildText(entry, "lastPlaybackPosition", str, didl_namespace_upnp);
if (NPT_FAILED(str.ToInteger(value))) value = 0;
m_MiscInfo.last_position = value;

This comment has been minimized.

Copy link
@Montellese

Montellese Dec 5, 2016

Member

Unnecessary change.

@@ -400,7 +400,8 @@ PLT_MediaServer::ParseTagList(const NPT_String& updates, NPT_Map<NPT_String,NPT_
for (NPT_List<NPT_XmlNode*>::Iterator children = didl_partial->GetChildren().GetFirstItem(); children; children++) {
NPT_XmlElementNode* child = (*children)->AsElementNode();
if (!child) continue;
tags[child->GetTag()] = *child->GetText();
auto txt = child->GetText();

This comment has been minimized.

Copy link
@Montellese

Montellese Dec 5, 2016

Member

Did you run into trouble here? Please don't use auto as this is not our code and if we want to upstream our changes we cannot assume that they enforce a C++11 (or newer) compiler as well.

This comment has been minimized.

Copy link
@phate89

phate89 Dec 5, 2016

Author Contributor

Yes.. gettext returns a pointer that might be null. If we pass an empty tag to update the playerstate (<xbmc:lastplayerstate></xbmc:lastplayerstate>) we get a null from gettext. So With *child->GetText() we try to retrieve an object from a null pointer and it crash

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2016

@Montellese
If I'm not wrong we already encode the string with PLT_Didl::AppendXmlEscape

@phate89 phate89 force-pushed the phate89:fix_upnp_iso_resume branch from 1e63e9d to 97abe32 Dec 5, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2016

fixed latest comments

@@ -400,7 +400,8 @@ PLT_MediaServer::ParseTagList(const NPT_String& updates, NPT_Map<NPT_String,NPT_
for (NPT_List<NPT_XmlNode*>::Iterator children = didl_partial->GetChildren().GetFirstItem(); children; children++) {
NPT_XmlElementNode* child = (*children)->AsElementNode();
if (!child) continue;
tags[child->GetTag()] = *child->GetText();

This comment has been minimized.

Copy link
@Montellese

Montellese Dec 8, 2016

Member

Would you mind splitting this into a separate commit as it's a fundamental fix independent of any Kodi logic.

@Montellese

This comment has been minimized.

Copy link
Member

commented Dec 8, 2016

Moving the generic fix into a separate commit would make upstreaming easier. The rest is fine.

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2016

@Montellese no problem. I have to split only the commit or also the patch and commit patch?

@Montellese

This comment has been minimized.

Copy link
Member

commented Dec 8, 2016

If you split the commit you also have to redo the patch(es).

@phate89 phate89 force-pushed the phate89:fix_upnp_iso_resume branch from 97abe32 to c661bb4 Dec 12, 2016

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2016

@Montellese udpated with 2 separate patches
jenkins build this please

@phate89 phate89 merged commit d655fb0 into xbmc:master Dec 14, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins4kodi You did a great job. Have a cookie.
Details
@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Dec 17, 2016

@Montellese that bumps the database. is that something we should still do in beta?

@Montellese

This comment has been minimized.

Copy link
Member

commented Dec 17, 2016

Personally I probably wouldn't backport a database schema change this close to RC.

@phate89

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2016

I added the backport needed tag because it's a bug fix and I didn't consider the bump.
The db changes are really small since it only adds a new field in the views..
Otoh it's a bug that's there since forever. I noticed the bug4 months ago looking at the code (not because I actually use this) and I think it's never been reported at all so it's fine by me either way..

@MartijnKaijser MartijnKaijser modified the milestone: L 18.0-alpha1 Dec 18, 2016

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Dec 18, 2016

Thanks for fixing it but lets keep db version as is in Krypton :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.