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

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

Jan Ekström:
> On Mon, Jun 27, 2022 at 10:12 AM Jan Ekström <jeebjp@gmail.com> wrote:
>>
>> On Thu, Jun 23, 2022 at 1:11 PM Jan Ekström <jeebjp@gmail.com> wrote:
>>>
>>> On Thu, Jun 23, 2022 at 11:57 AM 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>
>>>>> ---
>>>>>  libavcodec/Makefile           |  1 +
>>>>>  libavformat/Makefile          |  1 +
>>>>>  libavformat/ac3_bitrate_tab.c | 22 ++++++++++++++
>>>>>  libavformat/movenc.c          | 55 +++++++++++++++++------------------
>>>>>  4 files changed, 51 insertions(+), 28 deletions(-)
>>>>>  create mode 100644 libavformat/ac3_bitrate_tab.c
>>>>>
>>>>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>>>>> index 10697d31f7..7e0d278e3d 100644
>>>>> --- a/libavcodec/Makefile
>>>>> +++ b/libavcodec/Makefile
>>>>> @@ -1018,6 +1018,7 @@ STLIBOBJS-$(CONFIG_FLV_MUXER)          += mpeg4audio_sample_rates.o
>>>>>  STLIBOBJS-$(CONFIG_HLS_DEMUXER)        += ac3_channel_layout_tab.o
>>>>>  STLIBOBJS-$(CONFIG_MATROSKA_DEMUXER)   += mpeg4audio_sample_rates.o
>>>>>  STLIBOBJS-$(CONFIG_MOV_DEMUXER)        += ac3_channel_layout_tab.o
>>>>> +STLIBOBJS-$(CONFIG_MOV_MUXER)          += ac3_bitrate_tab.o
>>>>>  STLIBOBJS-$(CONFIG_MXF_MUXER)          += golomb.o
>>>>>  STLIBOBJS-$(CONFIG_MP3_MUXER)          += mpegaudiotabs.o
>>>>>  STLIBOBJS-$(CONFIG_NUT_MUXER)          += mpegaudiotabs.o
>>>>> 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
>>>>> 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;
>>>>> +            break;
>>>>> +        }
>>>>> +    }
>>>>
>>>> Why don't you just export the bit rate code instead of rederiving it?
>>>> This should be easy now that you intend to disallow AC-3 frames with
>>>> bsid > 8.
>>>>
>>>
>>> Other reasons for why I originally switched to this method are:
>>>
>>> 1. The first comment I got on IRC from Anton when requesting for
>>> comments on the set was "why don't you make it (the header parsing)
>>> properly public?". My intent is to fix a bug in writing the AC-3 box
>>> with inputs where the read data does not start with an AC-3 start code
>>> (such as MPEG-TS streams that for some reason decide to mux AC-3 in a
>>> manner where PES packets do not start with the start of a packet), not
>>> make such decisions/changes.
>>> 2. The value itself is of limited value to other uses of header
>>> parsing. E-AC-3 for example just utilizes the interpreted value.
>>> 3. No requirement to think about additional library mismatches since
>>> both libraries will have their own table in these versions. Not like
>>> we support such mismatches, but not coming up with new spots where it
>>> is possible did seem favorable.
>>>
>>> In other words, not extending the existing avpriv parsing seemed like
>>> a Good Idea if you attempt to optimize for "less possibilities for
>>> ad-hoc requests", and "possibly safer". My plan was to get initial fix
>>> on this side done and merged, and then move onto improving the parser
>>> framework to flag packets that clearly are not proper in some manner -
>>> as currently it seems like the API user has no way of knowing whether
>>> the buffer they received from a parser is an actual usable/"valid
>>> looking" packet or not, as apparently everything is supposed to be
>>> returned that the API user pushes in, just split into blocks.
>>>
>>> Anyways, if you clearly prefer the extension of the avpriv stuff, then
>>> please check if the original v1 plus the bsid > 8 limiting commit from
>>> v4 are acceptable to you. If yes, I can then apply that mix - for me
>>> at this point the main thing is to be able to move from getting this
>>> fix on the muxer side upstreamed, and then to the next thing.
>>
>> To make it more concrete, this is how it would look.
>>
>> I rebased v1 of this patch set and cherry-picked the bsid limiting
>> patch on top: https://github.com/jeeb/ffmpeg/commits/movenc_use_ac3_parser_for_ac3_v5
>>
>> Is this what you would consider acceptable? Of course the avpriv
>> extension commit would get a minor version bump added to it.
>>
> 
> A friendly ping. I would like to know whether this version (v4) is
> good enough, or if I should post the branch I linked as v5 (which goes
> back to v1, plus the bsid limitation) to allow this patch set to
> proceed.
> 

I prefer v5, but you obviously prefer v4. But I don't want to force my
preferences upon you, so I won't block you from applying v4.

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