On 11/07/2012 2:17 PM, Martin Storsjö wrote:
>> 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.
OK.
>> 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.
I misread this part. Please ignore.
>
>>> + 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.
Well, perhaps there are pre-defined channel orderings for wav files.
For some reason I was thinking of channel masks.
>>> + 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.
Seems I missed the inverse bit. My question is why do the both use
CODEC_FLAG_GLOBAL_HEADER? Seems pretty hacky and confusing to me.
>> 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.
Thanks.
>> 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.
I'm more wondering if this is a valid thing for libfdk-aac to do? I'm
clearly no AAC expert.
And, apologies for some of the obvious things I didn't notice.
- Derek
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
