Re: [FFmpeg-devel] [PATCH v2 4/6] avformat/mux: add proper support for full N:M bitstream filtering

2020-04-25 Thread Andreas Rheinhardt
Marton Balint:
> Previously only 1:1 bitstream filters were supported, the end of the stream 
> was
> not signalled to the bitstream filters and time base changes were ignored.
> 
> Signed-off-by: Marton Balint 
> ---
>  libavformat/mux.c | 91 
> ++-
>  1 file changed, 57 insertions(+), 34 deletions(-)
> 
> diff --git a/libavformat/mux.c b/libavformat/mux.c
> index 4118d221e0..c2b6d4461e 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -822,14 +822,13 @@ static int prepare_input_packet(AVFormatContext *s, 
> AVPacket *pkt)
>  return 0;
>  }
>  
> -static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) {
> -AVStream *st = s->streams[pkt->stream_index];
> +static int need_auto_bsf(AVFormatContext *s, AVStream *st, AVPacket *pkt) {

{ belongs in a line of its own.

>  int ret;
>  
>  if (!(s->flags & AVFMT_FLAG_AUTO_BSF))
> -return 1;
> +return 0;
>  
> -if (s->oformat->check_bitstream) {
> +if (pkt && s->oformat->check_bitstream) {
>  if (!st->internal->bitstream_checked) {
>  if ((ret = s->oformat->check_bitstream(s, pkt)) < 0)
>  return ret;
> @@ -838,31 +837,7 @@ static int do_packet_auto_bsf(AVFormatContext *s, 
> AVPacket *pkt) {
>  }
>  }
>  
> -if (st->internal->bsfc) {
> -AVBSFContext *ctx = st->internal->bsfc;
> -// TODO: when any bitstream filter requires flushing at EOF, we'll 
> need to
> -// flush each stream's BSF chain on write_trailer.
> -if ((ret = av_bsf_send_packet(ctx, pkt)) < 0) {
> -av_log(ctx, AV_LOG_ERROR,
> -"Failed to send packet to filter %s for stream %d\n",
> -ctx->filter->name, pkt->stream_index);
> -return ret;
> -}
> -// TODO: when any automatically-added bitstream filter is generating 
> multiple
> -// output packets for a single input one, we'll need to call this in 
> a loop
> -// and write each output packet.
> -if ((ret = av_bsf_receive_packet(ctx, pkt)) < 0) {
> -if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF)
> -return 0;
> -av_log(ctx, AV_LOG_ERROR,
> -"Failed to receive packet from filter %s for stream 
> %d\n",
> -ctx->filter->name, pkt->stream_index);
> -if (s->error_recognition & AV_EF_EXPLODE)
> -return ret;
> -return 0;
> -}
> -}
> -return 1;
> +return !!st->internal->bsfc;
>  }
>  
>  static int interleaved_write_packet(AVFormatContext *s, AVPacket *pkt, int 
> flush);
> @@ -889,17 +864,56 @@ static int write_packet_common(AVFormatContext *s, 
> AVStream *st, AVPacket *pkt,
>  }
>  }
>  
> +static int write_packets_from_bsfs(AVFormatContext *s, AVStream *st, 
> AVPacket *pkt, int interleaved)
> +{
> +AVBSFContext *bsfc = st->internal->bsfc;
> +AVPacket opkt = {0};

The doc of av_bsf_receive_packet() states that this packet "should be
"clean" (i.e. freshly allocated with av_packet_alloc() or unreffed with
av_packet_unref())". Simply zeroing it does not fulfill this requirement.
I suggest an alternative approach: We only need a spare packet for
av_bsf_receive_packet() if we don't already have a packet; and this
happens if and only if this function is called from av_write_trailer().
So this spare packet should reside in av_write_trailer() and be properly
initialized there (so that it can be used to signal EOF to the bsf); pkt
should be the only packet in this function. This avoids unnecessary
initializations.

> +int ret;
> +
> +if ((ret = av_bsf_send_packet(bsfc, pkt)) < 0) {
> +av_log(s, AV_LOG_ERROR,
> +"Failed to send packet to filter %s for stream %d\n",
> +bsfc->filter->name, st->index);
> +return ret;
> +}
> +
> +do {
> +ret = av_bsf_receive_packet(bsfc, );
> +if (ret < 0) {
> +if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF)
> +ret = 0;

You can just return 0 here and remove the next check.

> +if (ret < 0) {
> +av_log(s, AV_LOG_ERROR, "Error applying bitstream filters to 
> an output "
> +   "packet for stream #%d: %s\n", st->index, 
> av_err2str(ret));
> +if (!(s->error_recognition & AV_EF_EXPLODE) && ret != 
> AVERROR(ENOMEM))
> +continue;
> +}
> +return ret;
> +}
> +av_packet_rescale_ts(, bsfc->time_base_out, st->time_base);
> +ret = write_packet_common(s, st, , interleaved);
> +if (!interleaved) // write_packet_common already unrefed opkt for 
> interleaved

Unfortunately not. It does this only if interleaved_write_packet() has
been reached. If not, opkt leaks.

If you followed the above recommendation, then the memleak for the
interleaved codepath would be 

Re: [FFmpeg-devel] [PATCH v2 4/6] avformat/mux: add proper support for full N:M bitstream filtering

2020-04-18 Thread John Stebbins
On Sat, 2020-04-18 at 21:18 +0200, Marton Balint wrote:
> Previously only 1:1 bitstream filters were supported, the end of the
> stream was
> not signalled to the bitstream filters and time base changes were
> ignored.
> 
> Signed-off-by: Marton Balint 
> ---
>  libavformat/mux.c | 91 ++---
> --
>  1 file changed, 57 insertions(+), 34 deletions(-)
> 

FWIW, this works as expected with my PGS splitter bsf
___
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".