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

[Misc] Under-the-hood fixes, improvements and tests (pt 2) #32481

Merged
merged 3 commits into from
Jul 29, 2023

Conversation

dirkf
Copy link
Contributor

@dirkf dirkf commented Jul 28, 2023

Boilerplate: own/yt-dlp code, bug fix/improvement ## Please follow the guide below
  • You will be asked some questions, please read them carefully and answer honestly
  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])
  • Use Preview tab to see how your pull request will actually look like

Before submitting a pull request make sure you have:

In order to be accepted and merged into youtube-dl each piece of code must be in public domain or released under Unlicense. Check one of the following options:

  • Except for code taken from yt-dlp for which either this or the below has already been asserted, I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

  • Bug fix
  • Improvement
  • New extractor
  • New feature

Description of your pull request and other information

This PR encapsulates various under-the-hood fixes, improvements and tests.

* avoid Node 12 deprecation
* support nested encodings
* support optional `br` encoding, if brotli package is installed
* support optional 'compress' encoding, if ncompress package is installed
* response `Content-Encoding` has only unprocessed encodings, or removed
* response `Content-Length` is decoded length (usable for filesize metadata)
* use zlib for both deflate and gzip decompression
* some elements taken from yt-dlp: thx especially coletdjnz
* move processing to YoutubeDLHandler
* also process `Location` header for redirect
* use tests from yt-dlp/yt-dlp#7662
@dirkf dirkf merged commit abef534 into ytdl-org:master Jul 29, 2023
14 checks passed
Comment on lines +3209 to +3213
# PyPi ncompress package implements 'compress' Content-Encoding
try:
import ncompress as compat_ncompress
except ImportError:
compat_ncompress = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this ever been observed in the wild? I'm against adding a whole new dependency for a use case that's never encountered.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding indicates compress has been deprecated for decades

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It says:

... this content-encoding is not used by many browsers today ...

Not by Mozilla, eg. One guy on the Web seemed to want it, though it's hardly a yt-dl application and it was indeed more than two decades ago.

However as compress is in RFC 9110 and easy* to support, I included it.

It's a fairly trivial point since encoding is negotiated and compress is arguably a different case from br (Myspace vs Instagram). We would never have seen a bug report if some site offered compress but not gzip or deflate. You could surely be absolved if yt-dlp failed to reflect this feature, even given the slicker dependency system.

Separately, I wonder what sites do today if the client says Accept-Encoding: identity (so no encoding). Would that provoke/alleviate fingerprinting issues?

* Considering negotiation, the PR doesn't advertise any optional encodings, though, since I forgot to include that change.

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.

2 participants