Vorbis & Opus support #769

Sigill · 2020-06-20T21:50:08Z

PR ritiek/spotify-downloader#292 by @BeeeWall already attempted to bring support for Vorbis and Opus as output formats, but some recent refactoring caused it to not be easily rebaseable.

I therefore restarted from scratch.

BeeeWall · 2020-06-20T21:52:52Z

I had completely forgotten I had even made that PR... I suppose I should close it in favor of this.

ritiek · 2020-06-21T12:43:13Z

spotdl/encode/tests/test_encode_base.py

@@ -92,6 +94,8 @@ def test_encoder_not_found_error(self):
        ("mp3", "mp3"),
        ("opus", "opus"),
        ("flac", "flac"),
+        ("ogg", "ogg"),
+        ("opus", "opus"),


We don't need another entry for opus here. A previous one is already defined few lines above.

Oh, I didn't event see it.
Was there already a support for Opus?

Yes, kinda. YouTube offers two audiostreams. One with the opus codec and the other with m4a codec. These input formats also need to be recognized by the encoder (along with the possible output formats), which is why the entry already exists. But now with your PR, we'll also be able to output to opus irrespective of whether the input format is or not opus, which is a good thing.

ritiek · 2020-06-21T12:46:17Z

Hello and thanks a lot for the PR! This looks pretty good as is but I'll check it in detail after some time, just in case.

ritiek · 2020-06-21T12:48:04Z

Dunno why the Travis build isn't running though.

ritiek · 2020-06-21T12:49:38Z

CHANGES.md

@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 The release dates mentioned follow the format `DD-MM-YYYY`.

 ## [Unreleased]
+- Add support for Vorbis & Opus output formats. ([@Sigill](https://github.com/Sigill)) (#769)


This needs to go under Added subsection (you'll have to create it) for consistency (like past releases).

ritiek · 2020-06-21T14:42:55Z

spotdl/metadata/embedders/default_embedder.py

        if metadata["year"]:
            audiofile["year"] = metadata["year"]
        provider = metadata["provider"]
        audiofile["comment"] = metadata["external_urls"][provider]
        if metadata["lyrics"]:
            audiofile["lyrics"] = metadata["lyrics"]

+    def _embed_mpb_picture(self, audiofile, metadata, cached_albumart, encoding):


Let's make cached_albumart default to None here too like with other methods.

I actually think it is better this way.
Since _embed_mpb_picture is an internal method, supposed to be called from one with a default value, it does not look necessary to add the default value.
This syntax is also consistent with _embed_basic_metadata, where the input parameters (audiofile, metadata, cached_albumart) stay separated from the extra encoding parameter, which plays a different "role".

Yep, I think that makes sense given this is an internal method. Alrighty, let's leave it as it is then!

ritiek · 2020-06-21T14:46:12Z

spotdl/metadata/embedders/default_embedder.py

+        elif encoding == "ogg" or encoding == "opus":
+            # From the Mutagen docs (https://mutagen.readthedocs.io/en/latest/user/vcomment.html)
+            image_data = image.write()
+            encoded_data = base64.b64encode(image_data)
+            vcomment_value = encoded_data.decode("ascii")
+            audiofile["metadata_block_picture"] = [vcomment_value]


Are you sure this really works? I tried downloading both in ogg and opus formats but the media seemed to contain no albumart when I checked with mpv or FFmpeg (though other metadata like title, artist, etc. seems to exist).

I believe it works, yes.
I just downloaded the same playlist in flac, ogg and opus, and in all formats, all the tracks had an albumart that VLC, FFMpeg & Easytag are able to display.
Are you sure that what you downloaded contains an albumart in the first place?

Are you sure that what you downloaded contains an albumart in the first place?

Yup, I'm pretty sure:

$ spotdl -s https://open.spotify.com/track/4Sfa7hdVkqlM8UW5LsSY3F -o opus

That shouldn't matter though as all tracks will atleast always use the albumart from YouTube if the corresponding albumart is not found on Spotfiy, which means the albumart should be present in every case (as long as --no-metadata isn't passed).

I'll check around to see what's up here.

Seems like my FFmpeg is having trouble (mpv probably fails too because it uses FFmpeg) reading the ablumart metadata. The albumart shows up fine on my Android with Pulsar app. 👍

Sigill · 2020-06-21T15:46:15Z

Do you prefer the option be named ogg or vorbis?
I used ogg without really thinking about it, but since both Vorbis & Opus use the same Ogg container, it would make sense to use vorbis instead.

ritiek · 2020-06-21T15:52:55Z

I see but let's keep it as ogg so that it is more about the file extension rather than the encoding.

ritiek

LGTM. Thanks again for the great contribution @Sigill!

mrkumar009 · 2020-06-23T07:49:59Z

@ritiek you're trying to view ogg/opus album art with ffmpeg, ffmpeg doesn't have support for that. It's a ffmpeg issue. Other media players can show ogg/opus album art.
For the PR, re-encoding could be done better, like I mentioned in my previous PR #678.
I don't know why @ritiek doesn't consider anything beyond his knowledge, sometimes there could be a good advice.
Why is still double entry of opus exists in PR, and you merged it already.

Sigill · 2020-06-23T08:18:18Z

My ffmpeg is able to report albumarts for ogg & opus.
This might just be a version issue.

Why is still double entry of opus exists in PR, and you merged it already.

Oh, my bad, I though there was the single spot that @ritiek mentioned, but there are two other duplicates. I'll fix that.

I saw the comments on the previous PR about re-encoding.
I didn't discussed anything on that subject because I thought the proposed solution was good enough for a first integration, but I'll happily iterate to integrate something better.
Can you clarify how this can be done better?

I think I agree with @ritiek (I'm not an audio expert, though, feel free to correct me), when doing opus -> opus, there's no benefit from re-encoding. Same goes from upsampling.
But I believe that if we want to satisfy everybody, we will have to expose most of the encoding parameters:

bitrate
sampling
CBR/VBR/ABR
maybe others...

ritiek · 2020-06-23T10:14:39Z

@ritiek you're trying to view ogg/opus album art with ffmpeg, ffmpeg doesn't have support for that. It's a ffmpeg issue. Other media players can show ogg/opus album art.

@Sigill has already addressed this:

My ffmpeg is able to report albumarts for ogg & opus.
This might just be a version issue.

For the PR, re-encoding could be done better, like I mentioned in my previous PR #678.

I see. I didn't pay much attention to the changes in your PR back then. I didn't realize that there was a difference between the sampling rate between the audio codecs provided by YouTube. You seem to have also posted a log of ffprobe in https://github.com/ritiek/spotify-downloader/issues/620#issuecomment-549136409 about the stream being 44100Hz but apparently it wasn't explicit enough to have caught my attention. Sorry about that.

If you like you can make a new PR for the sampling rate concerns.

Why is still double entry of opus exists in PR, and you merged it already.

Oh, my bad, I though there was the single spot that @ritiek mentioned, but there are two other duplicates. I'll fix that.

That was an oversight on my behalf as well.

I don't know why @ritiek doesn't consider anything beyond his knowledge, sometimes there could be a good advice.

Yea, he's really bad.

mrkumar009 · 2020-06-23T10:25:42Z

My ffmpeg is able to report albumarts for ogg & opus.
This might just be a version issue.

It shows just in command line as a vorbis comment (metadata_block_picture), that is ascii (b64 encoded image).
What I meant is that ffmpeg is unable to convert and display album art picture as video stream like mp3. That is why mpv doesn't show album art for ogg while for mp3 it does.

when doing opus -> opus, there's no benefit from re-encoding.

That is what I'm saying too. By default re-encoding should be avoided as much as possible to not to lose quality.

Same goes from upsampling.

https://github.com/Sigill/spotify-downloader/blob/c8bbc7ad1902eae14bd661846299edca5cf7c7bb/spotdl/encode/encoders/ffmpeg.py#L15-L19

here it is sampling 44kHz AAC stream to 48kHz mp3, same with FLAC in line 19. FLAC supports 44kHz.

If you like you can make a new PR for the sampling rate concerns.

I thought that you would change that in refactoring, It is just small change. If you are not sure, I will be happy to make a PR.

Yea, he's really bad.

Nah, you just said you missed my comment log. I think you're just too busy.

ritiek · 2020-06-23T10:31:40Z

If you are not sure, I will be happy to make a PR.

Alright, I'll be glad to merge it. By the way, do we really need to explicitly specify the output sample rate to FFmpeg? Shouldn't FFmpeg automatically be able to determine the output sample rate from sample rate in the input audio? I just tried and FFmpeg seems to keep the sample rate for the output audio same as in the input audio. Is this supposed be the right way?

mrkumar009 · 2020-06-23T11:15:59Z

Yes ffmpeg keeps sample rate same, if codec supports it. Otherwise it will automatically change to supported sampling rate.
If you convert a m4a 44100 Hz to opus, it will automatically changes to 48000 Hz.
So it will be wise to leave that to just ffmpeg, but I would say we should change bitrate option for some codecs that can use variable bit rate for quality, like vorbis uses quality setting from -1 to 10. It accepts -q:a or -aq parameters for encoding. -q:a 5 generates around 160k bitrate file which is almost transparent.

ritiek · 2020-06-23T11:43:09Z

Sounds good to me.

ritiek · 2020-06-26T10:52:21Z

Just a gentle ping guys. Are you up on making PRs?

Why is still double entry of opus exists in PR, and you merged it already.

Oh, my bad, I though there was the single spot that @ritiek mentioned, but there are two other duplicates. I'll fix that.

@Sigill

If you are not sure, I will be happy to make a PR.

@mrkumar009

If you're busy atm, let me know so I'll do it myself.

Sigill · 2020-06-26T11:38:09Z

I'm AFK for the weekend, feel free to give it a go. Le ven. 26 juin 2020 à 12:52, Ritiek Malhotra <notifications@github.com> a écrit :

…

Just a gentle ping guys. Are you up on making PRs? Why is still double entry of opus exists in PR, and you merged it already. Oh, my bad, I though there was the single spot that @ritiek <https://github.com/ritiek> mentioned, but there are two other duplicates. I'll fix that. @Sigill <https://github.com/Sigill> If you are not sure, I will be happy to make a PR. @mrkumar009 <https://github.com/mrkumar009> If you're busy atm, let me know so I'll do it myself. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/ritiek/spotify-downloader/pull/769#issuecomment-650116917>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR4YMIAMGMQFAGHSYSSPE3RYR4XFANCNFSM4ODR2CJA> .

As mentioned in #769.

FFmpeg implicitly outputs media at the desired sample rate. So, there is no need to explicitly specify a sample rate. This also eliminates the possibility where the original sample rate may differ drastically from the output sample rate. Also refer to #769.

As mentioned in #769.

FFmpeg implicitly outputs media at the desired sample rate. So, there is no need to explicitly specify a sample rate. This also eliminates the possibility where the original sample rate may differ drastically from the output sample rate. Also refer to #769.

BeeeWall mentioned this pull request Jun 20, 2020

Add support for Ogg (Opus and Vorbis) #292

Closed

ritiek reviewed Jun 21, 2020

View reviewed changes

Sigill force-pushed the vorbis_opus_support branch from 5eee174 to 71b9742 Compare June 21, 2020 13:05

ritiek reviewed Jun 21, 2020

View reviewed changes

Sigill added 3 commits June 21, 2020 17:41

Add support for Vorbis output format

a81f83d

Add support for Opus output format

c8bbc7a

Update Changelog

419e80e

Sigill force-pushed the vorbis_opus_support branch from 71b9742 to 419e80e Compare June 21, 2020 15:42

ritiek approved these changes Jun 21, 2020

View reviewed changes

ritiek merged commit 91a7386 into spotDL:master Jun 21, 2020

ritiek added a commit that referenced this pull request Jun 29, 2020

Remove duplicate opus entries

617a706

As mentioned in #769.

Sigill deleted the vorbis_opus_support branch June 30, 2020 19:17

ritiek added a commit that referenced this pull request Jul 1, 2020

Remove duplicate opus entries

ef5cbe1

As mentioned in #769.

ritiek mentioned this pull request Jul 1, 2020

FFmpeg fixes #776

Merged

ritiek added this to the v2.2.0 milestone Jul 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vorbis & Opus support #769

Vorbis & Opus support #769

Sigill commented Jun 20, 2020

BeeeWall commented Jun 20, 2020

ritiek Jun 21, 2020

Sigill Jun 21, 2020

ritiek Jun 21, 2020

ritiek commented Jun 21, 2020

ritiek commented Jun 21, 2020

ritiek Jun 21, 2020 •

edited

Loading

Sigill Jun 21, 2020

ritiek Jun 21, 2020

Sigill Jun 21, 2020 •

edited

Loading

ritiek Jun 21, 2020

ritiek Jun 21, 2020

Sigill Jun 21, 2020

ritiek Jun 21, 2020

ritiek Jun 21, 2020

Sigill commented Jun 21, 2020

ritiek commented Jun 21, 2020

ritiek left a comment

mrkumar009 commented Jun 23, 2020

Sigill commented Jun 23, 2020

ritiek commented Jun 23, 2020

mrkumar009 commented Jun 23, 2020

ritiek commented Jun 23, 2020

mrkumar009 commented Jun 23, 2020

ritiek commented Jun 23, 2020

ritiek commented Jun 26, 2020

Sigill commented Jun 26, 2020 via email

Vorbis & Opus support #769

Vorbis & Opus support #769

Conversation

Sigill commented Jun 20, 2020

BeeeWall commented Jun 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritiek commented Jun 21, 2020

ritiek commented Jun 21, 2020

ritiek Jun 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sigill Jun 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sigill commented Jun 21, 2020

ritiek commented Jun 21, 2020

ritiek left a comment

Choose a reason for hiding this comment

mrkumar009 commented Jun 23, 2020

Sigill commented Jun 23, 2020

ritiek commented Jun 23, 2020

mrkumar009 commented Jun 23, 2020

ritiek commented Jun 23, 2020

mrkumar009 commented Jun 23, 2020

ritiek commented Jun 23, 2020

ritiek commented Jun 26, 2020

Sigill commented Jun 26, 2020 via email

ritiek Jun 21, 2020 •

edited

Loading

Sigill Jun 21, 2020 •

edited

Loading