diff mbox series

[FFmpeg-devel,2/2] avcodec/bsf: add av_bsf_join() to chain bitstream filters

Message ID 20200408224231.23126-2-cus@passwd.hu
State New
Headers show
Series [FFmpeg-devel,1/2] avcodec/bsf: use simplified algorithm for bsf_list chained filtering
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork warning New warnings during build
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Marton Balint April 8, 2020, 10:42 p.m. UTC
Signed-off-by: Marton Balint <cus@passwd.hu>
---
 doc/APIchanges       |  3 +++
 libavcodec/avcodec.h | 19 ++++++++++++++++
 libavcodec/bsf.c     | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 libavcodec/version.h |  2 +-
 4 files changed, 85 insertions(+), 1 deletion(-)

Comments

Andreas Rheinhardt April 9, 2020, 10:15 p.m. UTC | #1
Marton Balint:
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  doc/APIchanges       |  3 +++
>  libavcodec/avcodec.h | 19 ++++++++++++++++
>  libavcodec/bsf.c     | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  libavcodec/version.h |  2 +-
>  4 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index f1d7eac2ee..1473742139 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2020-04-xx - xxxxxxxxxx - lavc 58.77.102 - avcodec.h
> +  Add av_bsf_join() to chain bitstream filters.
> +
>  2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
>    av_read_frame() now guarantees to handle uninitialized input packets
>    and to return refcounted packets on success.
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 8fc0ad92c9..2812055e8a 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -6056,6 +6056,25 @@ int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt);
>   */
>  void av_bsf_flush(AVBSFContext *ctx);
>  
> +/**
> + * Join a new bitstream filter to an already operating one.
> + *
> + * @param[in,out] out_bsf *out_bsf contains the existing, already initialized bitstream
> + *                        filter, the new filter will be joined to the output of this.
> + *                        It can be NULL, in which case an empty filter is assumed.
> + *                        Upon successful return *out_bsf will contain the new, combined
> + *                        bitstream filter which will be initialized.
> + * @param[in]     bsf     the bitstream filter to be joined to the output of *out_bsf.
> + *                        This filter must be uninitialized but it must be ready to be
> + *                        initalized, so input codec parameters and time base must be
> + *                        set if *out_bsf is NULL, otherwise the function will set these
> + *                        automatically based on the output parameters of *out_bsf.
> + *                        Upon successful return the bsf will be initialized.
> + *
> + * @return 0 on success, negative on error.

One needs to be more explicit about what happens on error: bsf may be
in a partially initialized state and is essentially only good for
freeing (maybe the bsf parameter should be AVBSFContext **, too, and
automatically free bsf on error?). But IMO we should aim to not cripple
*out_bsf and document this.

> + */
> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf);
> +
>  /**
>   * Free a bitstream filter context and everything associated with it; write NULL
>   * into the supplied pointer.
> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
> index b9fc771a88..1bca28d1ae 100644
> --- a/libavcodec/bsf.c
> +++ b/libavcodec/bsf.c
> @@ -487,6 +487,68 @@ end:
>      return ret;
>  }
>  
> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf)
> +{
> +    BSFListContext *lst;
> +    AVBSFContext *obsf = *out_bsf;
> +    AVBSFContext *new_list_bsf = NULL;
> +    int ret;
> +
> +    if (!obsf) {
> +        ret = av_bsf_init(bsf);
> +        if (ret < 0)
> +            return ret;
> +        *out_bsf = bsf;
> +        return 0;
> +    }
> +
> +    if (obsf->filter != &ff_list_bsf) {
> +        ret = av_bsf_alloc(&ff_list_bsf, &new_list_bsf);
> +        if (ret < 0)
> +            return ret;
> +        lst = new_list_bsf->priv_data;
> +        ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, obsf);
> +        if (ret < 0)
> +            goto fail;
> +        ret = avcodec_parameters_copy(new_list_bsf->par_in, obsf->par_in);
> +        if (ret < 0)
> +            goto fail;
> +        new_list_bsf->time_base_in = obsf->time_base_in;
> +        obsf = new_list_bsf;
> +    } else {
> +        lst = obsf->priv_data;
> +    }
> +
> +    ret = avcodec_parameters_copy(bsf->par_in, lst->bsfs[lst->nb_bsfs-1]->par_out);
> +    if (ret < 0)
> +        goto fail;
> +    bsf->time_base_in = lst->bsfs[lst->nb_bsfs-1]->time_base_out;
> +
> +    ret = av_bsf_init(&bsf);
> +    if (ret < 0)
> +        goto fail;
> +
> +    ret = avcodec_parameters_copy(obsf->par_out, bsf->par_out);

This will change obsf even on failure (when *out_bsf was already of type
ff_list_bsf). This is something that ff_stream_add_bitstream_filter()
currently doesn't do. I think we should leave obsf in a mostly unchanged
state even if it implies an unnecessary allocation (it is really only a
single allocation). This can be achieved by taking obsf's par_out in the
else branch above and replacing it with fresh AVCodecParameters. On
success, the old par_out is freed; on failure it is restored.

> +    if (ret < 0)
> +        goto fail;
> +    obsf->time_base_out = bsf->time_base_out;
> +
> +    ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, bsf);
> +    if (ret < 0)
> +        goto fail;
> +
> +    *out_bsf = obsf;
> +    return 0;
> +
> +fail:
> +    if (new_list_bsf) {
> +        if (lst->nb_bsfs)
> +            lst->bsfs[0] = NULL;
> +        av_bsf_free(&new_list_bsf);
> +    }
> +    return ret;
> +}
> +
>  static int bsf_parse_single(const char *str, AVBSFList *bsf_lst)
>  {
>      char *bsf_name, *bsf_options_str, *buf;
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index f4d1d4de21..dadca75430 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -29,7 +29,7 @@
>  
>  #define LIBAVCODEC_VERSION_MAJOR  58
>  #define LIBAVCODEC_VERSION_MINOR  77
> -#define LIBAVCODEC_VERSION_MICRO 101
> +#define LIBAVCODEC_VERSION_MICRO 102

Adding a new public function always requires a minor bump.
>  
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>                                                 LIBAVCODEC_VERSION_MINOR, \
> 

Finally, (I feel bad having to tell you this given that you have clearly
invested a lot of time in this) we currently have no case where we need
more than one automatically inserted bitstream filter per stream. We do
not know whether the current code does the right thing in case more than
one packet needs to be analyzed to determine whether to insert bitstream
filters (maybe the packets should instead be cached so that no bsf
misses out on the initial packets?); of course what is the right thing
might be format and codec-dependent. So I don't know whether we should
add this function now (given that public API is hard to remove).

This does not mean that we should not use a bsf_list bsf to simplify
automatic bitstream filtering in mux.c (it really simplifies this alot
and gets rid of code duplication). But I think we should leave the topic
of adding a bsf to an already existing bsf open so that it can be
determined when the need indeed arises.

- Andreas
Marton Balint April 10, 2020, 12:11 a.m. UTC | #2
On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:

> Marton Balint:
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  doc/APIchanges       |  3 +++
>>  libavcodec/avcodec.h | 19 ++++++++++++++++
>>  libavcodec/bsf.c     | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  libavcodec/version.h |  2 +-
>>  4 files changed, 85 insertions(+), 1 deletion(-)
>> 
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index f1d7eac2ee..1473742139 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>
>>  API changes, most recent first:
>> 
>> +2020-04-xx - xxxxxxxxxx - lavc 58.77.102 - avcodec.h
>> +  Add av_bsf_join() to chain bitstream filters.
>> +
>>  2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
>>    av_read_frame() now guarantees to handle uninitialized input packets
>>    and to return refcounted packets on success.
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index 8fc0ad92c9..2812055e8a 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -6056,6 +6056,25 @@ int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt);
>>   */
>>  void av_bsf_flush(AVBSFContext *ctx);
>> 
>> +/**
>> + * Join a new bitstream filter to an already operating one.
>> + *
>> + * @param[in,out] out_bsf *out_bsf contains the existing, already initialized bitstream
>> + *                        filter, the new filter will be joined to the output of this.
>> + *                        It can be NULL, in which case an empty filter is assumed.
>> + *                        Upon successful return *out_bsf will contain the new, combined
>> + *                        bitstream filter which will be initialized.
>> + * @param[in]     bsf     the bitstream filter to be joined to the output of *out_bsf.
>> + *                        This filter must be uninitialized but it must be ready to be
>> + *                        initalized, so input codec parameters and time base must be
>> + *                        set if *out_bsf is NULL, otherwise the function will set these
>> + *                        automatically based on the output parameters of *out_bsf.
>> + *                        Upon successful return the bsf will be initialized.
>> + *
>> + * @return 0 on success, negative on error.
>
> One needs to be more explicit about what happens on error: bsf may be
> in a partially initialized state and is essentially only good for
> freeing (maybe the bsf parameter should be AVBSFContext **, too, and
> automatically free bsf on error?). But IMO we should aim to not cripple
> *out_bsf and document this.
>
>> + */
>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf);
>> +
>>  /**
>>   * Free a bitstream filter context and everything associated with it; write NULL
>>   * into the supplied pointer.
>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>> index b9fc771a88..1bca28d1ae 100644
>> --- a/libavcodec/bsf.c
>> +++ b/libavcodec/bsf.c
>> @@ -487,6 +487,68 @@ end:
>>      return ret;
>>  }
>> 
>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf)
>> +{
>> +    BSFListContext *lst;
>> +    AVBSFContext *obsf = *out_bsf;
>> +    AVBSFContext *new_list_bsf = NULL;
>> +    int ret;
>> +
>> +    if (!obsf) {
>> +        ret = av_bsf_init(bsf);
>> +        if (ret < 0)
>> +            return ret;
>> +        *out_bsf = bsf;
>> +        return 0;
>> +    }
>> +
>> +    if (obsf->filter != &ff_list_bsf) {
>> +        ret = av_bsf_alloc(&ff_list_bsf, &new_list_bsf);
>> +        if (ret < 0)
>> +            return ret;
>> +        lst = new_list_bsf->priv_data;
>> +        ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, obsf);
>> +        if (ret < 0)
>> +            goto fail;
>> +        ret = avcodec_parameters_copy(new_list_bsf->par_in, obsf->par_in);
>> +        if (ret < 0)
>> +            goto fail;
>> +        new_list_bsf->time_base_in = obsf->time_base_in;
>> +        obsf = new_list_bsf;
>> +    } else {
>> +        lst = obsf->priv_data;
>> +    }
>> +
>> +    ret = avcodec_parameters_copy(bsf->par_in, lst->bsfs[lst->nb_bsfs-1]->par_out);
>> +    if (ret < 0)
>> +        goto fail;
>> +    bsf->time_base_in = lst->bsfs[lst->nb_bsfs-1]->time_base_out;
>> +
>> +    ret = av_bsf_init(&bsf);
>> +    if (ret < 0)
>> +        goto fail;
>> +
>> +    ret = avcodec_parameters_copy(obsf->par_out, bsf->par_out);
>
> This will change obsf even on failure (when *out_bsf was already of type
> ff_list_bsf). This is something that ff_stream_add_bitstream_filter()
> currently doesn't do. I think we should leave obsf in a mostly unchanged
> state even if it implies an unnecessary allocation (it is really only a
> single allocation). This can be achieved by taking obsf's par_out in the
> else branch above and replacing it with fresh AVCodecParameters. On
> success, the old par_out is freed; on failure it is restored.
>
>> +    if (ret < 0)
>> +        goto fail;
>> +    obsf->time_base_out = bsf->time_base_out;
>> +
>> +    ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, bsf);
>> +    if (ret < 0)
>> +        goto fail;
>> +
>> +    *out_bsf = obsf;
>> +    return 0;
>> +
>> +fail:
>> +    if (new_list_bsf) {
>> +        if (lst->nb_bsfs)
>> +            lst->bsfs[0] = NULL;
>> +        av_bsf_free(&new_list_bsf);
>> +    }
>> +    return ret;
>> +}
>> +
>>  static int bsf_parse_single(const char *str, AVBSFList *bsf_lst)
>>  {
>>      char *bsf_name, *bsf_options_str, *buf;
>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>> index f4d1d4de21..dadca75430 100644
>> --- a/libavcodec/version.h
>> +++ b/libavcodec/version.h
>> @@ -29,7 +29,7 @@
>>
>>  #define LIBAVCODEC_VERSION_MAJOR  58
>>  #define LIBAVCODEC_VERSION_MINOR  77
>> -#define LIBAVCODEC_VERSION_MICRO 101
>> +#define LIBAVCODEC_VERSION_MICRO 102
>
> Adding a new public function always requires a minor bump.
>>
>>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>>                                                 LIBAVCODEC_VERSION_MINOR, \
>> 
>
> Finally, (I feel bad having to tell you this given that you have clearly
> invested a lot of time in this) we currently have no case where we need
> more than one automatically inserted bitstream filter per stream. We do
> not know whether the current code does the right thing in case more than
> one packet needs to be analyzed to determine whether to insert bitstream
> filters (maybe the packets should instead be cached so that no bsf
> misses out on the initial packets?); of course what is the right thing
> might be format and codec-dependent. So I don't know whether we should
> add this function now (given that public API is hard to remove).
>
> This does not mean that we should not use a bsf_list bsf to simplify
> automatic bitstream filtering in mux.c (it really simplifies this alot
> and gets rid of code duplication). But I think we should leave the topic
> of adding a bsf to an already existing bsf open so that it can be
> determined when the need indeed arises.

I see no sane way of sharing list bsf code other than adding public API, 
because mux.c is libavformat, bsf.c is libavcodec. This at least allows 
you to replace AVStreamInternal->nb_bsfcs and AVStreamInternal->bsfcs with 
a single bitstream filter to which you join additional ones.

Unless you want to remove the existing (admittedly unused, and potentially 
inadequate) support for multiple bitstream filters in AVStreamInternal, I 
really don't see a way which postpones adding this API.

Regards,
Marton
James Almer April 10, 2020, 12:34 a.m. UTC | #3
On 4/9/2020 9:11 PM, Marton Balint wrote:
> 
> 
> On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:
> 
>> Marton Balint:
>>> Signed-off-by: Marton Balint <cus@passwd.hu>
>>> ---
>>>  doc/APIchanges       |  3 +++
>>>  libavcodec/avcodec.h | 19 ++++++++++++++++
>>>  libavcodec/bsf.c     | 62
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  libavcodec/version.h |  2 +-
>>>  4 files changed, 85 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>> index f1d7eac2ee..1473742139 100644
>>> --- a/doc/APIchanges
>>> +++ b/doc/APIchanges
>>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>>
>>>  API changes, most recent first:
>>>
>>> +2020-04-xx - xxxxxxxxxx - lavc 58.77.102 - avcodec.h
>>> +  Add av_bsf_join() to chain bitstream filters.
>>> +
>>>  2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
>>>    av_read_frame() now guarantees to handle uninitialized input packets
>>>    and to return refcounted packets on success.
>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>> index 8fc0ad92c9..2812055e8a 100644
>>> --- a/libavcodec/avcodec.h
>>> +++ b/libavcodec/avcodec.h
>>> @@ -6056,6 +6056,25 @@ int av_bsf_receive_packet(AVBSFContext *ctx,
>>> AVPacket *pkt);
>>>   */
>>>  void av_bsf_flush(AVBSFContext *ctx);
>>>
>>> +/**
>>> + * Join a new bitstream filter to an already operating one.
>>> + *
>>> + * @param[in,out] out_bsf *out_bsf contains the existing, already
>>> initialized bitstream
>>> + *                        filter, the new filter will be joined to
>>> the output of this.
>>> + *                        It can be NULL, in which case an empty
>>> filter is assumed.
>>> + *                        Upon successful return *out_bsf will
>>> contain the new, combined
>>> + *                        bitstream filter which will be initialized.
>>> + * @param[in]     bsf     the bitstream filter to be joined to the
>>> output of *out_bsf.
>>> + *                        This filter must be uninitialized but it
>>> must be ready to be
>>> + *                        initalized, so input codec parameters and
>>> time base must be
>>> + *                        set if *out_bsf is NULL, otherwise the
>>> function will set these
>>> + *                        automatically based on the output
>>> parameters of *out_bsf.
>>> + *                        Upon successful return the bsf will be
>>> initialized.
>>> + *
>>> + * @return 0 on success, negative on error.
>>
>> One needs to be more explicit about what happens on error: bsf may be
>> in a partially initialized state and is essentially only good for
>> freeing (maybe the bsf parameter should be AVBSFContext **, too, and
>> automatically free bsf on error?). But IMO we should aim to not cripple
>> *out_bsf and document this.
>>
>>> + */
>>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf);
>>> +
>>>  /**
>>>   * Free a bitstream filter context and everything associated with
>>> it; write NULL
>>>   * into the supplied pointer.
>>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>>> index b9fc771a88..1bca28d1ae 100644
>>> --- a/libavcodec/bsf.c
>>> +++ b/libavcodec/bsf.c
>>> @@ -487,6 +487,68 @@ end:
>>>      return ret;
>>>  }
>>>
>>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf)
>>> +{
>>> +    BSFListContext *lst;
>>> +    AVBSFContext *obsf = *out_bsf;
>>> +    AVBSFContext *new_list_bsf = NULL;
>>> +    int ret;
>>> +
>>> +    if (!obsf) {
>>> +        ret = av_bsf_init(bsf);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +        *out_bsf = bsf;
>>> +        return 0;
>>> +    }
>>> +
>>> +    if (obsf->filter != &ff_list_bsf) {
>>> +        ret = av_bsf_alloc(&ff_list_bsf, &new_list_bsf);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +        lst = new_list_bsf->priv_data;
>>> +        ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, obsf);
>>> +        if (ret < 0)
>>> +            goto fail;
>>> +        ret = avcodec_parameters_copy(new_list_bsf->par_in,
>>> obsf->par_in);
>>> +        if (ret < 0)
>>> +            goto fail;
>>> +        new_list_bsf->time_base_in = obsf->time_base_in;
>>> +        obsf = new_list_bsf;
>>> +    } else {
>>> +        lst = obsf->priv_data;
>>> +    }
>>> +
>>> +    ret = avcodec_parameters_copy(bsf->par_in,
>>> lst->bsfs[lst->nb_bsfs-1]->par_out);
>>> +    if (ret < 0)
>>> +        goto fail;
>>> +    bsf->time_base_in = lst->bsfs[lst->nb_bsfs-1]->time_base_out;
>>> +
>>> +    ret = av_bsf_init(&bsf);
>>> +    if (ret < 0)
>>> +        goto fail;
>>> +
>>> +    ret = avcodec_parameters_copy(obsf->par_out, bsf->par_out);
>>
>> This will change obsf even on failure (when *out_bsf was already of type
>> ff_list_bsf). This is something that ff_stream_add_bitstream_filter()
>> currently doesn't do. I think we should leave obsf in a mostly unchanged
>> state even if it implies an unnecessary allocation (it is really only a
>> single allocation). This can be achieved by taking obsf's par_out in the
>> else branch above and replacing it with fresh AVCodecParameters. On
>> success, the old par_out is freed; on failure it is restored.
>>
>>> +    if (ret < 0)
>>> +        goto fail;
>>> +    obsf->time_base_out = bsf->time_base_out;
>>> +
>>> +    ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, bsf);
>>> +    if (ret < 0)
>>> +        goto fail;
>>> +
>>> +    *out_bsf = obsf;
>>> +    return 0;
>>> +
>>> +fail:
>>> +    if (new_list_bsf) {
>>> +        if (lst->nb_bsfs)
>>> +            lst->bsfs[0] = NULL;
>>> +        av_bsf_free(&new_list_bsf);
>>> +    }
>>> +    return ret;
>>> +}
>>> +
>>>  static int bsf_parse_single(const char *str, AVBSFList *bsf_lst)
>>>  {
>>>      char *bsf_name, *bsf_options_str, *buf;
>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>> index f4d1d4de21..dadca75430 100644
>>> --- a/libavcodec/version.h
>>> +++ b/libavcodec/version.h
>>> @@ -29,7 +29,7 @@
>>>
>>>  #define LIBAVCODEC_VERSION_MAJOR  58
>>>  #define LIBAVCODEC_VERSION_MINOR  77
>>> -#define LIBAVCODEC_VERSION_MICRO 101
>>> +#define LIBAVCODEC_VERSION_MICRO 102
>>
>> Adding a new public function always requires a minor bump.
>>>
>>>  #define LIBAVCODEC_VERSION_INT 
>>> AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>>>                                                
>>> LIBAVCODEC_VERSION_MINOR, \
>>>
>>
>> Finally, (I feel bad having to tell you this given that you have clearly
>> invested a lot of time in this) we currently have no case where we need
>> more than one automatically inserted bitstream filter per stream. We do
>> not know whether the current code does the right thing in case more than
>> one packet needs to be analyzed to determine whether to insert bitstream
>> filters (maybe the packets should instead be cached so that no bsf
>> misses out on the initial packets?); of course what is the right thing
>> might be format and codec-dependent. So I don't know whether we should
>> add this function now (given that public API is hard to remove).
>>
>> This does not mean that we should not use a bsf_list bsf to simplify
>> automatic bitstream filtering in mux.c (it really simplifies this alot
>> and gets rid of code duplication). But I think we should leave the topic
>> of adding a bsf to an already existing bsf open so that it can be
>> determined when the need indeed arises.
> 
> I see no sane way of sharing list bsf code other than adding public API,
> because mux.c is libavformat, bsf.c is libavcodec. This at least allows
> you to replace AVStreamInternal->nb_bsfcs and AVStreamInternal->bsfcs
> with a single bitstream filter to which you join additional ones.
> 
> Unless you want to remove the existing (admittedly unused, and
> potentially inadequate) support for multiple bitstream filters in
> AVStreamInternal, I really don't see a way which postpones adding this API.

Can't you achieve the same using existing bsf list API from within lavf?
av_bsf_list_alloc(), followed by any required amount of calls to
av_bsf_list_append(), av_bsf_list_append2() or av_bsf_list_parse_str()
to add each bsf, then av_bsf_list_finalize() at the end?
Marton Balint April 10, 2020, 8:25 a.m. UTC | #4
On Thu, 9 Apr 2020, James Almer wrote:

> On 4/9/2020 9:11 PM, Marton Balint wrote:
>> 
>> 
>> On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:
>> 
>>> Marton Balint:
>>>> Signed-off-by: Marton Balint <cus@passwd.hu>
>>>> ---
>>>>  doc/APIchanges       |  3 +++
>>>>  libavcodec/avcodec.h | 19 ++++++++++++++++
>>>>  libavcodec/bsf.c     | 62
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  libavcodec/version.h |  2 +-
>>>>  4 files changed, 85 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>>> index f1d7eac2ee..1473742139 100644
>>>> --- a/doc/APIchanges
>>>> +++ b/doc/APIchanges
>>>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>>>
>>>>  API changes, most recent first:
>>>>
>>>> +2020-04-xx - xxxxxxxxxx - lavc 58.77.102 - avcodec.h
>>>> +  Add av_bsf_join() to chain bitstream filters.
>>>> +
>>>>  2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
>>>>    av_read_frame() now guarantees to handle uninitialized input packets
>>>>    and to return refcounted packets on success.
>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>>> index 8fc0ad92c9..2812055e8a 100644
>>>> --- a/libavcodec/avcodec.h
>>>> +++ b/libavcodec/avcodec.h
>>>> @@ -6056,6 +6056,25 @@ int av_bsf_receive_packet(AVBSFContext *ctx,
>>>> AVPacket *pkt);
>>>>   */
>>>>  void av_bsf_flush(AVBSFContext *ctx);
>>>>
>>>> +/**
>>>> + * Join a new bitstream filter to an already operating one.
>>>> + *
>>>> + * @param[in,out] out_bsf *out_bsf contains the existing, already
>>>> initialized bitstream
>>>> + *                        filter, the new filter will be joined to
>>>> the output of this.
>>>> + *                        It can be NULL, in which case an empty
>>>> filter is assumed.
>>>> + *                        Upon successful return *out_bsf will
>>>> contain the new, combined
>>>> + *                        bitstream filter which will be initialized.
>>>> + * @param[in]     bsf     the bitstream filter to be joined to the
>>>> output of *out_bsf.
>>>> + *                        This filter must be uninitialized but it
>>>> must be ready to be
>>>> + *                        initalized, so input codec parameters and
>>>> time base must be
>>>> + *                        set if *out_bsf is NULL, otherwise the
>>>> function will set these
>>>> + *                        automatically based on the output
>>>> parameters of *out_bsf.
>>>> + *                        Upon successful return the bsf will be
>>>> initialized.
>>>> + *
>>>> + * @return 0 on success, negative on error.
>>>
>>> One needs to be more explicit about what happens on error: bsf may be
>>> in a partially initialized state and is essentially only good for
>>> freeing (maybe the bsf parameter should be AVBSFContext **, too, and
>>> automatically free bsf on error?). But IMO we should aim to not cripple
>>> *out_bsf and document this.
>>>
>>>> + */
>>>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf);
>>>> +
>>>>  /**
>>>>   * Free a bitstream filter context and everything associated with
>>>> it; write NULL
>>>>   * into the supplied pointer.
>>>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>>>> index b9fc771a88..1bca28d1ae 100644
>>>> --- a/libavcodec/bsf.c
>>>> +++ b/libavcodec/bsf.c
>>>> @@ -487,6 +487,68 @@ end:
>>>>      return ret;
>>>>  }
>>>>
>>>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf)
>>>> +{
>>>> +    BSFListContext *lst;
>>>> +    AVBSFContext *obsf = *out_bsf;
>>>> +    AVBSFContext *new_list_bsf = NULL;
>>>> +    int ret;
>>>> +
>>>> +    if (!obsf) {
>>>> +        ret = av_bsf_init(bsf);
>>>> +        if (ret < 0)
>>>> +            return ret;
>>>> +        *out_bsf = bsf;
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    if (obsf->filter != &ff_list_bsf) {
>>>> +        ret = av_bsf_alloc(&ff_list_bsf, &new_list_bsf);
>>>> +        if (ret < 0)
>>>> +            return ret;
>>>> +        lst = new_list_bsf->priv_data;
>>>> +        ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, obsf);
>>>> +        if (ret < 0)
>>>> +            goto fail;
>>>> +        ret = avcodec_parameters_copy(new_list_bsf->par_in,
>>>> obsf->par_in);
>>>> +        if (ret < 0)
>>>> +            goto fail;
>>>> +        new_list_bsf->time_base_in = obsf->time_base_in;
>>>> +        obsf = new_list_bsf;
>>>> +    } else {
>>>> +        lst = obsf->priv_data;
>>>> +    }
>>>> +
>>>> +    ret = avcodec_parameters_copy(bsf->par_in,
>>>> lst->bsfs[lst->nb_bsfs-1]->par_out);
>>>> +    if (ret < 0)
>>>> +        goto fail;
>>>> +    bsf->time_base_in = lst->bsfs[lst->nb_bsfs-1]->time_base_out;
>>>> +
>>>> +    ret = av_bsf_init(&bsf);
>>>> +    if (ret < 0)
>>>> +        goto fail;
>>>> +
>>>> +    ret = avcodec_parameters_copy(obsf->par_out, bsf->par_out);
>>>
>>> This will change obsf even on failure (when *out_bsf was already of type
>>> ff_list_bsf). This is something that ff_stream_add_bitstream_filter()
>>> currently doesn't do. I think we should leave obsf in a mostly unchanged
>>> state even if it implies an unnecessary allocation (it is really only a
>>> single allocation). This can be achieved by taking obsf's par_out in the
>>> else branch above and replacing it with fresh AVCodecParameters. On
>>> success, the old par_out is freed; on failure it is restored.
>>>
>>>> +    if (ret < 0)
>>>> +        goto fail;
>>>> +    obsf->time_base_out = bsf->time_base_out;
>>>> +
>>>> +    ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, bsf);
>>>> +    if (ret < 0)
>>>> +        goto fail;
>>>> +
>>>> +    *out_bsf = obsf;
>>>> +    return 0;
>>>> +
>>>> +fail:
>>>> +    if (new_list_bsf) {
>>>> +        if (lst->nb_bsfs)
>>>> +            lst->bsfs[0] = NULL;
>>>> +        av_bsf_free(&new_list_bsf);
>>>> +    }
>>>> +    return ret;
>>>> +}
>>>> +
>>>>  static int bsf_parse_single(const char *str, AVBSFList *bsf_lst)
>>>>  {
>>>>      char *bsf_name, *bsf_options_str, *buf;
>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>> index f4d1d4de21..dadca75430 100644
>>>> --- a/libavcodec/version.h
>>>> +++ b/libavcodec/version.h
>>>> @@ -29,7 +29,7 @@
>>>>
>>>>  #define LIBAVCODEC_VERSION_MAJOR  58
>>>>  #define LIBAVCODEC_VERSION_MINOR  77
>>>> -#define LIBAVCODEC_VERSION_MICRO 101
>>>> +#define LIBAVCODEC_VERSION_MICRO 102
>>>
>>> Adding a new public function always requires a minor bump.
>>>>
>>>>  #define LIBAVCODEC_VERSION_INT 
>>>> AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>>>>                                                
>>>> LIBAVCODEC_VERSION_MINOR, \
>>>>
>>>
>>> Finally, (I feel bad having to tell you this given that you have clearly
>>> invested a lot of time in this) we currently have no case where we need
>>> more than one automatically inserted bitstream filter per stream. We do
>>> not know whether the current code does the right thing in case more than
>>> one packet needs to be analyzed to determine whether to insert bitstream
>>> filters (maybe the packets should instead be cached so that no bsf
>>> misses out on the initial packets?); of course what is the right thing
>>> might be format and codec-dependent. So I don't know whether we should
>>> add this function now (given that public API is hard to remove).
>>>
>>> This does not mean that we should not use a bsf_list bsf to simplify
>>> automatic bitstream filtering in mux.c (it really simplifies this alot
>>> and gets rid of code duplication). But I think we should leave the topic
>>> of adding a bsf to an already existing bsf open so that it can be
>>> determined when the need indeed arises.
>> 
>> I see no sane way of sharing list bsf code other than adding public API,
>> because mux.c is libavformat, bsf.c is libavcodec. This at least allows
>> you to replace AVStreamInternal->nb_bsfcs and AVStreamInternal->bsfcs
>> with a single bitstream filter to which you join additional ones.
>> 
>> Unless you want to remove the existing (admittedly unused, and
>> potentially inadequate) support for multiple bitstream filters in
>> AVStreamInternal, I really don't see a way which postpones adding this API.
>
> Can't you achieve the same using existing bsf list API from within lavf?
> av_bsf_list_alloc(), followed by any required amount of calls to
> av_bsf_list_append(), av_bsf_list_append2() or av_bsf_list_parse_str()
> to add each bsf, then av_bsf_list_finalize() at the end?

That API only works if you have all the bitstream filters before 
finalizing the list, you can't append a BSF to an already finalized 
list. The implementation in mux.c supports adding a BSF, then maybe 
passing a few packets through it, then appending another BSF to the list - 
this can happen if .check_bitstream_filters() adds a bsf but returns 0, 
and at a later packet it adds another bsf.

Regards,
Marton
Andreas Rheinhardt April 10, 2020, 10:58 a.m. UTC | #5
Marton Balint:
> 
> 
> On Thu, 9 Apr 2020, James Almer wrote:
> 
>> On 4/9/2020 9:11 PM, Marton Balint wrote:
>>>
>>>
>>> On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:
>>>
>>>> Marton Balint:
>>>>> Signed-off-by: Marton Balint <cus@passwd.hu>
>>>>> ---
>>>>>  doc/APIchanges       |  3 +++
>>>>>  libavcodec/avcodec.h | 19 ++++++++++++++++
>>>>>  libavcodec/bsf.c     | 62
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  libavcodec/version.h |  2 +-
>>>>>  4 files changed, 85 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>>>> index f1d7eac2ee..1473742139 100644
>>>>> --- a/doc/APIchanges
>>>>> +++ b/doc/APIchanges
>>>>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>>>>
>>>>>  API changes, most recent first:
>>>>>
>>>>> +2020-04-xx - xxxxxxxxxx - lavc 58.77.102 - avcodec.h
>>>>> +  Add av_bsf_join() to chain bitstream filters.
>>>>> +
>>>>>  2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
>>>>>    av_read_frame() now guarantees to handle uninitialized input
>>>>> packets
>>>>>    and to return refcounted packets on success.
>>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>>>> index 8fc0ad92c9..2812055e8a 100644
>>>>> --- a/libavcodec/avcodec.h
>>>>> +++ b/libavcodec/avcodec.h
>>>>> @@ -6056,6 +6056,25 @@ int av_bsf_receive_packet(AVBSFContext *ctx,
>>>>> AVPacket *pkt);
>>>>>   */
>>>>>  void av_bsf_flush(AVBSFContext *ctx);
>>>>>
>>>>> +/**
>>>>> + * Join a new bitstream filter to an already operating one.
>>>>> + *
>>>>> + * @param[in,out] out_bsf *out_bsf contains the existing, already
>>>>> initialized bitstream
>>>>> + *                        filter, the new filter will be joined to
>>>>> the output of this.
>>>>> + *                        It can be NULL, in which case an empty
>>>>> filter is assumed.
>>>>> + *                        Upon successful return *out_bsf will
>>>>> contain the new, combined
>>>>> + *                        bitstream filter which will be initialized.
>>>>> + * @param[in]     bsf     the bitstream filter to be joined to the
>>>>> output of *out_bsf.
>>>>> + *                        This filter must be uninitialized but it
>>>>> must be ready to be
>>>>> + *                        initalized, so input codec parameters and
>>>>> time base must be
>>>>> + *                        set if *out_bsf is NULL, otherwise the
>>>>> function will set these
>>>>> + *                        automatically based on the output
>>>>> parameters of *out_bsf.
>>>>> + *                        Upon successful return the bsf will be
>>>>> initialized.
>>>>> + *
>>>>> + * @return 0 on success, negative on error.
>>>>
>>>> One needs to be more explicit about what happens on error: bsf may be
>>>> in a partially initialized state and is essentially only good for
>>>> freeing (maybe the bsf parameter should be AVBSFContext **, too, and
>>>> automatically free bsf on error?). But IMO we should aim to not cripple
>>>> *out_bsf and document this.
>>>>
>>>>> + */
>>>>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf);
>>>>> +
>>>>>  /**
>>>>>   * Free a bitstream filter context and everything associated with
>>>>> it; write NULL
>>>>>   * into the supplied pointer.
>>>>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>>>>> index b9fc771a88..1bca28d1ae 100644
>>>>> --- a/libavcodec/bsf.c
>>>>> +++ b/libavcodec/bsf.c
>>>>> @@ -487,6 +487,68 @@ end:
>>>>>      return ret;
>>>>>  }
>>>>>
>>>>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf)
>>>>> +{
>>>>> +    BSFListContext *lst;
>>>>> +    AVBSFContext *obsf = *out_bsf;
>>>>> +    AVBSFContext *new_list_bsf = NULL;
>>>>> +    int ret;
>>>>> +
>>>>> +    if (!obsf) {
>>>>> +        ret = av_bsf_init(bsf);
>>>>> +        if (ret < 0)
>>>>> +            return ret;
>>>>> +        *out_bsf = bsf;
>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>> +    if (obsf->filter != &ff_list_bsf) {
>>>>> +        ret = av_bsf_alloc(&ff_list_bsf, &new_list_bsf);
>>>>> +        if (ret < 0)
>>>>> +            return ret;
>>>>> +        lst = new_list_bsf->priv_data;
>>>>> +        ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs,
>>>>> obsf);
>>>>> +        if (ret < 0)
>>>>> +            goto fail;
>>>>> +        ret = avcodec_parameters_copy(new_list_bsf->par_in,
>>>>> obsf->par_in);
>>>>> +        if (ret < 0)
>>>>> +            goto fail;
>>>>> +        new_list_bsf->time_base_in = obsf->time_base_in;
>>>>> +        obsf = new_list_bsf;
>>>>> +    } else {
>>>>> +        lst = obsf->priv_data;
>>>>> +    }
>>>>> +
>>>>> +    ret = avcodec_parameters_copy(bsf->par_in,
>>>>> lst->bsfs[lst->nb_bsfs-1]->par_out);
>>>>> +    if (ret < 0)
>>>>> +        goto fail;
>>>>> +    bsf->time_base_in = lst->bsfs[lst->nb_bsfs-1]->time_base_out;
>>>>> +
>>>>> +    ret = av_bsf_init(&bsf);
>>>>> +    if (ret < 0)
>>>>> +        goto fail;
>>>>> +
>>>>> +    ret = avcodec_parameters_copy(obsf->par_out, bsf->par_out);
>>>>
>>>> This will change obsf even on failure (when *out_bsf was already of
>>>> type
>>>> ff_list_bsf). This is something that ff_stream_add_bitstream_filter()
>>>> currently doesn't do. I think we should leave obsf in a mostly
>>>> unchanged
>>>> state even if it implies an unnecessary allocation (it is really only a
>>>> single allocation). This can be achieved by taking obsf's par_out in
>>>> the
>>>> else branch above and replacing it with fresh AVCodecParameters. On
>>>> success, the old par_out is freed; on failure it is restored.
>>>>
>>>>> +    if (ret < 0)
>>>>> +        goto fail;
>>>>> +    obsf->time_base_out = bsf->time_base_out;
>>>>> +
>>>>> +    ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, bsf);
>>>>> +    if (ret < 0)
>>>>> +        goto fail;
>>>>> +
>>>>> +    *out_bsf = obsf;
>>>>> +    return 0;
>>>>> +
>>>>> +fail:
>>>>> +    if (new_list_bsf) {
>>>>> +        if (lst->nb_bsfs)
>>>>> +            lst->bsfs[0] = NULL;
>>>>> +        av_bsf_free(&new_list_bsf);
>>>>> +    }
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>>  static int bsf_parse_single(const char *str, AVBSFList *bsf_lst)
>>>>>  {
>>>>>      char *bsf_name, *bsf_options_str, *buf;
>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>> index f4d1d4de21..dadca75430 100644
>>>>> --- a/libavcodec/version.h
>>>>> +++ b/libavcodec/version.h
>>>>> @@ -29,7 +29,7 @@
>>>>>
>>>>>  #define LIBAVCODEC_VERSION_MAJOR  58
>>>>>  #define LIBAVCODEC_VERSION_MINOR  77
>>>>> -#define LIBAVCODEC_VERSION_MICRO 101
>>>>> +#define LIBAVCODEC_VERSION_MICRO 102
>>>>
>>>> Adding a new public function always requires a minor bump.
>>>>>
>>>>>  #define LIBAVCODEC_VERSION_INT 
>>>>> AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>>>>>                                                
>>>>> LIBAVCODEC_VERSION_MINOR, \
>>>>>
>>>>
>>>> Finally, (I feel bad having to tell you this given that you have
>>>> clearly
>>>> invested a lot of time in this) we currently have no case where we need
>>>> more than one automatically inserted bitstream filter per stream. We do
>>>> not know whether the current code does the right thing in case more
>>>> than
>>>> one packet needs to be analyzed to determine whether to insert
>>>> bitstream
>>>> filters (maybe the packets should instead be cached so that no bsf
>>>> misses out on the initial packets?); of course what is the right thing
>>>> might be format and codec-dependent. So I don't know whether we should
>>>> add this function now (given that public API is hard to remove).
>>>>
>>>> This does not mean that we should not use a bsf_list bsf to simplify
>>>> automatic bitstream filtering in mux.c (it really simplifies this alot
>>>> and gets rid of code duplication). But I think we should leave the
>>>> topic
>>>> of adding a bsf to an already existing bsf open so that it can be
>>>> determined when the need indeed arises.
>>>
>>> I see no sane way of sharing list bsf code other than adding public API,
>>> because mux.c is libavformat, bsf.c is libavcodec. This at least allows
>>> you to replace AVStreamInternal->nb_bsfcs and AVStreamInternal->bsfcs
>>> with a single bitstream filter to which you join additional ones.
>>>
>>> Unless you want to remove the existing (admittedly unused, and
>>> potentially inadequate) support for multiple bitstream filters in
>>> AVStreamInternal, I really don't see a way which postpones adding
>>> this API.
>>
When I wrote the above, I thought that we might restrict the code in
mux.c to a single bsf by use of the list bsf and that we postpone the
exact details of how to add a bsf to an already existing list to the
time when it is really needed; in particular, we should not strive to
mimic what the current code does in this case because we do not know
whether this is indeed the right thing.

Thinking about this a bit further has made me realize that there is
another problem: It might be that a muxer might need the output of the
first bitstream filter in order to determine whether to add another
bitstream filter to the bsf chain. This is a usecase that can't be
fulfilled with av_bsf_join() and I think it can't be fulfilled with a
single list-bsf at all.
In this case, one would essentially put what you have put into
need_auto_bsf() into the loop to be called at the end of the current
filter chain* and if it resulted in a new filter being appended, the
packet would of course need to be sent to said new filter.
This scenario can definitely not be solved with av_bsf_join() (it's not
designed for partially filtered packets). And in this case the code in
mux.c will have to deal with multiple bsfs anyway (the old one as well
as the new one), so that no simplifications from using a single bsf will
be achievable.

So my current stance is: Keep support for manual bsf lists in mux.c, but
leave the details of how to actually add multiple bsf to the future: The
only supported case right now is adding multiple bsf in the same
check_packet() call and this should be kept, but there is no reason to
already move need_auto_bsf() into the loop. Fixing this is not realistic
right now because we don't have a real usecase. And furthermore, it
would delay your patchset even further.
It also means that the first patch of this patchset should be applied,
but not the second.

- Andreas

*: So when check_packet() is called, the muxer knows that this packet
has already been through the whole already existing chain.


>> Can't you achieve the same using existing bsf list API from within lavf?
>> av_bsf_list_alloc(), followed by any required amount of calls to
>> av_bsf_list_append(), av_bsf_list_append2() or av_bsf_list_parse_str()
>> to add each bsf, then av_bsf_list_finalize() at the end?
> 
> That API only works if you have all the bitstream filters before
> finalizing the list, you can't append a BSF to an already finalized
> list. The implementation in mux.c supports adding a BSF, then maybe
> passing a few packets through it, then appending another BSF to the list
> - this can happen if .check_bitstream_filters() adds a bsf but returns
> 0, and at a later packet it adds another bsf.
> 
> Regards,
> Marton
Marton Balint April 10, 2020, 11:52 a.m. UTC | #6
On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:

> Marton Balint:
>> 
>> 
>> On Thu, 9 Apr 2020, James Almer wrote:
>> 
>>> On 4/9/2020 9:11 PM, Marton Balint wrote:
>>>>
>>>>
>>>> On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:
>>>>
>>>>> Marton Balint:
>>>>>> Signed-off-by: Marton Balint <cus@passwd.hu>
>>>>>> ---
>>>>>>  doc/APIchanges       |  3 +++
>>>>>>  libavcodec/avcodec.h | 19 ++++++++++++++++
>>>>>>  libavcodec/bsf.c     | 62
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  libavcodec/version.h |  2 +-
>>>>>>  4 files changed, 85 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>>>>> index f1d7eac2ee..1473742139 100644
>>>>>> --- a/doc/APIchanges
>>>>>> +++ b/doc/APIchanges
>>>>>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>>>>>
>>>>>>  API changes, most recent first:
>>>>>>
>>>>>> +2020-04-xx - xxxxxxxxxx - lavc 58.77.102 - avcodec.h
>>>>>> +  Add av_bsf_join() to chain bitstream filters.
>>>>>> +
>>>>>>  2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
>>>>>>    av_read_frame() now guarantees to handle uninitialized input
>>>>>> packets
>>>>>>    and to return refcounted packets on success.
>>>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>>>>> index 8fc0ad92c9..2812055e8a 100644
>>>>>> --- a/libavcodec/avcodec.h
>>>>>> +++ b/libavcodec/avcodec.h
>>>>>> @@ -6056,6 +6056,25 @@ int av_bsf_receive_packet(AVBSFContext *ctx,
>>>>>> AVPacket *pkt);
>>>>>>   */
>>>>>>  void av_bsf_flush(AVBSFContext *ctx);
>>>>>>
>>>>>> +/**
>>>>>> + * Join a new bitstream filter to an already operating one.
>>>>>> + *
>>>>>> + * @param[in,out] out_bsf *out_bsf contains the existing, already
>>>>>> initialized bitstream
>>>>>> + *                        filter, the new filter will be joined to
>>>>>> the output of this.
>>>>>> + *                        It can be NULL, in which case an empty
>>>>>> filter is assumed.
>>>>>> + *                        Upon successful return *out_bsf will
>>>>>> contain the new, combined
>>>>>> + *                        bitstream filter which will be initialized.
>>>>>> + * @param[in]     bsf     the bitstream filter to be joined to the
>>>>>> output of *out_bsf.
>>>>>> + *                        This filter must be uninitialized but it
>>>>>> must be ready to be
>>>>>> + *                        initalized, so input codec parameters and
>>>>>> time base must be
>>>>>> + *                        set if *out_bsf is NULL, otherwise the
>>>>>> function will set these
>>>>>> + *                        automatically based on the output
>>>>>> parameters of *out_bsf.
>>>>>> + *                        Upon successful return the bsf will be
>>>>>> initialized.
>>>>>> + *
>>>>>> + * @return 0 on success, negative on error.
>>>>>
>>>>> One needs to be more explicit about what happens on error: bsf may be
>>>>> in a partially initialized state and is essentially only good for
>>>>> freeing (maybe the bsf parameter should be AVBSFContext **, too, and
>>>>> automatically free bsf on error?). But IMO we should aim to not cripple
>>>>> *out_bsf and document this.
>>>>>
>>>>>> + */
>>>>>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf);
>>>>>> +
>>>>>>  /**
>>>>>>   * Free a bitstream filter context and everything associated with
>>>>>> it; write NULL
>>>>>>   * into the supplied pointer.
>>>>>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>>>>>> index b9fc771a88..1bca28d1ae 100644
>>>>>> --- a/libavcodec/bsf.c
>>>>>> +++ b/libavcodec/bsf.c
>>>>>> @@ -487,6 +487,68 @@ end:
>>>>>>      return ret;
>>>>>>  }
>>>>>>
>>>>>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf)
>>>>>> +{
>>>>>> +    BSFListContext *lst;
>>>>>> +    AVBSFContext *obsf = *out_bsf;
>>>>>> +    AVBSFContext *new_list_bsf = NULL;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (!obsf) {
>>>>>> +        ret = av_bsf_init(bsf);
>>>>>> +        if (ret < 0)
>>>>>> +            return ret;
>>>>>> +        *out_bsf = bsf;
>>>>>> +        return 0;
>>>>>> +    }
>>>>>> +
>>>>>> +    if (obsf->filter != &ff_list_bsf) {
>>>>>> +        ret = av_bsf_alloc(&ff_list_bsf, &new_list_bsf);
>>>>>> +        if (ret < 0)
>>>>>> +            return ret;
>>>>>> +        lst = new_list_bsf->priv_data;
>>>>>> +        ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs,
>>>>>> obsf);
>>>>>> +        if (ret < 0)
>>>>>> +            goto fail;
>>>>>> +        ret = avcodec_parameters_copy(new_list_bsf->par_in,
>>>>>> obsf->par_in);
>>>>>> +        if (ret < 0)
>>>>>> +            goto fail;
>>>>>> +        new_list_bsf->time_base_in = obsf->time_base_in;
>>>>>> +        obsf = new_list_bsf;
>>>>>> +    } else {
>>>>>> +        lst = obsf->priv_data;
>>>>>> +    }
>>>>>> +
>>>>>> +    ret = avcodec_parameters_copy(bsf->par_in,
>>>>>> lst->bsfs[lst->nb_bsfs-1]->par_out);
>>>>>> +    if (ret < 0)
>>>>>> +        goto fail;
>>>>>> +    bsf->time_base_in = lst->bsfs[lst->nb_bsfs-1]->time_base_out;
>>>>>> +
>>>>>> +    ret = av_bsf_init(&bsf);
>>>>>> +    if (ret < 0)
>>>>>> +        goto fail;
>>>>>> +
>>>>>> +    ret = avcodec_parameters_copy(obsf->par_out, bsf->par_out);
>>>>>
>>>>> This will change obsf even on failure (when *out_bsf was already of
>>>>> type
>>>>> ff_list_bsf). This is something that ff_stream_add_bitstream_filter()
>>>>> currently doesn't do. I think we should leave obsf in a mostly
>>>>> unchanged
>>>>> state even if it implies an unnecessary allocation (it is really only a
>>>>> single allocation). This can be achieved by taking obsf's par_out in
>>>>> the
>>>>> else branch above and replacing it with fresh AVCodecParameters. On
>>>>> success, the old par_out is freed; on failure it is restored.
>>>>>
>>>>>> +    if (ret < 0)
>>>>>> +        goto fail;
>>>>>> +    obsf->time_base_out = bsf->time_base_out;
>>>>>> +
>>>>>> +    ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, bsf);
>>>>>> +    if (ret < 0)
>>>>>> +        goto fail;
>>>>>> +
>>>>>> +    *out_bsf = obsf;
>>>>>> +    return 0;
>>>>>> +
>>>>>> +fail:
>>>>>> +    if (new_list_bsf) {
>>>>>> +        if (lst->nb_bsfs)
>>>>>> +            lst->bsfs[0] = NULL;
>>>>>> +        av_bsf_free(&new_list_bsf);
>>>>>> +    }
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>>  static int bsf_parse_single(const char *str, AVBSFList *bsf_lst)
>>>>>>  {
>>>>>>      char *bsf_name, *bsf_options_str, *buf;
>>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>>> index f4d1d4de21..dadca75430 100644
>>>>>> --- a/libavcodec/version.h
>>>>>> +++ b/libavcodec/version.h
>>>>>> @@ -29,7 +29,7 @@
>>>>>>
>>>>>>  #define LIBAVCODEC_VERSION_MAJOR  58
>>>>>>  #define LIBAVCODEC_VERSION_MINOR  77
>>>>>> -#define LIBAVCODEC_VERSION_MICRO 101
>>>>>> +#define LIBAVCODEC_VERSION_MICRO 102
>>>>>
>>>>> Adding a new public function always requires a minor bump.
>>>>>>
>>>>>>  #define LIBAVCODEC_VERSION_INT 
>>>>>> AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>>>>>>                                                
>>>>>> LIBAVCODEC_VERSION_MINOR, \
>>>>>>
>>>>>
>>>>> Finally, (I feel bad having to tell you this given that you have
>>>>> clearly
>>>>> invested a lot of time in this) we currently have no case where we need
>>>>> more than one automatically inserted bitstream filter per stream. We do
>>>>> not know whether the current code does the right thing in case more
>>>>> than
>>>>> one packet needs to be analyzed to determine whether to insert
>>>>> bitstream
>>>>> filters (maybe the packets should instead be cached so that no bsf
>>>>> misses out on the initial packets?); of course what is the right thing
>>>>> might be format and codec-dependent. So I don't know whether we should
>>>>> add this function now (given that public API is hard to remove).
>>>>>
>>>>> This does not mean that we should not use a bsf_list bsf to simplify
>>>>> automatic bitstream filtering in mux.c (it really simplifies this alot
>>>>> and gets rid of code duplication). But I think we should leave the
>>>>> topic
>>>>> of adding a bsf to an already existing bsf open so that it can be
>>>>> determined when the need indeed arises.
>>>>
>>>> I see no sane way of sharing list bsf code other than adding public API,
>>>> because mux.c is libavformat, bsf.c is libavcodec. This at least allows
>>>> you to replace AVStreamInternal->nb_bsfcs and AVStreamInternal->bsfcs
>>>> with a single bitstream filter to which you join additional ones.
>>>>
>>>> Unless you want to remove the existing (admittedly unused, and
>>>> potentially inadequate) support for multiple bitstream filters in
>>>> AVStreamInternal, I really don't see a way which postpones adding
>>>> this API.
>>>
> When I wrote the above, I thought that we might restrict the code in
> mux.c to a single bsf by use of the list bsf and that we postpone the
> exact details of how to add a bsf to an already existing list to the
> time when it is really needed; in particular, we should not strive to
> mimic what the current code does in this case because we do not know
> whether this is indeed the right thing.
>
> Thinking about this a bit further has made me realize that there is
> another problem: It might be that a muxer might need the output of the
> first bitstream filter in order to determine whether to add another
> bitstream filter to the bsf chain. This is a usecase that can't be
> fulfilled with av_bsf_join() and I think it can't be fulfilled with a
> single list-bsf at all.
> In this case, one would essentially put what you have put into
> need_auto_bsf() into the loop to be called at the end of the current
> filter chain* and if it resulted in a new filter being appended, the
> packet would of course need to be sent to said new filter.
> This scenario can definitely not be solved with av_bsf_join() (it's not
> designed for partially filtered packets). And in this case the code in
> mux.c will have to deal with multiple bsfs anyway (the old one as well
> as the new one), so that no simplifications from using a single bsf will
> be achievable.
>
> So my current stance is: Keep support for manual bsf lists in mux.c, but
> leave the details of how to actually add multiple bsf to the future: The
> only supported case right now is adding multiple bsf in the same
> check_packet() call and this should be kept, but there is no reason to
> already move need_auto_bsf() into the loop. Fixing this is not realistic
> right now because we don't have a real usecase. And furthermore, it
> would delay your patchset even further.
> It also means that the first patch of this patchset should be applied,
> but not the second.

But how do you share code then?

Also I don't follow your logic, if you say that no new API should be added 
until it is used, then why not remove the unused multiple bsf lists from 
AVStreamInternal and re-add it when it is actually needed? That is the 
only way I see to not add duplicated list code.

Regards,
Marton
Andreas Rheinhardt April 10, 2020, 1:21 p.m. UTC | #7
Marton Balint:
> 
> 
> On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:
> 
>> Marton Balint:
>>>
>>>
>>> On Thu, 9 Apr 2020, James Almer wrote:
>>>
>>>> On 4/9/2020 9:11 PM, Marton Balint wrote:
>>>>>
>>>>>
>>>>> On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:
>>>>>
>>>>>> Marton Balint:
>>>>>>> Signed-off-by: Marton Balint <cus@passwd.hu>
>>>>>>> ---
>>>>>>>  doc/APIchanges       |  3 +++
>>>>>>>  libavcodec/avcodec.h | 19 ++++++++++++++++
>>>>>>>  libavcodec/bsf.c     | 62
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  libavcodec/version.h |  2 +-
>>>>>>>  4 files changed, 85 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>>>>>> index f1d7eac2ee..1473742139 100644
>>>>>>> --- a/doc/APIchanges
>>>>>>> +++ b/doc/APIchanges
>>>>>>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>>>>>>
>>>>>>>  API changes, most recent first:
>>>>>>>
>>>>>>> +2020-04-xx - xxxxxxxxxx - lavc 58.77.102 - avcodec.h
>>>>>>> +  Add av_bsf_join() to chain bitstream filters.
>>>>>>> +
>>>>>>>  2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
>>>>>>>    av_read_frame() now guarantees to handle uninitialized input
>>>>>>> packets
>>>>>>>    and to return refcounted packets on success.
>>>>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>>>>>> index 8fc0ad92c9..2812055e8a 100644
>>>>>>> --- a/libavcodec/avcodec.h
>>>>>>> +++ b/libavcodec/avcodec.h
>>>>>>> @@ -6056,6 +6056,25 @@ int av_bsf_receive_packet(AVBSFContext *ctx,
>>>>>>> AVPacket *pkt);
>>>>>>>   */
>>>>>>>  void av_bsf_flush(AVBSFContext *ctx);
>>>>>>>
>>>>>>> +/**
>>>>>>> + * Join a new bitstream filter to an already operating one.
>>>>>>> + *
>>>>>>> + * @param[in,out] out_bsf *out_bsf contains the existing, already
>>>>>>> initialized bitstream
>>>>>>> + *                        filter, the new filter will be joined to
>>>>>>> the output of this.
>>>>>>> + *                        It can be NULL, in which case an empty
>>>>>>> filter is assumed.
>>>>>>> + *                        Upon successful return *out_bsf will
>>>>>>> contain the new, combined
>>>>>>> + *                        bitstream filter which will be
>>>>>>> initialized.
>>>>>>> + * @param[in]     bsf     the bitstream filter to be joined to the
>>>>>>> output of *out_bsf.
>>>>>>> + *                        This filter must be uninitialized but it
>>>>>>> must be ready to be
>>>>>>> + *                        initalized, so input codec parameters and
>>>>>>> time base must be
>>>>>>> + *                        set if *out_bsf is NULL, otherwise the
>>>>>>> function will set these
>>>>>>> + *                        automatically based on the output
>>>>>>> parameters of *out_bsf.
>>>>>>> + *                        Upon successful return the bsf will be
>>>>>>> initialized.
>>>>>>> + *
>>>>>>> + * @return 0 on success, negative on error.
>>>>>>
>>>>>> One needs to be more explicit about what happens on error: bsf may be
>>>>>> in a partially initialized state and is essentially only good for
>>>>>> freeing (maybe the bsf parameter should be AVBSFContext **, too, and
>>>>>> automatically free bsf on error?). But IMO we should aim to not
>>>>>> cripple
>>>>>> *out_bsf and document this.
>>>>>>
>>>>>>> + */
>>>>>>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf);
>>>>>>> +
>>>>>>>  /**
>>>>>>>   * Free a bitstream filter context and everything associated with
>>>>>>> it; write NULL
>>>>>>>   * into the supplied pointer.
>>>>>>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>>>>>>> index b9fc771a88..1bca28d1ae 100644
>>>>>>> --- a/libavcodec/bsf.c
>>>>>>> +++ b/libavcodec/bsf.c
>>>>>>> @@ -487,6 +487,68 @@ end:
>>>>>>>      return ret;
>>>>>>>  }
>>>>>>>
>>>>>>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf)
>>>>>>> +{
>>>>>>> +    BSFListContext *lst;
>>>>>>> +    AVBSFContext *obsf = *out_bsf;
>>>>>>> +    AVBSFContext *new_list_bsf = NULL;
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    if (!obsf) {
>>>>>>> +        ret = av_bsf_init(bsf);
>>>>>>> +        if (ret < 0)
>>>>>>> +            return ret;
>>>>>>> +        *out_bsf = bsf;
>>>>>>> +        return 0;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    if (obsf->filter != &ff_list_bsf) {
>>>>>>> +        ret = av_bsf_alloc(&ff_list_bsf, &new_list_bsf);
>>>>>>> +        if (ret < 0)
>>>>>>> +            return ret;
>>>>>>> +        lst = new_list_bsf->priv_data;
>>>>>>> +        ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs,
>>>>>>> obsf);
>>>>>>> +        if (ret < 0)
>>>>>>> +            goto fail;
>>>>>>> +        ret = avcodec_parameters_copy(new_list_bsf->par_in,
>>>>>>> obsf->par_in);
>>>>>>> +        if (ret < 0)
>>>>>>> +            goto fail;
>>>>>>> +        new_list_bsf->time_base_in = obsf->time_base_in;
>>>>>>> +        obsf = new_list_bsf;
>>>>>>> +    } else {
>>>>>>> +        lst = obsf->priv_data;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    ret = avcodec_parameters_copy(bsf->par_in,
>>>>>>> lst->bsfs[lst->nb_bsfs-1]->par_out);
>>>>>>> +    if (ret < 0)
>>>>>>> +        goto fail;
>>>>>>> +    bsf->time_base_in = lst->bsfs[lst->nb_bsfs-1]->time_base_out;
>>>>>>> +
>>>>>>> +    ret = av_bsf_init(&bsf);
>>>>>>> +    if (ret < 0)
>>>>>>> +        goto fail;
>>>>>>> +
>>>>>>> +    ret = avcodec_parameters_copy(obsf->par_out, bsf->par_out);
>>>>>>
>>>>>> This will change obsf even on failure (when *out_bsf was already of
>>>>>> type
>>>>>> ff_list_bsf). This is something that ff_stream_add_bitstream_filter()
>>>>>> currently doesn't do. I think we should leave obsf in a mostly
>>>>>> unchanged
>>>>>> state even if it implies an unnecessary allocation (it is really
>>>>>> only a
>>>>>> single allocation). This can be achieved by taking obsf's par_out in
>>>>>> the
>>>>>> else branch above and replacing it with fresh AVCodecParameters. On
>>>>>> success, the old par_out is freed; on failure it is restored.
>>>>>>
>>>>>>> +    if (ret < 0)
>>>>>>> +        goto fail;
>>>>>>> +    obsf->time_base_out = bsf->time_base_out;
>>>>>>> +
>>>>>>> +    ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, bsf);
>>>>>>> +    if (ret < 0)
>>>>>>> +        goto fail;
>>>>>>> +
>>>>>>> +    *out_bsf = obsf;
>>>>>>> +    return 0;
>>>>>>> +
>>>>>>> +fail:
>>>>>>> +    if (new_list_bsf) {
>>>>>>> +        if (lst->nb_bsfs)
>>>>>>> +            lst->bsfs[0] = NULL;
>>>>>>> +        av_bsf_free(&new_list_bsf);
>>>>>>> +    }
>>>>>>> +    return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>>  static int bsf_parse_single(const char *str, AVBSFList *bsf_lst)
>>>>>>>  {
>>>>>>>      char *bsf_name, *bsf_options_str, *buf;
>>>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>>>> index f4d1d4de21..dadca75430 100644
>>>>>>> --- a/libavcodec/version.h
>>>>>>> +++ b/libavcodec/version.h
>>>>>>> @@ -29,7 +29,7 @@
>>>>>>>
>>>>>>>  #define LIBAVCODEC_VERSION_MAJOR  58
>>>>>>>  #define LIBAVCODEC_VERSION_MINOR  77
>>>>>>> -#define LIBAVCODEC_VERSION_MICRO 101
>>>>>>> +#define LIBAVCODEC_VERSION_MICRO 102
>>>>>>
>>>>>> Adding a new public function always requires a minor bump.
>>>>>>>
>>>>>>>  #define LIBAVCODEC_VERSION_INT 
>>>>>>> AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>>>>>>>                                                
>>>>>>> LIBAVCODEC_VERSION_MINOR, \
>>>>>>>
>>>>>>
>>>>>> Finally, (I feel bad having to tell you this given that you have
>>>>>> clearly
>>>>>> invested a lot of time in this) we currently have no case where we
>>>>>> need
>>>>>> more than one automatically inserted bitstream filter per stream.
>>>>>> We do
>>>>>> not know whether the current code does the right thing in case more
>>>>>> than
>>>>>> one packet needs to be analyzed to determine whether to insert
>>>>>> bitstream
>>>>>> filters (maybe the packets should instead be cached so that no bsf
>>>>>> misses out on the initial packets?); of course what is the right
>>>>>> thing
>>>>>> might be format and codec-dependent. So I don't know whether we
>>>>>> should
>>>>>> add this function now (given that public API is hard to remove).
>>>>>>
>>>>>> This does not mean that we should not use a bsf_list bsf to simplify
>>>>>> automatic bitstream filtering in mux.c (it really simplifies this
>>>>>> alot
>>>>>> and gets rid of code duplication). But I think we should leave the
>>>>>> topic
>>>>>> of adding a bsf to an already existing bsf open so that it can be
>>>>>> determined when the need indeed arises.
>>>>>
>>>>> I see no sane way of sharing list bsf code other than adding public
>>>>> API,
>>>>> because mux.c is libavformat, bsf.c is libavcodec. This at least
>>>>> allows
>>>>> you to replace AVStreamInternal->nb_bsfcs and AVStreamInternal->bsfcs
>>>>> with a single bitstream filter to which you join additional ones.
>>>>>
>>>>> Unless you want to remove the existing (admittedly unused, and
>>>>> potentially inadequate) support for multiple bitstream filters in
>>>>> AVStreamInternal, I really don't see a way which postpones adding
>>>>> this API.
>>>>
>> When I wrote the above, I thought that we might restrict the code in
>> mux.c to a single bsf by use of the list bsf and that we postpone the
>> exact details of how to add a bsf to an already existing list to the
>> time when it is really needed; in particular, we should not strive to
>> mimic what the current code does in this case because we do not know
>> whether this is indeed the right thing.
>>
>> Thinking about this a bit further has made me realize that there is
>> another problem: It might be that a muxer might need the output of the
>> first bitstream filter in order to determine whether to add another
>> bitstream filter to the bsf chain. This is a usecase that can't be
>> fulfilled with av_bsf_join() and I think it can't be fulfilled with a
>> single list-bsf at all.
>> In this case, one would essentially put what you have put into
>> need_auto_bsf() into the loop to be called at the end of the current
>> filter chain* and if it resulted in a new filter being appended, the
>> packet would of course need to be sent to said new filter.
>> This scenario can definitely not be solved with av_bsf_join() (it's not
>> designed for partially filtered packets). And in this case the code in
>> mux.c will have to deal with multiple bsfs anyway (the old one as well
>> as the new one), so that no simplifications from using a single bsf will
>> be achievable.
>>
>> So my current stance is: Keep support for manual bsf lists in mux.c, but
>> leave the details of how to actually add multiple bsf to the future: The
>> only supported case right now is adding multiple bsf in the same
>> check_packet() call and this should be kept, but there is no reason to
>> already move need_auto_bsf() into the loop. Fixing this is not realistic
>> right now because we don't have a real usecase. And furthermore, it
>> would delay your patchset even further.
>> It also means that the first patch of this patchset should be applied,
>> but not the second.
> 
> But how do you share code then?
> 
> Also I don't follow your logic, if you say that no new API should be
> added until it is used, then why not remove the unused multiple bsf
> lists from AVStreamInternal and re-add it when it is actually needed?
> That is the only way I see to not add duplicated list code.
> 
I reconsidered again. My earlier objection was based on the belief that
there is no way to add an already initialized filter to a bsf(-list).
But there actually is: One just has to set the list's idx so that it
tries to drain the new filter first. This is safe.

So if a muxer ever needs the output of earlier filters to decide whether
to append new filters, this could be solved using the bsf-list API as
follows: check_bitstream() initializes the new bsf and already sends the
packet it received to the new bsf. Then it adds the new bsf to the old
bsf(-list) (this will have to be done in some form of av_bsf_join() and
said function would have to set the internal idx of the list to make it
drain the new bsf first; if the new bsf doesn't output anything yet, it
will automatically try the earlier filters again). Of course, not every
check_bitstream() function would have to do this itself as this part can
be factored out into a common function, just like adding a bsf is now
factored out into ff_stream_add_bitstream_filter().
(It should go without saying that check_bitstream() would no longer
receive a const AVPacket * and that the naming would also have to be
adapted if the above would be implemented.)

So I am now confident that we can do anything that a manual bsf list can
do with the bsf-list API as well.* So I see no reason any more to keep
bsf-lists in mux.c; it only needs to support a single bsf.
But given that we don't have a usecase for multiple automatically
inserted bsf at all at the moment, I also don't see a reason why we
should add av_bsf_join() now.

- Andreas
James Almer April 10, 2020, 2:09 p.m. UTC | #8
On 4/10/2020 5:25 AM, Marton Balint wrote:
> 
> 
> On Thu, 9 Apr 2020, James Almer wrote:
> 
>> On 4/9/2020 9:11 PM, Marton Balint wrote:
>>>
>>>
>>> On Fri, 10 Apr 2020, Andreas Rheinhardt wrote:
>>>
>>>> Marton Balint:
>>>>> Signed-off-by: Marton Balint <cus@passwd.hu>
>>>>> ---
>>>>>  doc/APIchanges       |  3 +++
>>>>>  libavcodec/avcodec.h | 19 ++++++++++++++++
>>>>>  libavcodec/bsf.c     | 62
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  libavcodec/version.h |  2 +-
>>>>>  4 files changed, 85 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>>>> index f1d7eac2ee..1473742139 100644
>>>>> --- a/doc/APIchanges
>>>>> +++ b/doc/APIchanges
>>>>> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>>>>>
>>>>>  API changes, most recent first:
>>>>>
>>>>> +2020-04-xx - xxxxxxxxxx - lavc 58.77.102 - avcodec.h
>>>>> +  Add av_bsf_join() to chain bitstream filters.
>>>>> +
>>>>>  2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
>>>>>    av_read_frame() now guarantees to handle uninitialized input
>>>>> packets
>>>>>    and to return refcounted packets on success.
>>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>>>> index 8fc0ad92c9..2812055e8a 100644
>>>>> --- a/libavcodec/avcodec.h
>>>>> +++ b/libavcodec/avcodec.h
>>>>> @@ -6056,6 +6056,25 @@ int av_bsf_receive_packet(AVBSFContext *ctx,
>>>>> AVPacket *pkt);
>>>>>   */
>>>>>  void av_bsf_flush(AVBSFContext *ctx);
>>>>>
>>>>> +/**
>>>>> + * Join a new bitstream filter to an already operating one.
>>>>> + *
>>>>> + * @param[in,out] out_bsf *out_bsf contains the existing, already
>>>>> initialized bitstream
>>>>> + *                        filter, the new filter will be joined to
>>>>> the output of this.
>>>>> + *                        It can be NULL, in which case an empty
>>>>> filter is assumed.
>>>>> + *                        Upon successful return *out_bsf will
>>>>> contain the new, combined
>>>>> + *                        bitstream filter which will be initialized.
>>>>> + * @param[in]     bsf     the bitstream filter to be joined to the
>>>>> output of *out_bsf.
>>>>> + *                        This filter must be uninitialized but it
>>>>> must be ready to be
>>>>> + *                        initalized, so input codec parameters and
>>>>> time base must be
>>>>> + *                        set if *out_bsf is NULL, otherwise the
>>>>> function will set these
>>>>> + *                        automatically based on the output
>>>>> parameters of *out_bsf.
>>>>> + *                        Upon successful return the bsf will be
>>>>> initialized.
>>>>> + *
>>>>> + * @return 0 on success, negative on error.
>>>>
>>>> One needs to be more explicit about what happens on error: bsf may be
>>>> in a partially initialized state and is essentially only good for
>>>> freeing (maybe the bsf parameter should be AVBSFContext **, too, and
>>>> automatically free bsf on error?). But IMO we should aim to not cripple
>>>> *out_bsf and document this.
>>>>
>>>>> + */
>>>>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf);
>>>>> +
>>>>>  /**
>>>>>   * Free a bitstream filter context and everything associated with
>>>>> it; write NULL
>>>>>   * into the supplied pointer.
>>>>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>>>>> index b9fc771a88..1bca28d1ae 100644
>>>>> --- a/libavcodec/bsf.c
>>>>> +++ b/libavcodec/bsf.c
>>>>> @@ -487,6 +487,68 @@ end:
>>>>>      return ret;
>>>>>  }
>>>>>
>>>>> +int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf)
>>>>> +{
>>>>> +    BSFListContext *lst;
>>>>> +    AVBSFContext *obsf = *out_bsf;
>>>>> +    AVBSFContext *new_list_bsf = NULL;
>>>>> +    int ret;
>>>>> +
>>>>> +    if (!obsf) {
>>>>> +        ret = av_bsf_init(bsf);
>>>>> +        if (ret < 0)
>>>>> +            return ret;
>>>>> +        *out_bsf = bsf;
>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>> +    if (obsf->filter != &ff_list_bsf) {
>>>>> +        ret = av_bsf_alloc(&ff_list_bsf, &new_list_bsf);
>>>>> +        if (ret < 0)
>>>>> +            return ret;
>>>>> +        lst = new_list_bsf->priv_data;
>>>>> +        ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs,
>>>>> obsf);
>>>>> +        if (ret < 0)
>>>>> +            goto fail;
>>>>> +        ret = avcodec_parameters_copy(new_list_bsf->par_in,
>>>>> obsf->par_in);
>>>>> +        if (ret < 0)
>>>>> +            goto fail;
>>>>> +        new_list_bsf->time_base_in = obsf->time_base_in;
>>>>> +        obsf = new_list_bsf;
>>>>> +    } else {
>>>>> +        lst = obsf->priv_data;
>>>>> +    }
>>>>> +
>>>>> +    ret = avcodec_parameters_copy(bsf->par_in,
>>>>> lst->bsfs[lst->nb_bsfs-1]->par_out);
>>>>> +    if (ret < 0)
>>>>> +        goto fail;
>>>>> +    bsf->time_base_in = lst->bsfs[lst->nb_bsfs-1]->time_base_out;
>>>>> +
>>>>> +    ret = av_bsf_init(&bsf);
>>>>> +    if (ret < 0)
>>>>> +        goto fail;
>>>>> +
>>>>> +    ret = avcodec_parameters_copy(obsf->par_out, bsf->par_out);
>>>>
>>>> This will change obsf even on failure (when *out_bsf was already of
>>>> type
>>>> ff_list_bsf). This is something that ff_stream_add_bitstream_filter()
>>>> currently doesn't do. I think we should leave obsf in a mostly
>>>> unchanged
>>>> state even if it implies an unnecessary allocation (it is really only a
>>>> single allocation). This can be achieved by taking obsf's par_out in
>>>> the
>>>> else branch above and replacing it with fresh AVCodecParameters. On
>>>> success, the old par_out is freed; on failure it is restored.
>>>>
>>>>> +    if (ret < 0)
>>>>> +        goto fail;
>>>>> +    obsf->time_base_out = bsf->time_base_out;
>>>>> +
>>>>> +    ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, bsf);
>>>>> +    if (ret < 0)
>>>>> +        goto fail;
>>>>> +
>>>>> +    *out_bsf = obsf;
>>>>> +    return 0;
>>>>> +
>>>>> +fail:
>>>>> +    if (new_list_bsf) {
>>>>> +        if (lst->nb_bsfs)
>>>>> +            lst->bsfs[0] = NULL;
>>>>> +        av_bsf_free(&new_list_bsf);
>>>>> +    }
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>>  static int bsf_parse_single(const char *str, AVBSFList *bsf_lst)
>>>>>  {
>>>>>      char *bsf_name, *bsf_options_str, *buf;
>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>> index f4d1d4de21..dadca75430 100644
>>>>> --- a/libavcodec/version.h
>>>>> +++ b/libavcodec/version.h
>>>>> @@ -29,7 +29,7 @@
>>>>>
>>>>>  #define LIBAVCODEC_VERSION_MAJOR  58
>>>>>  #define LIBAVCODEC_VERSION_MINOR  77
>>>>> -#define LIBAVCODEC_VERSION_MICRO 101
>>>>> +#define LIBAVCODEC_VERSION_MICRO 102
>>>>
>>>> Adding a new public function always requires a minor bump.
>>>>>
>>>>>  #define LIBAVCODEC_VERSION_INT 
>>>>> AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>>>>>                                                
>>>>> LIBAVCODEC_VERSION_MINOR, \
>>>>>
>>>>
>>>> Finally, (I feel bad having to tell you this given that you have
>>>> clearly
>>>> invested a lot of time in this) we currently have no case where we need
>>>> more than one automatically inserted bitstream filter per stream. We do
>>>> not know whether the current code does the right thing in case more
>>>> than
>>>> one packet needs to be analyzed to determine whether to insert
>>>> bitstream
>>>> filters (maybe the packets should instead be cached so that no bsf
>>>> misses out on the initial packets?); of course what is the right thing
>>>> might be format and codec-dependent. So I don't know whether we should
>>>> add this function now (given that public API is hard to remove).
>>>>
>>>> This does not mean that we should not use a bsf_list bsf to simplify
>>>> automatic bitstream filtering in mux.c (it really simplifies this alot
>>>> and gets rid of code duplication). But I think we should leave the
>>>> topic
>>>> of adding a bsf to an already existing bsf open so that it can be
>>>> determined when the need indeed arises.
>>>
>>> I see no sane way of sharing list bsf code other than adding public API,
>>> because mux.c is libavformat, bsf.c is libavcodec. This at least allows
>>> you to replace AVStreamInternal->nb_bsfcs and AVStreamInternal->bsfcs
>>> with a single bitstream filter to which you join additional ones.
>>>
>>> Unless you want to remove the existing (admittedly unused, and
>>> potentially inadequate) support for multiple bitstream filters in
>>> AVStreamInternal, I really don't see a way which postpones adding
>>> this API.
>>
>> Can't you achieve the same using existing bsf list API from within lavf?
>> av_bsf_list_alloc(), followed by any required amount of calls to
>> av_bsf_list_append(), av_bsf_list_append2() or av_bsf_list_parse_str()
>> to add each bsf, then av_bsf_list_finalize() at the end?
> 
> That API only works if you have all the bitstream filters before
> finalizing the list, you can't append a BSF to an already finalized
> list. The implementation in mux.c supports adding a BSF, then maybe
> passing a few packets through it, then appending another BSF to the list
> - this can happen if .check_bitstream_filters() adds a bsf but returns
> 0, and at a later packet it adds another bsf.
> 
> Regards,
> Marton

I see what you mean. Do we really need the option to insert a bsf to a
given stream after a few packets have already been processed? Any
required bsf should be present since the beginning. They are all written
to either modify the data or pass it through depending if something must
be done or not.

What could show up say five packets into the stream that would require a
bsf to be inserted at that point and couldn't have been present since
the beginning? Passing packets through within a bsf is almost free, so
there's not a lot to win by postponing inserting a bsf. You'll simply be
delegating the task of analyzing the bitstream to the muxer's
check_bitstream(), when the bsf can already do it without any extra steps.
Marton Balint April 10, 2020, 2:56 p.m. UTC | #9
On Fri, 10 Apr 2020, James Almer wrote:

> On 4/10/2020 5:25 AM, Marton Balint wrote:

[...]

>>>>>
>>>>> Finally, (I feel bad having to tell you this given that you have
>>>>> clearly
>>>>> invested a lot of time in this) we currently have no case where we need
>>>>> more than one automatically inserted bitstream filter per stream. We do
>>>>> not know whether the current code does the right thing in case more
>>>>> than
>>>>> one packet needs to be analyzed to determine whether to insert
>>>>> bitstream
>>>>> filters (maybe the packets should instead be cached so that no bsf
>>>>> misses out on the initial packets?); of course what is the right thing
>>>>> might be format and codec-dependent. So I don't know whether we should
>>>>> add this function now (given that public API is hard to remove).
>>>>>
>>>>> This does not mean that we should not use a bsf_list bsf to simplify
>>>>> automatic bitstream filtering in mux.c (it really simplifies this alot
>>>>> and gets rid of code duplication). But I think we should leave the
>>>>> topic
>>>>> of adding a bsf to an already existing bsf open so that it can be
>>>>> determined when the need indeed arises.
>>>>
>>>> I see no sane way of sharing list bsf code other than adding public API,
>>>> because mux.c is libavformat, bsf.c is libavcodec. This at least allows
>>>> you to replace AVStreamInternal->nb_bsfcs and AVStreamInternal->bsfcs
>>>> with a single bitstream filter to which you join additional ones.
>>>>
>>>> Unless you want to remove the existing (admittedly unused, and
>>>> potentially inadequate) support for multiple bitstream filters in
>>>> AVStreamInternal, I really don't see a way which postpones adding
>>>> this API.
>>>
>>> Can't you achieve the same using existing bsf list API from within lavf?
>>> av_bsf_list_alloc(), followed by any required amount of calls to
>>> av_bsf_list_append(), av_bsf_list_append2() or av_bsf_list_parse_str()
>>> to add each bsf, then av_bsf_list_finalize() at the end?
>> 
>> That API only works if you have all the bitstream filters before
>> finalizing the list, you can't append a BSF to an already finalized
>> list. The implementation in mux.c supports adding a BSF, then maybe
>> passing a few packets through it, then appending another BSF to the list
>> - this can happen if .check_bitstream_filters() adds a bsf but returns
>> 0, and at a later packet it adds another bsf.
>> 
>> Regards,
>> Marton
>
> I see what you mean. Do we really need the option to insert a bsf to a
> given stream after a few packets have already been processed? Any
> required bsf should be present since the beginning. They are all written
> to either modify the data or pass it through depending if something must
> be done or not.
>
> What could show up say five packets into the stream that would require a
> bsf to be inserted at that point and couldn't have been present since
> the beginning? Passing packets through within a bsf is almost free, so
> there's not a lot to win by postponing inserting a bsf. You'll simply be
> delegating the task of analyzing the bitstream to the muxer's
> check_bitstream(), when the bsf can already do it without any extra steps.

Ok, so this also points me to the direction of removing the support of 
multiple BSFs from AVStreamInternal, because if a demuxer wants multiple 
bsfs it can simply create a single list bsf in one go.

Is there anybody who objects removing support for multiple bsfs is 
AVStreamInternal?

Thanks,
Marton
James Almer April 10, 2020, 5 p.m. UTC | #10
On 4/10/2020 11:56 AM, Marton Balint wrote:
> 
> 
> On Fri, 10 Apr 2020, James Almer wrote:
> 
>> On 4/10/2020 5:25 AM, Marton Balint wrote:
> 
> [...]
> 
>>>>>>
>>>>>> Finally, (I feel bad having to tell you this given that you have
>>>>>> clearly
>>>>>> invested a lot of time in this) we currently have no case where we
>>>>>> need
>>>>>> more than one automatically inserted bitstream filter per stream.
>>>>>> We do
>>>>>> not know whether the current code does the right thing in case more
>>>>>> than
>>>>>> one packet needs to be analyzed to determine whether to insert
>>>>>> bitstream
>>>>>> filters (maybe the packets should instead be cached so that no bsf
>>>>>> misses out on the initial packets?); of course what is the right
>>>>>> thing
>>>>>> might be format and codec-dependent. So I don't know whether we
>>>>>> should
>>>>>> add this function now (given that public API is hard to remove).
>>>>>>
>>>>>> This does not mean that we should not use a bsf_list bsf to simplify
>>>>>> automatic bitstream filtering in mux.c (it really simplifies this
>>>>>> alot
>>>>>> and gets rid of code duplication). But I think we should leave the
>>>>>> topic
>>>>>> of adding a bsf to an already existing bsf open so that it can be
>>>>>> determined when the need indeed arises.
>>>>>
>>>>> I see no sane way of sharing list bsf code other than adding public
>>>>> API,
>>>>> because mux.c is libavformat, bsf.c is libavcodec. This at least
>>>>> allows
>>>>> you to replace AVStreamInternal->nb_bsfcs and AVStreamInternal->bsfcs
>>>>> with a single bitstream filter to which you join additional ones.
>>>>>
>>>>> Unless you want to remove the existing (admittedly unused, and
>>>>> potentially inadequate) support for multiple bitstream filters in
>>>>> AVStreamInternal, I really don't see a way which postpones adding
>>>>> this API.
>>>>
>>>> Can't you achieve the same using existing bsf list API from within
>>>> lavf?
>>>> av_bsf_list_alloc(), followed by any required amount of calls to
>>>> av_bsf_list_append(), av_bsf_list_append2() or av_bsf_list_parse_str()
>>>> to add each bsf, then av_bsf_list_finalize() at the end?
>>>
>>> That API only works if you have all the bitstream filters before
>>> finalizing the list, you can't append a BSF to an already finalized
>>> list. The implementation in mux.c supports adding a BSF, then maybe
>>> passing a few packets through it, then appending another BSF to the list
>>> - this can happen if .check_bitstream_filters() adds a bsf but returns
>>> 0, and at a later packet it adds another bsf.
>>>
>>> Regards,
>>> Marton
>>
>> I see what you mean. Do we really need the option to insert a bsf to a
>> given stream after a few packets have already been processed? Any
>> required bsf should be present since the beginning. They are all written
>> to either modify the data or pass it through depending if something must
>> be done or not.
>>
>> What could show up say five packets into the stream that would require a
>> bsf to be inserted at that point and couldn't have been present since
>> the beginning? Passing packets through within a bsf is almost free, so
>> there's not a lot to win by postponing inserting a bsf. You'll simply be
>> delegating the task of analyzing the bitstream to the muxer's
>> check_bitstream(), when the bsf can already do it without any extra
>> steps.
> 
> Ok, so this also points me to the direction of removing the support of
> multiple BSFs from AVStreamInternal, because if a demuxer wants multiple
> bsfs it can simply create a single list bsf in one go.

This will also allow you to simplify things by moving the
oformat->check_bitstream() calls from do_packet_auto_bsf() to
init_muxer(), right after the call to oformat->init(), since you're not
longer constrained by the need for packets mid muxing.

> 
> Is there anybody who objects removing support for multiple bsfs is
> AVStreamInternal?
> 
> Thanks,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andreas Rheinhardt April 10, 2020, 5:42 p.m. UTC | #11
James Almer:
> On 4/10/2020 11:56 AM, Marton Balint wrote:
>>
>>
>> On Fri, 10 Apr 2020, James Almer wrote:
>>
>>> On 4/10/2020 5:25 AM, Marton Balint wrote:
>>
>> [...]
>>
>>>>>>>
>>>>>>> Finally, (I feel bad having to tell you this given that you have
>>>>>>> clearly
>>>>>>> invested a lot of time in this) we currently have no case where we
>>>>>>> need
>>>>>>> more than one automatically inserted bitstream filter per stream.
>>>>>>> We do
>>>>>>> not know whether the current code does the right thing in case more
>>>>>>> than
>>>>>>> one packet needs to be analyzed to determine whether to insert
>>>>>>> bitstream
>>>>>>> filters (maybe the packets should instead be cached so that no bsf
>>>>>>> misses out on the initial packets?); of course what is the right
>>>>>>> thing
>>>>>>> might be format and codec-dependent. So I don't know whether we
>>>>>>> should
>>>>>>> add this function now (given that public API is hard to remove).
>>>>>>>
>>>>>>> This does not mean that we should not use a bsf_list bsf to simplify
>>>>>>> automatic bitstream filtering in mux.c (it really simplifies this
>>>>>>> alot
>>>>>>> and gets rid of code duplication). But I think we should leave the
>>>>>>> topic
>>>>>>> of adding a bsf to an already existing bsf open so that it can be
>>>>>>> determined when the need indeed arises.
>>>>>>
>>>>>> I see no sane way of sharing list bsf code other than adding public
>>>>>> API,
>>>>>> because mux.c is libavformat, bsf.c is libavcodec. This at least
>>>>>> allows
>>>>>> you to replace AVStreamInternal->nb_bsfcs and AVStreamInternal->bsfcs
>>>>>> with a single bitstream filter to which you join additional ones.
>>>>>>
>>>>>> Unless you want to remove the existing (admittedly unused, and
>>>>>> potentially inadequate) support for multiple bitstream filters in
>>>>>> AVStreamInternal, I really don't see a way which postpones adding
>>>>>> this API.
>>>>>
>>>>> Can't you achieve the same using existing bsf list API from within
>>>>> lavf?
>>>>> av_bsf_list_alloc(), followed by any required amount of calls to
>>>>> av_bsf_list_append(), av_bsf_list_append2() or av_bsf_list_parse_str()
>>>>> to add each bsf, then av_bsf_list_finalize() at the end?
>>>>
>>>> That API only works if you have all the bitstream filters before
>>>> finalizing the list, you can't append a BSF to an already finalized
>>>> list. The implementation in mux.c supports adding a BSF, then maybe
>>>> passing a few packets through it, then appending another BSF to the list
>>>> - this can happen if .check_bitstream_filters() adds a bsf but returns
>>>> 0, and at a later packet it adds another bsf.
>>>>
>>>> Regards,
>>>> Marton
>>>
>>> I see what you mean. Do we really need the option to insert a bsf to a
>>> given stream after a few packets have already been processed? Any
>>> required bsf should be present since the beginning. They are all written
>>> to either modify the data or pass it through depending if something must
>>> be done or not.
>>>
>>> What could show up say five packets into the stream that would require a
>>> bsf to be inserted at that point and couldn't have been present since
>>> the beginning? Passing packets through within a bsf is almost free, so
>>> there's not a lot to win by postponing inserting a bsf. You'll simply be
>>> delegating the task of analyzing the bitstream to the muxer's
>>> check_bitstream(), when the bsf can already do it without any extra
>>> steps.
>>
>> Ok, so this also points me to the direction of removing the support of
>> multiple BSFs from AVStreamInternal, because if a demuxer wants multiple
>> bsfs it can simply create a single list bsf in one go.
> 
> This will also allow you to simplify things by moving the
> oformat->check_bitstream() calls from do_packet_auto_bsf() to
> init_muxer(), right after the call to oformat->init(), since you're not
> longer constrained by the need for packets mid muxing.
> 
If it is decided that we no longer need to actually look at packet data
any more for adding bitstream filters, then we can actually remove the
check_bitstream functions altogether and let the muxer set things up
during init.

But this is not how I understood Marton. He only wanted to take away the
muxer's ability to add bitstream filters during multiple calls to
check_bitstream().

Btw: need_auto_bsf() from [1] can also be simplified a lot: One adds a
new field to AVStreamInternal bsf_checking_status. Possible values are 1
(checking is finished and one needs bsf), 0 (checking is finished and no
bsf are needed) and -1 (checking not finished). If the
AVFMT_FLAG_AUTO_BSF is not set or if the AVOutputFormat doesn't have a
check_bitstream() function, it is set to 0 during init, otherwise to -1.
need_auto_bsf() would then begin with "if
(st->internal->bsf_checking_status >= 0) return
st->internal->bsf_checking_status;". Otherwise, the bitstream would
really be checked (the check for the existence of the check_bitstream
function could then be omitted, too, of course), which would update
bsf_checking_status accordingly.

- Andreas

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/259204.html
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index f1d7eac2ee..1473742139 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,9 @@  libavutil:     2017-10-21
 
 API changes, most recent first:
 
+2020-04-xx - xxxxxxxxxx - lavc 58.77.102 - avcodec.h
+  Add av_bsf_join() to chain bitstream filters.
+
 2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
   av_read_frame() now guarantees to handle uninitialized input packets
   and to return refcounted packets on success.
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 8fc0ad92c9..2812055e8a 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -6056,6 +6056,25 @@  int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt);
  */
 void av_bsf_flush(AVBSFContext *ctx);
 
+/**
+ * Join a new bitstream filter to an already operating one.
+ *
+ * @param[in,out] out_bsf *out_bsf contains the existing, already initialized bitstream
+ *                        filter, the new filter will be joined to the output of this.
+ *                        It can be NULL, in which case an empty filter is assumed.
+ *                        Upon successful return *out_bsf will contain the new, combined
+ *                        bitstream filter which will be initialized.
+ * @param[in]     bsf     the bitstream filter to be joined to the output of *out_bsf.
+ *                        This filter must be uninitialized but it must be ready to be
+ *                        initalized, so input codec parameters and time base must be
+ *                        set if *out_bsf is NULL, otherwise the function will set these
+ *                        automatically based on the output parameters of *out_bsf.
+ *                        Upon successful return the bsf will be initialized.
+ *
+ * @return 0 on success, negative on error.
+ */
+int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf);
+
 /**
  * Free a bitstream filter context and everything associated with it; write NULL
  * into the supplied pointer.
diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
index b9fc771a88..1bca28d1ae 100644
--- a/libavcodec/bsf.c
+++ b/libavcodec/bsf.c
@@ -487,6 +487,68 @@  end:
     return ret;
 }
 
+int av_bsf_join(AVBSFContext **out_bsf, AVBSFContext *bsf)
+{
+    BSFListContext *lst;
+    AVBSFContext *obsf = *out_bsf;
+    AVBSFContext *new_list_bsf = NULL;
+    int ret;
+
+    if (!obsf) {
+        ret = av_bsf_init(bsf);
+        if (ret < 0)
+            return ret;
+        *out_bsf = bsf;
+        return 0;
+    }
+
+    if (obsf->filter != &ff_list_bsf) {
+        ret = av_bsf_alloc(&ff_list_bsf, &new_list_bsf);
+        if (ret < 0)
+            return ret;
+        lst = new_list_bsf->priv_data;
+        ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, obsf);
+        if (ret < 0)
+            goto fail;
+        ret = avcodec_parameters_copy(new_list_bsf->par_in, obsf->par_in);
+        if (ret < 0)
+            goto fail;
+        new_list_bsf->time_base_in = obsf->time_base_in;
+        obsf = new_list_bsf;
+    } else {
+        lst = obsf->priv_data;
+    }
+
+    ret = avcodec_parameters_copy(bsf->par_in, lst->bsfs[lst->nb_bsfs-1]->par_out);
+    if (ret < 0)
+        goto fail;
+    bsf->time_base_in = lst->bsfs[lst->nb_bsfs-1]->time_base_out;
+
+    ret = av_bsf_init(&bsf);
+    if (ret < 0)
+        goto fail;
+
+    ret = avcodec_parameters_copy(obsf->par_out, bsf->par_out);
+    if (ret < 0)
+        goto fail;
+    obsf->time_base_out = bsf->time_base_out;
+
+    ret = av_dynarray_add_nofree(&lst->bsfs, &lst->nb_bsfs, bsf);
+    if (ret < 0)
+        goto fail;
+
+    *out_bsf = obsf;
+    return 0;
+
+fail:
+    if (new_list_bsf) {
+        if (lst->nb_bsfs)
+            lst->bsfs[0] = NULL;
+        av_bsf_free(&new_list_bsf);
+    }
+    return ret;
+}
+
 static int bsf_parse_single(const char *str, AVBSFList *bsf_lst)
 {
     char *bsf_name, *bsf_options_str, *buf;
diff --git a/libavcodec/version.h b/libavcodec/version.h
index f4d1d4de21..dadca75430 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -29,7 +29,7 @@ 
 
 #define LIBAVCODEC_VERSION_MAJOR  58
 #define LIBAVCODEC_VERSION_MINOR  77
-#define LIBAVCODEC_VERSION_MICRO 101
+#define LIBAVCODEC_VERSION_MICRO 102
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
                                                LIBAVCODEC_VERSION_MINOR, \