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

Fix audio/webm files #58

Merged
merged 1 commit into from
Jun 25, 2021
Merged

Fix audio/webm files #58

merged 1 commit into from
Jun 25, 2021

Conversation

kiskoza
Copy link
Contributor

@kiskoza kiskoza commented Jun 15, 2021

Hi. Recently I encountered a bug using Active Storage when I recorded some audio in a Chrome browser (only supported mimetype is audio/webm), uploaded it to the server and got it back as video/webm - which broke the frontend logic to show it in an audio player instead of a video player. During my investigation I found this gem and noticed that it doesn't know anything about audio/webm.

In the WebM Project docs, you can find the following information:

  • MIME-type: video/webm
  • Audio-only MIME-type: audio/webm
  • Video codec: The Codec ID SHOULD be "V_VP8" or "V_VP9".
  • Audio codec: The Codec ID SHOULD be "A_VORBIS" or "A_OPUS".

That means a video webm file should contain at least one video stream (but don't need to check for audios) and an audio webm file should contain at least one audio stream (but should not have any video streams - this rule should be after the video's). I added these rules to the tables.rb file with a new sample file for audio/webm

Is there anything else I need to add regarding these changes?

Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

The tables.rb file is generated using the XML data files in the repo. We recently switched from Mimemagic to Apache Tika. It seems audio/webm isn't a type Tika knows about. Could you please add the mime info to data/custom.xml and regenerate the file (using bundle exec rake tables)? Thanks!

@kiskoza
Copy link
Contributor Author

kiskoza commented Jun 16, 2021

Thanks for the feedback, I added my changes to custom.xml and regenerated the tables (got the same result, but the new line for audio/webm moved to the audios) and force pushed all of it in one commit.

We should mention this rake task in a contribution guide to help future contributors, it was easier to make the changes there now I'm aware of this file 😄

@kiskoza kiskoza requested a review from gmcgibbon June 16, 2021 07:42
Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

We should mention this rake task in a contribution guide to help future contributors, it was easier to make the changes there now I'm aware of this file 😄

Yes, totally agree. I will look at adding this.

@gmcgibbon gmcgibbon merged commit 04de621 into rails:main Jun 25, 2021
@gmcgibbon gmcgibbon mentioned this pull request Jun 25, 2021
@ClearlyClaire
Copy link

This seems to break some webm files, for instance this one, presumably because the V_VP8 string appears after 4096 bytes.

@kiskoza
Copy link
Contributor Author

kiskoza commented Oct 15, 2021

hi @ClearlyClaire . thanks for raising this issue. I think we should scan the whole file for the signatures as the webm's container format doesn't have a strict order for the contained streams.

@gmcgibbon what do you think? is there a way to scan the whole file?

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.

3 participants