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

[Epidemic Sound] Add new extractor #32628

Merged
merged 7 commits into from
Dec 6, 2023

Conversation

realRobotix
Copy link
Contributor

@realRobotix realRobotix commented Nov 6, 2023

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

see #31462

@dirkf dirkf linked an issue Nov 28, 2023 that may be closed by this pull request
5 tasks
Copy link
Contributor

@dirkf dirkf left a comment

Choose a reason for hiding this comment

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

Thanks for your work!

I've made a couple of suggestions. Please consider those and check the result of the CI tests too (NB the tests may appear to succeed even if individual extractor tests fail).

youtube_dl/extractor/epidemicsound.py Outdated Show resolved Hide resolved
youtube_dl/extractor/epidemicsound.py Outdated Show resolved Hide resolved
@realRobotix
Copy link
Contributor Author

realRobotix commented Nov 29, 2023

The Extractor is now using traverse_obj and type checking and I also added support for downloading separate tracks.
Example: https://www.epidemicsound.com/track/mj8GTTwsZd/ has separate tracks for Melody, Instruments, Bass, Drums and a Full Mix.

Copy link
Contributor

@dirkf dirkf left a comment

Choose a reason for hiding this comment

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

With the new suggestions (etc) implemented I get this

$ python -m youtube_dl -F 'https://www.epidemicsound.com/track/mj8GTTwsZd/'
[EpidemicSound] mj8GTTwsZd: Downloading JSON metadata
[info] Available formats for 148700:
format code  extension  resolution note
bass         mp3        unknown    track ID 433551 
drums        mp3        unknown    track ID 433547 
instruments  mp3        unknown    track ID 433550 
melody       mp3        unknown    track ID 433548 
full         mp3        unknown    track ID 433549  (best)
$

What do you think?

youtube_dl/extractor/epidemicsound.py Outdated Show resolved Hide resolved
youtube_dl/extractor/epidemicsound.py Show resolved Hide resolved
'url': 'https://www.epidemicsound.com/track/mj8GTTwsZd/',
'md5': 'c82b745890f9baf18dc2f8d568ee3830',
'info_dict': {
'id': 'mj8GTTwsZd',
Copy link
Contributor

Choose a reason for hiding this comment

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

Now you prompted me to look at the JSON!

Should id be the id from the JSON? If so return the video_id as display_id.

Also:

  • isExplicit could be used to set age_limit
  • releaseDate could be used to set release_timestamp
  • genres could be used to set categories
  • cover could be a fallback for imageUrl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why a user would want the id of the json. I'd just leave it out for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ID is meant to be a unique value that specifies the video (1-1 mapping). We can't really tell if the slug in the URL is going to be unique, but the numeric ID in the JSON looks like an internal index that ought to be unique. Putting the slug as display_id is consistent with the way other extractors work.

Here's another thing, though. Will people normally want the full version only, or the whole set of instrumental tracks? If the latter, the extractor should return all the formats as a playlist, and then the user would have to use --no-playlist to force the current behaviour. Or with the current behaviour, you can get multiple formats with comma-separated formats, like -f "full,drums", but the user has to list the formats explicitly as there's no -f all.

youtube_dl/extractor/epidemicsound.py Show resolved Hide resolved
youtube_dl/extractor/epidemicsound.py Outdated Show resolved Hide resolved
youtube_dl/extractor/epidemicsound.py Outdated Show resolved Hide resolved
youtube_dl/extractor/epidemicsound.py Show resolved Hide resolved
@realRobotix
Copy link
Contributor Author

this should be ready to merge now

Copy link
Contributor

@dirkf dirkf left a comment

Choose a reason for hiding this comment

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

As you say, just about ready to merge, but see the 3 points I've listed.

youtube_dl/extractor/epidemicsound.py Outdated Show resolved Hide resolved
youtube_dl/extractor/epidemicsound.py Show resolved Hide resolved
'url': 'https://www.epidemicsound.com/track/mj8GTTwsZd/',
'md5': 'c82b745890f9baf18dc2f8d568ee3830',
'info_dict': {
'id': 'mj8GTTwsZd',
Copy link
Contributor

Choose a reason for hiding this comment

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

The ID is meant to be a unique value that specifies the video (1-1 mapping). We can't really tell if the slug in the URL is going to be unique, but the numeric ID in the JSON looks like an internal index that ought to be unique. Putting the slug as display_id is consistent with the way other extractors work.

Here's another thing, though. Will people normally want the full version only, or the whole set of instrumental tracks? If the latter, the extractor should return all the formats as a playlist, and then the user would have to use --no-playlist to force the current behaviour. Or with the current behaviour, you can get multiple formats with comma-separated formats, like -f "full,drums", but the user has to list the formats explicitly as there's no -f all.

@realRobotix
Copy link
Contributor Author

I think most people are just interested in the full mix but there is an issue with downloading multiple formats at the same time because the filenames are all the same. What is the recommended way to fix this? Just append the format to the title?

@dirkf
Copy link
Contributor

dirkf commented Dec 5, 2023

To get distinct filenames, the user must specify an output template containing %(format_id) (or possibly %(autonumber) - can't recall ATM whether that increments for the same video ID). The default template just depends on the ID and title, which have to be the same for all formats.

If we return a playlist, the ID and/or title could be varied per track.

@realRobotix
Copy link
Contributor Author

Ok sure, but I still think most people just want the full version and having to specify that every time would be unnecessary. Should be ready to finally merge now.

@dirkf dirkf merged commit b1bbc1e into ytdl-org:master Dec 6, 2023
14 checks passed
@dirkf
Copy link
Contributor

dirkf commented Dec 6, 2023

And thanks again!

github-actions bot added a commit to hellopony/youtube-dl that referenced this pull request Dec 6, 2023
* https://github.com/ytdl-org/youtube-dl:
  [core] Fix format string injection for metadata JSON filename message.
  [Epidemic Sound] Add new extractor (ytdl-org#32628)
  [Imgur] Overhaul extractor module (ytdl-org#32612)
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.

Epidemic Sound
2 participants