From e69a203c70c107b0e71e2f09728e4a0de4450857 Mon Sep 17 00:00:00 2001 From: CastagnaIT Date: Fri, 25 Aug 2023 16:09:53 +0200 Subject: [PATCH 1/3] [AdaptiveTree] Removed some uses of RemoveParameters The current RemoveParameters method can be misleading and can delete required url parts --- src/parser/DASHTree.cpp | 4 ++-- src/parser/HLSTree.cpp | 6 +++--- src/parser/SmoothTree.cpp | 2 +- src/utils/UrlUtils.cpp | 24 +++++++++++++++++++----- src/utils/UrlUtils.h | 19 +++++++++++++------ 5 files changed, 38 insertions(+), 17 deletions(-) diff --git a/src/parser/DASHTree.cpp b/src/parser/DASHTree.cpp index 10c14300e..407ae16bd 100644 --- a/src/parser/DASHTree.cpp +++ b/src/parser/DASHTree.cpp @@ -98,7 +98,7 @@ bool adaptive::CDashTree::Open(std::string_view url, m_manifestRespHeaders = headers; manifest_url_ = url; - base_url_ = URL::RemoveParameters(url.data()); + base_url_ = URL::GetUrlPath(url.data()); if (!ParseManifest(data)) return false; @@ -1514,7 +1514,7 @@ void adaptive::CDashTree::RefreshLiveSegments() { manifestUrl = manifest_url_; if (!manifestParams.empty()) - manifestUrl = URL::RemoveParameters(manifestUrl, false); + manifestUrl = URL::RemoveParameters(manifestUrl); } else manifestUrl = location_; diff --git a/src/parser/HLSTree.cpp b/src/parser/HLSTree.cpp index 865244e75..a6d1facc1 100644 --- a/src/parser/HLSTree.cpp +++ b/src/parser/HLSTree.cpp @@ -151,7 +151,7 @@ bool adaptive::CHLSTree::Open(std::string_view url, SaveManifest(nullptr, data, url); manifest_url_ = url; - base_url_ = URL::RemoveParameters(url.data()); + base_url_ = URL::GetUrlPath(url.data()); if (!ParseManifest(data)) { @@ -205,7 +205,7 @@ PLAYLIST::PrepareRepStatus adaptive::CHLSTree::prepareRepresentation(PLAYLIST::C SaveManifest(adp, resp.data, manifestUrl); - rep->SetBaseUrl(URL::RemoveParameters(resp.effectiveUrl)); + rep->SetBaseUrl(URL::GetUrlPath(resp.effectiveUrl)); EncryptionType currentEncryptionType = EncryptionType::CLEAR; @@ -354,7 +354,7 @@ PLAYLIST::PrepareRepStatus adaptive::CHLSTree::prepareRepresentation(PLAYLIST::C if (rep->GetContainerType() == ContainerType::NOTYPE) { // Try find the container type on the representation according to the file extension - std::string url = URL::RemoveParameters(line, false); + std::string url = URL::RemoveParameters(line); // Remove domain on absolute url, to not confuse top-level domain as extension url = url.substr(URL::GetDomainUrl(url).size()); diff --git a/src/parser/SmoothTree.cpp b/src/parser/SmoothTree.cpp index 5aab39c07..28ad769f8 100644 --- a/src/parser/SmoothTree.cpp +++ b/src/parser/SmoothTree.cpp @@ -33,7 +33,7 @@ bool adaptive::CSmoothTree::Open(std::string_view url, SaveManifest("", data, ""); manifest_url_ = url; - base_url_ = URL::RemoveParameters(url.data()); + base_url_ = URL::GetUrlPath(url.data()); if (!ParseManifest(data)) return false; diff --git a/src/utils/UrlUtils.cpp b/src/utils/UrlUtils.cpp index 5cbfdaa3e..acaa1f83f 100644 --- a/src/utils/UrlUtils.cpp +++ b/src/utils/UrlUtils.cpp @@ -164,18 +164,32 @@ std::string UTILS::URL::GetParameters(std::string& url) { return ""; } -std::string UTILS::URL::RemoveParameters(std::string url, bool removeFilenameParam /* = true */) +std::string UTILS::URL::RemoveParameters(std::string url) { size_t paramsPos = url.find('?'); if (paramsPos != std::string::npos) url.resize(paramsPos); - if (removeFilenameParam) + return url; +} + +std::string UTILS::URL::GetUrlPath(std::string url) +{ + if (url.empty()) + return url; + + size_t paramsPos = url.find('?'); + if (paramsPos != std::string::npos) + url.resize(paramsPos); + + // The part of the base url after last / is not a directory so will not be taken into account + if (url.back() != '/') { - size_t slashPos = url.find_last_of('/'); - if (slashPos != std::string::npos && slashPos != (url.find("://") + 2)) - url.resize(slashPos + 1); + size_t slashPos = url.rfind("/"); + if (slashPos > url.find("://") + 3) + url.erase(slashPos + 1); } + return url; } diff --git a/src/utils/UrlUtils.h b/src/utils/UrlUtils.h index 52be1d2e6..23659bc64 100644 --- a/src/utils/UrlUtils.h +++ b/src/utils/UrlUtils.h @@ -53,13 +53,20 @@ std::string GetParametersFromPlaceholder(std::string& url, std::string_view plac */ std::string GetParameters(std::string& url); -/*! \brief Remove URL parameters e.g. "?q=something" - * \param url An URL - * \param removeFilenameParam If true remove the last URL level if it - * contains a filename with extension - * \return The URL without parameters +/*! + * \brief Remove URL parameters e.g. "?q=something" + * \param url An URL + * \return The URL without parameters + */ +std::string RemoveParameters(std::string url); + +/*! + * \brief Get the url path, by removing file part and parameters + * e.g. https://sample.com/part1/part2?test become https://sample.com/part1/ + * \param url An URL + * \return The URL path */ -std::string RemoveParameters(std::string url, bool removeFilenameParam = true); +std::string GetUrlPath(std::string url); /*! \brief Append a string of parameters to an URL with/without pre-existents params * \param url URL where append the parameters From 1ef93cb110f47a4a0826a7f618cf06b6a1154427 Mon Sep 17 00:00:00 2001 From: CastagnaIT Date: Sat, 26 Aug 2023 15:51:13 +0200 Subject: [PATCH 2/3] [HLSTree] Check for already add uri variant/rendition Some manifests can have multiple audio rendition GROUPS with differents audio codecs means that multiple Variants with same uri but different AUDIO group may exists and we should not add them more times Similar thing seem to happens for Renditions despite seem less common thing, more Renditions with identical properties (and so same uri) but different GROUP-ID may exists, and we should not add them more times --- src/parser/HLSTree.cpp | 33 +++++++++++++++++++++++++++++++++ src/parser/HLSTree.h | 2 ++ 2 files changed, 35 insertions(+) diff --git a/src/parser/HLSTree.cpp b/src/parser/HLSTree.cpp index a6d1facc1..a3eb53083 100644 --- a/src/parser/HLSTree.cpp +++ b/src/parser/HLSTree.cpp @@ -16,6 +16,7 @@ #include "../utils/log.h" #include "kodi/tools/StringUtils.h" +#include #include #include @@ -974,6 +975,19 @@ bool adaptive::CHLSTree::ParseMultivariantPlaylist(const std::string& data) rend.m_isForced = attribs["FORCED"] == "YES"; rend.m_characteristics = attribs["CHARACTERISTICS"]; rend.m_uri = attribs["URI"]; + std::string uri = attribs["URI"]; + + if (!uri.empty()) + { + // Check if this uri has been already added + if (std::any_of(pl.m_audioRenditions.cbegin(), pl.m_audioRenditions.cend(), + [&uri](const Rendition& v) { return v.m_uri == uri; }) || + std::any_of(pl.m_subtitleRenditions.cbegin(), pl.m_subtitleRenditions.cend(), + [&uri](const Rendition& v) { return v.m_uri == uri; })) + { + rend.m_isUriDuplicate = true; + } + } if (streamType == StreamType::AUDIO) pl.m_audioRenditions.emplace_back(rend); @@ -1017,6 +1031,13 @@ bool adaptive::CHLSTree::ParseMultivariantPlaylist(const std::string& data) var.m_groupIdSubtitles = attribs["SUBTITLES"]; var.m_uri = uri; + // Check if this uri has been already added + if (std::any_of(pl.m_variants.cbegin(), pl.m_variants.cend(), + [&uri](const Variant& v) { return v.m_uri == uri; })) + { + var.m_isUriDuplicate = true; + } + pl.m_variants.emplace_back(var); } else if (tagName == "#EXT-X-SESSION-KEY") @@ -1050,6 +1071,10 @@ bool adaptive::CHLSTree::ParseMultivariantPlaylist(const std::string& data) // Add audio renditions (do not take in account variants references) for (const Rendition& r : pl.m_audioRenditions) { + // There may be multiple renditions with the same uri but different GROUP-ID + if (r.m_isUriDuplicate) + continue; + auto newAdpSet = CAdaptationSet::MakeUniquePtr(period.get()); auto newRepr = CRepresentation::MakeUniquePtr(newAdpSet.get()); @@ -1096,6 +1121,10 @@ bool adaptive::CHLSTree::ParseMultivariantPlaylist(const std::string& data) // Add subtitles renditions (do not take in account variants references) for (const Rendition& r : pl.m_subtitleRenditions) { + // There may be multiple renditions with the same uri but different GROUP-ID + if (r.m_isUriDuplicate) + continue; + auto newAdpSet = CAdaptationSet::MakeUniquePtr(period.get()); auto newRepr = CRepresentation::MakeUniquePtr(newAdpSet.get()); @@ -1112,6 +1141,10 @@ bool adaptive::CHLSTree::ParseMultivariantPlaylist(const std::string& data) // Add variants for (const Variant& var : pl.m_variants) { + // There may be multiple variants with the same uri but different AUDIO group + if (var.m_isUriDuplicate) + continue; + if (var.m_bandwidth == 0) LOG::LogF(LOGWARNING, "Variant with malformed bandwidth attribute"); diff --git a/src/parser/HLSTree.h b/src/parser/HLSTree.h index 121c72c52..f49be0feb 100644 --- a/src/parser/HLSTree.h +++ b/src/parser/HLSTree.h @@ -79,6 +79,7 @@ class ATTR_DLL_LOCAL CHLSTree : public AdaptiveTree std::string m_characteristics; std::string m_uri; int m_features{REND_FEATURE_NONE}; + bool m_isUriDuplicate{false}; // Another rendition have same uri }; // \brief Usually refer to an EXT-X-STREAM-INF tag @@ -91,6 +92,7 @@ class ATTR_DLL_LOCAL CHLSTree : public AdaptiveTree std::string m_groupIdAudio; std::string m_groupIdSubtitles; std::string m_uri; + bool m_isUriDuplicate{false}; // Another variant have same uri }; struct MultivariantPlaylist From ec75b625109052b3571d4df89045f06f5843066b Mon Sep 17 00:00:00 2001 From: CastagnaIT Date: Sat, 26 Aug 2023 15:22:39 +0200 Subject: [PATCH 3/3] [HLSTree] Ensure repr baseurl is set between periods --- src/parser/HLSTree.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/parser/HLSTree.cpp b/src/parser/HLSTree.cpp index a3eb53083..e67e85f5d 100644 --- a/src/parser/HLSTree.cpp +++ b/src/parser/HLSTree.cpp @@ -489,11 +489,17 @@ PLAYLIST::PrepareRepStatus adaptive::CHLSTree::prepareRepresentation(PLAYLIST::C m_periods.push_back(std::move(newPeriod)); } else + { + // Period(s) already created by a previously downloaded manifest child period = m_periods[discontCount].get(); + } newStartNumber += rep->SegmentTimeline().GetSize(); adp = period->GetAdaptationSets()[adpSetPos].get(); - rep = adp->GetRepresentations()[reprPos].get(); + // When we switch to a repr of another period we need to set current base url + CRepresentation* switchRep = adp->GetRepresentations()[reprPos].get(); + switchRep->SetBaseUrl(rep->GetBaseUrl()); + rep = switchRep; currentSegStartPts = 0;