On 11/07/2012 8:38 AM, Martin Storsjö wrote:
> For enabling VBR, the general consensus seems to be to use the
> qscale flag. There doesn't seem to be any consistent way to
> indicate the actual desired quality though. Both libfaac and
> libmp3lame calculate avctx->global_quality / FF_QP2LAMBDA and set
> that as the libraries' VBR quality parameters, with wildly different
> results. On libmp3lame, the VBR quality parameter is between 0 (best)
> and 10 (worst), while the scale goes in the opposite direction for
> libfaac, where higher quality values gives you better quality.
>
> Therefore, for now, I just pass the actual value of
> avctx->global_quality through. You can set it to values between 1
> and 5:
> 1 - about 32 kbps/channel
> 2 - about 40 kbps/channel
> 3 - about 48-56 kbps/channel
> 4 - about 64 kbps/channel
> 5 - about 80-96 kbps/channel
> ---
> -Spun off Google Android sources, OpenCore and VisualOn libraries provide
> +Spun off Google Android sources, OpenCore, VisualOn and Fraunhofer
> +libraries provide
> encoders for a number of audio codecs.
These lines should probably be merged, no?
> +static const AVOption aac_enc_options[] = {
> + { "afterburner", "Afterburner (improved quality)", offsetof(AACContext,
> afterburner), AV_OPT_TYPE_INT, { 1 }, 0, 1, AV_OPT_FLAG_AUDIO_PARAM |
> AV_OPT_FLAG_ENCODING_PARAM },
> + { "eld_sbr", "Enable SBR for ELD (for SBR in other configurations, use
> the -profile parameter)", offsetof(AACContext, eld_sbr), AV_OPT_TYPE_INT, { 0
> }, 0, 1, AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_ENCODING_PARAM },
> + { "signaling", "SBR/PS signaling style", offsetof(AACContext,
> signaling), AV_OPT_TYPE_INT, { -1 }, -1, 2, AV_OPT_FLAG_AUDIO_PARAM |
> AV_OPT_FLAG_ENCODING_PARAM, "signaling" },
> + { "default", "Choose signaling implicitly (explicit hierarchical by
> default, implicit if global header is disabled)", 0, AV_OPT_TYPE_CONST, { -1
> }, 0, 0, AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_ENCODING_PARAM, "signaling" },
> + { "implicit", "Implicit backwards compatible signaling", 0,
> AV_OPT_TYPE_CONST, { 0 }, 0, 0, AV_OPT_FLAG_AUDIO_PARAM |
> AV_OPT_FLAG_ENCODING_PARAM, "signaling" },
> + { "explicit_sbr", "Explicit SBR, implicit PS signaling", 0,
> AV_OPT_TYPE_CONST, { 1 }, 0, 0, AV_OPT_FLAG_AUDIO_PARAM |
> AV_OPT_FLAG_ENCODING_PARAM, "signaling" },
> + { "explicit_hierarchical", "Explicit hierarchical signaling", 0,
> AV_OPT_TYPE_CONST, { 2 }, 0, 0, AV_OPT_FLAG_AUDIO_PARAM |
> AV_OPT_FLAG_ENCODING_PARAM, "signaling" },
> + { NULL }
> +};
> +
> +static const AVClass aac_enc_class = {
> + "libfdk_aac", av_default_item_name, aac_enc_options,
> LIBAVUTIL_VERSION_INT
> +};
Both of these can be broken up nicely so they're not lolhuge.
This can be done in many areas in this patch, so I will
refrain from noting them all.
> +#if FF_API_OLD_ENCODE_AUDIO
> + av_freep(&avctx->coded_frame);
> +#endif
Unrelated to the review -- are we dropping this sort of
thing for the next release?
> +static av_cold int aac_encode_init(AVCodecContext *avctx)
> +{
> + AACContext *s = avctx->priv_data;
> + int ret = AVERROR(EINVAL);
> + AACENC_InfoStruct info = { 0 };
> + CHANNEL_MODE mode;
> + AACENC_ERROR err;
> + int aot = FF_PROFILE_AAC_LOW + 1;
> +
> + if ((err = aacEncOpen(&s->handle, 0, avctx->channels)) != AACENC_OK) {
> + av_log(avctx, AV_LOG_ERROR, "Unable to open the encoder: %s\n",
> aac_get_error(err));
> + goto error;
> + }
I know this is personal taste, but is there not a more elegant
way to handle errors? :/
> + if (avctx->profile != FF_PROFILE_UNKNOWN)
> + aot = avctx->profile + 1;
> + if ((err = aacEncoder_SetParam(s->handle, AACENC_AOT, aot)) !=
> AACENC_OK) {
> + av_log(avctx, AV_LOG_ERROR, "Unable to set the AOT %d: %s\n", aot,
> aac_get_error(err));
> + goto error;
> + }
Needs a line break between these two if statements. Also, perhaps
it should print a warning if it is passed an invalid profile?
Might be better than silently falling back on aac_low.
> + if ((err = aacEncoder_SetParam(s->handle, AACENC_CHANNELORDER, 1)) !=
> AACENC_OK) {
> + av_log(avctx, AV_LOG_ERROR, "Unable to set wav channel order %d:
> %s\n", mode, aac_get_error(err));
> + goto error;
> + }
Not sure what 'wav channel order' means? WAVEFORMATEXTENSIBLE?
> + if (avctx->flags & CODEC_FLAG_QSCALE) {
> + int mode = av_clip(avctx->global_quality, 1, 5);
We should print a warning instead of silently clipping.
> + if ((err = aacEncoder_SetParam(s->handle, AACENC_TRANSMUX, avctx->flags
> & CODEC_FLAG_GLOBAL_HEADER ? 0 : 2)) != AACENC_OK) {
> + av_log(avctx, AV_LOG_ERROR, "Unable to set the transmux format:
> %s\n", aac_get_error(err));
> + goto error;
> + }
Can you add a comment or something here? It's not immediately
clear what's happening.
> + if (s->signaling < 0)
> + s->signaling = avctx->flags & CODEC_FLAG_GLOBAL_HEADER ? 2 : 0;
We use this exact thing directly above. Should be set, then used there
instead of duplicating code.
> + if ((err = aacEncoder_SetParam(s->handle, AACENC_AFTERBURNER,
> s->afterburner)) != AACENC_OK) {
> + av_log(avctx, AV_LOG_ERROR, "Unable to set afterburner to %d: %s\n",
> s->afterburner, aac_get_error(err));
> + goto error;
> + }
Isn't the only possible option for afterburner 0/1? Shouldn't we force
it to a boolean? (!!var)
> +#if FF_API_OLD_ENCODE_AUDIO
> + avctx->coded_frame = avcodec_alloc_frame();
> + if (!avctx->coded_frame)
> + return AVERROR(ENOMEM);
> +#endif
Shouldn't all the mallocing be done before we go through the
trouble of initializing the decoder? Also, this is the only
time goto error isn't used. Why?
> + if (!avctx->extradata) {
> + ret = AVERROR(ENOMEM);
> + goto error;
> + }
Isn't it possible, while using the old audio API, that coded_frame
gets allocated, but then extradata allocation fails, but coded_frame
is never freed? Seems like a memleak.
Secondly, does fdk-aac not have a cleanup function for all the mem
it has allocated?
> + in_ptr = frame->data[0];
> + in_buffer_size = 2 * avctx->channels * frame->nb_samples;
> + in_buffer_element_size = 2;
> +
> + in_args.numInSamples = avctx->channels * frame->nb_samples;
> + in_buf.numBufs = 1;
> + in_buf.bufs = &in_ptr;
> + in_buf.bufferIdentifiers = &in_buffer_identifier;
> + in_buf.bufSizes = &in_buffer_size;
> + in_buf.bufElSizes = &in_buffer_element_size;
In the words of Diego...
nit: Align.
> + if ((ret = ff_alloc_packet(avpkt, FFMAX(8192, 768 * avctx->channels)))) {
> + av_log(avctx, AV_LOG_ERROR, "Error getting output packet\n");
> + return ret;
> + }
Comment on where FFMAX(8192, 768 * avctx->channels) comes from, please.
> + out_ptr = avpkt->data;
> + out_buffer_size = avpkt->size;
> + out_buffer_element_size = 1;
> + out_buf.numBufs = 1;
> + out_buf.bufs = &out_ptr;
> + out_buf.bufferIdentifiers = &out_buffer_identifier;
> + out_buf.bufSizes = &out_buffer_size;
> + out_buf.bufElSizes = &out_buffer_element_size;
The alignment faery beckons ye!
> + if (!out_args.numOutBytes)
> + return 0;
Is this really proper?
> + /* Get the next frame pts/duration */
> + ff_af_queue_remove(&s->afq, avctx->frame_size, &avpkt->pts,
> + &avpkt->duration);
s#pts/#pts & #
> + avpkt->size = out_args.numOutBytes;
> + *got_packet_ptr = 1;
Needs faery dust.
- Derek
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
