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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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
_______________________________________________
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".