Send custom headers with segment downloads #25

glennguy · 2017-04-27T13:03:08Z

This fixes #20. I'm very new to c++ so I don't know if this implementation is a good way of going about it particularly because it relies on a global variable, but it works nonetheless.

peak3d · 2017-04-27T13:37:42Z

src/main.cpp

@@ -35,6 +35,7 @@

 ADDON::CHelper_libXBMC_addon *xbmc = 0;
 std::uint16_t kodiDisplayWidth(0), kodiDisplayHeight(0);
+std::string pipe;


For me it feels wrong to use header fields from manifest requestl for the underlying stream files.
Its great that it works for the example in issue #20, but there could be some other constellations in the future.

Additional Headers should be passed using a new listItem property and has to be kept in session or adaptive_tree_ instance. They should then be passed to AdaptvieStream::download as a parameter value.

Beside this singletons on global scope (pipe) are not longer supported in future binary addons because multiple instances of the addon can be opened (pls. see agile branch)

Sure thing. I've slowly managed to figure it out - I've got it to take the info from the listitem property, store in the session, pass the info to the adaptivetree_ instance so it can be passed to the download function in main.

Working now on how to parse the curl options string

matthuisman · 2017-04-27T23:50:23Z

Something like this?
master...matthuisman:master

(that is forked from agile)

I don't know how to compile the addons so it's really just to maybe help out @glennguy
There is probably some type-o's etc.

But is that what you had in mind @peak3d ?

I'm still not sure what is best name for all the variables.
It could be more than just headers - it's more like extra Curl parameters...

peak3d · 2017-04-28T09:06:01Z

@matthuisman yes, exactly this way, there are no kodi changes needed as you already fugured out.
I'll look closer at it today.

Edit: naming: I'm working internally with CURLOPTIONS (and not with the URL | headers convention) because the | convention could be removed in the future)
https://github.com/peak3d/inputstream.adaptive/blob/master/src/main.cpp#L62

From naming I would suppose something wich reflects this: inputstream.adaptive.segment_curl_option_header)

I know its long name, but its at least readable.

matthuisman · 2017-04-29T21:12:20Z

@peak3d
Do you think maybe KodiAdaptiveStream::download could be changed to just accept a URL & a map of headers.

bool KodiAdaptiveStream::download(const char* url, std::map<string, string> &headers)

The range header and segment_curl_option_headers could be set in the AdaptiveStream::download_segment() function.

Is there an existing method that takes a curl header string and converts to a map?
eg. "Content-Type=application%2Fx-www-form-urlencoded%2FX-Forwarded-For=202.89.4.222"
and returns {'Content-Type': 'application/x-www-form-urlencoded', 'X-Forwarded-For':202.89.4.222}

If we don't want to append the string to the URL and let KODI handle it - then I guess we need a way of parsing it our-self?

Unless curl add-header can accept that string of multiple headers?

My initial idea was allowing multiple headers to be added using the & to separate them (like the | method).
Therefore, the name would be inputstream.adaptive.segment_curl_option_headers ?

glennguy · 2017-04-30T11:19:37Z

My initial idea was allowing multiple headers to be added using the & to separate them (like the | method)

@peak3d Should we just have a string of header options that are delimited by '&' and the header values url encoded, just like kodi accepts after the pipe?

Also, should there be a 'pre-approved' list of headers eg. as there is in the Kodi source here and here?

peak3d · 2017-04-30T11:47:27Z

@glennguy yes, pass headers somehow separated through the new listitem property, pass the string at initialization time to the AdaptiveTree. AdaptiveStream has access to the AdaptiveTree and can pass it as string (const std::string &) to the download override methode.

In download you have to split (there is a split function in helpers.h/cpp) and loop over the list and call CURL_AddOption (making another split for key value there).

Depending if you want to solve it by yourself or not, I have a little time tomorrow and can put it in if you want.

Edit: for sure you can create a std::vector<std::pair> once and pass it around, I would avoid a std::map because std:map has an overhad not needed here (sorted key access tree). I use std::map only if I need an access with an key value (map[key]) what is not needed here, we only want to iterate over the headers.

glennguy · 2017-05-01T02:05:00Z

@peak3d Thanks for the help, I think I should be able to solve it now. I'll push some changes, later today hopefully.

glennguy · 2017-05-01T04:04:48Z

@peak3d @matthuisman Changes made - tested with the following .strm file:

#KODIPROP:inputstreamaddon=inputstream.adaptive
#KODIPROP:inputstream.adaptive.manifest_type=mpd
#KODIPROP:inputstream.adaptive.segment_curl_option_headers=X-Forwarded-For=202.89.4.222&User-Agent=Mozilla/5.0%20%28X11%3B%20Ubuntu%3B%20Linux%20x86_64%3B%20rv%3A53.0%29%20Gecko/20100101%20Firefox/53.0
#KODIPROP:inputstream.adaptive.license_type=com.widevine.alpha
#KODIPROP:inputstream.adaptive.license_key=https://wvlic.brightcove.com/proxy/5375730767001|Content-Type=application%2Fx-www-form-urlencoded&X-Forwarded-For=202.89.4.222|A{SSM}|
http://dashtvnz-a.akamaihd.net/963482467001/963482467001_5375755467001_5375730767001.mpd?pubId=963482467001&videoId=5375730767001|X-Forwarded-For=202.89.4.222

I followed the example in wvdecrypter.cpp which made it quite easy in the end.

matthuisman · 2017-05-01T04:10:34Z

src/main.cpp

@@ -232,6 +219,15 @@ bool KodiAdaptiveStream::download(const char* urlPointer, const char* rangeHeade
    xbmc->CURLAddOption(file, XFILE::CURL_OPTION_HEADER, "Range", rangeHeader);
  xbmc->CURLAddOption(file, XFILE::CURL_OPTION_HEADER, "Connection", "keep-alive");
  xbmc->CURLAddOption(file, XFILE::CURL_OPTION_PROTOCOL, "acceptencoding", "gzip, deflate");
+  if (strcmp(segmentHeaders, "") != 0)
+  {
+    std::vector<std::string> header, headers = split(segmentHeaders, '&');


If the string is encoded.. would you not be splitting on %26?

Maybe best to decode the entire string before this first split?
This would cover both the use of & or %26

Added benifit of not needing the decode in the for loop

No. '&' is used as a delimiter to separate the keys. The values are what is encoded.

CurlAddOption seems to take care of the decoding itself. The convention in the above example I posted is the same as what Kodi requires after the pipe from add-ons.

Oh. OK.
Sorry, i thought it was following the pipe convention (whole string URL encoded)

matthuisman · 2017-05-01T04:15:20Z

src/main.cpp

  : manifest_type_(manifestType)
  , mpdFileURL_(strURL)
  , license_key_(strLicKey)
  , license_type_(strLicType)
  , license_data_(strLicData)
+  , segment_headers_(strSegmentHeaders)


Do we need to set the variable segment_headers_ at all? (It's not actually used anywhere.)

Could we not just pass strSegmentHeaders direct to adaptiveTree?
adaptiveTree_->segment_headers_ = strSegmentHeaders;

Right you are. Removed.

matthuisman · 2017-05-01T04:28:38Z

Still seems a bit odd having to define the same headers 3x times...

@peak3d
Are you sure we couldn't maybe swap it to a inputstream.adaptive.stream_headers (or session_headers)
And it applies to the "whole" inputstream session (every curl call for that stream- Key, MPD, Segments).
So, no longer required on the the stream URL, or key URL.

Individual specific headers could still be used by the pipe method on the key (eg. form encoded) / mpd if needed.

#KODIPROP:inputstreamaddon=inputstream.adaptive
#KODIPROP:inputstream.adaptive.manifest_type=mpd
#KODIPROP:inputstream.adaptive.stream_headers=X-Forwarded-For=202.89.4.222&User-Agent=Mozilla/5.0%20%28X11%3B%20Ubuntu%3B%20Linux%20x86_64%3B%20rv%3A53.0%29%20Gecko/20100101%20Firefox/53.0
#KODIPROP:inputstream.adaptive.license_type=com.widevine.alpha
#KODIPROP:inputstream.adaptive.license_key=https://wvlic.brightcove.com/proxy/5375730767001|Content-Type=application%2Fx-www-form-urlencoded|A{SSM}|
http://dashtvnz-a.akamaihd.net/963482467001/963482467001_5375755467001_5375730767001.mpd?pubId=963482467001&videoId=5375730767001

JinRonin · 2017-05-01T07:42:46Z

the motivation to allow custom headers should not be to bypass geo restrictions...

matthuisman · 2017-05-01T21:40:10Z

@JinRonin
Your right. However, there may be other good uses - eg: a required User-agent header.

glennguy · 2017-05-02T02:33:50Z

@JinRonin up until recently Netflix has filtered certain User-Agent headers. Seeing that large amount of work that has recently gone into adding support to play Netflix content, and once the kodi-agile fork is merged and Kodi 18 is released the assumed following large amount of users taking advantage of it, there's a high likelihood that that Kodi User-Agent headers will be filtered on Netflix's end.

The stream file posted by @matthuisman in issue #20 has merely been a convenient way to test the implementation of this feature. I don't believe there has been any explicit or implicit statement at least from myself that the motivation behind this PR has been to bypass geoblocking.

matthuisman · 2017-05-02T07:49:46Z

I often find users who are in the correct country but use a VPN.
Therefore their IPs will be shown as another country.
By them setting the X-Forward-For header to their actual IP, they may be able to view the content for their actual country.

matthuisman · 2017-05-02T20:53:56Z

Off-topic.
But does anyone else get Github notifications really late?
Mine seem to come in 24-48 hours after the actual activity.

peak3d · 2017-05-03T08:21:01Z

I'll look today over the changes, sry for late response...

peak3d · 2017-05-03T08:47:14Z

Are you sure we couldn't maybe swap it to a inputstream.adaptive.stream_headers (or session_headers)

@matthuisman: stream-key is already in the license_key template, therefore only manifest / media has to be decided.

Ok, lets start with an general Header key, wich is valid for both manifest + media (not license_key!!)
If we / you need a different control for headers between manifest and media, we'll define new keys for them wich overrides the general headers described above.

But pls. transport the header keys as bespoken from session -> tree -> download()

matthuisman · 2017-05-03T22:49:04Z

@peak3d

In the MPD download function, the URL might have headers in it (using |)
We would want these to over-ride any custom headers?

Therefore, we would need to set our custom headers before the URL is parsed.
I think we can do file.CURLAddOption before a file.CURLCreate.

DashTree and SmoothTree call download directly when getting the MPD file.
Feels strange editing these and adding stream_headers_ to their download call.

I suggest we have an override download function in AdapativeTree.cpp that accepts just a url and then calls download with the url and streamheaders?
That way, we don't need to modify the individual trees?

Here is some changes to look at (I haven't done the h files)
agile...matthuisman:agile

I also don't know how to compile the addons so it's just to maybe help out @glennguy :)

I also added our headers before parsing the URL in the other download function.
This is just so it matches and for some reason if there were | headers in the segment URL, they would over-write our headers.

Thoughts?

peak3d · 2017-05-04T12:40:18Z

In the MPD download function, the URL might have headers in it (using |)
We would want these to over-ride any custom headers?

It was not expected that someone uses | for the mpd call, but for sure you are rugth, this is possible.
My opinion how we should proceed with the headers (on session initialization)

if stream_headers listitem key is given:
a.) parse this key value into mpd_header pairs, make a copy (media_header)
b.) remove the | separated part from mpd_url (mix is not supported)
if stream_headers listitem key is NOT given:
a.) get the | separated header from mpd_url (if any) and parse it into mpd_header. media_header remains empty.

After that you have 2 parsed maps, one for tree, one for stream.
Pass those during the initialization to Tree and Stream so they can pass the header fields to download.

download then ONLY works with the extra passed headers, the | separated has been removed earlier.
IMO its always a good idea to choose one concept on a very early stage instead passing around multiple options.
After this change it should not feel strange to pass the "map" to the download function.

glennguy · 2017-05-04T21:47:38Z

@peak3d sounds good, thanks. I'll get some changes made tonight.

glennguy · 2017-05-05T13:16:12Z

@peak3d I think I've satisfied your comment above.

mpd_url is always stripped of pipe and anything following it.

If there is no stream_headers listitem set, then if there is pipe/headers then it is used for the manifest download only

If there is a stream_headers listitem, then it is used for both manifest and media.

I've left the headers in string format as it seemed a lot easier passing it around, and the parsing is done in the main/download functions.

peak3d

I don't want to be picky but there are some general principles wich should be respected:

write functions if something is dome multiple times
keep it simple / group similar things together
try to choose a readable workflow.

I know that especially the third point depends on the person who looks on it, but on the last end I'm the person who maintains the code and I have to understand things also if I haven't looked on things some months.

peak3d · 2017-05-05T13:44:57Z

src/common/AdaptiveStream.cpp

@@ -96,7 +96,8 @@ bool AdaptiveStream::download_segment()
    rangeHeader = rangebuf;
  }

-  return download(strURL.c_str(), rangeHeader);
+  media_headers_ = tree_.media_headers_;
+  return download(strURL.c_str(), rangeHeader, media_headers_.c_str());


We don't need 2 parameters to pass headers,
We should use a std::map to keep things simple (I know I was against it but here it makles sense)
media_header_['Range'] = rangeHeader? rangeHeader : "";

peak3d · 2017-05-05T13:46:32Z

src/common/AdaptiveTree.cpp

@@ -48,6 +48,8 @@ namespace adaptive
    , download_speed_(0.0)
    , average_download_speed_(0.0f)
    , encryptionState_(ENCRYTIONSTATE_UNENCRYPTED)
+    , manifest_headers_("")


stl classes have their own constructor, no need to assign a value

peak3d · 2017-05-05T13:47:18Z

src/common/AdaptiveTree.h

@@ -192,6 +192,7 @@ namespace adaptive
    bool has_timeshift_buffer_;

    uint32_t bandwidth_;
+    std::string manifest_headers_, media_headers_;


media_headers don't have to be in tree after the last duiscussion (separate headers for mpd / media)
should be std::map

peak3d · 2017-05-05T13:51:40Z

src/common/AdaptiveStream.h

@@ -79,6 +79,7 @@ namespace adaptive
    const AdaptiveTree::Segment *current_seg_;
    //We assume that a single segment can build complete frames
    std::string segment_buffer_;
+    std::string media_headers_;


peak3d · 2017-05-05T13:52:44Z

src/common/AdaptiveTree.h

@@ -223,7 +224,7 @@ namespace adaptive
    bool empty(){ return !current_period_ || current_period_->adaptationSets_.empty(); };
    const AdaptationSet *GetAdaptationSet(unsigned int pos) const { return current_period_ && pos < current_period_->adaptationSets_.size() ? current_period_->adaptationSets_[pos] : 0; };
 protected:
-  virtual bool download(const char* url);
+  virtual bool download(const char* url, const char* manifestHeaders);


const std::map &

peak3d · 2017-05-05T13:55:48Z

src/main.cpp

@@ -219,6 +229,15 @@ bool KodiAdaptiveStream::download(const char* url, const char* rangeHeader)
    xbmc->CURLAddOption(file, XFILE::CURL_OPTION_HEADER, "Range", rangeHeader);


obsolete if Range is in headerMap

peak3d · 2017-05-05T13:58:09Z

src/main.cpp

@@ -919,6 +938,10 @@ Session::Session(MANIFEST_TYPE manifestType, const char *strURL, const char *str
    b64_decode(strCert, sz, server_certificate_.UseData(), dstsz);
    server_certificate_.SetDataSize(dstsz);
  }
+
+  adaptiveTree_->manifest_headers_ = strManifestHeaders;
+  adaptiveTree_->media_headers_ = strMediaHeaders;


media_headers_ pls not in tree

media_heders should be passed to AdaptiveStream here:
https://github.com/peak3d/inputstream.adaptive/blob/agile/src/main.cpp#L1521

peak3d · 2017-05-05T13:58:58Z

src/main.cpp

@@ -1434,7 +1457,8 @@ extern "C" {
  {
    xbmc->Log(ADDON::LOG_DEBUG, "Open()");

-    const char *lt(""), *lk(""), *ld(""), *lsc("");
+    const char *lt(""), *lk(""), *ld(""), *lsc(""), *manh(""), *medh(""), *m_strURL;
+    m_strURL = props.m_strURL;


why is this an member? we don't need it permanent.
Should be local variable

peak3d · 2017-05-05T14:14:11Z

src/main.cpp

@@ -1468,6 +1501,15 @@ extern "C" {
      }
    }

+    if (strcmp(manh, "") == 0)


This is very confusing for me, after 5 times reading I still don't understand why need 3 strings for this task.

IMO this is the place where:

make a copy of props.m_strURL

look if | is in string, if so:

if inputstream.adaptive.stream_headers is not given, parse it into mahd

remove the | part from the copy.

Parsing of a string for header parts should be done in a function (parseheader(std::map, const char *)

Doing the parsing stuff should be only done once (here!!), having it in download() means that we parse the same stuff every segment again, this is not very resource friendly.

peak3d · 2017-05-05T14:14:52Z

src/main.cpp

@@ -1434,7 +1457,8 @@ extern "C" {
  {
    xbmc->Log(ADDON::LOG_DEBUG, "Open()");

-    const char *lt(""), *lk(""), *ld(""), *lsc("");
+    const char *lt(""), *lk(""), *ld(""), *lsc(""), *manh(""), *medh(""), *m_strURL;


manh / medh are std::map<std::string, std::string>

glennguy · 2017-05-06T12:55:42Z

@peak3d Thank you so much for the feedback, not being picky at all. My apologies that it seems as I don't really know what I'm doing with regards to a lot of the changes, I'm very much a beginner (less than a month) at c++. I am keen to contribute but please say if it is unhelpful at my skill/experience level.

I think I've addressed the issues now. We're now using maps to pass the headers around, and there is a separate function in helpers.cpp to parse them.

matthuisman · 2017-05-07T23:43:56Z

@glennguy @peak3d

Maybe it can drop down to a single download function in main.cpp now?

peak3d · 2017-05-08T07:09:59Z

@glennguy way of programming is different from dev to dev, its like starting a friendship where each other has to figure out how the other one's character is. I really appreciate that you go the way with me, the future is important and the more people contribute the better.
Thanx for your patience!

Pls. leave 2 download methds so far as we do not know what's happening in the future.
My feeling is, that we will be happy to have them separated in the future.

I'll look over the changes today, keep head up :-)

peak3d

Nearly done, great work! Only the 2 minor cosmetics and we're done!

peak3d · 2017-05-11T15:56:35Z

src/main.cpp

 {
  // open the file
  void* file = xbmc->CURLCreate(url);
  if (!file)
    return false;
  xbmc->CURLAddOption(file, XFILE::CURL_OPTION_PROTOCOL, "seekable", "0");
  xbmc->CURLAddOption(file, XFILE::CURL_OPTION_PROTOCOL, "acceptencoding", "gzip");
+  if (!manifestHeaders.empty())


Not needed to request for empty() here.

If you are lazy in writing you could also do:
for (const auto &entry : manifestHeaders)
{

peak3d · 2017-05-11T15:56:53Z

src/main.cpp

  xbmc->CURLAddOption(file, XFILE::CURL_OPTION_HEADER, "Connection", "keep-alive");
  xbmc->CURLAddOption(file, XFILE::CURL_OPTION_PROTOCOL, "acceptencoding", "gzip, deflate");
+  if (!mediaHeaders.empty())


pls. see previous comment

matthuisman · 2017-05-11T22:01:38Z

Great work guys!
Looking forward to this :)

I'm guessing this change will only be in KODI 18.0 builds as this is on agile branch?
(Not back-ported to current 17.1?)

glennguy · 2017-05-12T14:02:13Z

@peak3d all sorted, and changed to 'lazy' for loop. Seems a lot easier to read that way. Thanks for your patience and spoon feeding me some of it, it has helped me learn a lot.

@matthuisman this PR is against the master branch so its for 17.1. I imagine it will get pulled into the agile branch but @peak3d will decide that. I see you mentioned you haven't got a build environment set up for this, let me know if you need some help - I can build for some platforms.

peak3d · 2017-05-12T14:28:18Z

Thanx @glennguy !

priyavadan · 2019-04-13T14:28:19Z

sorry for reviving an old thread, so do we just need to define the stream_headers and that will apply the headers to both media and mpd? I am trying out the X-Forwarded-For passed to the stream headers.

Send custom headers with segment downloads

b19b4f9

peak3d reviewed Apr 27, 2017

View reviewed changes

glennguy added 2 commits May 1, 2017 13:48

Reimplement custom headers using listitem property

19eebc2

Removed previous header implementation

23a1d00

matthuisman reviewed May 1, 2017

View reviewed changes

Remove redundant string

1edf286

Separate manifest and media headers

7e4d29e

peak3d reviewed May 5, 2017

View reviewed changes

Rework headers again using maps

964fbe3

peak3d reviewed May 11, 2017

View reviewed changes

Remove unneccessary condition test

57d1651

peak3d merged commit 7094e24 into xbmc:master May 12, 2017

glennguy deleted the headers branch March 21, 2021 12:12

		@@ -219,6 +229,15 @@ bool KodiAdaptiveStream::download(const char* url, const char* rangeHeader)
		xbmc->CURLAddOption(file, XFILE::CURL_OPTION_HEADER, "Range", rangeHeader);

Send custom headers with segment downloads #25

Send custom headers with segment downloads #25

Conversation

glennguy commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthuisman commented Apr 27, 2017 • edited

peak3d commented Apr 28, 2017 • edited

matthuisman commented Apr 29, 2017 • edited

glennguy commented Apr 30, 2017

peak3d commented Apr 30, 2017 • edited

glennguy commented May 1, 2017

glennguy commented May 1, 2017

matthuisman May 1, 2017 • edited

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

matthuisman commented May 1, 2017 • edited

JinRonin commented May 1, 2017

matthuisman commented May 1, 2017

glennguy commented May 2, 2017

matthuisman commented May 2, 2017

matthuisman commented May 2, 2017

peak3d commented May 3, 2017

peak3d commented May 3, 2017 • edited

matthuisman commented May 3, 2017 • edited

peak3d commented May 4, 2017

glennguy commented May 4, 2017

glennguy commented May 5, 2017

peak3d left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennguy commented May 6, 2017

matthuisman commented May 7, 2017

peak3d commented May 8, 2017

peak3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthuisman commented May 11, 2017

glennguy commented May 12, 2017

peak3d commented May 12, 2017

priyavadan commented Apr 13, 2019

matthuisman commented Apr 27, 2017 •

edited

peak3d commented Apr 28, 2017 •

edited

matthuisman commented Apr 29, 2017 •

edited

peak3d commented Apr 30, 2017 •

edited

matthuisman May 1, 2017 •

edited

matthuisman commented May 1, 2017 •

edited

peak3d commented May 3, 2017 •

edited

matthuisman commented May 3, 2017 •

edited