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

[Anton Khirnov <anton@khirnov.net>]

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.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".