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-requ...@ffmpeg.org with subject "unsubscribe".
