Allow to transcode a file on-the-fly and download it from the web ui #2489

antlarr · 2017-03-23T18:03:08Z

This commit adds two config options to the web plugin:
convert_extension and convert_command.

When convert_extension is defined (for example, as 'mp3'), a new
option "convert and download as mp3" is shown next to the download link
for files that are not already in mp3 format.

This also adds a config global variable to the javascript code, which
is filled with data obtained from the new /config route, which returns
configuration values useful in the UI (so far, only convert_extension).

The code to convert music to another format was mainly copied
from the convert plugin, and put in a new /item/<item_id>/converted_file
route. The convert command used by default is the same as in the convert
plugin too, but can be configured with the convert_command option.

To enable this, just define in the configuration:

web:
    convert_extension: mp3

And search for any non-mp3 file in the web plugin.

mention-bot · 2017-03-23T18:03:09Z

@antlarr, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sampsyo, @geigerzaehler and @obiesmans to be potential reviewers.

antlarr · 2017-03-23T18:11:43Z

Btw, as part of the tests I did for this I tried downloading (and converting) files with unicode characters in their filenames (like ／ or á). That fails due to a problem in flask, so I also submitted pallets/flask#2223 to fix it properly and get correct filenames generated in all flask applications. If that PR is not accepted for some reason in flask, I'll submit another (not so nice) fix here. Note that the problem with unicode characters is not introduced by this PR, but also happens with the regular download link.

Freso

I've only got stylistic comments to add…

Freso · 2017-03-23T19:01:40Z

beetsplug/web/__init__.py

@@ -25,6 +25,11 @@
 from werkzeug.routing import BaseConverter, PathConverter
 import os
 import json
+import tempfile
+from string import Template


This should go with the other from anything_but_spam import spam imports, instead of the middle of the import eggs ones.

Thanks for the review. I fixed all the isues you mentioned

Freso · 2017-03-23T19:05:16Z

beetsplug/web/__init__.py

+            )
+        )
+    attachment_filename = os.path.basename(util.py3_path(item.path))
+    attachment_filename = attachment_filename[:attachment_filename.rfind('.')] + extension


The line is too long, as per https://travis-ci.org/beetbox/beets/jobs/214352835

Freso · 2017-03-23T19:05:27Z

beetsplug/web/__init__.py

@@ -348,6 +423,10 @@ def func(lib, opts, args):
            app.config['JSONIFY_PRETTYPRINT_REGULAR'] = False

            app.config['INCLUDE_PATHS'] = self.config['include_paths']
+            if 'convert_extension' in self.config:
+                app.config['convert_extension'] = self.config['convert_extension'].as_str()


The line is too long, as per https://travis-ci.org/beetbox/beets/jobs/214352835

Freso · 2017-03-23T19:05:34Z

beetsplug/web/__init__.py

+            if 'convert_extension' in self.config:
+                app.config['convert_extension'] = self.config['convert_extension'].as_str()
+            if 'convert_command' in self.config:
+                app.config['convert_command'] = self.config['convert_command'].as_str()


The line is too long, as per https://travis-ci.org/beetbox/beets/jobs/214352835

sampsyo · 2017-03-24T03:39:56Z

Wow; this is cool! Interestingly, we've talked about a flexible system for this kind of transcoding in the AURA spec, where the idea is that you could specify whatever file formats you're willing to accept: https://github.com/beetbox/aura/blob/master/docs/api.rst#audio-formats-and-quality

Is there any code that currently needs to be duplicated from the convert plugin? If so, should we move any of it to a library that we can share?

This commit adds two config options to the web plugin: convert_extension and convert_command. When convert_extension is defined (for example, as 'mp3'), a new option "convert and download as mp3" is shown next to the download link for files that are not already in mp3 format. This also adds a config global variable to the javascript code, which is filled with data obtained from the new /config route, which returns configuration values useful in the UI (so far, only convert_extension). The code to convert music to another format was mainly copied from the convert plugin, and put in a new /item/<item_id>/converted_file route. The convert command used by default is the same as in the convert plugin too, but can be configured with the convert_command option. To enable this, just define in the configuration: ``` web: convert_extension: mp3 ``` And search for any non-mp3 file in the web plugin.

antlarr · 2017-05-08T09:14:39Z

Doing a simple git rebase -i on master and not changing anything in the git-rebase-todo should just replay the changes but it gave some conflicts on commits unrelated to my branch. This gave me a hard time rebasing my branch so that it applied cleanly on master. Because of that, I decided to just branch from master again, cherry-pick my changes rebasing them at the same time and then force-push the changes to the origin branch of this pull request. That's the reason they appear as new commits.

sampsyo · 2017-06-14T23:22:32Z

Hi, @antlarr—thanks for your patience while I'm digging myself out of a PR review backlog! I'm aware I have more significant changes from you to review too, but while I'm on my way there, here are a couple of comments:

Eventually, we'll need to add documentation for the new options (and the URL scheme) to the web plugin.
The transcoding logic here is pretty similar to the core logic for invoking FFmpeg in the convert plugin. To avoid code duplication, I think it would make sense to make a core module that both plugins can share to do this basic transcoding stuff. The idea is similar to the beets.art module we added some time back, which is shared among all the various plugins that need to embed album art. (The code originally lived in the embedart plugin, but it became necessary for other plugins to do the same thing as a subroutine.)
I don't quite see what's going on with the config value on the JavaScript side—is it possible that's out of date?

jtpavlock · 2020-07-10T05:30:55Z

@antlarr any interest in continuing this?

stale · 2020-11-18T00:50:57Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Freso suggested changes Mar 23, 2017

View reviewed changes

Freso approved these changes Mar 28, 2017

View reviewed changes

antlarr force-pushed the web_convert_and_download_support branch 2 times, most recently from 2c6ae61 to b0d53b0 Compare May 8, 2017 08:51

antlarr added 2 commits May 8, 2017 11:04

Fix some E501 errors and moved import to the right location

666872f

antlarr force-pushed the web_convert_and_download_support branch from b0d53b0 to 666872f Compare May 8, 2017 09:07

stale bot added the stale label Nov 18, 2020

stale bot closed this Nov 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to transcode a file on-the-fly and download it from the web ui #2489

Allow to transcode a file on-the-fly and download it from the web ui #2489

antlarr commented Mar 23, 2017

mention-bot commented Mar 23, 2017

antlarr commented Mar 23, 2017

Freso left a comment

Freso Mar 23, 2017

antlarr Mar 23, 2017

Freso Mar 23, 2017

Freso Mar 23, 2017

Freso Mar 23, 2017

sampsyo commented Mar 24, 2017

antlarr commented May 8, 2017

sampsyo commented Jun 14, 2017

jtpavlock commented Jul 10, 2020

stale bot commented Nov 18, 2020

Allow to transcode a file on-the-fly and download it from the web ui #2489

Allow to transcode a file on-the-fly and download it from the web ui #2489

Conversation

antlarr commented Mar 23, 2017

mention-bot commented Mar 23, 2017

antlarr commented Mar 23, 2017

Freso left a comment

Choose a reason for hiding this comment

Freso Mar 23, 2017

Choose a reason for hiding this comment

antlarr Mar 23, 2017

Choose a reason for hiding this comment

Freso Mar 23, 2017

Choose a reason for hiding this comment

Freso Mar 23, 2017

Choose a reason for hiding this comment

Freso Mar 23, 2017

Choose a reason for hiding this comment

sampsyo commented Mar 24, 2017

antlarr commented May 8, 2017

sampsyo commented Jun 14, 2017

jtpavlock commented Jul 10, 2020

stale bot commented Nov 18, 2020