Jan Ekström:
> On Tue, Jun 21, 2022 at 10:30 AM Andreas Rheinhardt
> <andreas.rheinha...@outlook.com> wrote:
>>
>> Jan Ekström:
>>> On Mon, Jun 20, 2022 at 12:10 PM Andreas Rheinhardt
>>> <andreas.rheinha...@outlook.com> wrote:
>>>>
>>>> Jan Ekström:
>>>>> From: Jan Ekström <jan.ekst...@24i.com>
>>>>>
>>>>> Signed-off-by: Jan Ekström <jan.ekst...@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-requ...@ffmpeg.org with subject "unsubscribe".
