On Wed, 11 Jul 2012, Derek Buitenhuis wrote:
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?
I guess they could - this keeps the diff clearer and smaller IMO, but if
you prefer I can rewrap the lines...
+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.
Umm, you mean wrapping the lines? I prefer not doing that for the
avoptions, that usually ends up even more unreaable. I can wrap some of
the function calls below though.
+#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?
At the next ABI bump I think, yes.
+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.
Where does it silently fall back on aac_low? If you have set
avctx->profile, we will use that profile, and if the library doesn't
support it, it will fail at this point, indicating that it doesn't support
this AOT.
+ 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?
I guess that's the proper name for it. The encoder has a choice between
mpeg channel ordering and wav file format channel ordering, where the
latter is what we use in libavcodec.
+ if (avctx->flags & CODEC_FLAG_QSCALE) {
+ int mode = av_clip(avctx->global_quality, 1, 5);
We should print a warning instead of silently clipping.
Ok
+ 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.
Will do
+ 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.
No, it's not exactly the same, it's the inverse, and the fact that both
use numbers 0 and 2 is a coincidence.
Above is
if (extradata requested) { mp4 format } else { adts format }.
This one is:
if (no signalling mode requested by the caller)
if (extradata requested) { explicit hierarchical signalling } else
{ implicit signalling }
Where mp4 format is 0, adts format is 2, implicit signalling is 0 and
hierarchical signalling is 2.
+ 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)
It shouldn't be needed, the AVOption limits it to the range 0-1.
+#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?
It should use goto error, I'll fix it.
+ 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?
Umm, this one does use goto error, which calls aac_encode_close, which
frees coded_frame, and calls aacEncClose for freeing what the lib
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.
Will do
+ 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.
I copied this from vo-aacenc, where Justin added it at some point. I think
the encoder lib has a comment about the bounds of the encoded data, I'll
try to look it up.
+ 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?
I don't see what wouldn't be proper here. We consumed the input data, but
the encoder didn't output anything yet, so we signal that everything is ok
but no data was returned.
+ /* Get the next frame pts/duration */
+ ff_af_queue_remove(&s->afq, avctx->frame_size, &avpkt->pts,
+ &avpkt->duration);
s#pts/#pts & #
Ok
+ avpkt->size = out_args.numOutBytes;
+ *got_packet_ptr = 1;
Needs faery dust.
Ok
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
