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

Video audio fix #1662

Closed
wants to merge 11 commits into from
Closed

Conversation

tbostic32
Copy link
Collaborator

@tbostic32 tbostic32 commented Jun 29, 2021

Updating rabbit and perspective videos with appropriate audio codecs to work with chrome and firefox.

Closes issue(s):

Need for Final Call:
This will require a 1-week call for review


How to Review And Approve

  • Go to the “Files changed” tab
  • Here you will have the option to leave comments on different lines.
  • Once the review is completed, find the “Review changes” button in the top right, select “Approve” (if you are really confident in the rule) or "Request changes" and click “Submit review”.
  • Make sure to also review the proposed Final Call period. In case of disagreement, the longer period wins.

@tbostic32
Copy link
Collaborator Author

I'm not sure why this is bringing six commits with it, they seem to be tagging along from when I updated my fork to the most recent version of main fork.

@Jym77 Jym77 added the Editorial For editorial changes that does not change the meaning of a rule or Glossary term label Jul 1, 2021
Copy link
Collaborator

@Jym77 Jym77 left a comment

Choose a reason for hiding this comment

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

I've honestly not tested whether the files are better or worse than the previous one… I guess you know what you do 😁 and in any case, that doesn't hurt the rules themselve.

Copy link
Member

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

If we're going to get rid of these files, we should also get rid of the track elements that reference them. If all major browsers support webm now (they didn't when we wrote this) it might be better for us to get rid of all mp4 files, instead of just the ones with the problem.

@tbostic32
Copy link
Collaborator Author

@WilcoFiers Regarding removing the mp4 files and only keeping the webm files, you can see the support on caniuse: https://caniuse.com/webm. It seems like safari only has partial support and older versions of browsers (7 or 8 years old) may not support them. I don't have a way to test safari at the moment so I can't tell exactly what partial-support means, but I think it may be safer to continue the current approach of both mp4 and webm.

Secondly, I am not getting rid of any of the files, I'm unsure why the files changed only shows things deleted, but if you visit the branch those files do very much still exist and have just been updated.

@WilcoFiers
Copy link
Member

@tbostic32 Looking at the reason for the partial support, it seems that it's going to work on the latest version of MacOS. I tested it on my machine, seems to work. I think we can take out all the mp4s.

@tbostic32
Copy link
Collaborator Author

The MP4s have been removed and I removed/changed all references to them. Any changed refs are now webms.

Couple thoughts:

  1. A bit weird to remove all mp4s when the perspective videos are all mp4s and no webms.
  2. Good find on the partial support for MacOS, Do we not have concerns that users might be using an older version for which it does not work? If thats not a concern then we should be good to go.

@WilcoFiers WilcoFiers self-requested a review July 15, 2021 13:24
@dd8
Copy link
Collaborator

dd8 commented Jul 22, 2021

There are 3 things that need to be supported by the browser and/or OS for video with audio to work:

  1. the browser needs to support the container format for video and sound to play (e.g. WebM, MP4)
  2. the browser needs to support the audio codec for sound to play (e.g. AAC, Opus, FLAC)
  3. the browser needs to support the video codec for video to play (e.g. AVC, AV1, VP9)

Unfortunately caniuse.com conflates these, and switching to WebM may not improve matters (because the codecs still need to be supported). The problem with the MP4s was the A52 (AC3) Audio audio codecs used by some of the videos have much worse support than the AAC codec.

MDN has a very good overview of this stuff:
https://developer.mozilla.org/en-US/docs/Web/Media/Formats

MDN recommendation for maximum compatibility - published in September 2020:

An MP4 container and the AVC (H.264) video codec, ideally with AAC as your audio codec
https://developer.mozilla.org/en-US/docs/Web/Media/Formats/Video_codecs#recommendations_for_everyday_videos

I think one issue with WebM is it's not supported in Safari on macOS < 11.3 - Safari uses media format support / codecs built into macOS / iOS. Other browsers use a mix of their own codecs and codecs supplied by the OS (but no 2 browsers support the same set of codecs). That's why video formats are such a terrible mess.

@tbostic32
Copy link
Collaborator Author

tbostic32 commented Aug 2, 2021

@dd8 Thanks for that great overview! I briefly (and much less eloquently) mentioned the codecs in the associated issue #1620 and further up in this conversation is a brief discussion on the support for webm vs. mp4. The gist is that we did solve the codec error with the audio, but had decided to reduce the number of resources we needed to keep by deleting the mp4.

@WilcoFiers due to @dd8's comment, should we revisit the discussion of support for webms?

@WilcoFiers
Copy link
Member

@tbostic32 Well that's a bummer. I was hoping that with Safari 11.3 we would be able to move over to just supporting the one format. Mark's explanation does help understand why the audio got lost on the some of the videos. It just has the wrong encoding. Kind of explains another video problem I was having a while back too.

We can discuss on Thursday's call what to do next, but from the sound of it we'll just need to recreate those mp4 videos with the correct audio encoding.

@tbostic32
Copy link
Collaborator Author

tbostic32 commented Aug 3, 2021

I had made the audio codec fixes in a previous commit 986cdc5. A pull request at that commit is now up at #1680.

I made a separate PR for that since I am not sure how to rewind a PR to a specific commit on a branch, if there is a better way let me know and I can update this PR.

@tbostic32 tbostic32 closed this Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editorial For editorial changes that does not change the meaning of a rule or Glossary term
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some video examples should have audio, but they do not [1ec09b]
5 participants