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 <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.
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
_______________________________________________
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".
