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

Marton Balint cus at passwd.hu
Tue Apr 28 00:35:29 EEST 2020



On Sun, 26 Apr 2020, Andreas Rheinhardt wrote:

> 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.)

That was my intention, but it creeped back during a rebase. Will delete 
it.

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

Good idea. Will change it to do exactly this.

>
> (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.)

This is a good point as well, I have reworked the patch which adds N:M 
bitstream filtering to always apply bitstream filters if they are already 
added. So automatic bitstream filtering flag will only affect 
->check_bitstream() calls.

In general however, if the user disables automatic bitstream filtering 
then usually broken files will be generated...

>
>>  }
>>
>>  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.

Probably this can be further simplified, but for now I only factorized out 
the audio rechunking part. I'd rather do further simplifications in 
subsequent patches.

> (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?)

Actually my patch which adds N:M bitstream filtering also does a timebase 
conversion to the packets received from the bsfs before passing them onto 
the muxer. So the muxer always receives packets in its proper timebase.

>
>> +}
>> +
>> +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.

The mxf muxer only uses the timestamps for interleaving packets in a way 
which is suitable for the mxf muxer, so wrong timestamps do not 
really matter as long as the order of packets is correct.

>
>> +        // 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"
>>

Regards,
Marton


More information about the ffmpeg-devel mailing list