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

[extractor/vk] playlist fix for new API requirements #6122

Merged
merged 11 commits into from
Feb 13, 2023

Conversation

the-marenga
Copy link
Contributor

IMPORTANT: PRs without the template will be CLOSED

Description of your pull request and other information

As reported here, VK playlist downloads stopped working. The reason is, that their API has been changed yet again and now requires "playlist_ID" instead of just "ID" as a request parameter. With this fix everything works again

Fixes #4930 (comment)

PLEASE FOLLOW THE GUIDE BELOW

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp 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?

@the-marenga the-marenga mentioned this pull request Jan 31, 2023
9 tasks
yt_dlp/extractor/vk.py Outdated Show resolved Hide resolved
@pukkandan
Copy link
Member

Have you tested that the other code branches does not need this change?

@the-marenga
Copy link
Contributor Author

Yes, the other branches are for URLs, that either provide their own valid section id, or download an entire channel 'all'. Both of these are still what VK uses in their requests unchanged.

I did however just notice, that I forgot to update the playlist test, so I fixed that.

When doing so I also noticed, that most of the single video (VKIE) tests either fail for various reasons, or get skipped because the video has been removed. I think they were already failing months ago, but I am not sure. If you want me to, I can just update all of the tests in the extractor (either in this PR, or another one?) to make sure nothing there has been broken by this or an older API change

@pukkandan
Copy link
Member

If you want me to, I can just update all of the tests in the extractor (either in this PR, or another one?) to make sure nothing there has been broken by this or an older API change

Sure, you can do it in same PR

- thumbnails do not need to be jpgs
- removed tests, that tested for restrictions, or features that no longer exist
- added test for clips
- fixed audio extraction for wall posts
@the-marenga
Copy link
Contributor Author

I updated all the tests and it seems like they did in fact change their site somewhere else.

They changed their wallposts audio data to JSON and removed the url encryption, which crashed the previously already non-functioning audio extraction completely. I fixed that, but downloading still returned an error from the 'FFmpegFixupM3u8PP', because the audio stream is mp3 and not the expected aac. I changed the container to mov to hide the error for now. The files would however actually need a simple fixup, as they are currently only playable with a handful of media players. A real fix for this would be to have a url check in the fixup, to ommit the incompatible stuff:

if all(self._needs_fixup(info)):
    if 'vkuseraudio.net' in info['url']:
        self._fixup('Fixing MPEG-TS in MP4 container', info['filepath'], [
            *self.stream_copy_opts(), '-f', 'mp4'])
    else:
        self._fixup('Fixing MPEG-TS in MP4 container', info['filepath'], [
            *self.stream_copy_opts(), '-f', 'mp4', '-bsf:a', 'aac_adtstoasc'])

but that seems like an inelegant way to do this and I am not familiar enough with the codebase to come up with a better alternative. For now you would either have to play the files in something like mpv, or use -x to extract the actual audio manually.

VK also lifted a lot of restrictions to being registered, so a lot of tests for edge cases are no longer required and some don't need to be skipped anymore.

All tests succeed now

yt_dlp/extractor/vk.py Show resolved Hide resolved
yt_dlp/extractor/vk.py Outdated Show resolved Hide resolved
@pukkandan
Copy link
Member

The files would however actually need a simple fixup, as they are currently only playable with a handful of media players.

If it's mp3, what kind of fixup does it need?

yt_dlp/extractor/vk.py Outdated Show resolved Hide resolved
@pukkandan pukkandan added site-bug Issue with a specific website needs-testing Patch needs testing labels Feb 8, 2023
@the-marenga
Copy link
Contributor Author

Great, the files are playable and have no corruptions.
I just changed the author extraction class, because vk changed it within the last few days and updated the tests.
Only thing left would be merging the audio & video entries again when returning, but I couldn't figure out how to append the audio entries to the playlist_from_matches without basically reverting that change from your cleanup back to using 'playlist_result'

@pukkandan
Copy link
Member

Only thing left would be merging the audio & video entries again when returning, but I couldn't figure out how to append the audio entries to the playlist_from_matches without basically reverting that change from your cleanup back to using 'playlist_result'

My bad. Reverted

@the-marenga
Copy link
Contributor Author

When doing final testing, I noticed, that irrelevant reaction clips in the comments will also be picked up. That seemed like a bug to me, so I fixed that by only searching for videos in the actual post body. Otherwise everything should be good

@bashonly bashonly linked an issue Feb 13, 2023 that may be closed by this pull request
10 tasks
@pukkandan pukkandan removed the needs-testing Patch needs testing label Feb 13, 2023
@pukkandan pukkandan merged commit a9c6854 into yt-dlp:master Feb 13, 2023
aalsuwaidi pushed a commit to aalsuwaidi/yt-dlp that referenced this pull request Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
site-bug Issue with a specific website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[VK Video] "Unknown error" when downloading playlist's JSON metadata
2 participants