[vlc-devel] new aac encoder for review

Sergio M. Ammirata, Ph.D. sergio at ammirata.net
Sat Oct 6 20:02:06 CEST 2012


Here is the updated patch set for VLC. Also, the fdk-aac lib patch already
made it to git, so you just need to grab latest from there.

I also changed the EOF trigger to a null buffer as discussed in IRC.

More comments below Š

-- 
Sergio M. Ammirata, Ph.D.



On 10/4/12 7:14 PM, "Jean-Baptiste Kempf" <jb at videolan.org> wrote:

>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?

No, I did not.

>
>> --- 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.

Done Š The extra variable (b_purge_required) and "if" condition can be
removed once all other audio encoders are patched to accept a null buffer
as an EOF trigger.

>
>> +#include <stdio.h>
>> +#include <stdint.h>
>> +#include <unistd.h>
>> +#include <stdlib.h>
>You need all those?

It turns out I did not need any of them.

>
>> +/* 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?


Deleted

>
>> +//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.

Deleted

>
>> +    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

Fixed

>
>> +    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.

Fixed


>
>> +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 :)

Done

>
>> +    /* 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.

Done

>
>> +    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?

Changed them all to 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)

Added new error path with appropriate encoder closing function.

>
>> +    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

Done

>
>> +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.

Done

>
>> +      //msg_Dbg( p_enc, "i_samples_left %i, i_samples %i",
>> +      //              i_samples_left, i_samples);
>Remove this.

Done

>
>> +      /* 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.

Changed it to the true max buffer size.

>
>> 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.

Done

>
>Please:
> - reindent the Encoding part
> - remove tabs (2 or 3 left)
> - remove trailing spaces

I think I got all the tabs and trailing spaces and indentation right now Š

>
>Best,
>
>-- 
>Jean-Baptiste Kempf
>http://www.jbkempf.com/ - +33 672 704 734
>Sent from my Electronic Device
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>http://mailman.videolan.org/listinfo/vlc-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: fdkaac_vlc_encoder_module.patch
Type: application/octet-stream
Size: 22656 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20121006/53cc6dc0/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vlc_purge_audio_encoder.patch
Type: application/octet-stream
Size: 1980 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20121006/53cc6dc0/attachment-0001.obj>


More information about the vlc-devel mailing list