diff mbox series

[FFmpeg-devel,7/8] fftools/ffmpeg_demux: implement -bsf for input

Message ID 20240105164251.28935-7-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,1/8] fftools/ffmpeg_demux: replace abort() by av_assert0(0) | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Anton Khirnov Jan. 5, 2024, 4:42 p.m. UTC
Previously bitstream filters could only be applied right before muxing,
this allows to apply them right after demuxing.
---
 Changelog                       |   1 +
 doc/ffmpeg.texi                 |  22 +++--
 fftools/ffmpeg_demux.c          | 139 ++++++++++++++++++++++++++++----
 fftools/ffmpeg_opt.c            |   2 +-
 tests/fate/ffmpeg.mak           |   5 ++
 tests/ref/fate/ffmpeg-bsf-input |  18 +++++
 6 files changed, 164 insertions(+), 23 deletions(-)
 create mode 100644 tests/ref/fate/ffmpeg-bsf-input

Comments

Stefano Sabatini Jan. 6, 2024, 12:12 p.m. UTC | #1
On date Friday 2024-01-05 17:42:50 +0100, Anton Khirnov wrote:
> Previously bitstream filters could only be applied right before muxing,
> this allows to apply them right after demuxing.
> ---
>  Changelog                       |   1 +
>  doc/ffmpeg.texi                 |  22 +++--
>  fftools/ffmpeg_demux.c          | 139 ++++++++++++++++++++++++++++----
>  fftools/ffmpeg_opt.c            |   2 +-
>  tests/fate/ffmpeg.mak           |   5 ++
>  tests/ref/fate/ffmpeg-bsf-input |  18 +++++
>  6 files changed, 164 insertions(+), 23 deletions(-)
>  create mode 100644 tests/ref/fate/ffmpeg-bsf-input
> 
> diff --git a/Changelog b/Changelog
> index 5b2899d05b..f8191d88c7 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -18,6 +18,7 @@ version <next>:
>  - lavu/eval: introduce randomi() function in expressions
>  - VVC decoder
>  - fsync filter
> +- ffmpeg CLI -bsf option may now be used for input as well as output
>  
>  version 6.1:
>  - libaribcaption decoder
> diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
> index d75517b443..59f7badcb6 100644
> --- a/doc/ffmpeg.texi
> +++ b/doc/ffmpeg.texi
> @@ -2093,26 +2093,34 @@ an output mpegts file:
>  ffmpeg -i inurl -streamid 0:33 -streamid 1:36 out.ts
>  @end example
>  
> -@item -bsf[:@var{stream_specifier}] @var{bitstream_filters} (@emph{output,per-stream})
> -Apply bitstream filters to matching streams.
> +@item -bsf[:@var{stream_specifier}] @var{bitstream_filters} (@emph{input/output,per-stream})
> +Apply bitstream filters to matching streams. The filters are applied to each
> +packet as it is received from the demuxer (when used as an input option) or
> +before it is sent to the muxer (when used as an output option).
>  
>  @var{bitstream_filters} is a comma-separated list of bitstream filter
> -specifications. The specified bitstream filters are applied to coded packets in
> -the order they are written in. Each bitstream filter specification is of the
> -form
> +specifications, each of the form
>  @example
>  @var{filter}[=@var{optname0}=@var{optval0}:@var{optname1}=@var{optval1}:...]
>  @end example
>  Any of the ',=:' characters that are to be a part of an option value need to be
>  escaped with a backslash.
>  
> -Use the @code{-bsfs} option to get the list of bitstream filters.
> +Use the @code{-bsfs} option to get the list of bitstream filters. E.g.

This looks spurious, since this suggests the example is about the
listing, and it's applying a weird order of example/explanation
(rather than the opposite).

What about something as:
--------------------------
[...]
Use the @code{-bsfs} option to get the list of bitstream filters.

Some examples follow.

To apply the @code{h264_mp4toannexb} bitstream filter (which converts
MP4-encapsulated H.264 stream to Annex B) to the @emph{input} video stream:
@example
...
@end example

To apply the @code{mov2textsub} bitstream filter (which extracts text from MOV
subtitles) to the @emph{output} subtitle stream:
@example
...
@end example

Note, however, that since both examples use @code{-c copy}, it matters
little whether the filters are applied on input or output - that would
change if transcoding was happening.
--------------------------

[...]
> +on input or output - that would change if transcoding was hapenning.

hapenning typo

>  
>  @item -tag[:@var{stream_specifier}] @var{codec_tag} (@emph{input/output,per-stream})
>  Force a tag/fourcc for matching streams.
> diff --git a/fftools/ffmpeg_demux.c b/fftools/ffmpeg_demux.c
> index eae1f0bde5..16d4f67e59 100644
> --- a/fftools/ffmpeg_demux.c
> +++ b/fftools/ffmpeg_demux.c
> @@ -34,6 +34,7 @@
>  #include "libavutil/time.h"
>  #include "libavutil/timestamp.h"
>  
> +#include "libavcodec/bsf.h"
>  #include "libavcodec/packet.h"
>  
>  #include "libavformat/avformat.h"
> @@ -71,6 +72,8 @@ typedef struct DemuxStream {
>  
>      const AVCodecDescriptor *codec_desc;
>  
> +    AVBSFContext *bsf;
> +
>      /* number of packets successfully read for this stream */
>      uint64_t nb_packets;
>      // combined size of all the packets read
> @@ -118,6 +121,8 @@ typedef struct Demuxer {
>  typedef struct DemuxThreadContext {
>      // packet used for reading from the demuxer
>      AVPacket *pkt_demux;
> +    // packet for reading from BSFs
> +    AVPacket *pkt_bsf;
>  } DemuxThreadContext;
>  
>  static DemuxStream *ds_from_ist(InputStream *ist)
> @@ -513,13 +518,17 @@ static int do_send(Demuxer *d, DemuxStream *ds, AVPacket *pkt, unsigned flags,
>      return 0;
>  }
>  
> -static int demux_send(Demuxer *d, DemuxStream *ds, AVPacket *pkt, unsigned flags)
> +static int demux_send(Demuxer *d, DemuxThreadContext *dt, DemuxStream *ds,
> +                      AVPacket *pkt, unsigned flags)
>  {
>      InputFile  *f = &d->f;
>      int ret;
>  
> +    // pkt can be NULL only when flushing BSFs
> +    av_assert0(ds->bsf || pkt);
> +
>      // send heartbeat for sub2video streams
> -    if (d->pkt_heartbeat && pkt->pts != AV_NOPTS_VALUE) {
> +    if (d->pkt_heartbeat && pkt && pkt->pts != AV_NOPTS_VALUE) {
>          for (int i = 0; i < f->nb_streams; i++) {
>              DemuxStream *ds1 = ds_from_ist(f->streams[i]);
>  
> @@ -537,10 +546,69 @@ static int demux_send(Demuxer *d, DemuxStream *ds, AVPacket *pkt, unsigned flags
>          }
>      }
>  
> -    ret = do_send(d, ds, pkt, flags, "demuxed");
> -    if (ret < 0)
> -        return ret;
> +    if (ds->bsf) {
> +        if (pkt)
> +            av_packet_rescale_ts(pkt, pkt->time_base, ds->bsf->time_base_in);
>  
> +        ret = av_bsf_send_packet(ds->bsf, pkt);
> +        if (ret < 0) {
> +            if (pkt)
> +                av_packet_unref(pkt);

> +            av_log(ds, AV_LOG_ERROR, "Error submitting a packet for filtering: %s\n",

might be useful to signal the stream identifier

> +                   av_err2str(ret));

possibly redundant? (IIRC this is shown anyway in the outer level failure message)

> +            return ret;
> +        }
> +

> +        while (1) {
> +            ret = av_bsf_receive_packet(ds->bsf, dt->pkt_bsf);
> +            if (ret == AVERROR(EAGAIN))
> +                return 0;
> +            else if (ret < 0) {
> +                if (ret != AVERROR_EOF)

> +                    av_log(ds, AV_LOG_ERROR,
> +                           "Error applying bitstream filters to a packet: %s\n",
> +                           av_err2str(ret));

ditto

> +                return ret;
> +            }
> +
> +            dt->pkt_bsf->time_base = ds->bsf->time_base_out;
> +
> +            ret = do_send(d, ds, dt->pkt_bsf, 0, "filtered");
> +            if (ret < 0) {
> +                av_packet_unref(dt->pkt_bsf);
> +                return ret;
> +            }
> +        }
> +    } else {
> +        ret = do_send(d, ds, pkt, flags, "demuxed");
> +        if (ret < 0)
> +            return ret;
> +    }
> +
> +    return 0;
> +}
> +
> +static int demux_bsf_flush(Demuxer *d, DemuxThreadContext *dt)
> +{
> +    InputFile *f = &d->f;
> +    int ret;
> +
> +    for (unsigned i = 0; i < f->nb_streams; i++) {
> +        DemuxStream *ds = ds_from_ist(f->streams[i]);
> +
> +        if (!ds->bsf)
> +            continue;
> +
> +        ret = demux_send(d, dt, ds, NULL, 0);
> +        ret = (ret == AVERROR_EOF) ? 0 : (ret < 0) ? ret : AVERROR_BUG;
> +        if (ret < 0) {

> +            av_log(ds, AV_LOG_ERROR, "Error flushing BSFs: %s\n",
> +                   av_err2str(ret));

ditto

> +            return ret;
> +        }
> +
> +        av_bsf_flush(ds->bsf);
> +    }
>  
>      return 0;
>  }
> @@ -573,6 +641,7 @@ static void thread_set_name(InputFile *f)
>  static void demux_thread_uninit(DemuxThreadContext *dt)
>  {
>      av_packet_free(&dt->pkt_demux);
> +    av_packet_free(&dt->pkt_bsf);
>  
>      memset(dt, 0, sizeof(*dt));
>  }
> @@ -585,6 +654,10 @@ static int demux_thread_init(DemuxThreadContext *dt)
>      if (!dt->pkt_demux)
>          return AVERROR(ENOMEM);
>  
> +    dt->pkt_bsf = av_packet_alloc();
> +    if (!dt->pkt_bsf)
> +        return AVERROR(ENOMEM);
> +
>      return 0;
>  }
>  
> @@ -619,10 +692,22 @@ static void *input_thread(void *arg)
>              continue;
>          }
>          if (ret < 0) {
> +            int ret_bsf;
> +
> +            if (ret == AVERROR_EOF)
> +                av_log(d, AV_LOG_VERBOSE, "EOF while reading input\n");
> +            else {
> +                av_log(d, AV_LOG_ERROR, "Error during demuxing: %s\n",
> +                       av_err2str(ret));
> +                ret = exit_on_error ? ret : 0;
> +            }
> +
> +            ret_bsf = demux_bsf_flush(d, &dt);
> +            ret = err_merge(ret == AVERROR_EOF ? 0 : ret, ret_bsf);
> +
>              if (d->loop) {
>                  /* signal looping to our consumers */
>                  dt.pkt_demux->stream_index = -1;
> -
>                  ret = sch_demux_send(d->sch, f->index, dt.pkt_demux, 0);
>                  if (ret >= 0)
>                      ret = seek_to_start(d, (Timestamp){ .ts = dt.pkt_demux->pts,
> @@ -633,14 +718,6 @@ static void *input_thread(void *arg)
>                  /* fallthrough to the error path */
>              }
>  
> -            if (ret == AVERROR_EOF)
> -                av_log(d, AV_LOG_VERBOSE, "EOF while reading input\n");
> -            else {
> -                av_log(d, AV_LOG_ERROR, "Error during demuxing: %s\n",
> -                       av_err2str(ret));
> -                ret = exit_on_error ? ret : 0;
> -            }
> -
>              break;
>          }
>  
> @@ -677,7 +754,7 @@ static void *input_thread(void *arg)
>          if (d->readrate)
>              readrate_sleep(d);
>  
> -        ret = demux_send(d, ds, dt.pkt_demux, send_flags);
> +        ret = demux_send(d, &dt, ds, dt.pkt_demux, send_flags);
>          if (ret < 0)
>              break;
>      }
> @@ -735,9 +812,11 @@ static void demux_final_stats(Demuxer *d)
>  static void ist_free(InputStream **pist)
>  {
>      InputStream *ist = *pist;
> +    DemuxStream *ds;
>  
>      if (!ist)
>          return;
> +    ds = ds_from_ist(ist);
>  
>      dec_free(&ist->decoder);
>  
> @@ -749,6 +828,8 @@ static void ist_free(InputStream **pist)
>      avcodec_free_context(&ist->dec_ctx);
>      avcodec_parameters_free(&ist->par);
>  
> +    av_bsf_free(&ds->bsf);
> +
>      av_freep(pist);
>  }
>  
> @@ -1039,6 +1120,7 @@ static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
>      const char *hwaccel = NULL;
>      char *hwaccel_output_format = NULL;
>      char *codec_tag = NULL;
> +    char *bsfs = NULL;
>      char *next;
>      char *discard_str = NULL;
>      int ret;
> @@ -1258,6 +1340,33 @@ static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
>          return ret;
>      }
>  
> +    MATCH_PER_STREAM_OPT(bitstream_filters, str, bsfs, ic, st);
> +    if (bsfs) {
> +        ret = av_bsf_list_parse_str(bsfs, &ds->bsf);
> +        if (ret < 0) {
> +            av_log(ist, AV_LOG_ERROR,
> +                   "Error parsing bitstream filter sequence '%s': %s\n",
> +                   bsfs, av_err2str(ret));
> +            return ret;
> +        }
> +
> +        ret = avcodec_parameters_copy(ds->bsf->par_in, ist->par);
> +        if (ret < 0)
> +            return ret;
> +        ds->bsf->time_base_in = ist->st->time_base;
> +
> +        ret = av_bsf_init(ds->bsf);
> +        if (ret < 0) {
> +            av_log(ist, AV_LOG_ERROR, "Error initializing bitstream filters: %s\n",
> +                   av_err2str(ret));
> +            return ret;
> +        }
> +
> +        ret = avcodec_parameters_copy(ist->par, ds->bsf->par_out);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
>      ds->codec_desc = avcodec_descriptor_get(ist->par->codec_id);
>  
>      return 0;
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index c189cf373b..76b50c0bad 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -1919,7 +1919,7 @@ const OptionDef options[] = {
>          "0 = use frame rate (video) or sample rate (audio),"
>          "-1 = match source time base", "ratio" },
>  
> -    { "bsf", OPT_TYPE_STRING, OPT_SPEC | OPT_EXPERT | OPT_OUTPUT,
> +    { "bsf", OPT_TYPE_STRING, OPT_SPEC | OPT_EXPERT | OPT_OUTPUT | OPT_INPUT,
>          { .off = OFFSET(bitstream_filters) },
>          "A comma-separated list of bitstream filters", "bitstream_filters", },
>  
> diff --git a/tests/fate/ffmpeg.mak b/tests/fate/ffmpeg.mak
> index 1bfd5c1b31..df955df4d0 100644
> --- a/tests/fate/ffmpeg.mak
> +++ b/tests/fate/ffmpeg.mak
> @@ -256,3 +256,8 @@ FATE_SAMPLES_FFMPEG-$(call FRAMECRC, MPEGVIDEO, MPEG2VIDEO) += fate-ffmpeg-input
>  fate-ffmpeg-error-rate-fail: CMD = ffmpeg -i $(TARGET_SAMPLES)/mkv/h264_tta_undecodable.mkv -c:v copy -f null -; test $$? -eq 69
>  fate-ffmpeg-error-rate-pass: CMD = ffmpeg -i $(TARGET_SAMPLES)/mkv/h264_tta_undecodable.mkv -c:v copy -f null - -max_error_rate 1
>  FATE_SAMPLES_FFMPEG-$(call ENCDEC, PCM_S16LE TTA, NULL MATROSKA) += fate-ffmpeg-error-rate-fail fate-ffmpeg-error-rate-pass
> +
> +# test input -bsf
> +# use -stream_loop, because it tests flushing bsfs
> +fate-ffmpeg-bsf-input: CMD = framecrc -stream_loop 2 -bsf setts=PTS*2 -i $(TARGET_SAMPLES)/hevc/extradata-reload-multi-stsd.mov -c copy
> +FATE_SAMPLES_FFMPEG-$(call FRAMECRC, MOV, , SETTS_BSF) += fate-ffmpeg-bsf-input
> diff --git a/tests/ref/fate/ffmpeg-bsf-input b/tests/ref/fate/ffmpeg-bsf-input
> new file mode 100644
> index 0000000000..67dd57cf6d
> --- /dev/null
> +++ b/tests/ref/fate/ffmpeg-bsf-input
> @@ -0,0 +1,18 @@
> +#extradata 0:      110, 0xb4031479
> +#tb 0: 1/25
> +#media_type 0: video
> +#codec_id 0: hevc
> +#dimensions 0: 128x128
> +#sar 0: 1/1
> +0,          0,          0,        1,     2108, 0x57c38f64
> +0,          2,          2,        1,       31, 0xabe10d25, F=0x0
> +0,          4,          4,        1,     1915, 0xd430347f, S=1,      109
> +0,          6,          6,        1,       35, 0xc4ad0d4c, F=0x0
> +0,          8,          8,        1,     2108, 0x57c38f64, S=1,      110
> +0,         10,         10,        1,       31, 0xabe10d25, F=0x0
> +0,         12,         12,        1,     1915, 0xd430347f, S=1,      109
> +0,         14,         14,        1,       35, 0xc4ad0d4c, F=0x0
> +0,         16,         16,        1,     2108, 0x57c38f64, S=1,      110
> +0,         18,         18,        1,       31, 0xabe10d25, F=0x0
> +0,         20,         20,        1,     1915, 0xd430347f, S=1,      109
> +0,         22,         22,        1,       35, 0xc4ad0d4c, F=0x0

LGTM otherwise (and nice feature!).
Anton Khirnov Jan. 17, 2024, 9:02 a.m. UTC | #2
Quoting Stefano Sabatini (2024-01-06 13:12:19)
> 
> This looks spurious, since this suggests the example is about the
> listing, and it's applying a weird order of example/explanation
> (rather than the opposite).

I see nothing weird about this order, it's the standard way it is done
in most literature I encounter. I find the reverse order you're
suggesting far more weird and unnatural.
Stefano Sabatini Jan. 20, 2024, 11:32 a.m. UTC | #3
On date Wednesday 2024-01-17 10:02:31 +0100, Anton Khirnov wrote:
> Quoting Stefano Sabatini (2024-01-06 13:12:19)
> > 
> > This looks spurious, since this suggests the example is about the
> > listing, and it's applying a weird order of example/explanation
> > (rather than the opposite).

Use the @code{-bsfs} option to get the list of bitstream filters. E.g.
@example
...

The problem here is that "E.g." is placed close to a statement about
the listing, therefore it might sound like the example is about the
listing (which is not).

> I see nothing weird about this order, it's the standard way it is done
> in most literature I encounter. I find the reverse order you're
> suggesting far more weird and unnatural.

When you present an example you usually start with an explanation
(what it does) and then present the command, not the other way around.

Also the following:
--------------------------------------
ffmpeg -bsf:v h264_mp4toannexb -i h264.mp4 -c:v copy -an out.h264
@end example
applies the @code{h264_mp4toannexb} bitstream filter (which converts
MP4-encapsulated H.264 stream to Annex B) to the @emph{input} video stream.

On the other hand,
@example
ffmpeg -i file.mov -an -vn -bsf:s mov2textsub -c:s copy -f rawvideo sub.txt
@end example
applies the @code{mov2textsub} bitstream filter (which extracts text from MOV
subtitles) to the @emph{output} subtitle stream. Note, however, that since both
examples use @code{-c copy}, it matters little whether the filters are applied
on input or output - that would change if transcoding was hapenning.
---------------------------------------

this makes the reader need to correlate the two examples to figure
them out, that's why I reworked the presentation in my suggestion as a
more linear sequence of presentation/command/presentation/command.

In general examples should focus on how a task can be done, not on the
explanation of the command itself.
Anton Khirnov Jan. 21, 2024, 5:43 p.m. UTC | #4
Quoting Stefano Sabatini (2024-01-20 12:32:42)
> On date Wednesday 2024-01-17 10:02:31 +0100, Anton Khirnov wrote:
> > Quoting Stefano Sabatini (2024-01-06 13:12:19)
> > > 
> > > This looks spurious, since this suggests the example is about the
> > > listing, and it's applying a weird order of example/explanation
> > > (rather than the opposite).
> 
> Use the @code{-bsfs} option to get the list of bitstream filters. E.g.
> @example
> ...
> 
> The problem here is that "E.g." is placed close to a statement about
> the listing, therefore it might sound like the example is about the
> listing (which is not).

I moved it to a new paragraph.

> > I see nothing weird about this order, it's the standard way it is done
> > in most literature I encounter. I find the reverse order you're
> > suggesting far more weird and unnatural.
> 
> When you present an example you usually start with an explanation
> (what it does) and then present the command, not the other way around.

I don't, neither does most literature I can recall. Typically you first
present a thing, then explain its structure. Explaning the structure of
something the reader has not seen yet is backwards, unnatural, and hard
to understand.

> 
> Also the following:
> --------------------------------------
> ffmpeg -bsf:v h264_mp4toannexb -i h264.mp4 -c:v copy -an out.h264
> @end example
> applies the @code{h264_mp4toannexb} bitstream filter (which converts
> MP4-encapsulated H.264 stream to Annex B) to the @emph{input} video stream.
> 
> On the other hand,
> @example
> ffmpeg -i file.mov -an -vn -bsf:s mov2textsub -c:s copy -f rawvideo sub.txt
> @end example
> applies the @code{mov2textsub} bitstream filter (which extracts text from MOV
> subtitles) to the @emph{output} subtitle stream. Note, however, that since both
> examples use @code{-c copy}, it matters little whether the filters are applied
> on input or output - that would change if transcoding was hapenning.
> ---------------------------------------
> 
> this makes the reader need to correlate the two examples to figure
> them out, that's why I reworked the presentation in my suggestion as a
> more linear sequence of presentation/command/presentation/command.
> 
> In general examples should focus on how a task can be done, not on the
> explanation of the command itself.

I disagree. Examples should focus on whatever can be usefully explained
with an example.
Stefano Sabatini Jan. 21, 2024, 6:22 p.m. UTC | #5
On date Sunday 2024-01-21 18:43:36 +0100, Anton Khirnov wrote:
> Quoting Stefano Sabatini (2024-01-20 12:32:42)
[...]
> > When you present an example you usually start with an explanation
> > (what it does) and then present the command, not the other way around.
> 
> I don't, neither does most literature I can recall. Typically you first
> present a thing, then explain its structure. Explaning the structure of
> something the reader has not seen yet is backwards, unnatural, and hard
> to understand.

I still don't understand what "literature" you are referring to.

If you see most examples in the FFmpeg docs they are in the form:
@item
This does this and that...:
@example
...
@end example

An explanation is presented *before* introducing the example itself,
in other words plain English before the actual command/code.
Anton Khirnov Jan. 21, 2024, 6:35 p.m. UTC | #6
Quoting Stefano Sabatini (2024-01-21 19:22:35)
> On date Sunday 2024-01-21 18:43:36 +0100, Anton Khirnov wrote:
> > Quoting Stefano Sabatini (2024-01-20 12:32:42)
> [...]
> > > When you present an example you usually start with an explanation
> > > (what it does) and then present the command, not the other way around.
> > 
> > I don't, neither does most literature I can recall. Typically you first
> > present a thing, then explain its structure. Explaning the structure of
> > something the reader has not seen yet is backwards, unnatural, and hard
> > to understand.
> 
> I still don't understand what "literature" you are referring to.

Various manuals and textbooks I've read.

> If you see most examples in the FFmpeg docs they are in the form:

Our documentation is widely considered to be somewhere between atrocious
and unusable (and sometimes actively misleading), so the fact that it
does something in a specific way does not at all mean that it's a good
idea.

I have also personally seen (and fixed) countless instances of
mindlessly perpetuated cargo cults in it.
Stefano Sabatini Jan. 21, 2024, 7:15 p.m. UTC | #7
On date Sunday 2024-01-21 19:35:01 +0100, Anton Khirnov wrote:
> Quoting Stefano Sabatini (2024-01-21 19:22:35)
> > On date Sunday 2024-01-21 18:43:36 +0100, Anton Khirnov wrote:
> > > Quoting Stefano Sabatini (2024-01-20 12:32:42)
> > [...]
> > > > When you present an example you usually start with an explanation
> > > > (what it does) and then present the command, not the other way around.
> > > 
> > > I don't, neither does most literature I can recall. Typically you first
> > > present a thing, then explain its structure. Explaning the structure of
> > > something the reader has not seen yet is backwards, unnatural, and hard
> > > to understand.
> > 
> > I still don't understand what "literature" you are referring to.
> 
> Various manuals and textbooks I've read.
> 
> > If you see most examples in the FFmpeg docs they are in the form:
> 

> Our documentation is widely considered to be somewhere between atrocious
> and unusable

nah, it's not so bad, also this applies to most documentation

Besides FFmpeg is possibly the most sophisticated existing toolkit in
terms of features/configuration, so this is somehow expected (at least
if you expect a tutorial rather than a reference).

> (and sometimes actively misleading), so the fact that it
> does something in a specific way does not at all mean that it's a good
> idea.

So what do you propose instead? The fact that it is not perfect does
not mean that everything is bad.
Anton Khirnov Jan. 22, 2024, 8:57 a.m. UTC | #8
Quoting Stefano Sabatini (2024-01-21 20:15:46)
> On date Sunday 2024-01-21 19:35:01 +0100, Anton Khirnov wrote:
> > Quoting Stefano Sabatini (2024-01-21 19:22:35)
> > > On date Sunday 2024-01-21 18:43:36 +0100, Anton Khirnov wrote:
> > > > Quoting Stefano Sabatini (2024-01-20 12:32:42)
> > > [...]
> > > > > When you present an example you usually start with an explanation
> > > > > (what it does) and then present the command, not the other way around.
> > > > 
> > > > I don't, neither does most literature I can recall. Typically you first
> > > > present a thing, then explain its structure. Explaning the structure of
> > > > something the reader has not seen yet is backwards, unnatural, and hard
> > > > to understand.
> > > 
> > > I still don't understand what "literature" you are referring to.
> > 
> > Various manuals and textbooks I've read.
> > 
> > > If you see most examples in the FFmpeg docs they are in the form:
> > 
> 
> > Our documentation is widely considered to be somewhere between atrocious
> > and unusable
> 
> nah, it's not so bad, also this applies to most documentation
> 
> Besides FFmpeg is possibly the most sophisticated existing toolkit in
> terms of features/configuration, so this is somehow expected (at least
> if you expect a tutorial rather than a reference).

I wouldn't be so sure. E.g. Qt has a bigger and more complex API than
ours, yet its documentation is more complete and coherent.

> > (and sometimes actively misleading), so the fact that it
> > does something in a specific way does not at all mean that it's a good
> > idea.
> 
> So what do you propose instead? The fact that it is not perfect does
> not mean that everything is bad.

I'm not saying everything is bad. I'm saying this specific way of
writing examples is bad and should be changed. Which is what I'm doing
in this patch.
diff mbox series

Patch

diff --git a/Changelog b/Changelog
index 5b2899d05b..f8191d88c7 100644
--- a/Changelog
+++ b/Changelog
@@ -18,6 +18,7 @@  version <next>:
 - lavu/eval: introduce randomi() function in expressions
 - VVC decoder
 - fsync filter
+- ffmpeg CLI -bsf option may now be used for input as well as output
 
 version 6.1:
 - libaribcaption decoder
diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
index d75517b443..59f7badcb6 100644
--- a/doc/ffmpeg.texi
+++ b/doc/ffmpeg.texi
@@ -2093,26 +2093,34 @@  an output mpegts file:
 ffmpeg -i inurl -streamid 0:33 -streamid 1:36 out.ts
 @end example
 
-@item -bsf[:@var{stream_specifier}] @var{bitstream_filters} (@emph{output,per-stream})
-Apply bitstream filters to matching streams.
+@item -bsf[:@var{stream_specifier}] @var{bitstream_filters} (@emph{input/output,per-stream})
+Apply bitstream filters to matching streams. The filters are applied to each
+packet as it is received from the demuxer (when used as an input option) or
+before it is sent to the muxer (when used as an output option).
 
 @var{bitstream_filters} is a comma-separated list of bitstream filter
-specifications. The specified bitstream filters are applied to coded packets in
-the order they are written in. Each bitstream filter specification is of the
-form
+specifications, each of the form
 @example
 @var{filter}[=@var{optname0}=@var{optval0}:@var{optname1}=@var{optval1}:...]
 @end example
 Any of the ',=:' characters that are to be a part of an option value need to be
 escaped with a backslash.
 
-Use the @code{-bsfs} option to get the list of bitstream filters.
+Use the @code{-bsfs} option to get the list of bitstream filters. E.g.
 @example
-ffmpeg -i h264.mp4 -c:v copy -bsf:v h264_mp4toannexb -an out.h264
+ffmpeg -bsf:v h264_mp4toannexb -i h264.mp4 -c:v copy -an out.h264
 @end example
+applies the @code{h264_mp4toannexb} bitstream filter (which converts
+MP4-encapsulated H.264 stream to Annex B) to the @emph{input} video stream.
+
+On the other hand,
 @example
 ffmpeg -i file.mov -an -vn -bsf:s mov2textsub -c:s copy -f rawvideo sub.txt
 @end example
+applies the @code{mov2textsub} bitstream filter (which extracts text from MOV
+subtitles) to the @emph{output} subtitle stream. Note, however, that since both
+examples use @code{-c copy}, it matters little whether the filters are applied
+on input or output - that would change if transcoding was hapenning.
 
 @item -tag[:@var{stream_specifier}] @var{codec_tag} (@emph{input/output,per-stream})
 Force a tag/fourcc for matching streams.
diff --git a/fftools/ffmpeg_demux.c b/fftools/ffmpeg_demux.c
index eae1f0bde5..16d4f67e59 100644
--- a/fftools/ffmpeg_demux.c
+++ b/fftools/ffmpeg_demux.c
@@ -34,6 +34,7 @@ 
 #include "libavutil/time.h"
 #include "libavutil/timestamp.h"
 
+#include "libavcodec/bsf.h"
 #include "libavcodec/packet.h"
 
 #include "libavformat/avformat.h"
@@ -71,6 +72,8 @@  typedef struct DemuxStream {
 
     const AVCodecDescriptor *codec_desc;
 
+    AVBSFContext *bsf;
+
     /* number of packets successfully read for this stream */
     uint64_t nb_packets;
     // combined size of all the packets read
@@ -118,6 +121,8 @@  typedef struct Demuxer {
 typedef struct DemuxThreadContext {
     // packet used for reading from the demuxer
     AVPacket *pkt_demux;
+    // packet for reading from BSFs
+    AVPacket *pkt_bsf;
 } DemuxThreadContext;
 
 static DemuxStream *ds_from_ist(InputStream *ist)
@@ -513,13 +518,17 @@  static int do_send(Demuxer *d, DemuxStream *ds, AVPacket *pkt, unsigned flags,
     return 0;
 }
 
-static int demux_send(Demuxer *d, DemuxStream *ds, AVPacket *pkt, unsigned flags)
+static int demux_send(Demuxer *d, DemuxThreadContext *dt, DemuxStream *ds,
+                      AVPacket *pkt, unsigned flags)
 {
     InputFile  *f = &d->f;
     int ret;
 
+    // pkt can be NULL only when flushing BSFs
+    av_assert0(ds->bsf || pkt);
+
     // send heartbeat for sub2video streams
-    if (d->pkt_heartbeat && pkt->pts != AV_NOPTS_VALUE) {
+    if (d->pkt_heartbeat && pkt && pkt->pts != AV_NOPTS_VALUE) {
         for (int i = 0; i < f->nb_streams; i++) {
             DemuxStream *ds1 = ds_from_ist(f->streams[i]);
 
@@ -537,10 +546,69 @@  static int demux_send(Demuxer *d, DemuxStream *ds, AVPacket *pkt, unsigned flags
         }
     }
 
-    ret = do_send(d, ds, pkt, flags, "demuxed");
-    if (ret < 0)
-        return ret;
+    if (ds->bsf) {
+        if (pkt)
+            av_packet_rescale_ts(pkt, pkt->time_base, ds->bsf->time_base_in);
 
+        ret = av_bsf_send_packet(ds->bsf, pkt);
+        if (ret < 0) {
+            if (pkt)
+                av_packet_unref(pkt);
+            av_log(ds, AV_LOG_ERROR, "Error submitting a packet for filtering: %s\n",
+                   av_err2str(ret));
+            return ret;
+        }
+
+        while (1) {
+            ret = av_bsf_receive_packet(ds->bsf, dt->pkt_bsf);
+            if (ret == AVERROR(EAGAIN))
+                return 0;
+            else if (ret < 0) {
+                if (ret != AVERROR_EOF)
+                    av_log(ds, AV_LOG_ERROR,
+                           "Error applying bitstream filters to a packet: %s\n",
+                           av_err2str(ret));
+                return ret;
+            }
+
+            dt->pkt_bsf->time_base = ds->bsf->time_base_out;
+
+            ret = do_send(d, ds, dt->pkt_bsf, 0, "filtered");
+            if (ret < 0) {
+                av_packet_unref(dt->pkt_bsf);
+                return ret;
+            }
+        }
+    } else {
+        ret = do_send(d, ds, pkt, flags, "demuxed");
+        if (ret < 0)
+            return ret;
+    }
+
+    return 0;
+}
+
+static int demux_bsf_flush(Demuxer *d, DemuxThreadContext *dt)
+{
+    InputFile *f = &d->f;
+    int ret;
+
+    for (unsigned i = 0; i < f->nb_streams; i++) {
+        DemuxStream *ds = ds_from_ist(f->streams[i]);
+
+        if (!ds->bsf)
+            continue;
+
+        ret = demux_send(d, dt, ds, NULL, 0);
+        ret = (ret == AVERROR_EOF) ? 0 : (ret < 0) ? ret : AVERROR_BUG;
+        if (ret < 0) {
+            av_log(ds, AV_LOG_ERROR, "Error flushing BSFs: %s\n",
+                   av_err2str(ret));
+            return ret;
+        }
+
+        av_bsf_flush(ds->bsf);
+    }
 
     return 0;
 }
@@ -573,6 +641,7 @@  static void thread_set_name(InputFile *f)
 static void demux_thread_uninit(DemuxThreadContext *dt)
 {
     av_packet_free(&dt->pkt_demux);
+    av_packet_free(&dt->pkt_bsf);
 
     memset(dt, 0, sizeof(*dt));
 }
@@ -585,6 +654,10 @@  static int demux_thread_init(DemuxThreadContext *dt)
     if (!dt->pkt_demux)
         return AVERROR(ENOMEM);
 
+    dt->pkt_bsf = av_packet_alloc();
+    if (!dt->pkt_bsf)
+        return AVERROR(ENOMEM);
+
     return 0;
 }
 
@@ -619,10 +692,22 @@  static void *input_thread(void *arg)
             continue;
         }
         if (ret < 0) {
+            int ret_bsf;
+
+            if (ret == AVERROR_EOF)
+                av_log(d, AV_LOG_VERBOSE, "EOF while reading input\n");
+            else {
+                av_log(d, AV_LOG_ERROR, "Error during demuxing: %s\n",
+                       av_err2str(ret));
+                ret = exit_on_error ? ret : 0;
+            }
+
+            ret_bsf = demux_bsf_flush(d, &dt);
+            ret = err_merge(ret == AVERROR_EOF ? 0 : ret, ret_bsf);
+
             if (d->loop) {
                 /* signal looping to our consumers */
                 dt.pkt_demux->stream_index = -1;
-
                 ret = sch_demux_send(d->sch, f->index, dt.pkt_demux, 0);
                 if (ret >= 0)
                     ret = seek_to_start(d, (Timestamp){ .ts = dt.pkt_demux->pts,
@@ -633,14 +718,6 @@  static void *input_thread(void *arg)
                 /* fallthrough to the error path */
             }
 
-            if (ret == AVERROR_EOF)
-                av_log(d, AV_LOG_VERBOSE, "EOF while reading input\n");
-            else {
-                av_log(d, AV_LOG_ERROR, "Error during demuxing: %s\n",
-                       av_err2str(ret));
-                ret = exit_on_error ? ret : 0;
-            }
-
             break;
         }
 
@@ -677,7 +754,7 @@  static void *input_thread(void *arg)
         if (d->readrate)
             readrate_sleep(d);
 
-        ret = demux_send(d, ds, dt.pkt_demux, send_flags);
+        ret = demux_send(d, &dt, ds, dt.pkt_demux, send_flags);
         if (ret < 0)
             break;
     }
@@ -735,9 +812,11 @@  static void demux_final_stats(Demuxer *d)
 static void ist_free(InputStream **pist)
 {
     InputStream *ist = *pist;
+    DemuxStream *ds;
 
     if (!ist)
         return;
+    ds = ds_from_ist(ist);
 
     dec_free(&ist->decoder);
 
@@ -749,6 +828,8 @@  static void ist_free(InputStream **pist)
     avcodec_free_context(&ist->dec_ctx);
     avcodec_parameters_free(&ist->par);
 
+    av_bsf_free(&ds->bsf);
+
     av_freep(pist);
 }
 
@@ -1039,6 +1120,7 @@  static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
     const char *hwaccel = NULL;
     char *hwaccel_output_format = NULL;
     char *codec_tag = NULL;
+    char *bsfs = NULL;
     char *next;
     char *discard_str = NULL;
     int ret;
@@ -1258,6 +1340,33 @@  static int ist_add(const OptionsContext *o, Demuxer *d, AVStream *st)
         return ret;
     }
 
+    MATCH_PER_STREAM_OPT(bitstream_filters, str, bsfs, ic, st);
+    if (bsfs) {
+        ret = av_bsf_list_parse_str(bsfs, &ds->bsf);
+        if (ret < 0) {
+            av_log(ist, AV_LOG_ERROR,
+                   "Error parsing bitstream filter sequence '%s': %s\n",
+                   bsfs, av_err2str(ret));
+            return ret;
+        }
+
+        ret = avcodec_parameters_copy(ds->bsf->par_in, ist->par);
+        if (ret < 0)
+            return ret;
+        ds->bsf->time_base_in = ist->st->time_base;
+
+        ret = av_bsf_init(ds->bsf);
+        if (ret < 0) {
+            av_log(ist, AV_LOG_ERROR, "Error initializing bitstream filters: %s\n",
+                   av_err2str(ret));
+            return ret;
+        }
+
+        ret = avcodec_parameters_copy(ist->par, ds->bsf->par_out);
+        if (ret < 0)
+            return ret;
+    }
+
     ds->codec_desc = avcodec_descriptor_get(ist->par->codec_id);
 
     return 0;
diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index c189cf373b..76b50c0bad 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -1919,7 +1919,7 @@  const OptionDef options[] = {
         "0 = use frame rate (video) or sample rate (audio),"
         "-1 = match source time base", "ratio" },
 
-    { "bsf", OPT_TYPE_STRING, OPT_SPEC | OPT_EXPERT | OPT_OUTPUT,
+    { "bsf", OPT_TYPE_STRING, OPT_SPEC | OPT_EXPERT | OPT_OUTPUT | OPT_INPUT,
         { .off = OFFSET(bitstream_filters) },
         "A comma-separated list of bitstream filters", "bitstream_filters", },
 
diff --git a/tests/fate/ffmpeg.mak b/tests/fate/ffmpeg.mak
index 1bfd5c1b31..df955df4d0 100644
--- a/tests/fate/ffmpeg.mak
+++ b/tests/fate/ffmpeg.mak
@@ -256,3 +256,8 @@  FATE_SAMPLES_FFMPEG-$(call FRAMECRC, MPEGVIDEO, MPEG2VIDEO) += fate-ffmpeg-input
 fate-ffmpeg-error-rate-fail: CMD = ffmpeg -i $(TARGET_SAMPLES)/mkv/h264_tta_undecodable.mkv -c:v copy -f null -; test $$? -eq 69
 fate-ffmpeg-error-rate-pass: CMD = ffmpeg -i $(TARGET_SAMPLES)/mkv/h264_tta_undecodable.mkv -c:v copy -f null - -max_error_rate 1
 FATE_SAMPLES_FFMPEG-$(call ENCDEC, PCM_S16LE TTA, NULL MATROSKA) += fate-ffmpeg-error-rate-fail fate-ffmpeg-error-rate-pass
+
+# test input -bsf
+# use -stream_loop, because it tests flushing bsfs
+fate-ffmpeg-bsf-input: CMD = framecrc -stream_loop 2 -bsf setts=PTS*2 -i $(TARGET_SAMPLES)/hevc/extradata-reload-multi-stsd.mov -c copy
+FATE_SAMPLES_FFMPEG-$(call FRAMECRC, MOV, , SETTS_BSF) += fate-ffmpeg-bsf-input
diff --git a/tests/ref/fate/ffmpeg-bsf-input b/tests/ref/fate/ffmpeg-bsf-input
new file mode 100644
index 0000000000..67dd57cf6d
--- /dev/null
+++ b/tests/ref/fate/ffmpeg-bsf-input
@@ -0,0 +1,18 @@ 
+#extradata 0:      110, 0xb4031479
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: hevc
+#dimensions 0: 128x128
+#sar 0: 1/1
+0,          0,          0,        1,     2108, 0x57c38f64
+0,          2,          2,        1,       31, 0xabe10d25, F=0x0
+0,          4,          4,        1,     1915, 0xd430347f, S=1,      109
+0,          6,          6,        1,       35, 0xc4ad0d4c, F=0x0
+0,          8,          8,        1,     2108, 0x57c38f64, S=1,      110
+0,         10,         10,        1,       31, 0xabe10d25, F=0x0
+0,         12,         12,        1,     1915, 0xd430347f, S=1,      109
+0,         14,         14,        1,       35, 0xc4ad0d4c, F=0x0
+0,         16,         16,        1,     2108, 0x57c38f64, S=1,      110
+0,         18,         18,        1,       31, 0xabe10d25, F=0x0
+0,         20,         20,        1,     1915, 0xd430347f, S=1,      109
+0,         22,         22,        1,       35, 0xc4ad0d4c, F=0x0