-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Update video_id regexp to account for recent abc.com changes (fixes #25197 and #25207) #25216
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified this fix allows downloading from abc.com. I reviewed the code change, it looks fine to me.
@@ -138,7 +138,7 @@ def _real_extract(self, url): | |||
# from http://freeform.go.com/shows/shadowhunters/episodes/season-2/1-this-guilty-blood | |||
r'data-video-id=["\']*(VDKA\w+)', | |||
# https://abc.com/shows/the-rookie/episode-guide/season-02/03-the-bet | |||
r'\b(?:video)?id["\']\s*:\s*["\'](VDKA\w+)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not touch old patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not minding my own business, but it looks like your requested change was implemented by @tmthywynn8 a couple of months ago. I'm not that familiar with the GitHub workflow, but it looks like this is just waiting for your approval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I made the necessary changes in commit 783d296 and requested another review to make sure that I fulfilled the requirements. I'm not sure what the procedure is suppose to be, so maybe requesting another review was the wrong thing to do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure yet but I think removing the original pattern (by replacing it with the new pattern) fixed the problem, and putting it back has re-broken it. My suspicion is that both patterns are matched on the page in question but one returns an invalid ID. Or perhaps a valid ID that nevertheless yields an error when the code later tries to retrieve it.
If I'm right, undoing the requested change will get it working again. But that's not really a solution, and again I'm not sure yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me so long to get to this, but I just tested #25216 and it works for me as-is. So my theory above is completely wrong.
Leaves original pattern untouched.
the fix still has not been added to youtube-dl. what is going on? |
May I respectfully inquire as to what is delaying this fix? ABC is a very popular site and has the habit of reducing retention of free and logged-in content at seemingly random times. A timely fix would be most welcome. I am assuming (possibly incorrectly) that dstftw's request is the one we are waiting on, as he has been handling this issue. |
Could we please get this merged? |
The problem has changed. I was stupid enough to not check logs until today. I'm no longer getting the Adobe Pass error. Now I'm getting "ExtractorError: Go said: 1025:Unable to retrieve required files for playback. Please try again in a few minutes." |
5e26784
to
da2069f
Compare
What's going on with this PR? This has been implemented in youtube-dlc months ago: blackjack4494#149 (comment) |
It will not be merged until maintainers fly to US, purchase a cable package, and test it themselves. And no, I'm not joking (well maybe a little bit). |
Would SSH access to a Raspberry Pi on a US ISP work? |
Don't you need to play the vids too ? |
I think this could be handled by rsync or X11 forwarding (depending on how testing would need to get done). Or you know, whoever provides the SSH endpoint could also make sure the mp4 (or whatever) file could make sure the file is playable. |
I have a subscription and would be willing to provide any needed access to the maintainers. I'm located in the US. |
I don't have the login, but I also have a residential IP address in the United States. |
* https://github.com/ytdl-org/youtube-dl: [youtube] Remove unused code [go] Improve video id extraction (closes ytdl-org#25207, closes ytdl-org#25216, closes ytdl-org#26058) [test_execution] Add test for lazy extractors (refs ytdl-org#28780) [test_youtube_misc] Move YoutubeIE.extract_id test into separate module [youtube] Fix lazy extractors (closes ytdl-org#28780) [bbc] Extract full description from __INITIAL_DATA__ (refs ytdl-org#28774) [bbc] Extract description and timestamp from __INITIAL_DATA__ (ytdl-org#28774)
Please follow the guide below
x
into all the boxes [ ] relevant to your pull request (like that [x])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:
What is the purpose of your pull request?
Description of your pull request and other information
This pull request fixes #25197 and #25207 by accounting for the new changes on abc.com—first noticed on Wednesday, May 6. Now, the unique video ID can be found by searching for videoIdCode; note that only one instance will be found), potentially avoiding any false positives, e.g., other video IDs referenced on the page.
The solution was to change the regexp on line 141 and simplifying it for the new name on the video's source. The Rookie video in the examples dictionary can be used to verify the new changes.