asterisk
asterisk copied to clipboard
asterisk
format_mp3: Add MP3 write support.
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.
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.
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.
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.
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.
"if LAME is present, then load the new LAME-based format instead and if not, load the original mpg123-based one?" That.
Gotcha... how would that work in the build system? We have defaultenabled and conflicts, but I guess which one is enabled by default would depend on whether LAME is present, so that couldn't be static XML. Is there another way?
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.
Sure, that makes sense.
Should the new module go in formats like all the ones or addons like the existing MP3 one? (And why was the original one in addons instead of formats, anyways? Licensing?)
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.
"if LAME is present, then load the new LAME-based format instead and if not, load the original mpg123-based one?" That.
Gotcha... how would that work in the build system? We have
defaultenabledandconflicts, but I guess which one is enabled by default would depend on whether LAME is present, so that couldn't be static XML. Is there another way?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.
Sure, that makes sense.
Should the new module go in
formatslike all the ones oraddonslike the existing MP3 one? (And why was the original one in addons instead of formats, anyways? Licensing?)
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:
- Fixed a case in write(), when lame encode stops after five minutes (reinitialize lame)
- Support for more than one (8000Hz) sample rate
Only one disadvantage:
Due to how lame decoder (hip_decode* functions in lame API) work, a sliding window approach can't be used, therefor the whole mp3 file is loaded into memory. This is a bit wasteful with memory, and has a hard upper limit on the mp3 length (currently about 72 hours@8000 Hz). However this also makes positional functions (ftell,fseek,ftrunc) in the Asterisk format API easy.
But the previous fact makes this PR's hybrid approach a somehow desirable.
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 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
"if LAME is present, then load the new LAME-based format instead and if not, load the original mpg123-based one?" That.
Gotcha... how would that work in the build system? We have
defaultenabledandconflicts, but I guess which one is enabled by default would depend on whether LAME is present, so that couldn't be static XML. Is there another way?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.
Sure, that makes sense. Should the new module go in
formatslike all the ones oraddonslike the existing MP3 one? (And why was the original one in addons instead of formats, anyways? Licensing?)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:
- Fixed a case in write(), when lame encode stops after five minutes (reinitialize lame)
- Support for more than one (8000Hz) sample rate
Only one disadvantage: Due to how lame decoder (
hip_decode*functions in lame API) work, a sliding window approach can't be used, therefor the whole mp3 file is loaded into memory. This is a bit wasteful with memory, and has a hard upper limit on the mp3 length (currently about 72 hours@8000 Hz). However this also makes positional functions (ftell,fseek,ftrunc) in the Asterisk format API easy.But the previous fact makes this PR's hybrid approach a somehow desirable.
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 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.
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.
Here it is: https://github.com/asterisk/asterisk/pull/704 But I recommend not closing this one (yet). We should debate with the maintainers, which direction to go with.
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.
Here it is: #704 But I recommend not closing this one (yet). We should debate with the maintainers, which direction to go with.
I think they have already indicated in the comments on this PR they prefer the separate module approach. I'll leave it open for a bit, but unless anything has changed, I'll likely close this out later this week.