[FFmpeg-devel] [PATCH v2 6/6] avformat/audiointerleave: only keep the retime functionality of the audio interleaver

Andreas Rheinhardt andreas.rheinhardt at gmail.com
Sun Apr 26 03:32:07 EEST 2020


Marton Balint:
> And rename it to retimeinterleave, use the pcm_rechunk bitstream filter for
> rechunking.
> 
> By seperating the two functions we hopefully get cleaner code.
> 
> Signed-off-by: Marton Balint <cus at passwd.hu>
> ---
>  configure                                          |  2 +
>  libavformat/Makefile                               |  4 +-
>  libavformat/gxfenc.c                               | 29 ++++++++----
>  libavformat/mux.c                                  |  1 -
>  libavformat/mxfenc.c                               | 32 ++++++++++----
>  libavformat/retimeinterleave.c                     | 51 ++++++++++++++++++++++
>  .../{audiointerleave.h => retimeinterleave.h}      | 31 ++++++-------
>  libavformat/utils.c                                |  1 -
>  8 files changed, 111 insertions(+), 40 deletions(-)
>  create mode 100644 libavformat/retimeinterleave.c
>  rename libavformat/{audiointerleave.h => retimeinterleave.h} (57%)

Why don't you delete audiointerleave.c? (I would have expected to see
more deletions than insertions.)

> 
> diff --git a/configure b/configure
> index 4f285f0074..f5a84c31bd 100755
> --- a/configure
> +++ b/configure
> @@ -2722,6 +2722,7 @@ fraps_decoder_select="bswapdsp huffman"
>  g2m_decoder_deps="zlib"
>  g2m_decoder_select="blockdsp idctdsp jpegtables"
>  g729_decoder_select="audiodsp"
> +gxf_encoder_select="pcm_rechunk_bsf"
>  h261_decoder_select="mpegvideo"
>  h261_encoder_select="mpegvideoenc"
>  h263_decoder_select="h263_parser h263dsp mpegvideo qpeldsp"
> @@ -2794,6 +2795,7 @@ mv30_decoder_select="aandcttables blockdsp"
>  mvha_decoder_deps="zlib"
>  mvha_decoder_select="llviddsp"
>  mwsc_decoder_deps="zlib"
> +mxf_encoder_select="pcm_rechunk_bsf"
>  mxpeg_decoder_select="mjpeg_decoder"
>  nellymoser_decoder_select="mdct sinewin"
>  nellymoser_encoder_select="audio_frame_queue mdct sinewin"
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index d4bed3c113..56ca55fbd5 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -205,7 +205,7 @@ OBJS-$(CONFIG_GIF_DEMUXER)               += gifdec.o
>  OBJS-$(CONFIG_GSM_DEMUXER)               += gsmdec.o
>  OBJS-$(CONFIG_GSM_MUXER)                 += rawenc.o
>  OBJS-$(CONFIG_GXF_DEMUXER)               += gxf.o
> -OBJS-$(CONFIG_GXF_MUXER)                 += gxfenc.o audiointerleave.o
> +OBJS-$(CONFIG_GXF_MUXER)                 += gxfenc.o retimeinterleave.o
>  OBJS-$(CONFIG_G722_DEMUXER)              += g722.o rawdec.o
>  OBJS-$(CONFIG_G722_MUXER)                += rawenc.o
>  OBJS-$(CONFIG_G723_1_DEMUXER)            += g723_1.o
> @@ -347,7 +347,7 @@ OBJS-$(CONFIG_MUSX_DEMUXER)              += musx.o
>  OBJS-$(CONFIG_MV_DEMUXER)                += mvdec.o
>  OBJS-$(CONFIG_MVI_DEMUXER)               += mvi.o
>  OBJS-$(CONFIG_MXF_DEMUXER)               += mxfdec.o mxf.o
> -OBJS-$(CONFIG_MXF_MUXER)                 += mxfenc.o mxf.o audiointerleave.o avc.o
> +OBJS-$(CONFIG_MXF_MUXER)                 += mxfenc.o mxf.o retimeinterleave.o avc.o
>  OBJS-$(CONFIG_MXG_DEMUXER)               += mxg.o
>  OBJS-$(CONFIG_NC_DEMUXER)                += ncdec.o
>  OBJS-$(CONFIG_NISTSPHERE_DEMUXER)        += nistspheredec.o pcm.o
> diff --git a/libavformat/gxfenc.c b/libavformat/gxfenc.c
> index e7536a6a7e..e95ae99cba 100644
> --- a/libavformat/gxfenc.c
> +++ b/libavformat/gxfenc.c
> @@ -27,7 +27,7 @@
>  #include "avformat.h"
>  #include "internal.h"
>  #include "gxf.h"
> -#include "audiointerleave.h"
> +#include "retimeinterleave.h"
>  
>  #define GXF_AUDIO_PACKET_SIZE 65536
>  
> @@ -44,7 +44,7 @@ typedef struct GXFTimecode{
>  } GXFTimecode;
>  
>  typedef struct GXFStreamContext {
> -    AudioInterleaveContext aic;
> +    RetimeInterleaveContext aic;
>      uint32_t track_type;
>      uint32_t sample_size;
>      uint32_t sample_rate;
> @@ -813,14 +813,12 @@ static int gxf_write_header(AVFormatContext *s)
>                  return -1;
>              }
>          }
> +        ff_retime_interleave_init(&sc->aic, st->time_base);
>          /* FIXME first 10 audio tracks are 0 to 9 next 22 are A to V */
>          sc->media_info = media_info<<8 | ('0'+tracks[media_info]++);
>          sc->order = s->nb_streams - st->index;
>      }
>  
> -    if (ff_audio_interleave_init(s, GXF_samples_per_frame, (AVRational){ 1, 48000 }) < 0)
> -        return -1;
> -
>      if (tcr && vsc)
>          gxf_init_timecode(s, &gxf->tc, tcr->value, vsc->fields);
>  
> @@ -877,8 +875,6 @@ static void gxf_deinit(AVFormatContext *s)
>  {
>      GXFContext *gxf = s->priv_data;
>  
> -    ff_audio_interleave_close(s);
> -
>      av_freep(&gxf->flt_entries);
>      av_freep(&gxf->map_offsets);
>  }
> @@ -1016,8 +1012,22 @@ static int gxf_interleave_packet(AVFormatContext *s, AVPacket *out, AVPacket *pk
>  {
>      if (pkt && s->streams[pkt->stream_index]->codecpar->codec_type == AVMEDIA_TYPE_VIDEO)
>          pkt->duration = 2; // enforce 2 fields
> -    return ff_audio_rechunk_interleave(s, out, pkt, flush,
> -                               ff_interleave_packet_per_dts, gxf_compare_field_nb);
> +    return ff_retime_interleave(s, out, pkt, flush,
> +                                ff_interleave_packet_per_dts, gxf_compare_field_nb);
> +}
> +
> +static int gxf_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt)
> +{
> +    int ret = 1;
> +    AVStream *st = s->streams[pkt->stream_index];
> +
> +    if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) {
> +        char arg[32];
> +        snprintf(arg, sizeof(arg), "n=%d", GXF_samples_per_frame);
> +        ret = ff_stream_add_bitstream_filter(st, "pcm_rechunk", arg);
> +    }
> +
> +    return ret;

I wonder whether these bsfs should not really be inserted in
init()/write_header(). After all, you don't analyze the packets at all.

(What actually happens if a user disables automatic bitstream filtering?
Do the muxers error out or write invalid files? I hope one doesn't run
into an assert.)

>  }
>  
>  AVOutputFormat ff_gxf_muxer = {
> @@ -1031,5 +1041,6 @@ AVOutputFormat ff_gxf_muxer = {
>      .write_packet      = gxf_write_packet,
>      .write_trailer     = gxf_write_trailer,
>      .deinit            = gxf_deinit,
> +    .check_bitstream   = gxf_check_bitstream,
>      .interleave_packet = gxf_interleave_packet,
>  };
> diff --git a/libavformat/mux.c b/libavformat/mux.c
> index c2b6d4461e..b189450dd7 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -37,7 +37,6 @@
>  #include "libavutil/parseutils.h"
>  #include "libavutil/time.h"
>  #include "riff.h"
> -#include "audiointerleave.h"
>  #include "url.h"
>  #include <stdarg.h>
>  #if CONFIG_NETWORK
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index bfce531f54..d381821116 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -52,7 +52,7 @@
>  #include "libavcodec/h264_ps.h"
>  #include "libavcodec/golomb.h"
>  #include "libavcodec/internal.h"
> -#include "audiointerleave.h"
> +#include "retimeinterleave.h"
>  #include "avformat.h"
>  #include "avio_internal.h"
>  #include "internal.h"
> @@ -79,7 +79,7 @@ typedef struct MXFIndexEntry {
>  } MXFIndexEntry;
>  
>  typedef struct MXFStreamContext {
> -    AudioInterleaveContext aic;
> +    RetimeInterleaveContext aic;
>      UID track_essence_element_key;
>      int index;               ///< index in mxf_essence_container_uls table
>      const UID *codec_ul;
> @@ -2593,6 +2593,7 @@ static int mxf_write_header(AVFormatContext *s)
>                  return -1;
>              }
>          }
> +        ff_retime_interleave_init(&sc->aic, av_inv_q(mxf->tc.rate));
>  
>          if (sc->index == -1) {
>              sc->index = mxf_get_essence_container_ul_index(st->codecpar->codec_id);
> @@ -2646,9 +2647,6 @@ static int mxf_write_header(AVFormatContext *s)
>          return AVERROR(ENOMEM);
>      mxf->timecode_track->index = -1;
>  
> -    if (ff_audio_interleave_init(s, 0, av_inv_q(mxf->tc.rate)) < 0)
> -        return -1;
> -
>      return 0;
>  }
>  
> @@ -3010,8 +3008,6 @@ static void mxf_deinit(AVFormatContext *s)
>  {
>      MXFContext *mxf = s->priv_data;
>  
> -    ff_audio_interleave_close(s);
> -
>      av_freep(&mxf->index_entries);
>      av_freep(&mxf->body_partition_offset);
>      if (mxf->timecode_track) {
> @@ -3087,8 +3083,23 @@ static int mxf_compare_timestamps(AVFormatContext *s, const AVPacket *next,
>  
>  static int mxf_interleave(AVFormatContext *s, AVPacket *out, AVPacket *pkt, int flush)
>  {
> -    return ff_audio_rechunk_interleave(s, out, pkt, flush,
> -                               mxf_interleave_get_packet, mxf_compare_timestamps);
> +    return ff_retime_interleave(s, out, pkt, flush,
> +                                mxf_interleave_get_packet, mxf_compare_timestamps);
> +}
> +
> +static int mxf_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt)
> +{
> +    int ret = 1;
> +    AVStream *st = s->streams[pkt->stream_index];
> +    MXFStreamContext *sc = st->priv_data;
> +
> +    if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) {
> +        char arg[32];
> +        snprintf(arg, sizeof(arg), "r=%d/%d", sc->aic.time_base.den, sc->aic.time_base.num);
> +        ret = ff_stream_add_bitstream_filter(st, "pcm_rechunk", arg);
> +    }
> +
> +    return ret;
>  }
>  
>  #define MXF_COMMON_OPTIONS \
> @@ -3171,6 +3182,7 @@ AVOutputFormat ff_mxf_muxer = {
>      .deinit            = mxf_deinit,
>      .flags             = AVFMT_NOTIMESTAMPS,
>      .interleave_packet = mxf_interleave,
> +    .check_bitstream   = mxf_check_bitstream,
>      .priv_class        = &mxf_muxer_class,
>  };
>  
> @@ -3187,6 +3199,7 @@ AVOutputFormat ff_mxf_d10_muxer = {
>      .deinit            = mxf_deinit,
>      .flags             = AVFMT_NOTIMESTAMPS,
>      .interleave_packet = mxf_interleave,
> +    .check_bitstream   = mxf_check_bitstream,
>      .priv_class        = &mxf_d10_muxer_class,
>  };
>  
> @@ -3204,5 +3217,6 @@ AVOutputFormat ff_mxf_opatom_muxer = {
>      .deinit            = mxf_deinit,
>      .flags             = AVFMT_NOTIMESTAMPS,
>      .interleave_packet = mxf_interleave,
> +    .check_bitstream   = mxf_check_bitstream,
>      .priv_class        = &mxf_opatom_muxer_class,
>  };
> diff --git a/libavformat/retimeinterleave.c b/libavformat/retimeinterleave.c
> new file mode 100644
> index 0000000000..9f874e3626
> --- /dev/null
> +++ b/libavformat/retimeinterleave.c
> @@ -0,0 +1,51 @@
> +/*
> + * Retime Interleaving functions
> + *
> + * Copyright (c) 2009 Baptiste Coudurier <baptiste dot coudurier at gmail dot com>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "libavutil/mathematics.h"
> +#include "avformat.h"
> +#include "retimeinterleave.h"
> +#include "internal.h"
> +
> +void ff_retime_interleave_init(RetimeInterleaveContext *aic, AVRational time_base)

aic doesn't fit anymore now that you renamed the struct to
RetimeInterleaveContext.

> +{
> +    aic->time_base = time_base;

Can't one avoid using different timebases here? After all, a muxer is
free to set the streams' timebases as it sees fit; you can thereby
simply demand that the user already delivers the packets in the correct
timebase. But I have to admit not to understand the timestamp logic in
mxfenc.c.
(This made me wonder: If an automatically inserted bitstream filter sets
its output timebase differently from the input timebase, then the
packets will arrive at the muxer with a different timebase than
st->time_base. Whose job would it be to convert to the timebase the
muxer wanted? I'd say the muxer, because the generic code doesn't even
know whether the stream's timebase has intentionally been set by the
muxer or whether the muxer can already handle any timebase. Furthermore,
it is not really important right now as there is no case where this
happens. Maybe we should document the timebase handling of automatically
inserted bsfs a bit so that we don't rely on this undocumented feature
in libavformat?)

> +}
> +
> +int ff_retime_interleave(AVFormatContext *s, AVPacket *out, AVPacket *pkt, int flush,
> +                        int (*get_packet)(AVFormatContext *, AVPacket *, AVPacket *, int),
> +                        int (*compare_ts)(AVFormatContext *, const AVPacket *, const AVPacket *))
> +{
> +    int ret;
> +
> +    if (pkt) {
> +        AVStream *st = s->streams[pkt->stream_index];
> +        RetimeInterleaveContext *aic = st->priv_data;
> +        pkt->duration = av_rescale_q(pkt->duration, st->time_base, aic->time_base);

What actually happens if a user uses the mxf muxer with av_write_frame()
and not av_interleaved_write_frame()? This code would not get executed
at all, so that the timing would be completely wrong (unless both
timebases would already coincide), even if the user supplied the packets
in the correct order.

> +        // rewrite pts and dts to be decoded time line position
> +        pkt->pts = pkt->dts = aic->dts;
> +        aic->dts += pkt->duration;
> +        if ((ret = ff_interleave_add_packet(s, pkt, compare_ts)) < 0)
> +            return ret;
> +    }
> +
> +    return get_packet(s, out, NULL, flush);
> +}
> diff --git a/libavformat/audiointerleave.h b/libavformat/retimeinterleave.h
> similarity index 57%
> rename from libavformat/audiointerleave.h
> rename to libavformat/retimeinterleave.h
> index 0933310f4c..de0a7442b0 100644
> --- a/libavformat/audiointerleave.h
> +++ b/libavformat/retimeinterleave.h
> @@ -20,36 +20,31 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   */
>  
> -#ifndef AVFORMAT_AUDIOINTERLEAVE_H
> -#define AVFORMAT_AUDIOINTERLEAVE_H
> +#ifndef AVFORMAT_RETIMEINTERLEAVE_H
> +#define AVFORMAT_RETIMEINTERLEAVE_H
>  
> -#include "libavutil/fifo.h"
>  #include "avformat.h"
>  
> -typedef struct AudioInterleaveContext {
> -    AVFifoBuffer *fifo;
> -    unsigned fifo_size;           ///< size of currently allocated FIFO
> -    int64_t n;                    ///< number of generated packets
> -    int64_t nb_samples;           ///< number of generated samples
> +typedef struct RetimeInterleaveContext {
>      uint64_t dts;                 ///< current dts
> -    int sample_size;              ///< size of one sample all channels included
> -    int samples_per_frame;        ///< samples per frame if fixed, 0 otherwise
> -    AVRational time_base;         ///< time base of output audio packets
> -} AudioInterleaveContext;
> +    AVRational time_base;         ///< time base of output packets
> +} RetimeInterleaveContext;
>  
> -int ff_audio_interleave_init(AVFormatContext *s, const int samples_per_frame, AVRational time_base);
> -void ff_audio_interleave_close(AVFormatContext *s);
> +/**
> + * Init the retime interleave context
> + */
> +void ff_retime_interleave_init(RetimeInterleaveContext *aic, AVRational time_base);
>  
>  /**
> - * Rechunk audio PCM packets per AudioInterleaveContext->samples_per_frame
> - * and interleave them correctly.
> - * The first element of AVStream->priv_data must be AudioInterleaveContext
> + * Retime packets per RetimeInterleaveContext->time_base and interleave them
> + * correctly.
> + * The first element of AVStream->priv_data must be RetimeInterleaveContext
>   * when using this function.
>   *
>   * @param get_packet function will output a packet when streams are correctly interleaved.
>   * @param compare_ts function will compare AVPackets and decide interleaving order.
>   */
> -int ff_audio_rechunk_interleave(AVFormatContext *s, AVPacket *out, AVPacket *pkt, int flush,
> +int ff_retime_interleave(AVFormatContext *s, AVPacket *out, AVPacket *pkt, int flush,
>                          int (*get_packet)(AVFormatContext *, AVPacket *, AVPacket *, int),
>                          int (*compare_ts)(AVFormatContext *, const AVPacket *, const AVPacket *));
>  
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index eff73252ec..25ac5f9446 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -41,7 +41,6 @@
>  #include "libavcodec/internal.h"
>  #include "libavcodec/raw.h"
>  
> -#include "audiointerleave.h"
>  #include "avformat.h"
>  #include "avio_internal.h"
>  #include "id3v2.h"
> 



More information about the ffmpeg-devel mailing list