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

[Andreas Rheinhardt]

Marton Balint:
> 
> 
> 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.
> 
This bsf (like all bsfs) can also be used standalone, i.e. not as
automatically inserted bsf for muxing. Is it still checked in this scenario?

>>
>>> +    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 deemed this an API violation, but that was premature, sorry. You are
right: It rally seems to be the most simple way to implement this. And
although this loop will currently terminate after at most two
iterations, nothing bad would happen if it were more (there have been
proposals for changes to the bsf API recently and although this proposal
did not intend to store more than one unfiltered packet, being
compatible with this possibility is good and not a defect).

- Andreas