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

Apply .aac extension so playback works in downstream Anki clients #8

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

MarvNC
Copy link
Member

@MarvNC MarvNC commented Mar 5, 2023

From ctpk:

By default, if an audio media type does not appear in this switch statement, it gets the ".mp3" file extension. This causes issues with aac files in the iphone anki app.

Copy link
Collaborator

@djahandarie djahandarie left a comment

Choose a reason for hiding this comment

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

LGTM. We should potentially also look for other filetypes that would be reasonable to include. Or maybe there is a library that does this task which would be better to use.

Holding on merging for a bit until we have the project fully setup.

@djahandarie djahandarie added the kind/bug The issue or PR is regarding a bug label Mar 5, 2023
@yuki-tsubaki
Copy link

yuki-tsubaki commented Mar 17, 2023

I think simply adding support for problem-cases as they come is the best approach for this. The current code covers all of the widespread audio formats. Outsourcing this to a library would likely be more hassle than it is worth.

The reason being that 'file types' for media do not one-to-one correspond to file extensions (which are also handled differently by the end media player). As such, the file extension needed (for programs that don't properly detect the container and encodings used or otherwise override that information with the 'hint' from the file extension) will vary (because in the current landscape of feature-full, complex containers the problem is more on their side). I.e. a general solution to this is impractical as it depends on the quirks in player's implementations.

Currently, the only other addition that I predict could be necessary is extending handling of opus (a more modern encoding used in ogg containers, the successor to vorbis, and becoming the standard 'backend' format (i.e. if audio is intended to be streamed, it is probably using opus)), which is already partially supported through audio/ogg. Adding audio/opus as another case that returns .ogg would solve this. Adding a .opus extension would be semantically preferable, but that is irrelevant if it is much worse supported .

Table with some standard extension-mime-type pairs
.opus was mentioned earlier in this doc page (it it not here because technically it is an alias for a .ogg using the opus codec.
Media container formats (file types) - Web media technologies | MDN
You could get more from Wikipedia on media containers, but those may be less standard.

As a heads-up and note on the original problem
I am working on re-encoding the pronunciation audio files used in the themoeway/local-audio-yomichan to see how much smaller I can make the database while still sounding nice (my testing for the NHK set gets me to 33.75% the original size for a random sample of 500 sounds). This is the set that causes the AAC error here and I would be replacing it with a .opus file (or maybe ogg if that is more compatible).

@djahandarie
Copy link
Collaborator

@yuki-tsubaki Thanks for providing the background and reasoning! I looked a bit into a library like https://github.com/broofa/mime, and while it does seem to largely cover everything we have, there are some very minor differences (e.g., x-pn-wave) and I don't have the context of what such code is even for, so it'd be hard to seriously evaluate. So I'm happy to just continue maintaining this file, especially since it is quite minimal.

@djahandarie djahandarie changed the title Set file extension for aac audio media type Properly apply .aac extension so playback works in downstream Anki clients Mar 23, 2023
@djahandarie djahandarie changed the title Properly apply .aac extension so playback works in downstream Anki clients Apply .aac extension so playback works in downstream Anki clients Mar 23, 2023
@djahandarie djahandarie marked this pull request as draft March 23, 2023 14:02
@djahandarie djahandarie marked this pull request as ready for review March 23, 2023 14:02
@djahandarie djahandarie merged commit e358d21 into themoeway:master Mar 23, 2023
Kuuuube pushed a commit to Kuuuube/yomitan that referenced this pull request Feb 3, 2024
Bumps [fake-indexeddb](https://github.com/dumbmatter/fakeIndexedDB) from 4.0.0 to 4.0.1.
- [Release notes](https://github.com/dumbmatter/fakeIndexedDB/releases)
- [Changelog](https://github.com/dumbmatter/fakeIndexedDB/blob/master/CHANGELOG.md)
- [Commits](dumbmatter/fakeIndexedDB@v4.0.0...v4.0.1)

---
updated-dependencies:
- dependency-name: fake-indexeddb
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug The issue or PR is regarding a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants