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

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

On Wed, Jun 22, 2022 at 1:05 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Jan Ekström:
> > 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).
> >
>
> Lots of the values in ff_ac3_bitrate_tab have the property that the
> value divided by 2 or 4 is also in ff_ac3_bitrate_tab. This means you
> some AC-3 with bsid == 9 or bsid == 10 might pass through and be muxed,
> yet it is my interpretation of the spec you linked that this is simply
> out-of-spec. So you should not allow it.

If not out-of-spec it is at the very least a case of "not defined by
spec". Is it correct to interpret and then index (which leads to those
values being shown correctly by parsers which actually read/present
the information from the box, but depending on which range of values
are actually utilized by this fork of the format we might have
non-applicable values), or should the bits just be passed which will
be shown as the wrong value but it matches the higher bits?

> The best way for this is to
> simply add bsid to AC3HeaderInfo. After having restricted yourself in
> this way AC3HeaderInfo.bit_rate can be used as-is (if I am not mistaken).

Not knowing the limitations of DNET and how it most likely was not
meant to go into this container mapping, I cannot disagree. My initial
attempt was to just handle this by checking if the resulting bit rate
fit within the requirements of the container mapping. If not, error
out. Thankfully the bsid is already in there:

info->substream[hdr->substreamid].bsid  = hdr->bitstream_id;

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