On 16/04/14 15:47, wm4 wrote:
> On Wed, 16 Apr 2014 09:00:15 +0200
> Luca Barbato <[email protected]> wrote:
>
>> ---
>> libavresample/avresample.h | 54 +++++++++++++++++++++
>> libavresample/utils.c | 118
>> +++++++++++++++++++++++++++++++++++++++++++++
>> libavutil/error.h | 2 +
>> 3 files changed, 174 insertions(+)
>>
>> diff --git a/libavresample/avresample.h b/libavresample/avresample.h
>> index 3358628..702a767 100644
>> --- a/libavresample/avresample.h
>> +++ b/libavresample/avresample.h
>> @@ -96,6 +96,7 @@
>> #include "libavutil/avutil.h"
>> #include "libavutil/channel_layout.h"
>> #include "libavutil/dict.h"
>> +#include "libavutil/frame.h"
>> #include "libavutil/log.h"
>>
>> #include "libavresample/version.h"
>> @@ -410,6 +411,59 @@ int avresample_available(AVAudioResampleContext *avr);
>> */
>> int avresample_read(AVAudioResampleContext *avr, uint8_t **output, int
>> nb_samples);
>>
>> +
>> +
>> +/**
>> + * Convert the samples in the input AVFrame and write them to the output
>> AVFrame.
>> + *
>> + * Input and output AVFrames must have channel_layout, sample_rate and
>> format set.
>> + *
>> + * The upper bound on the number of output samples is given by
>> + * avresample_available() + (avresample_get_delay() + number of input
>> samples) *
>> + * output sample rate / input sample rate.
>
> This isn't an exact count and should specify rounding. If the result of
> the division is not an integer, should it be rounded up or not? (If you
> do defensive programming, you'd round it up always, but maybe it's
> better not to rely on the user getting these things right.)
>
>> + * If the output AVFrame does not have the data pointers allocated a the
>> nb_samples
>
> "a the" -> typo?
>
>> + * will be set as described above and av_frame_get_buffer() will be called.
>
> You should specify whether a memory pool is used or not - I imagine it
> might matter for performance.
>
>> + * The output AVFrame can be NULL or have fewer allocated samples than
>> required.
>> + *
>> + * In this case, any remaining samples not written to the output will be
>> added
>> + * to an internal FIFO buffer, to be returned at the next call to this
>> function
>> + * or to avresample_convert() or to avresample_read().
>> + *
>> + * If converting sample rate, there may be data remaining in the internal
>> + * resampling delay buffer. avresample_get_delay() tells the number of
>> remaining
>> + * samples. To get this data as output, call this function or
>> avresample_convert()
>> + * with NULL input.
>> + *
>> + * At the end of the conversion process, there may be data remaining in the
>> + * internal FIFO buffer. avresample_available() tells the number of
>> remaining
>> + * samples. To get this data as output, either call this function or
>> avresample_convert()
>> + * with NULL input or call avresample_read().
>> + *
>> + * If the AVAudioResampleContext configuration does not match the output
>> and input AVFrame
>> + * settings the conversion does not take place and depending on which
>> AVFrame is not
>> + * matching AVERROR_OUTPUT_CHANGED, AVERROR_INPUT_CHANGED or
>> + * AVERROR_OUTPUT_CHANGED|AVERROR_INPUT_CHANGED is returned.
>
> You should also mention that the context needs to be initialized, and
> _how_ (maybe just mentioning avresample_config() is enough).
Actually if the context is not it would.
> IMO there should be a way to test whether the AVFrame configuration
> changes, so that the user can do its own flushing.
The function used to check can be made public, it is another point I'd
like to have feedback to.
> As far as I understand, avresample_config() actually flushes
> everything, so it's not enough. So maybe something like
> int avresample_needs_config(AVFrame *in, AVFrame *out) should be added.
avresample_config() resets the internal state and can be omitted since
the convert_frame would call it for you.
> The user should be encouraged to actually do the checking, maybe even
> provide example code for a proper conversion loop. Almost every API
> user will need such a conversion loop, but it's probably better than
> making the API trickier.
Yes, I will =)
> (Here's an alternative suggestion though: allow avresample_convert_frame
> to accept partial input... then it could handle flushing itself easily,
> though the user still needs to check whether the input frame was
> consumed. It also would change the AVFrame data pointers to advance the
> sample positions... well, maybe this suggestion is a bad idea.)
avresample_conver_frame(avr, out, NULL) should work for the purpose.
> As far as I can tell, it actually returns 0 on success, and sets
> nb_samples on the output frame instead.
It is one of the open questions, which do you like better?
>> + */
>> +int avresample_convert_frame(AVAudioResampleContext *avr,
>> + AVFrame *output, AVFrame *input);
>> +
>> +int avresample_config(AVAudioResampleContext *avr, AVFrame *out, AVFrame
>> *in,
>> + AVDictionary **opts);
>
> Obviously missing doxygen. But it seems straightforward enough. I find
> the opts parameter a bit strange. You could just set such options on
> the avr context itself. I guess the question is, how do we handle if
> certain parameters get moved to AVFrame: is the AVFrame value
> preferred? Or the context options? Do the context options even indicate
> always whether they were set by the user, or is it not possible to
> distinguish set options from options that are just initialized to their
> default value?
This function would match
avresample_open2(avr, opts)
That will be introduced later (see the blueprint on the wiki).
> Depending on the answer, going with opts would be best, but then the
> avr context AVOptions mechanism becomes at least partially
> dangerous/useless: what happens if the user sets such an AVOption on
> the context, but a later extension of AVFrame overrides it?
The options should set details that the avframe does not deliver (e.g.
resampler specific tunings).
>> +int avresample_config(AVAudioResampleContext *avr, AVFrame *out, AVFrame
>> *in,
>> + AVDictionary **opts)
>> +{
>> + int ret;
>> +
>> + if (avresample_is_open(avr)) {
>> + avresample_close(avr);
>> + }
>
> This sure looks strange. Maybe avresample_close() should work even on a
> closed context.
Actually it should, we might introduce an avresample_reset though.
>> +static int config_changed(AVAudioResampleContext *avr,
>> + AVFrame *out, AVFrame *in)
>> +{
>> + int ret = 0;
>> +
>> + if (in) {
>> + if (avr->in_channel_layout != in->channel_layout ||
>> + avr->in_sample_rate != in->sample_rate ||
>> + avr->in_sample_fmt != in->format) {
>> + ret |= AVERROR_INPUT_CHANGED;
>> + }
>> + }
>> +
>> + if (out) {
>> + if (avr->out_channel_layout != out->channel_layout ||
>> + avr->out_sample_rate != out->sample_rate ||
>> + avr->out_sample_fmt != out->format) {
>> + ret |= AVERROR_OUTPUT_CHANGED;
>> + }
>> + }
>> +
>> + return ret;
>> +}
This one could be exposed in case one wants do its own wrappers, maybe.
>> +
>> +static inline int convert_frame(AVAudioResampleContext *avr,
>> + AVFrame *out, AVFrame *in)
>> +{
>> + int ret;
>> + uint8_t **out_data = NULL, **in_data = NULL;
>> + int out_linesize = 0, in_linesize = 0;
>> + int out_nb_samples = 0, in_nb_samples = 0;
>> +
>> + if (out) {
>> + out_data = out->extended_data;
>> + out_linesize = out->linesize[0];
>> + out_nb_samples = out->nb_samples;
>> + }
>> +
>> + if (in) {
>> + in_data = in->extended_data;
>> + in_linesize = in->linesize[0];
>> + in_nb_samples = in->nb_samples;
>> + }
>> +
>> + ret = avresample_convert(avr, out_data, out_linesize,
>> + out_nb_samples,
>> + in_data, in_linesize,
>> + in_nb_samples);
>> +
>> + if (ret > 0) {
>> + out->nb_samples = ret;
>> + return 0;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static inline int output_frame_samples(AVAudioResampleContext *avr,
>> + AVFrame *out, AVFrame *in)
>> +{
>> + return avresample_available(avr) +
>> + (avresample_get_delay(avr) + in->nb_samples) *
>> + out->sample_rate / in->sample_rate;
>> +}
>> +
>> +int avresample_convert_frame(AVAudioResampleContext *avr,
>> + AVFrame *out, AVFrame *in)
>> +{
>> + int ret;
>> +
>> + if (!avresample_is_open(avr)) {
>> + if ((ret = avresample_config(avr, out, in, NULL)) < 0)
>> + return ret;
>> + if ((ret = avresample_open(avr)) < 0)
>> + return ret;
>
> Doesn't avresample_config already open the context?
I was undecided where to put the open call. Thus the rfc =)
> It's very unusual that AVERROR codes can be bitwise or-ed, so maybe
> this should be documented. What's with these random looking magic
> numbers anyway?
Low collision chances =) Initially those were having the AVRESAMPLE_
namespace.
I'll add more documentation anyway.
Thanks for the review =)
lu
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
