[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