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

["Jan Ekström" <jeebjp@gmail.com>]

On Thu, Jun 23, 2022 at 11:57 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Jan Ekström:
> > From: Jan Ekström <jan.ekstrom@24i.com>
> >
> > Signed-off-by: Jan Ekström <jan.ekstrom@24i.com>
> > ---
> >  libavcodec/Makefile           |  1 +
> >  libavformat/Makefile          |  1 +
> >  libavformat/ac3_bitrate_tab.c | 22 ++++++++++++++
> >  libavformat/movenc.c          | 55 +++++++++++++++++------------------
> >  4 files changed, 51 insertions(+), 28 deletions(-)
> >  create mode 100644 libavformat/ac3_bitrate_tab.c
> >
> > diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> > index 10697d31f7..7e0d278e3d 100644
> > --- a/libavcodec/Makefile
> > +++ b/libavcodec/Makefile
> > @@ -1018,6 +1018,7 @@ STLIBOBJS-$(CONFIG_FLV_MUXER)          += mpeg4audio_sample_rates.o
> >  STLIBOBJS-$(CONFIG_HLS_DEMUXER)        += ac3_channel_layout_tab.o
> >  STLIBOBJS-$(CONFIG_MATROSKA_DEMUXER)   += mpeg4audio_sample_rates.o
> >  STLIBOBJS-$(CONFIG_MOV_DEMUXER)        += ac3_channel_layout_tab.o
> > +STLIBOBJS-$(CONFIG_MOV_MUXER)          += ac3_bitrate_tab.o
> >  STLIBOBJS-$(CONFIG_MXF_MUXER)          += golomb.o
> >  STLIBOBJS-$(CONFIG_MP3_MUXER)          += mpegaudiotabs.o
> >  STLIBOBJS-$(CONFIG_NUT_MUXER)          += mpegaudiotabs.o
> > 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
> > 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;
> > +            break;
> > +        }
> > +    }
>
> Why don't you just export the bit rate code instead of rederiving it?
> This should be easy now that you intend to disallow AC-3 frames with
> bsid > 8.
>

Other reasons for why I originally switched to this method are:

1. The first comment I got on IRC from Anton when requesting for
comments on the set was "why don't you make it (the header parsing)
properly public?". My intent is to fix a bug in writing the AC-3 box
with inputs where the read data does not start with an AC-3 start code
(such as MPEG-TS streams that for some reason decide to mux AC-3 in a
manner where PES packets do not start with the start of a packet), not
make such decisions/changes.
2. The value itself is of limited value to other uses of header
parsing. E-AC-3 for example just utilizes the interpreted value.
3. No requirement to think about additional library mismatches since
both libraries will have their own table in these versions. Not like
we support such mismatches, but not coming up with new spots where it
is possible did seem favorable.

In other words, not extending the existing avpriv parsing seemed like
a Good Idea if you attempt to optimize for "less possibilities for
ad-hoc requests", and "possibly safer". My plan was to get initial fix
on this side done and merged, and then move onto improving the parser
framework to flag packets that clearly are not proper in some manner -
as currently it seems like the API user has no way of knowing whether
the buffer they received from a parser is an actual usable/"valid
looking" packet or not, as apparently everything is supposed to be
returned that the API user pushes in, just split into blocks.

Anyways, if you clearly prefer the extension of the avpriv stuff, then
please check if the original v1 plus the bsid > 8 limiting commit from
v4 are acceptable to you. If yes, I can then apply that mix - for me
at this point the main thing is to be able to move from getting this
fix on the muxer side upstreamed, and then to the next thing.

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