[glennguy]

Hi @peak3d

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]

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)

[glennguy]

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]

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]

@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]

@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]

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]

@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]

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

[glennguy]

@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]

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

[glennguy]

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.

[matthuisman]

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

[matthuisman]

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;

[glennguy]

Right you are. Removed.

[matthuisman]

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]

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

[matthuisman]

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

[glennguy]

@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]

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]

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

[peak3d]

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

[peak3d]

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]

@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]

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]

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

[glennguy]

@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]

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]

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

[peak3d]

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

[peak3d]

std::map

[peak3d]

const std::map &

[peak3d]

obsolete if Range is in headerMap

[peak3d]

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]

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

[peak3d]

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]

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

[glennguy]

@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]

@glennguy @peak3d

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

[peak3d]

@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]

Not needed to request for empty() here.

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

[peak3d]

pls. see previous comment

[matthuisman]

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]

@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]

Thanx @glennguy !

[priyavadan]

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.