| Message ID | 20220617130443.188377-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> > --- > 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. > 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? > + 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;
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. Jan
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? - Andreas
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). Jan
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
On Wed, Jun 22, 2022 at 1:05 AM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > 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. If not out-of-spec it is at the very least a case of "not defined by spec". Is it correct to interpret and then index (which leads to those values being shown correctly by parsers which actually read/present the information from the box, but depending on which range of values are actually utilized by this fork of the format we might have non-applicable values), or should the bits just be passed which will be shown as the wrong value but it matches the higher bits? > 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). Not knowing the limitations of DNET and how it most likely was not meant to go into this container mapping, I cannot disagree. My initial attempt was to just handle this by checking if the resulting bit rate fit within the requirements of the container mapping. If not, error out. Thankfully the bsid is already in there: info->substream[hdr->substreamid].bsid = hdr->bitstream_id; Jan
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;