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 <c...@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?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
