[FFmpeg-devel] [PATCH 5/6] avformat/movenc: utilize existing AC-3 parsing workflow for AC-3

[Jan Ekström]

On Mon, Jun 20, 2022 at 12:10 PM Andreas Rheinhardt
<andreas.rheinhardt at outlook.com> wrote:
>
> Jan Ekström:
> > From: Jan Ekström <jan.ekstrom at 24i.com>
> >
> > Signed-off-by: Jan Ekström <jan.ekstrom at 24i.com>
> > ---
> >  libavformat/Makefile          |  1 +
> >  libavformat/ac3_bitrate_tab.c | 22 ++++++++++++++
> >  libavformat/movenc.c          | 55 +++++++++++++++++------------------
> >  3 files changed, 50 insertions(+), 28 deletions(-)
> >  create mode 100644 libavformat/ac3_bitrate_tab.c
> >
> > diff --git a/libavformat/Makefile b/libavformat/Makefile
> > index 1416bf31bd..c71de95b2f 100644
> > --- a/libavformat/Makefile
> > +++ b/libavformat/Makefile
> > @@ -699,6 +699,7 @@ SHLIBOBJS-$(CONFIG_FLV_MUXER)            += mpeg4audio_sample_rates.o
> >  SHLIBOBJS-$(CONFIG_HLS_DEMUXER)          += ac3_channel_layout_tab.o
> >  SHLIBOBJS-$(CONFIG_MATROSKA_DEMUXER)     += mpeg4audio_sample_rates.o
> >  SHLIBOBJS-$(CONFIG_MOV_DEMUXER)          += ac3_channel_layout_tab.o
> > +SHLIBOBJS-$(CONFIG_MOV_MUXER)            += ac3_bitrate_tab.o
> >  SHLIBOBJS-$(CONFIG_MP3_MUXER)            += mpegaudiotabs.o
> >  SHLIBOBJS-$(CONFIG_MXF_MUXER)            += golomb_tab.o
> >  SHLIBOBJS-$(CONFIG_NUT_MUXER)            += mpegaudiotabs.o
>
> The above line only takes care of duplicating ac3_bitrate_tab.o into
> lavf for shared builds; yet there needs to be a corresponding STLIBOBJS
> entry in lavc/Makefile so that lavc/ac3_bitrate_tab.o is compiled in
> case the mov muxer is enabled in a static build.
> It would work either way in this case because #2 made the mov muxer
> depend upon the ac3 parser which makes lavc/ac3_bitrate_tab.o be included.
>

OK, then I clearly missed something since I thought I was following
how you had done things earlier for the other table. Will look into
this tomorrow morning.

> > diff --git a/libavformat/ac3_bitrate_tab.c b/libavformat/ac3_bitrate_tab.c
> > new file mode 100644
> > index 0000000000..57b6181511
> > --- /dev/null
> > +++ b/libavformat/ac3_bitrate_tab.c
> > @@ -0,0 +1,22 @@
> > +/*
> > + * AC-3 bit rate table
> > + * copyright (c) 2001 Fabrice Bellard
> > + *
> > + * 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 "libavcodec/ac3_bitrate_tab.h"
> > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > index 103f958d75..a071f1cdd5 100644
> > --- a/libavformat/movenc.c
> > +++ b/libavformat/movenc.c
> > @@ -36,6 +36,7 @@
> >  #include "av1.h"
> >  #include "avc.h"
> >  #include "libavcodec/ac3_parser_internal.h"
> > +#include "libavcodec/ac3tab.h"
> >  #include "libavcodec/dnxhddata.h"
> >  #include "libavcodec/flac.h"
> >  #include "libavcodec/get_bits.h"
> > @@ -362,44 +363,42 @@ struct eac3_info {
> >
> >  static int mov_write_ac3_tag(AVFormatContext *s, AVIOContext *pb, MOVTrack *track)
> >  {
> > -    GetBitContext gbc;
> > +    struct eac3_info *info = track->eac3_priv;
> > +    int8_t ac3_bit_rate_code = -1;
> >      PutBitContext pbc;
> >      uint8_t buf[3];
> > -    int fscod, bsid, bsmod, acmod, lfeon, frmsizecod;
> >
> > -    if (track->vos_len < 7) {
> > +    if (!info || !info->ec3_done) {
> >          av_log(s, AV_LOG_ERROR,
> >                 "Cannot write moov atom before AC3 packets."
> >                 " Set the delay_moov flag to fix this.\n");
> >          return AVERROR(EINVAL);
> >      }
> >
> > -    avio_wb32(pb, 11);
> > -    ffio_wfourcc(pb, "dac3");
> > +    for (unsigned int i = 0; i < FF_ARRAY_ELEMS(ff_ac3_bitrate_tab); i++) {
> > +        if (info->data_rate == ff_ac3_bitrate_tab[i]) {
> > +            ac3_bit_rate_code = i;
>
> Anyway, I see that you use ff_ac3_bitrate_tab in lavf instead of adding
> the frame_size_code to AC3HeaderInfo. Did it turn out to be easier this
> way or what is the reason for this?

I did partially go through the reasoning in the cover letter, but
here's a short rewording:

1. This way doesn't extend the semi-public avpriv.
2. While reviewing I noticed that bsid 9, 10 streams (apparently some
RealAudio fork of AC-3) actually have the sample/bit rate values bit
shifted (divided by either 2 or 4). The table in the ETSI
specification is based on the effective bit rate. While it is highly
unlikely that someone would put that type of AC-3 into MP4, In that
case I'd then have to re-index the value based on the effective bit
rate instead of just taking the value from the bit stream in the
parser. So as the effective bit rate is already available through the
header parser, it just seems simpler to have the re-indexing happen on
the using side in this case.

Jan