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

Add audio uploads #11123

Merged
merged 2 commits into from
Jun 19, 2019
Merged

Add audio uploads #11123

merged 2 commits into from
Jun 19, 2019

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Jun 19, 2019

Fix #4827

Accept uploads of OGG, WAV, FLAC, OPUS and MP3 files, and converts them to OGG. Media attachments get a new audio type. In the UI, audio uploads are displayed identically to video uploads.


This is a second take on #9480 because the audio player design was giving me grief and it was ambigous whether or not we need to parse, save, and edit special information about the files, like title, artist, album, etc. An audio player looks barren without those things, but they introduce more complexity, require a system-level dependency (libtag1-dev), require deciding on subset of tags to duplicate into our own schema, and require a way to store cover art within the media attachment model.

@Gargron Gargron force-pushed the feature-audio-v2 branch from 27ec10d to 1804f7b Compare June 19, 2019 15:39
Fix #4827

Accept uploads of OGG, WAV, FLAC, OPUS and MP3 files, and converts
them to OGG. Media attachments get a new `audio` type. In the UI,
audio uploads are displayed identically to video uploads.
@Gargron Gargron force-pushed the feature-audio-v2 branch from 1804f7b to cdb7c5a Compare June 19, 2019 16:25
@@ -333,7 +333,7 @@ class Status extends ImmutablePureComponent {
media={status.get('media_attachments')}
/>
);
} else if (status.getIn(['media_attachments', 0, 'type']) === 'video') {
} else if (['video', 'audio'].includes(status.getIn(['media_attachments', 0, 'type']))) {
const video = status.getIn(['media_attachments', 0]);
Copy link
Member

Choose a reason for hiding this comment

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

mb rename video to something more generic?

Copy link
Member

Choose a reason for hiding this comment

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

attachment?

@@ -27,7 +27,7 @@
= render partial: 'stream_entries/poll', locals: { status: status, poll: status.preloadable_poll, autoplay: autoplay }

- if !status.media_attachments.empty?
- if status.media_attachments.first.video?
- if status.media_attachments.first.video? || status.media_attachments.first.audio?
Copy link
Member

Choose a reason for hiding this comment

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

let's put this into a method on MediaAttachment, like use_video_player?

@@ -31,7 +31,7 @@
= render partial: 'stream_entries/poll', locals: { status: status, poll: status.preloadable_poll, autoplay: autoplay }

- if !status.media_attachments.empty?
- if status.media_attachments.first.video?
- if status.media_attachments.first.video? || status.media_attachments.first.audio?
Copy link
Member

Choose a reason for hiding this comment

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

as above

@@ -100,7 +100,7 @@ def validate_media!

@media = @account.media_attachments.where(status_id: nil).where(id: @options[:media_ids].take(4).map(&:to_i))

raise Mastodon::ValidationError, I18n.t('media_attachments.validations.images_and_video') if @media.size > 1 && @media.find(&:video?)
raise Mastodon::ValidationError, I18n.t('media_attachments.validations.images_and_video') if @media.size > 1 && @media.find { |x| x.video? || x.audio? }
Copy link
Member

Choose a reason for hiding this comment

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

this should be larger_media_format? right?

@@ -19,7 +19,7 @@ def show
def player
@body_classes = 'player'
response.headers['X-Frame-Options'] = 'ALLOWALL'
raise ActiveRecord::RecordNotFound unless @media_attachment.video? || @media_attachment.gifv?
raise ActiveRecord::RecordNotFound unless @media_attachment.video? || @media_attachment.gifv? || @media_attachment.audio?
Copy link
Member

Choose a reason for hiding this comment

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

can/should this also be larger_media_format?

Copy link
Member

@nightpool nightpool left a comment

Choose a reason for hiding this comment

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

LGTM

@koyuawsmbrtn
Copy link
Contributor

Sick! 🎉

@tn5421
Copy link

tn5421 commented Jun 22, 2019

Is there any way we can simply embed the file as-is, without transcoding lossy source files to another format?

@Gargron
Copy link
Member Author

Gargron commented Jun 22, 2019

No, it won't be playable on all platforms then

@tn5421
Copy link

tn5421 commented Jun 22, 2019

Perhaps an option could be included in a users settings that allows them to override this default setting, with a warning given to the tune of your reply.

if I upload a 96kbps opus file and it gets transcoded to 192kbps mp3 it's going to sound like a 96kbps mp3 regardless of the extra file size

edit: i know remarkably little about actual software engineering so if this isn't tenable please let me know, i can be tooted at @tn5421@niu.moe

@Gargron
Copy link
Member Author

Gargron commented Jun 22, 2019

We use variable bitrate so I don't think the file will be bigger if the source isn't higher quality

@koyuawsmbrtn
Copy link
Contributor

Suggestion: Make the audio player smaller like in the previous PR

@trwnh
Copy link
Member

trwnh commented Jun 22, 2019

@tn5421 96kbps opus and 96kbps mp3 are not the same at all -- the 96kbps opus will sound vastly better since the opus codec is more efficient than mp3. also as stated, with variable bitrate the bitrate of a section of audio is allowed to dip down. there is still a quality loss with lossy->lossy transcode, but not so much that it makes the audio unusable.

what could be done is, if you are uploading an mp3 and it is already below a certain size or bitrate, then the transcode is skipped.

@Gargron
Copy link
Member Author

Gargron commented Jun 22, 2019

what could be done is, if you are uploading an mp3 and it is already below a certain size or bitrate, then the transcode is skipped.

Nothing needs to be done. That's how it is

hiyuki2578 pushed a commit to ProjectMyosotis/mastodon that referenced this pull request Oct 2, 2019
* Add audio uploads

Fix mastodon#4827

Accept uploads of OGG, WAV, FLAC, OPUS and MP3 files, and converts
them to OGG. Media attachments get a new `audio` type. In the UI,
audio uploads are displayed identically to video uploads.

* Improve code style
rtucker referenced this pull request in vulpineclub/mastodon Jan 7, 2021
* Add audio uploads

Fix mastodon#4827

Accept uploads of OGG, WAV, FLAC, OPUS and MP3 files, and converts
them to OGG. Media attachments get a new `audio` type. In the UI,
audio uploads are displayed identically to video uploads.

* Improve code style
messenjahofchrist pushed a commit to Origin-Creative/mastodon that referenced this pull request Jul 30, 2021
* Add audio uploads

Fix mastodon#4827

Accept uploads of OGG, WAV, FLAC, OPUS and MP3 files, and converts
them to OGG. Media attachments get a new `audio` type. In the UI,
audio uploads are displayed identically to video uploads.

* Improve code style
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.

Allow users to post audio files
5 participants