[vlc-devel] new aac encoder for review
Jean-Baptiste Kempf
jb at videolan.org
Fri Oct 5 01:14:18 CEST 2012
On Thu, Oct 04, 2012 at 04:20:55PM -0400, Sergio M. Ammirata, Ph.D. wrote :
> +PKG_ENABLE_MODULES_VLC([FDKAAC], [], [fdk-aac], [FDK-AAC encoder], [auto])
> +VLC_ADD_LIBS([FDKAAC],[ -lfdk-aac])
Are you sure you need the second line?
> --- a/include/vlc_codec.h
> +++ b/include/vlc_codec.h
> @@ -173,6 +173,9 @@ struct encoder_t
>
> /* Encoder config */
> config_chain_t *p_cfg;
> +
> + /* Does the encoder need purging at EOF? */
> + bool b_purge_required;
> };
This should be in a separated patch.
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <unistd.h>
> +#include <stdlib.h>
You need all those?
> +/* Wg4
> +AOUT_CHAN_LEFT,
> +AOUT_CHAN_RIGHT,
> +AOUT_CHAN_MIDDLELEFT,
> +AOUT_CHAN_MIDDLERIGHT,
> +AOUT_CHAN_REARLEFT,
> +AOUT_CHAN_REARRIGHT,
> +AOUT_CHAN_REARCENTER,
> +AOUT_CHAN_CENTER,
> +AOUT_CHAN_LFE,
> +*/
Why here?
> +//HE-AAC v2 == HE-AAC + PS == AAC-LC + SBR + PS
> +//HE-AAC == AAC-LC + SBR
No need for those comments, or at least, not here.
> + add_integer( ENC_CFG_PREFIX "vbr", 0, VBR_QUALITY_TEXT,
> + VBR_QUALITY_LONGTEXT, false )
> + change_integer_range (0, 5)
> + change_safe ()
Not sure if safe
> + add_bool( ENC_CFG_PREFIX "afterburner", true, AFTERBURNER_TEXT,
> + AFTERBURNER_LONGTEXT, false )
> + add_integer( ENC_CFG_PREFIX "signaling", SIGNALING_COMPATIBLE, SIGNALING_TEXT,
> + SIGNALING_LONGTEXT, false )
> + change_integer_range (0, 2)
> + change_safe ()
Idem.
> +struct encoder_sys_t
> +{
> + HANDLE_AACENCODER handle;
> + int i_aot;
> + bool b_eld_sbr;
> + int i_vbr;
> + bool b_afterburner;
> + int i_signaling;
> + int i_encoderdelay;
> + int i_frame_size;
> + double d_compression_ratio;
> + mtime_t i_pts_last;
Please repack the struct :)
> + /* Allocate the memory needed to store the encoder's structure */
> + if( ( p_sys = (encoder_sys_t *)malloc(sizeof(encoder_sys_t)) ) == NULL )
> + return VLC_ENOMEM;
you should split the first line and use unlikely.
> + p_enc->pf_encode_audio = EncodeAudio;
Do that in the end, when you know not to fail.
> + if ( b_profile_selected == false )
> + p_sys->i_aot = var_CreateGetInteger( p_enc, ENC_CFG_PREFIX "profile" );
> + else
> + p_sys->i_aot = i_profile;
> + p_sys->b_eld_sbr = var_CreateGetBool( p_enc, ENC_CFG_PREFIX "sbr" );
> + p_sys->i_vbr = var_CreateGetInteger( p_enc, ENC_CFG_PREFIX "vbr" );
> + p_sys->b_afterburner = var_CreateGetBool( p_enc, ENC_CFG_PREFIX "afterburner" );
> + p_sys->i_signaling = var_CreateGetInteger( p_enc, ENC_CFG_PREFIX "signaling" );
Sure about var_Create and not var_Inherit?
> + if ((p_sys->i_aot == PROFILE_AAC_HE || p_sys->i_aot == PROFILE_AAC_HE_v2) && p_sys->i_vbr > 3)
> + {
> + msg_Warn(p_enc, "Maximum VBR quality for this profile is 3, setting vbr=3");
> + p_sys->i_vbr = 3;
> + }
> + if ((erraac = aacEncOpen(&p_sys->handle, 0, p_enc->fmt_in.audio.i_channels)) != AACENC_OK) {
> + msg_Err(p_enc, "Unable to open encoder: %s", aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
> + if ( p_sys->i_aot == PROFILE_AAC_HE_v2 && p_enc->fmt_in.audio.i_channels != 2 )
> + {
> + msg_Err(p_enc, "The HE-AAC-v2 profile can only be used with stereo sources");
> + free( p_sys );
> + return VLC_EGENERIC;
Shouldn't you aacEncClose(&p_sys->handle); here?
> + }
> + if ( p_sys->i_aot == PROFILE_AAC_ELD && p_enc->fmt_in.audio.i_channels != 2 )
> + {
> + msg_Err(p_enc, "The ELD-AAC profile can only be used with stereo sources");
> + free( p_sys );
> + return VLC_EGENERIC;
and there?
> + }
> + if ((erraac = aacEncoder_SetParam(p_sys->handle, AACENC_AOT, p_sys->i_aot)) != AACENC_OK) {
> + msg_Err(p_enc, "Unable to set the Profile %i: %s", p_sys->i_aot, aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
> + if (p_sys->i_aot == PROFILE_AAC_ELD && p_sys->b_eld_sbr) {
> + if ((erraac = aacEncoder_SetParam(p_sys->handle, AACENC_SBR_MODE, 1)) != AACENC_OK) {
> + msg_Err(p_enc, "Unable to set SBR mode for ELD: %s", aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
> + }
> + if ((erraac = aacEncoder_SetParam(p_sys->handle, AACENC_SAMPLERATE,
> + p_enc->fmt_out.audio.i_rate)) != AACENC_OK) {
> + msg_Err(p_enc, "Unable to set the sample rate %i: %s",p_enc->fmt_out.audio.i_rate,
> + aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
> + if ((erraac = aacEncoder_SetParam(p_sys->handle, AACENC_CHANNELMODE, mode)) != AACENC_OK) {
> + msg_Err(p_enc, "Unable to set the channel mode: %s", aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
> + if ((erraac = aacEncoder_SetParam(p_sys->handle, AACENC_CHANNELORDER, CH_ORDER_WG4)) != AACENC_OK) {
> + msg_Err(p_enc, "Unable to set the sound channel order: %s", aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
> + if (p_sys->i_vbr != 0) {
> + if ((erraac = aacEncoder_SetParam(p_sys->handle,
> + AACENC_BITRATEMODE, p_sys->i_vbr)) != AACENC_OK) {
> + msg_Err(p_enc, "Unable to set the VBR bitrate mode: %s", aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
> + } else {
> + if (p_enc->fmt_out.i_bitrate == 0) {
> + if (p_sys->i_aot == PROFILE_AAC_HE_v2) {
> + sce = 1;
> + cpe = 0;
> + }
> + i_bitrate = (96*sce + 128*cpe) * p_enc->fmt_out.audio.i_rate / 44;
> + if (p_sys->i_aot == PROFILE_AAC_HE ||
> + p_sys->i_aot == PROFILE_AAC_HE_v2 ||
> + p_sys->b_eld_sbr)
> + i_bitrate /= 2;
> + p_enc->fmt_out.i_bitrate = i_bitrate;
> + msg_Info(p_enc, "Setting optimal bitrate of %i", i_bitrate);
> + }
> + else
> + {
> + i_bitrate = p_enc->fmt_out.i_bitrate;
> + }
> + if ((erraac = aacEncoder_SetParam(p_sys->handle, AACENC_BITRATE,
> + i_bitrate)) != AACENC_OK) {
> + msg_Err(p_enc, "Unable to set the bitrate %i: %s", i_bitrate,
> + aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
> + }
> + if ((erraac = aacEncoder_SetParam(p_sys->handle, AACENC_TRANSMUX, 0)) != AACENC_OK) {
> + msg_Err(p_enc, "Unable to set the ADTS transmux: %s", aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
> + if ((erraac = aacEncoder_SetParam(p_sys->handle, AACENC_SIGNALING_MODE,
> + (int)p_sys->i_signaling)) != AACENC_OK) {
> + /* use explicit backward compatible =1 */
> + /* use explicit hierarchical signaling =2 */
> + msg_Err(p_enc, "Unable to set signaling mode: %s", aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
> + if ((erraac = aacEncoder_SetParam(p_sys->handle, AACENC_AFTERBURNER,
> + (int)p_sys->b_afterburner)) != AACENC_OK) {
> + msg_Err(p_enc, "Unable to set the afterburner mode: %s", aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
> + if ((erraac = aacEncEncode(p_sys->handle, NULL, NULL, NULL, NULL)) != AACENC_OK) {
> + msg_Err(p_enc, "Unable to initialize the encoder: %s", aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
> + AACENC_InfoStruct info = { 0 };
> + if ((erraac = aacEncInfo(p_sys->handle, &info)) != AACENC_OK) {
> + msg_Err(p_enc, "Unable to get the encoder info: %s", aac_get_errorstring(erraac));
> + free( p_sys );
> + return VLC_EGENERIC;
> + }
Ok, you need a special function to check the parsing of those params and
the settings. You should do a common error path (goto ftw)
> + if( p_enc->fmt_out.i_extra )
> + {
> + p_enc->fmt_out.p_extra = malloc( p_enc->fmt_out.i_extra );
> + if ( p_enc->fmt_out.p_extra == NULL )
> + {
> + msg_Err(p_enc, "Unable to allocate fmt_out.p_extra");
> + free( p_sys );
> + return VLC_EGENERIC;
Same here.
> + // TODO: Add more debug info to this config printout
> + msg_Dbg(p_enc, "fmt_out.p_extra = %i", p_enc->fmt_out.i_extra);
Make it under #ifndef NDEBUG
> +static block_t *EncodeAudio( encoder_t *p_enc, aout_buffer_t *p_aout_buf )
> + encoder_sys_t *p_sys = p_enc->p_sys;
> + int16_t *p_buffer = (int16_t *)p_aout_buf->p_buffer;
> + int i_samples = p_aout_buf->i_nb_samples;
> + block_t *p_chain = NULL;
> + AACENC_ERROR erraac;
> + int i_loop_count = 0;;
> + int i_samples_left = i_samples;
> + mtime_t i_pts_out;
A bit of style, please.
> + //msg_Dbg( p_enc, "i_samples_left %i, i_samples %i",
> + // i_samples_left, i_samples);
Remove this.
> + /* The maximum packet size is 6144 bits aka 768 bytes per channel. */
> + uint8_t outbuf[20480];
Don't use magic numbers in the code, then.
> diff --git a/modules/stream_out/transcode/transcode.c b/modules/stream_out/transcode/transcode.c
> index f9f5e64..526dbf4 100644
> --- a/modules/stream_out/transcode/transcode.c
> +++ b/modules/stream_out/transcode/transcode.c
This should be in another patch.
Please:
- reindent the Encoding part
- remove tabs (2 or 3 left)
- remove trailing spaces
Best,
--
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device
More information about the vlc-devel
mailing list