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

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

On Tue, Jun 21, 2022 at 10:30 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Jan Ekström:
> > On Mon, Jun 20, 2022 at 12:10 PM 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>
> >>> ---
> >>>  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.
> >
>
> I have to admit not to get this point at all. Let's work through an
> example: You have bsid 10 and you use ff_ac3_bitrate_tab[17] (i.e. 576).
> AC3HeaderInfo.bit_rate will then contain 144000 (i.e. the value is
> already divided by four). Your data_rate will be set to 144 based upon
> this (if there is no higher datarate already encountered). But there is
> no entry for 144 in ff_ac3_bitrate_tab, so that one will enter the "No
> valid AC3 bit rate code for data rate" branch. Is this intended?
>

In short, yes. Relevant specification for AC-3 in MP4 is
https://www.etsi.org/deliver/etsi_ts/102300_102399/102366/01.04.01_60/ts_102366v010401p.pdf
, Annex F. bit_rate_code is defined there, which defines a set of
values based on the effective bit rate (albeit noted as derived from
(the topmost bits of) "frmsizcod"). The specification limits itself to
to bsid N..8, so technically the RealAudio bsid 9, 10 are out of
scope.

At the end of the day, it's a marking of the bit rate, and if you
cannot convey the actual bit rate by this table, then I think it's
valid to fail. With E-AC-3 the MP4 structure has the data_rate itself,
and not some index value :) . Of course the alternative course of
action is that you should just stick the original bits there, even if
then you would be advertising an "incorrect" bit rate on the container
level.

At the end of the day, I'm fine with either the extension of the
parser struct or the exposure of the table. It just felt like it's
alright to map the value according to the list, as there are cases
where the bits are not interpreted as-is by the parser (and it's
better to fail in the muxer than in the parser if you want to find the
index for the newly interpreted bit rate - which does not exist for
many bit shifted values).

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