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

Support per-stream proxies in HLS streams (attempt 4) #9412

Closed
wants to merge 13 commits into from

Conversation

jhol
Copy link
Contributor

@jhol jhol commented Mar 21, 2016

This branch supercedes #9398, #9240 and #8680, providing another attempt to add the ability for plugins to provide per-stream proxy configuration, including support for proxies with HLS.

The flow of URL information from plugins to HTTP requests for HLS streams is as follows:

[Plugin]
  -> CListItem
      -> CFileItem
          -> CDVDInputStream/CDVDInputStreamFFmpeg
              -> CPlayListM3U::GetBestBandwidthStream
                  -> CFile::Open
                      -> IFile/CCurlFile
                          -> libcurl
                              [HTTP Request playlist M3U8 file]

          -> CDVDInputStream
              -> CDVDDemuxFFmpeg
                  -> libffmpeg
                      [HTTP Request media chunks]

To enable per-stream proxy configuration, the five config values (proxy protocol, host, port, user-name and password) must follow the same path-ways.

  • The proxy configurations are carried by five new CFileItem properties: proxy.type, proxy.host, proxy.port, proxy.user and proxy.password.
  • Accessor methods (GetProxyType etc.) have been added to CDVDInputStream, which expose these property values.
  • enum CCurlFile::ProxyType has been moved to a common header so that it can be used in code beyond CCurlFile
  • CPlayListM3U::GetBestBandwidthStream has been moved to CDVDInputStreamFFmpeg, so that it can gain access to the proxy information from CDVDInputStream.
  • Use of the CFile::Open factory mechanism has been removed from this method. Instead CCurlFile is created directly, so that the proxy values can be applied to the object.
  • CDVDDemuxFFmpeg has access to CDVDInputStream, and adds the http_proxy string to the options dictionary if one has been specified.
  • Various other tidying patches are included.

This conversion also has the side-effect of stripping the proxy
configuration.
Formatting the CURL as a string, then parsing it back to a CURL
is redundant, and has the effect of stripping any additional CURL
object metadata.
Nothing attempts to override this method, and having it be
overridable serves no clear purpose.
@jhol
Copy link
Contributor Author

jhol commented Mar 21, 2016

@FernetMenta Any better this time? There is no CProxy class in this PR.

Now that the ProxyType enum has been moved to Proxy.h, there is a
risk that any changes to that structure could be made without
updating the proxyType2CUrlProxyType lookup table, possibly
creating a buffer overrun case.

Doing the conversion with a switch-case mitigates against this.
Since v3.0, FFmpeg has supported the http_proxy option for the HTTP
protocol.

Previously, CDVDDemuxFFmpeg would pass Kodi httpproxy CURL protocol
option through as a HTTP header, which the server would (hopefully)
ignore. With this change, if Kodi is built with an older version of
FFmpeg, FFmpeg will simply ignore the option - which is an
improvement over sending spurious HTTP headers.
@@ -166,10 +166,8 @@ bool CFileCache::Open(const CURL& url)

CLog::Log(LOGDEBUG,"CFileCache::Open - opening <%s> using cache", url.GetFileName().c_str());

m_sourcePath = url.Get();

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@FernetMenta
Copy link
Contributor

see comments. we are getting closer.

@@ -42,26 +48,23 @@ CDVDInputStreamFFmpeg::~CDVDInputStreamFFmpeg()

bool CDVDInputStreamFFmpeg::IsEOF()
{
if(m_aborted)

This comment was marked as spam.

@jhol
Copy link
Contributor Author

jhol commented Mar 22, 2016

Superceded by #9421

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants