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

[Andreas Rheinhardt]

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

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?

- Andreas