Skip to content

Implement Nellymoser decoder #1920

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

Merged
merged 1 commit into from
Feb 2, 2021
Merged

Conversation

relrelb
Copy link
Contributor

@relrelb relrelb commented Dec 11, 2020

Based on nellymoserdec.c of FFmpeg, which is itself based on nelly2pcm.

TODO

  • Implement a working prototype.
  • Depend on RustDCT to minimize code size.
  • Move to a separate crate.
  • Apply also for AudioCompression::Nellymoser16Khz and AudioCompression::Nellymoser8Khz.

Fixes #1121, fixes #1118 and partially #5, #929, #1839.

@Herschel Herschel mentioned this pull request Dec 24, 2020
@relrelb relrelb force-pushed the nellymoser branch 2 times, most recently from 2437424 to 75a35ed Compare January 2, 2021 00:03
@relrelb relrelb marked this pull request as ready for review January 2, 2021 00:20
@relrelb
Copy link
Contributor Author

relrelb commented Jan 2, 2021

Releasing the draft lock, ready for CR.

One thing that I'm still not sure about is how AudioCompression::Nellymoser16Khz and AudioCompression::Nellymoser8Khz should be treated. I didn't find any SWFs out there that use these compressions, nor I was able to create one, so I can't actually test them.

@Herschel
Copy link
Member

Herschel commented Jan 2, 2021

Thank you for your awesome work! Do you have any interest in moving the nelly2pcm-rs repo to the Ruffle organization? You would remain the maintainer of the repo. This is, of course, entirely up to you.

I don't know of any way to coax the Flash IDE to export the Nellymoser8k/16k codecs (I suspect these mostly come from FLV files, and it was added to SWF format for parity?) Perhaps @Dinnerbone could run scanner to search for Nellymoser8k/16k/Speex sound format.

@relrelb
Copy link
Contributor Author

relrelb commented Jan 2, 2021

Thank you for your awesome work! Do you have any interest in moving the nelly2pcm-rs repo to the Ruffle organization? You would remain the maintainer of the repo. This is, of course, entirely up to you.

Sure, actually it would be a much more suitable place for it. I just need a permission to create public repositories in ruffle-rs.

EDIT: On second thought, under the assumption that we intend this crate to be tightly-coupled with Ruffle, I don't see any advantage in placing it in a repository of its own. I see how it could easily complicate making changes and fixing bugs. Maybe we should put it as a separate crate on this repository, or even not in a separate crate at all, just like it used to be during the progress of this PR. @Herschel please let me know what do you think about that.

I don't know of any way to coax the Flash IDE to export the Nellymoser8k/16k codecs (I suspect these mostly come from FLV files, and it was added to SWF format for parity?) Perhaps @Dinnerbone could run scanner to search for Nellymoser8k/16k/Speex sound format.

Sounds good. In the worst-case we can simply remain not supporting those compressions.

@relrelb relrelb force-pushed the nellymoser branch 2 times, most recently from 597a444 to eeaa72c Compare January 2, 2021 16:42
@Herschel
Copy link
Member

Herschel commented Jan 3, 2021

After talking on Discord, we decided that codecs can go in the same repo as subcrates under a codecs director. They can still be a subcrate, which can be still published on crates.io separately. This matches the H.264 PR and also the swf crate, and makes it easier to do updates (we found it troublesome months ago when swf was a separate crate to have to update two separate repos to make fixes).

@relrelb relrelb force-pushed the nellymoser branch 2 times, most recently from 0cd91d7 to 10e7a3b Compare January 3, 2021 12:37
@Herschel Herschel force-pushed the nellymoser branch 2 times, most recently from c6494f3 to 711c06c Compare January 5, 2021 21:01
@relrelb
Copy link
Contributor Author

relrelb commented Jan 9, 2021

@Herschel Is there any reason for not merging this PR?

@relrelb relrelb force-pushed the nellymoser branch 2 times, most recently from c165f5e to 876a436 Compare January 9, 2021 13:30
@Herschel
Copy link
Member

Herschel commented Jan 9, 2021

Because Nellymoser and H.263 are the first proprietary formats we may support, I am consulting legal advice before proceeding. Thanks for the patience.

@Herschel
Copy link
Member

Herschel commented Jan 31, 2021

After consulting with a few people, I think we're in the clear on this codec, and it's best to keep this in a separate repo (sorry for the back and forth). This gives us some degree of separation in case we have to yank that specific crate for any reason, and these codecs are fairly decoupled from the rest of Ruffle (compared to swf).

You should have access to create a private repo under the GitHub org; let's call it nellymoser and move it there, and then I can make it public. Again, thanks for the patience.

@haraldbre
Copy link

When will nellymoser be available in the ruffle firefox extension? Seems this might solve some problems with missing sound.

@Herschel
Copy link
Member

Herschel commented Feb 2, 2021

Going to merge this, thanks for your work! In the future, I'd like to make nellymoser (and all codecs) optional features that are enabled by default.

@Herschel Herschel merged commit 074731e into ruffle-rs:master Feb 2, 2021
@relrelb relrelb deleted the nellymoser branch February 2, 2021 09:32
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.

Thing Thing Sound Effects Aren't Playing Some Movies Play Without Sound
3 participants