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

[curl] Accept all supported content encodings by default #15644

Merged
merged 3 commits into from Mar 20, 2019

Conversation

Projects
None yet
6 participants
@candrews
Copy link
Contributor

commented Mar 2, 2019

Description

By setting CURLOPT_ACCEPT_ENCODING to "" by default, curl will request all
supported content encodings.
Users can still use the "Encoding" protocol option to override this
behavior; setting "Encoding" to "" will cause CURLOPT_ACCEPT_ENCODING
to not be set, resulting in curl not accepting any content encoding
(matching the default behavior before).

Motivation and Context

I saw #15628 and wanted to improve upon that proposed solution.
This approach is basically what was suggested by @bol-van in the comment at #15628

How Has This Been Tested?

Compiled on Linux then used for an hour or so, mostly playing Youtube videos, looking at my security camera, and looking at pictures from my web server. I didn't notice any problems.

I setup my web server log the value of incoming request's Accept-Encoding header. With this PR, Kodi now sends deflate, gzip, br as is desired.

Screenshots (if appropriate):

n/a

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed
[curl] Accept all supported content encodings by default
By setting CURLOPT_ACCEPT_ENCODING to "" by default, curl will request all
supported content encodings.
Users can still use the "Encoding" protocol option to override this
behavior; setting "Encoding" to "" will cause CURLOPT_ACCEPT_ENCODING
to not be set, resulting in curl not accepting any content encoding
(matching the default behavior before).
@pkerling

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

@candrews What do you think about removing the few instances in the code that set a specific requested encoding? I think we could get rid of them as part of this PR.

@bol-van

This comment has been minimized.

Copy link

commented Mar 2, 2019

Pls check. Something is not good with brotli in kodi

I tried this list in pvr.iptvsimple
works fine :
http://91.92.66.82/trash/ttv-list/ttv.all.tag.iproxy.m3u|encoding=gzip
kodi hangs, although in wireshark query succeeds :
http://91.92.66.82/trash/ttv-list/ttv.all.tag.iproxy.m3u|encoding=br

In wireshark I can see http query but next packet from client is "FIN, ACK".
Looks like after sending headers kodi immediately closes socket
Everything send by the server is ignored then

Curl works good with --compressed parameter and server returns br response

I tested 18.0 windows version

@candrews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

@candrews What do you think about removing the few instances in the code that set a specific requested encoding? I think we could get rid of them as part of this PR.

Sounds good to me - I found two instances (one in Repository, one in ScraperUrl) that set the encoding and have removed each of them in separate commits that I added.

candrews added some commits Mar 2, 2019

In ScraperUrl, don't set content encoding to gzip explicitly
Instead, rely on the default behavior to request all supported encodings
In Repository, don't set content encoding to gzip explicitly
Instead, rely on the default behavior to request all supported encodings
@bol-van

This comment has been minimized.

Copy link

commented Mar 2, 2019

In linux brotli behaves differently but still not good.
Only 295 channels of 1300+ are loaded.
No names, no anything. Just "unknown channel"'. Kodi parse compressed data ?
libcurl has outdated br lib ?

@candrews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

@bol-van this sounds like a special case of a problem with this particular server - perhaps you should set the url protocol option to only use gzip?

@bol-van

This comment has been minimized.

Copy link

commented Mar 2, 2019

@bol-van this sounds like a special case of a problem with this particular server - perhaps you should set the url protocol option to only use gzip?

Can you offer any example url that works with br ?

@candrews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

I'm staying if this particular server has a problem with br, explicitly request something else.
For example, you said earlier:
http://91.92.66.82/trash/ttv-list/ttv.all.tag.iproxy.m3u|encoding=gzip
Use that and avoid brotli in this case.

Fwiw, I haven't had a problem with any urls I've tried and some have used brotli.

@bol-van

This comment has been minimized.

Copy link

commented Mar 2, 2019

I still think its worth studying why kodi fails with this particular url.
Because chrome, firefox, curl - all download without problems
May be brotli lib is outdated in kodi and thats why it misbehaves

@bol-van

This comment has been minimized.

Copy link

commented Mar 2, 2019

I checked userdata/addon_data/pvr.iptvsimple/iptv.m3u.cache
And this file is not plain. its brotli encoded
I successfully uncompressed it with brotli tools
So kodi fails to decompress result from server and tries to parse compressed data

In case of gzip iptv.m3u.cache is plain. Cache should be always plain.
pvr plugin manages cache. pvr.iptvsimple calls xbmc->readfile. readfile transparently handles everything and returns final uncompressed data.
in case of br it returns compressed data

@candrews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

Did you build Kodi yourself? Are you using the changes from this PR?

@bol-van

This comment has been minimized.

Copy link

commented Mar 2, 2019

I use 18.0 official build in windows and https://launchpad.net/~team-xbmc/+archive/ubuntu/ppa
for ubuntu 16
Trying to run ubuntu 18.10 livecd in vmware for newer curl but it sux. OS hangs..

@candrews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

The official builds don't include brotli support, so that explains the behavior you're seeing :-)

When you forced Kodi to request brotli, it did, then it couldn't decode it.

@bol-van

This comment has been minimized.

Copy link

commented Mar 2, 2019

The official builds don't include brotli support, so that explains the behavior you're seeing :-)

OK, that explains. then by default it should not put "br" to Accept-Encoding.
ubuntu 18.10 kodi 17.6 playlist not downloaded at all with BR

@candrews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

This PR will set Accept-Encoding to whatever curl supports. On my system, curl supports brotli, so it gets included. On yours, it doesn't, so it won't be included.

@pkerling

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

@candrews Did you test the change in CScraperUrl? I guess it should work fine but would be good to be safe here.

@candrews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

I'm using Kodi with this patch and haven't had any issues. Can you suggest something I can do to exercise that path? For example, is playing a YouTube video sufficient?

@pkerling

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

Unfortunately not, sorry. I'd have to look myself where and how this is used. This is also why I'm not completely sure whether it would work or not :-) I guess you'll have to try something with scrapers?

@wsnipex

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

@MilhouseVH can you include this in your builds please?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

@wsnipex will include it in the next nightly, #0304 on Monday.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@bol-van

This comment has been minimized.

Copy link

commented Mar 12, 2019

Ideally code should destinguish what are we getting now : live stream or something else.
May be leave all encodings but disable them if byte range specified ?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

The easiest test for the YouTube problem is to:

  1. Play a video from YouTube (anything from "Popular now" should do)
  2. Seek forwards a couple of minutes
  3. Try to seek in reverse (ie. backwards)

With this PR, every time you seek backwards (ie. Seek(-7), or StepBack) you will only ever seek forwards by the same amount.

With this PR, seeking forwards works only somewhat - it only ever seeks a small amount of time forward (about 10-20 seconds) even though you may attempt to incrementally seek several several minutes ahead. Jumping forward or backward to a specific timecode such as 5:00 also doesn't work.

This behaviour is reproducible on RPi2 and Generic (x86_64) LibreELEC builds with this PR, and also Ubuntu 17.10 with latest Kodi master ac9206b + 15644.

So in summary, seeking both forwards and backwards within a live stream is broken by this PR.

@candrews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

I think what's happening is that Kodi is sending the request with the Accept-Encoding header set to the various encodings (which is what this PR does) and the server is responding with content that's not encoded. However, Kodi currently assumes that if it requested encoded content, then the response is encoded - but that's not correct in this case.

My PR at #15643 fixes that problem by looking at the response (not the request!) to determine if the response is encoded.

I won't be able to test this myself until later tonight, but if someone else wants to test using both this PR and #15643 to see if the live stream problem is solved, I'd greatly appreciate it :)

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@candrews spot on, with #15643 now merged this PR seems to be working perfectly with YouTube content. I'll continue to include this PR in nightlies for feedback from those that reported issues, but it's looking much better now. Thanks!

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Both negative reports are now positive with the latest nightly build.

@candrews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Can this now be merged?

Thanks again!

@Rechi

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

If now all supported encodings are accepted by default, is there any use case where one would want to limit the accepted encodings?

else if (name == "acceptencoding" || name == "encoding")
SetAcceptEncoding(value);

@bol-van

This comment has been minimized.

Copy link

commented Mar 18, 2019

Who knows. There may be misbehaving servers. I see no reason in removing already present functionality such as appending "|encoding=" to url. It can be used as a quick fix

@wsnipex

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

I'd keep it as well, at least for v19

@candrews

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Shall this be merged then?

@pkerling pkerling merged commit 836ad54 into xbmc:master Mar 20, 2019

1 check passed

default You're awesome. Have a cookie
Details
@pkerling

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Didn't seem like there were any objections, thanks for contributing 🎉

@Rechi Rechi added this to the Leia 18.2-rc1 milestone Mar 20, 2019

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