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
format_mp3: Add MP3 write support. #665
base: master
Are you sure you want to change the base?
Conversation
632c796
to
98d7a76
Compare
cherry-pick-to: 18 |
Just my opinion, but it would make more sense to have a lame-based module if you have lame headers, and a mpg123 version (the current format_mp3) if you don't. It just feels wrong to mix and match in the same file. |
This adds the ability to write in the MP3 format; previously, MP3 data could only be read. Resolves: asterisk#664 UserNote: Asterisk now supports natively writing data in the MP3 format.
98d7a76
to
6029fd7
Compare
https://github.com/viktike/asterisk/blob/mp3_lame/addons/format_lame.c if anyone is interested. |
I agree with Sean, and what is the license for lame? |
It's LGPL: https://lame.sourceforge.io/license.txt If it were a separate module, what would be the overall goal? Continue to maintain two separate modules, one with read/write using LAME and one with read only using mpg123? Or deprecate the old module and fully switch to LAME? My original goal was to minimize changes for backwards compatibility, but maybe that's not important? |
Both existing, if lame is available then full support, if not available then nothing changes. |
... I'm probably misunderstanding something, but isn't that how it is here now? If LAME is available, support read/write, otherwise nothing changes. Or do you mean if LAME is present, then load the new LAME-based format instead and if not, load the original mpg123-based one? Only drawback will be that people not autoloading will have to manually select the new module, but since it's a new feature, maybe that's not important? Also, if we can do everything in LAME, what's the rationale for keeping the mpg123-only one around long term? Less stringent dependency? Other benefits of mpg123 over LAME? |
"if LAME is present, then load the new LAME-based format instead and if not, load the original mpg123-based one?" That. I don't agree with mixing and matching the implementations in a single module, thus two separate modules. The format_mp3 module as it exists now will remain. It's minimal, has received virtually no changes over a considerable amount of time, and its continued existence is not a burden. Removing it would be more disruptive than it is worth, even if notice were given. |
Gotcha... how would that work in the build system? We have
Sure, that makes sense. Should the new module go in Also I just noticed @viktike already went ahead and added read for LAME, do you want to just merge what you added into this PR? That seems to be closer to the desired end goal. |
I have no further information on the build system for such things than you do. Someone else may have input, but I do not. The module is in addons due to licensing. It was in a separate repo long ago, then got moved into an addons directory in Asterisk. I would prefer the new one also be in addons too for the same reason. I also need to refresh my brain on the whole licensing stuff in general. |
As I dig myself deeper into this, LAME's MP3 decoder ("hip") itself uses MPGLIB/MPG123. However there are advantages and a disadvantage to my implementation: Advantages:
Only one disadvantage: But the previous fact makes this PR's hybrid approach a somehow desirable.
I can do a new PR instead. I've left you in there as the author: https://github.com/viktike/asterisk/blob/mp3_lame/addons/format_lame.c#L4 |
Sounds good! If you want to put that up, I'll go ahead and close this one and we can review that. I'm not really sure how co-authoring works since I've not done it much myself, but it might just be adding "Co-Authored-By:" in your commit message. It would be good to state the pros/cons you just mentioned both in the commit message as well as at the top of the source code, so people are aware. I think at some point we need to run bootstrap, but I wanted to wait until this was further in to do that. |
Sounds good! If you want to put that up, I'll go ahead and close this one and we can review that.
Here it is: #704 |
I think they have already indicated in the comments on this PR they prefer the separate module approach. |
This adds the ability to write in the MP3 format;
previously, MP3 data could only be read.
Resolves: #664
UserNote: Asterisk now supports natively writing
data in the MP3 format.