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

[zoomus] Add new extractor #24751

Closed
wants to merge 7 commits into from
Closed

Conversation

Romern
Copy link

@Romern Romern commented Apr 12, 2020

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:

  • 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 is an extractor for Zoom.us recordings. Example link taken from here: #16597 .
Other related Issues: #23573 , #24643 , #16597

I don't know how to get the tests working, somehow the md5 is None, even though the file is being downloaded when using --test manually.

@dstftw
Copy link
Collaborator

dstftw commented Apr 13, 2020

Read coding conventions.

@Romern
Copy link
Author

Romern commented Apr 13, 2020

@dstftw I have adjusted the regexes to ignore non important fields and some other code convention stuff.
EDIT: (accidentaly @ wrong person, sorry)

@sheedy
Copy link

sheedy commented Apr 21, 2020

@Romern Does this PR avoid you having to open 2 separate tabs to dig out the request headers like I outlined here (#23573)?

@Romern
Copy link
Author

Romern commented Apr 21, 2020

@sheedy You simply drop the https://zoom.us/recording/play/* link into youtube-dl and add a password if required.

@SimonPickup
Copy link

Works well for me. Thanks @Romern

@proton1k
Copy link

proton1k commented Jun 2, 2020

Hello @Romern,
cloning your repository and running with developer suggestion with python -m youtube_dl but it does not find the right extractor.

J-58_: Requesting header
WARNING: Falling back on generic information extractor.

am I doing something incorrectly? I can help to debug or in the extractor itself.

@Romern
Copy link
Author

Romern commented Jun 2, 2020

@o1dnik It seems like your link does not match the pattern, could you post it? If its sensitive, you can censor the id.

@proton1k
Copy link

proton1k commented Jun 2, 2020

I'm not sure, but it should be publicly available. This is a TheEconomist magazine video recording
https://economist.zoom.us/rec/play/7JV4Ibuqrjg3S9XEtgSDBvYoW9XsfKqs1HIfq_oEmRq0VnlSNVChZ7AVNOKcIcCsGRw-mFPuwGMJ-58_

@Romern
Copy link
Author

Romern commented Jun 2, 2020

@o1dnik it should now work for arbitrary long ids.

@proton1k
Copy link

proton1k commented Jun 2, 2020

@Romern I confirm, works like charming! 👍

Copy link

@kcrkor kcrkor left a comment

Choose a reason for hiding this comment

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

The regex is not working. It seems there are unescaped slashes that python doesn't like. The correct one should look like this: https:\/\/(?:.*).?zoom.us\/rec(?:ording)?\/play\/(?P<id>[A-Za-z0-9\-_]+)

PS: Great work @Romern ! With the fixed regex it works!

@Romern
Copy link
Author

Romern commented Jun 4, 2020

@kcrkor In python you do not need to escape forward-slashes. On regex101.com you an set the regex-flavor to python and the current one works fine.

@karbassi
Copy link

karbassi commented Aug 4, 2020

@Romern Tested this with password and it works well.

@kcrkor thoughts on merging?

@kcrkor
Copy link

kcrkor commented Aug 5, 2020

@karbassi Looks good to me.

@karbassi
Copy link

karbassi commented Aug 5, 2020

@dstftw thoughts on merging?

@Romern
Copy link
Author

Romern commented Nov 16, 2020

Continued here: #27002

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

Successfully merging this pull request may close these issues.

7 participants