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 1:11 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> 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.

To make it more concrete, this is how it would look.

I rebased v1 of this patch set and cherry-picked the bsid limiting
patch on top: https://github.com/jeeb/ffmpeg/commits/movenc_use_ac3_parser_for_ac3_v5

Is this what you would consider acceptable? Of course the avpriv
extension commit would get a minor version bump added to it.

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