| Message ID | 20220623072408.38977-6-jeebjp@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | avformat/movenc: normalize on AC-3 parser usage | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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. > > - init_get_bits(&gbc, track->vos_data + 4, (track->vos_len - 4) * 8); > - fscod = get_bits(&gbc, 2); > - frmsizecod = get_bits(&gbc, 6); > - bsid = get_bits(&gbc, 5); > - bsmod = get_bits(&gbc, 3); > - acmod = get_bits(&gbc, 3); > - if (acmod == 2) { > - skip_bits(&gbc, 2); // dsurmod > - } else { > - if ((acmod & 1) && acmod != 1) > - skip_bits(&gbc, 2); // cmixlev > - if (acmod & 4) > - skip_bits(&gbc, 2); // surmixlev > + if (ac3_bit_rate_code < 0) { > + av_log(s, AV_LOG_ERROR, > + "No valid AC3 bit rate code for data rate of %d!\n", > + info->data_rate); > + return AVERROR(EINVAL); > } > - lfeon = get_bits1(&gbc); > + > + avio_wb32(pb, 11); > + ffio_wfourcc(pb, "dac3"); > > init_put_bits(&pbc, buf, sizeof(buf)); > - put_bits(&pbc, 2, fscod); > - put_bits(&pbc, 5, bsid); > - put_bits(&pbc, 3, bsmod); > - put_bits(&pbc, 3, acmod); > - put_bits(&pbc, 1, lfeon); > - put_bits(&pbc, 5, frmsizecod >> 1); // bit_rate_code > + put_bits(&pbc, 2, info->substream[0].fscod); > + put_bits(&pbc, 5, info->substream[0].bsid); > + put_bits(&pbc, 3, info->substream[0].bsmod); > + put_bits(&pbc, 3, info->substream[0].acmod); > + put_bits(&pbc, 1, info->substream[0].lfeon); > + put_bits(&pbc, 5, ac3_bit_rate_code); // bit_rate_code > put_bits(&pbc, 5, 0); // reserved > > flush_put_bits(&pbc); > @@ -5975,8 +5974,7 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt) > if ((par->codec_id == AV_CODEC_ID_DNXHD || > par->codec_id == AV_CODEC_ID_H264 || > par->codec_id == AV_CODEC_ID_HEVC || > - par->codec_id == AV_CODEC_ID_TRUEHD || > - par->codec_id == AV_CODEC_ID_AC3) && !trk->vos_len && > + par->codec_id == AV_CODEC_ID_TRUEHD) && !trk->vos_len && > !TAG_IS_AVCI(trk->tag)) { > /* copy frame to create needed atoms */ > trk->vos_len = size; > @@ -6053,7 +6051,8 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt) > } > } > > - } else if (par->codec_id == AV_CODEC_ID_EAC3) { > + } else if (par->codec_id == AV_CODEC_ID_EAC3 || > + par->codec_id == AV_CODEC_ID_AC3) { > size = handle_eac3(mov, pkt, trk); > if (size < 0) > return size;
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. Jan
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. Jan
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. Jan
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
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; + } + } - init_get_bits(&gbc, track->vos_data + 4, (track->vos_len - 4) * 8); - fscod = get_bits(&gbc, 2); - frmsizecod = get_bits(&gbc, 6); - bsid = get_bits(&gbc, 5); - bsmod = get_bits(&gbc, 3); - acmod = get_bits(&gbc, 3); - if (acmod == 2) { - skip_bits(&gbc, 2); // dsurmod - } else { - if ((acmod & 1) && acmod != 1) - skip_bits(&gbc, 2); // cmixlev - if (acmod & 4) - skip_bits(&gbc, 2); // surmixlev + if (ac3_bit_rate_code < 0) { + av_log(s, AV_LOG_ERROR, + "No valid AC3 bit rate code for data rate of %d!\n", + info->data_rate); + return AVERROR(EINVAL); } - lfeon = get_bits1(&gbc); + + avio_wb32(pb, 11); + ffio_wfourcc(pb, "dac3"); init_put_bits(&pbc, buf, sizeof(buf)); - put_bits(&pbc, 2, fscod); - put_bits(&pbc, 5, bsid); - put_bits(&pbc, 3, bsmod); - put_bits(&pbc, 3, acmod); - put_bits(&pbc, 1, lfeon); - put_bits(&pbc, 5, frmsizecod >> 1); // bit_rate_code + put_bits(&pbc, 2, info->substream[0].fscod); + put_bits(&pbc, 5, info->substream[0].bsid); + put_bits(&pbc, 3, info->substream[0].bsmod); + put_bits(&pbc, 3, info->substream[0].acmod); + put_bits(&pbc, 1, info->substream[0].lfeon); + put_bits(&pbc, 5, ac3_bit_rate_code); // bit_rate_code put_bits(&pbc, 5, 0); // reserved flush_put_bits(&pbc); @@ -5975,8 +5974,7 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt) if ((par->codec_id == AV_CODEC_ID_DNXHD || par->codec_id == AV_CODEC_ID_H264 || par->codec_id == AV_CODEC_ID_HEVC || - par->codec_id == AV_CODEC_ID_TRUEHD || - par->codec_id == AV_CODEC_ID_AC3) && !trk->vos_len && + par->codec_id == AV_CODEC_ID_TRUEHD) && !trk->vos_len && !TAG_IS_AVCI(trk->tag)) { /* copy frame to create needed atoms */ trk->vos_len = size; @@ -6053,7 +6051,8 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt) } } - } else if (par->codec_id == AV_CODEC_ID_EAC3) { + } else if (par->codec_id == AV_CODEC_ID_EAC3 || + par->codec_id == AV_CODEC_ID_AC3) { size = handle_eac3(mov, pkt, trk); if (size < 0) return size;