[FFmpeg-devel,16/24] ffmpeg: access output file chapters through a wrapper

Avoid accessing the muxer context directly, as this will become
forbidden in future commits.
---
 fftools/ffmpeg.c     | 15 +++++++++------
 fftools/ffmpeg.h     |  2 ++
 fftools/ffmpeg_mux.c |  7 +++++++
 3 files changed, 18 insertions(+), 6 deletions(-)

Anton Khirnov:
> Avoid accessing the muxer context directly, as this will become
> forbidden in future commits.
> ---
>  fftools/ffmpeg.c     | 15 +++++++++------
>  fftools/ffmpeg.h     |  2 ++
>  fftools/ffmpeg_mux.c |  7 +++++++
>  3 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 902f190191..d69e4119ef 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2909,12 +2909,15 @@ static void parse_forced_key_frames(char *kf, OutputStream *ost,
>              *next++ = 0;
>  
>          if (!memcmp(p, "chapters", 8)) {
> -
> -            AVFormatContext *avf = output_files[ost->file_index]->ctx;
> +            OutputFile *of = output_files[ost->file_index];
> +            AVChapter * const *ch;
> +            unsigned int    nb_ch;
>              int j;
>  
> -            if (avf->nb_chapters > INT_MAX - size ||
> -                !(pts = av_realloc_f(pts, size += avf->nb_chapters - 1,
> +            ch = of_get_chapters(of, &nb_ch);
> +
> +            if (nb_ch > INT_MAX - size ||
> +                !(pts = av_realloc_f(pts, size += nb_ch - 1,
>                                       sizeof(*pts)))) {
>                  av_log(NULL, AV_LOG_FATAL,
>                         "Could not allocate forced key frames array.\n");
> @@ -2923,8 +2926,8 @@ static void parse_forced_key_frames(char *kf, OutputStream *ost,
>              t = p[8] ? parse_time_or_die("force_key_frames", p + 8, 1) : 0;
>              t = av_rescale_q(t, AV_TIME_BASE_Q, avctx->time_base);
>  
> -            for (j = 0; j < avf->nb_chapters; j++) {
> -                AVChapter *c = avf->chapters[j];
> +            for (j = 0; j < nb_ch; j++) {
> +                const AVChapter *c = ch[j];
>                  av_assert1(index < size);
>                  pts[index++] = av_rescale_q(c->start, c->time_base,
>                                              avctx->time_base) + t;
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index 78c2295c8e..8119282aed 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -697,5 +697,7 @@ void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
>                       int unqueue);
>  int of_finished(OutputFile *of);
>  int64_t of_bytes_written(OutputFile *of);
> +AVChapter * const *
> +of_get_chapters(OutputFile *of, unsigned int *nb_chapters);
>  
>  #endif /* FFTOOLS_FFMPEG_H */
> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> index 3ee0fc0667..6c9f10db9c 100644
> --- a/fftools/ffmpeg_mux.c
> +++ b/fftools/ffmpeg_mux.c
> @@ -390,3 +390,10 @@ int64_t of_bytes_written(OutputFile *of)
>      return of->mux->final_filesize ? of->mux->final_filesize :
>              pb ? pb->bytes_written : -1;
>  }
> +
> +AVChapter * const *
> +of_get_chapters(OutputFile *of, unsigned int *nb_chapters)
> +{
> +    *nb_chapters = of->ctx->nb_chapters;
> +    return of->ctx->chapters;
> +}
> 

I don't see any benefit of factoring muxing out of OutputFile at all; to
the contrary, it adds another layer of indirection, of allocations for
your opaque structs, of prohibitions and of unnatural getters like this
one here. If your patchset were already applied and someone posted the
reverse of patch #15, I'd LGTM it, because it makes checking for the
bitexact flag local to the place where it is used and thereby avoids
storing these variables in a context for only one usage.
I also think it is more natural to store each OutputStream's queue in
the OutputStream instead of adding a new array to your struct Muxer.

- Andreas

Quoting Andreas Rheinhardt (2021-12-17 00:08:07)
> Anton Khirnov:
> > Avoid accessing the muxer context directly, as this will become
> > forbidden in future commits.
> > ---
> >  fftools/ffmpeg.c     | 15 +++++++++------
> >  fftools/ffmpeg.h     |  2 ++
> >  fftools/ffmpeg_mux.c |  7 +++++++
> >  3 files changed, 18 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 902f190191..d69e4119ef 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -2909,12 +2909,15 @@ static void parse_forced_key_frames(char *kf, OutputStream *ost,
> >              *next++ = 0;
> >  
> >          if (!memcmp(p, "chapters", 8)) {
> > -
> > -            AVFormatContext *avf = output_files[ost->file_index]->ctx;
> > +            OutputFile *of = output_files[ost->file_index];
> > +            AVChapter * const *ch;
> > +            unsigned int    nb_ch;
> >              int j;
> >  
> > -            if (avf->nb_chapters > INT_MAX - size ||
> > -                !(pts = av_realloc_f(pts, size += avf->nb_chapters - 1,
> > +            ch = of_get_chapters(of, &nb_ch);
> > +
> > +            if (nb_ch > INT_MAX - size ||
> > +                !(pts = av_realloc_f(pts, size += nb_ch - 1,
> >                                       sizeof(*pts)))) {
> >                  av_log(NULL, AV_LOG_FATAL,
> >                         "Could not allocate forced key frames array.\n");
> > @@ -2923,8 +2926,8 @@ static void parse_forced_key_frames(char *kf, OutputStream *ost,
> >              t = p[8] ? parse_time_or_die("force_key_frames", p + 8, 1) : 0;
> >              t = av_rescale_q(t, AV_TIME_BASE_Q, avctx->time_base);
> >  
> > -            for (j = 0; j < avf->nb_chapters; j++) {
> > -                AVChapter *c = avf->chapters[j];
> > +            for (j = 0; j < nb_ch; j++) {
> > +                const AVChapter *c = ch[j];
> >                  av_assert1(index < size);
> >                  pts[index++] = av_rescale_q(c->start, c->time_base,
> >                                              avctx->time_base) + t;
> > diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> > index 78c2295c8e..8119282aed 100644
> > --- a/fftools/ffmpeg.h
> > +++ b/fftools/ffmpeg.h
> > @@ -697,5 +697,7 @@ void of_write_packet(OutputFile *of, AVPacket *pkt, OutputStream *ost,
> >                       int unqueue);
> >  int of_finished(OutputFile *of);
> >  int64_t of_bytes_written(OutputFile *of);
> > +AVChapter * const *
> > +of_get_chapters(OutputFile *of, unsigned int *nb_chapters);
> >  
> >  #endif /* FFTOOLS_FFMPEG_H */
> > diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c
> > index 3ee0fc0667..6c9f10db9c 100644
> > --- a/fftools/ffmpeg_mux.c
> > +++ b/fftools/ffmpeg_mux.c
> > @@ -390,3 +390,10 @@ int64_t of_bytes_written(OutputFile *of)
> >      return of->mux->final_filesize ? of->mux->final_filesize :
> >              pb ? pb->bytes_written : -1;
> >  }
> > +
> > +AVChapter * const *
> > +of_get_chapters(OutputFile *of, unsigned int *nb_chapters)
> > +{
> > +    *nb_chapters = of->ctx->nb_chapters;
> > +    return of->ctx->chapters;
> > +}
> > 
> 
> I don't see any benefit of factoring muxing out of OutputFile at all; to
> the contrary, it adds another layer of indirection, of allocations for
> your opaque structs, of prohibitions and of unnatural getters like this
> one here.

The benefit is in making it clear which objects can be accessed by what
code. ffmpeg.c currently has a big problem with lack of structure -
random bits of code change random bits of state without much coherence
to it. As a result it is very hard to reason about the code or add new
features cleanly (or even at all, without breaking 10 random unrelated
use cases). In that light I see the new indirections and prohibitions as
an improvement.

The ultimate goal I'm aiming at here is splitting each component
(demuxer, decoder, filtergraph, encoder, muxer) into a separate object
with clearly defined interfaces. This will allow implementing certain
features people want, such as
- multithreading invidividual components
- sending a single encoder's output to multiple muxers
- looping an encoder back to a decoder

I agree that this specific getter is not very pretty (better suggestions
are welcome). But it's hard enough to move forward here at all, if I had
to worry about every commit smelling extra sweet I would never get
anywhere at all.

> If your patchset were already applied and someone posted the reverse
> of patch #15, I'd LGTM it, because it makes checking for the bitexact
> flag local to the place where it is used and thereby avoids storing
> these variables in a context for only one usage.

One reason why the bitexact refactor is an improvement on its own
(disregarding the above considerations), is that currently
set_encoder_id() needs to know how exactly the bitexact flags can be set
(either through -f[f]lags bitexact or -bitexact -- that's why it
initializes codec_flags to ost->enc_ctx->flags). With my patch, that
logic is concentrated into options parsing, which is where it belongs.

> I also think it is more natural to store each OutputStream's queue in
> the OutputStream instead of adding a new array to your struct Muxer.

The reasons for why it's done this way are in the commit message.