Adding MP3 encoding using lame as an option

[tachang]

@mpromonet Opening this pull request early to get feedback on what changes you'd like to see and if possible get merged in.

[mpromonet]

Hi,

Thanks for the pull request, I will look. At a glance, it seems there is no detection if lame is present, I guess it does not build if lame is not installed. Also the MP3 code seems inside the ALSA based class, it should better take place in MP3 overide.

Best Regards, Michel.

[tachang]

Understood. I need to add detection for Lame.

Also the MP3 code seems inside the ALSA based class, it should better take place in MP3 overide.

You are correct. However maybe I can convince you it makes more sense inside the ALSA class. Because generally ALSA outputs PCM and you start with PCM data. So the MP3 functionality should be within the ALSA class. I guess if you want ALSA class to stay "pure" you can have another inherited class like "ALSAWithCompressionOptions" class that allows you to specify MP3/Opus/AAC etc.

I guess my point is it would be cleaner to have the existing ALSA class support those additional compression options than having another subclass. Because then you'd have to go through code and pick which class to use (the one with ALSA or ALSAWithCompressionOptions. And at that point why not just pass into the class if you needed compression or not rather than switch back and forth between two classes.

[mpromonet]

Hi,

This is a classic antipattern to implement derived behaviour in base class, a common way to solve this is to use the template method pattern. Maybe some compress method in ALSA capture with an empty implementation and implementing MP3 compression in MP3 capture.

Best Regards, Michel.

[tachang]

Hi,

This is a classic antipattern to implement derived behaviour in base class, a common way to solve this is to use the template method pattern. Maybe some compress method in ALSA capture with an empty implementation and implementing MP3 compression in MP3 capture.

Best Regards, Michel.

Would it make sense to pass in a class that specifically is for compression? So the constructor for ALSACapture may accept a parameter ALSACapture(AudioCompressor *implementation).

class AudioCompressor:
   abstract method compress()

And so you may implement classes such as MP3AudioCompressor

class MP3AudioCompressor:
  function compress() {
     // Do MP3 compression
  }

And ALSACapture may have code such as:

if( AudioCompressor != NULL) {
   bytes = AudioCompressor(bytes)
}

Is that more in line with what you are thinking?

[mpromonet]

Hi,

This is far better.

Best Regards, Michel.

[tachang]

I know it's been a while but haven't taken a look into this yet. But I have been thinking about it recently and wondering if you ever plan to implement different encoders.

[mpromonet]

Hi @tachang

The idea of this project is to give a light solution to stream video (and then audio) through RTSP with low latency. There is more powerful solution to make conversion & compression like ffmpeg, gstreamer... For video the compression could be done through v4l2loopback device, for audio I don't know if ALSA allow to access to compressed format, this will be more in the idea of this project. So maybe for audio it will need to add some compression like opus, mp3 ...

Best Regards, Michel.

[tachang]

@mpromonet Thank you. I think mostly what I am asking is "how do you feel about adding light mp3/opus audio encoding support?" And along these lines, "would you be for or against it if I went ahead (or anyone else) implemented it". Would you want to see something like lightweight support for audio encoding?

I feel like raw PCM is pretty heavy to stream so something encoded like mp3 or opus would be ideal to support.