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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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

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