On Tue, 7 Apr 2020, Andreas Rheinhardt wrote:
Marton Balint:
Signed-off-by: Marton Balint <c...@passwd.hu>
---
Changelog | 1 +
doc/bitstream_filters.texi | 30 ++++++
libavcodec/Makefile | 1 +
libavcodec/bitstream_filters.c | 1 +
libavcodec/pcm_rechunk_bsf.c | 206 +++++++++++++++++++++++++++++++++++++++++
libavcodec/version.h | 4 +-
6 files changed, 241 insertions(+), 2 deletions(-)
create mode 100644 libavcodec/pcm_rechunk_bsf.c
diff --git a/Changelog b/Changelog
index 05b9a84562..dddaf02199 100644
--- a/Changelog
+++ b/Changelog
@@ -55,6 +55,7 @@ version <next>:
- CRI HCA decoder
- CRI HCA demuxer
- overlay_cuda filter
+- pcm_rechunk bitstream filter
[..]
+static int init(AVBSFContext *ctx)
+{
+ PCMContext *s = ctx->priv_data;
+ AVRational sr = av_make_q(ctx->par_in->sample_rate, 1);
+ int64_t max_samples;
+
+ ctx->time_base_out = av_inv_q(sr);
Is it actually guaranteed that par_in->sample_rate is not 0?
Yes, it is checked in mux.c:init_muxer.
+ s->in_pkt = av_packet_alloc();
+ s->out_pkt = av_packet_alloc();
+ if (!s->in_pkt || !s->out_pkt)
+ return AVERROR(ENOMEM);
These allocations will have been wasted if one errors out below, so they
should be moved to the end of this function.
Ok.
[..]
+static int rechunk_filter(AVBSFContext *ctx, AVPacket *pkt)
+{
+ PCMContext *s = ctx->priv_data;
+ AVRational sr = av_make_q(ctx->par_in->sample_rate, 1);
+ int nb_samples = s->frame_rate.num ? (av_rescale_q(s->n + 1, sr, s->frame_rate)
- s->dts) : s->nb_out_samples;
+ int data_size = nb_samples * s->sample_size;
+ int ret;
+
+ if (!s->out_pkt->data) {
+ ret = av_new_packet(s->out_pkt, s->max_packet_size);
+ if (ret < 0)
+ return ret;
+ s->out_pkt->size = 0;
+ }
+
+ do {
+ if (s->in_pkt->size) {
+ if (s->out_pkt->size || s->in_pkt->size < data_size) {
+ int drain = FFMIN(s->in_pkt->size, data_size -
s->out_pkt->size);
+ if (!s->out_pkt->size) {
+ ret = av_packet_copy_props(s->out_pkt, s->in_pkt);
+ if (ret < 0)
+ return ret;
+ }
+ memcpy(s->out_pkt->data + s->out_pkt->size, s->in_pkt->data,
drain);
+ s->out_pkt->size += drain;
+ s->in_pkt->size -= drain;
+ s->in_pkt->data += drain;
This could be aligned on =.
Ok.
+ if (s->out_pkt->size == data_size) {
+ av_packet_move_ref(pkt, s->out_pkt);
If the current pkt is a packet with a smaller amount of samples than the
maximum, then the data immediately after the packet data will not be the
(zeroed) padding, but uninitialized data before the zeroed padding. This
is not good (it won't lead to segfaults, but it might lead to Valgrind
warnings). See below for a suggestion how to fix this.
Ok.
+ return send_packet(s, nb_samples, pkt);
+ }
+ av_packet_unref(s->in_pkt);
If out_pkt initially already contained data and a new in_pkt provides
exactly as much data as needed to output another packet, then you will
set in_pkt->size to zero above, but you will do not unref it. Given that
the code treats "size == 0" as sign that the packet is blank, this will
lead to memleaks.
Ok.
+ } else if (s->in_pkt->size > data_size) {
+ ret = av_packet_ref(pkt, s->in_pkt);
+ if (ret < 0)
+ return ret;
+ pkt->size = data_size;
+ s->in_pkt->size -= data_size;
+ s->in_pkt->data += data_size;
+ return send_packet(s, nb_samples, pkt);
+ } else {
+ av_assert0(s->in_pkt->size == data_size);
+ av_packet_move_ref(pkt, s->in_pkt);
+ return send_packet(s, nb_samples, pkt);
+ }
+ }
+
+ ret = ff_bsf_get_packet_ref(ctx, s->in_pkt);
Doing this here in a loop is either pointless or an API violation (but
the internal API is not really documented anyway): The caller is
supposed to provide a packet via av_bsf_send_packet() and then call
av_bsf_receive_packet() until the bsf is completely drained. Then he
needs to send a new packet. The bsf meanwhile uses
ff_bsf_get_packet[_ref] to get the packet when
AVBitStreamFilter.filter() is executed. The bsf API implies that it is
impossible for two ff_bsf_get_packet_ref() calls to succeed in the same
AVBitStreamFilter.filter() call, so that your loop is actually a fake loop.
It is true that the loop is executed maximum 2 times, yet this loop seemed
(and still seems to me) the most simple way to implement processing of an
existing in_pkt and the next in_pkt.
I only rely on that ff_bsf_get_packet_ref() returns EAGAIN on subsequent
calls, and this looks like a safe bet. I don't see how a different
implementation can yield simpler code.
I therefore suggest to adapt rechunk_filter() as follows:
a) There is no reason at all why both in_pkt and out_pkt should be
non-blank at the same time (except during the brief period when one
copies from in to out, of course). The code needs to ensure this.
b) At the start of rechunk_filter(), you check for whether there is
enough data in in_pkt to output a complete packet. If there is, you
output it (and update in_pkt); if there is some, but not enough data in
in_pkt, you allocate exactly as much space in out_pkt as needed for the
next output packet, copy the remaining data (as well as the properties)
from in_pkt and unref in_pkt. (There is unfortunately no
av_packet_move_props(), maybe there should be one?)
c) If you did not exit rechunk_filter() in b), you read another packet
into in_pkt.
If out_pkt is empty and in_pkt contains enough data, you output this data.
If out_pkt is empty and in_pkt contains not enough data, you return
AVERROR(EAGAIN).
If out_pkt is not empty, you copy as much data as needed/available into
out_pkt (which already has the right size); if there is enough data, you
output out_pkt (and potentially unref in_pkt if it has been exhausted).
If there is not enough data, you unref in_pkt and return AVERROR(EAGAIN).
(Notice that max_packet_size can be removed from PCMContext.)
In case you are wondering whether a) could be used to omit one of the
packets: It is possible to omit out_pkt (but I am not sure how
advantageous it would be):
b') If there is enough data in in_pkt for a complete packet, you output
it (via av_packet_move_ref() or av_packet_ref()); if not and if there is
data left in in_pkt and if the actual buffer of in_pkt is not already of
the right size, you copy the props of in_pkt to pkt and allocate a
buffer of the right size (with padding, of course) in pkt and copy the
remaining data of in_pkt to pkt and reset in_pkt. (Alternatively, you
could move in_pkt to pkt and directly allocate an AVBufferRef with the
right size, copy the data, unref the old buffer and replace it with the
new one.)
If there is not enough data in in_pkt and if in_pkt already contains a
buffer of the right size, then you simply move in_pkt to pkt.
c') Then you get a new packet and put it into in_pkt. In case pkt
already contains data, you copy as much data as needed/available into
pkt. If it is enough, you output pkt. If it is not enough, in_pkt can be
unreferenced and you move pkt into in_pkt and return AVERROR(EAGAIN).
It should go without saying that you need to somehow record whether
in_pkt already contains a buffer of the right size (this is trivial in
the approach above: if out_pkt contains data it also contains a buffer
of the right size). I prefer the first approach.
Yeah, me too, that is what I (hopefully) will implement in the next
version of the patch.
+ if (ret == AVERROR_EOF && s->out_pkt->size) {
+ if (s->pad) {
+ memset(s->out_pkt->data + s->out_pkt->size, 0, data_size -
s->out_pkt->size);
+ s->out_pkt->size = data_size;
+ } else {
+ nb_samples = s->out_pkt->size / s->sample_size;
+ }
+ av_packet_move_ref(pkt, s->out_pkt);
+ return send_packet(s, nb_samples, pkt);
+ }
+ } while (ret >= 0);
+
+ return ret;
+}
+
+#define OFFSET(x) offsetof(PCMContext, x)
+#define FLAGS (AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_BSF_PARAM)
+static const AVOption options[] = {
+ { "nb_out_samples", "set the number of per-packet output samples",
OFFSET(nb_out_samples), AV_OPT_TYPE_INT, {.i64=1024}, 1, INT_MAX, FLAGS },
+ { "n", "set the number of per-packet output samples",
OFFSET(nb_out_samples), AV_OPT_TYPE_INT, {.i64=1024}, 1, INT_MAX, FLAGS },
+ { "pad", "pad last packet with zeros",
OFFSET(pad), AV_OPT_TYPE_BOOL, {.i64=1} , 0, 1, FLAGS },
+ { "p", "pad last packet with zeros",
OFFSET(pad), AV_OPT_TYPE_BOOL, {.i64=1} , 0, 1, FLAGS },
+ { "frame_rate", "set number of packets per second",
OFFSET(frame_rate), AV_OPT_TYPE_RATIONAL, {.dbl=0}, 0, INT_MAX, FLAGS },
+ { "r", "set number of packets per second",
OFFSET(frame_rate), AV_OPT_TYPE_RATIONAL, {.dbl=0}, 0, INT_MAX, FLAGS },
+ { NULL },
+};
+
+static const AVClass metadata_class = {
You seem to have copied this name from the *_metadata_bsf bitstream
filters. It is not really appropriate here.
Yes, will change it.
Thanks,
Marton
_______________________________________________
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".
