[FFmpeg-devel] [PATCH 7/8] avcodec/pcm_rechunk_bsf: add bitstream filter to rechunk pcm audio

Marton Balint cus at passwd.hu
Sat Apr 18 21:44:15 EEST 2020



On Tue, 7 Apr 2020, Andreas Rheinhardt wrote:

> Marton Balint:
>> Signed-off-by: Marton Balint <cus at passwd.hu>
>> ---
>>  Changelog                      |   1 +
>>  doc/bitstream_filters.texi     |  30 ++++++
>>  libavcodec/Makefile            |   1 +
>>  libavcodec/bitstream_filters.c |   1 +
>>  libavcodec/pcm_rechunk_bsf.c   | 206 +++++++++++++++++++++++++++++++++++++++++
>>  libavcodec/version.h           |   4 +-
>>  6 files changed, 241 insertions(+), 2 deletions(-)
>>  create mode 100644 libavcodec/pcm_rechunk_bsf.c
>> 
>> diff --git a/Changelog b/Changelog
>> index 05b9a84562..dddaf02199 100644
>> --- a/Changelog
>> +++ b/Changelog
>> @@ -55,6 +55,7 @@ version <next>:
>>  - CRI HCA decoder
>>  - CRI HCA demuxer
>>  - overlay_cuda filter
>> +- pcm_rechunk bitstream filter
>>

[..]

>> +static int init(AVBSFContext *ctx)
>> +{
>> +    PCMContext *s = ctx->priv_data;
>> +    AVRational sr = av_make_q(ctx->par_in->sample_rate, 1);
>> +    int64_t max_samples;
>> +
>> +    ctx->time_base_out = av_inv_q(sr);
>
> Is it actually guaranteed that par_in->sample_rate is not 0?

Yes, it is checked in mux.c:init_muxer.

>
>> +    s->in_pkt = av_packet_alloc();
>> +    s->out_pkt = av_packet_alloc();
>> +    if (!s->in_pkt || !s->out_pkt)
>> +        return AVERROR(ENOMEM);
>
> These allocations will have been wasted if one errors out below, so they
> should be moved to the end of this function.

Ok.

[..]


>> +static int rechunk_filter(AVBSFContext *ctx, AVPacket *pkt)
>> +{
>> +    PCMContext *s = ctx->priv_data;
>> +    AVRational sr = av_make_q(ctx->par_in->sample_rate, 1);
>> +    int nb_samples = s->frame_rate.num ? (av_rescale_q(s->n + 1, sr, s->frame_rate) - s->dts) : s->nb_out_samples;
>> +    int data_size = nb_samples * s->sample_size;
>> +    int ret;
>> +
>> +    if (!s->out_pkt->data) {
>> +        ret = av_new_packet(s->out_pkt, s->max_packet_size);
>> +        if (ret < 0)
>> +            return ret;
>> +        s->out_pkt->size = 0;
>> +    }
>> +
>> +    do {
>> +        if (s->in_pkt->size) {
>> +            if (s->out_pkt->size || s->in_pkt->size < data_size) {
>> +                int drain = FFMIN(s->in_pkt->size, data_size - s->out_pkt->size);
>> +                if (!s->out_pkt->size) {
>> +                    ret = av_packet_copy_props(s->out_pkt, s->in_pkt);
>> +                    if (ret < 0)
>> +                        return ret;
>> +                }
>> +                memcpy(s->out_pkt->data + s->out_pkt->size, s->in_pkt->data, drain);
>> +                s->out_pkt->size += drain;
>> +                s->in_pkt->size -= drain;
>> +                s->in_pkt->data += drain;
>
> This could be aligned on =.

Ok.

>
>> +                if (s->out_pkt->size == data_size) {
>> +                    av_packet_move_ref(pkt, s->out_pkt);
>
> If the current pkt is a packet with a smaller amount of samples than the
> maximum, then the data immediately after the packet data will not be the
> (zeroed) padding, but uninitialized data before the zeroed padding. This
> is not good (it won't lead to segfaults, but it might lead to Valgrind
> warnings). See below for a suggestion how to fix this.

Ok.

>
>> +                    return send_packet(s, nb_samples, pkt);
>> +                }
>> +                av_packet_unref(s->in_pkt);
>
> If out_pkt initially already contained data and a new in_pkt provides
> exactly as much data as needed to output another packet, then you will
> set in_pkt->size to zero above, but you will do not unref it. Given that
> the code treats "size == 0" as sign that the packet is blank, this will
> lead to memleaks.

Ok.

>
>> +            } else if (s->in_pkt->size > data_size) {
>> +                ret = av_packet_ref(pkt, s->in_pkt);
>> +                if (ret < 0)
>> +                    return ret;
>> +                pkt->size = data_size;
>> +                s->in_pkt->size -= data_size;
>> +                s->in_pkt->data += data_size;
>> +                return send_packet(s, nb_samples, pkt);
>> +            } else {
>> +                av_assert0(s->in_pkt->size == data_size);
>> +                av_packet_move_ref(pkt, s->in_pkt);
>> +                return send_packet(s, nb_samples, pkt);
>> +            }
>> +        }
>> +
>> +        ret = ff_bsf_get_packet_ref(ctx, s->in_pkt);
>
> Doing this here in a loop is either pointless or an API violation (but
> the internal API is not really documented anyway): The caller is
> supposed to provide a packet via av_bsf_send_packet() and then call
> av_bsf_receive_packet() until the bsf is completely drained. Then he
> needs to send a new packet. The bsf meanwhile uses
> ff_bsf_get_packet[_ref] to get the packet when
> AVBitStreamFilter.filter() is executed. The bsf API implies that it is
> impossible for two ff_bsf_get_packet_ref() calls to succeed in the same
> AVBitStreamFilter.filter() call, so that your loop is actually a fake loop.

It is true that the loop is executed maximum 2 times, yet this loop seemed 
(and still seems to me) the most simple way to implement processing of an 
existing in_pkt and the next in_pkt.

I only rely on that ff_bsf_get_packet_ref() returns EAGAIN on subsequent 
calls, and this looks like a safe bet. I don't see how a different 
implementation can yield simpler code.

>
> I therefore suggest to adapt rechunk_filter() as follows:
> a) There is no reason at all why both in_pkt and out_pkt should be
> non-blank at the same time (except during the brief period when one
> copies from in to out, of course). The code needs to ensure this.
> b) At the start of rechunk_filter(), you check for whether there is
> enough data in in_pkt to output a complete packet. If there is, you
> output it (and update in_pkt); if there is some, but not enough data in
> in_pkt, you allocate exactly as much space in out_pkt as needed for the
> next output packet, copy the remaining data (as well as the properties)
> from in_pkt and unref in_pkt. (There is unfortunately no
> av_packet_move_props(), maybe there should be one?)
> c) If you did not exit rechunk_filter() in b), you read another packet
> into in_pkt.
> If out_pkt is empty and in_pkt contains enough data, you output this data.
> If out_pkt is empty and in_pkt contains not enough data, you return
> AVERROR(EAGAIN).
> If out_pkt is not empty, you copy as much data as needed/available into
> out_pkt (which already has the right size); if there is enough data, you
> output out_pkt (and potentially unref in_pkt if it has been exhausted).
> If there is not enough data, you unref in_pkt and return AVERROR(EAGAIN).
>
> (Notice that max_packet_size can be removed from PCMContext.)
>
> In case you are wondering whether a) could be used to omit one of the
> packets: It is possible to omit out_pkt (but I am not sure how
> advantageous it would be):
>
> b') If there is enough data in in_pkt for a complete packet, you output
> it (via av_packet_move_ref() or av_packet_ref()); if not and if there is
> data left in in_pkt and if the actual buffer of in_pkt is not already of
> the right size, you copy the props of in_pkt to pkt and allocate a
> buffer of the right size (with padding, of course) in pkt and copy the
> remaining data of in_pkt to pkt and reset in_pkt. (Alternatively, you
> could move in_pkt to pkt and directly allocate an AVBufferRef with the
> right size, copy the data, unref the old buffer and replace it with the
> new one.)
> If there is not enough data in in_pkt and if in_pkt already contains a
> buffer of the right size, then you simply move in_pkt to pkt.
>
> c') Then you get a new packet and put it into in_pkt. In case pkt
> already contains data, you copy as much data as needed/available into
> pkt. If it is enough, you output pkt. If it is not enough, in_pkt can be
> unreferenced and you move pkt into in_pkt and return AVERROR(EAGAIN).
>
> It should go without saying that you need to somehow record whether
> in_pkt already contains a buffer of the right size (this is trivial in
> the approach above: if out_pkt contains data it also contains a buffer
> of the right size). I prefer the first approach.

Yeah, me too, that is what I (hopefully) will implement in the next 
version of the patch.

>
>> +        if (ret == AVERROR_EOF && s->out_pkt->size) {
>> +            if (s->pad) {
>> +                memset(s->out_pkt->data + s->out_pkt->size, 0, data_size - s->out_pkt->size);
>> +                s->out_pkt->size = data_size;
>> +            } else {
>> +                nb_samples = s->out_pkt->size / s->sample_size;
>> +            }
>> +            av_packet_move_ref(pkt, s->out_pkt);
>> +            return send_packet(s, nb_samples, pkt);
>> +        }
>> +    } while (ret >= 0);
>> +
>> +    return ret;
>> +}
>> +
>> +#define OFFSET(x) offsetof(PCMContext, x)
>> +#define FLAGS (AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_BSF_PARAM)
>> +static const AVOption options[] = {
>> +    { "nb_out_samples", "set the number of per-packet output samples", OFFSET(nb_out_samples),   AV_OPT_TYPE_INT, {.i64=1024}, 1, INT_MAX, FLAGS },
>> +    { "n",              "set the number of per-packet output samples", OFFSET(nb_out_samples),   AV_OPT_TYPE_INT, {.i64=1024}, 1, INT_MAX, FLAGS },
>> +    { "pad",            "pad last packet with zeros",                  OFFSET(pad),             AV_OPT_TYPE_BOOL, {.i64=1} ,   0,       1, FLAGS },
>> +    { "p",              "pad last packet with zeros",                  OFFSET(pad),             AV_OPT_TYPE_BOOL, {.i64=1} ,   0,       1, FLAGS },
>> +    { "frame_rate",     "set number of packets per second",            OFFSET(frame_rate),  AV_OPT_TYPE_RATIONAL, {.dbl=0},    0, INT_MAX, FLAGS },
>> +    { "r",              "set number of packets per second",            OFFSET(frame_rate),  AV_OPT_TYPE_RATIONAL, {.dbl=0},    0, INT_MAX, FLAGS },
>> +    { NULL },
>> +};
>> +
>> +static const AVClass metadata_class = {
>
> You seem to have copied this name from the *_metadata_bsf bitstream
> filters. It is not really appropriate here.

Yes, will change it.

Thanks,
Marton


More information about the ffmpeg-devel mailing list