* [FFmpeg-devel] [PATCH 1/6] avcodec: make AC-3 bit rate table available in a separate header
2022-06-17 13:04 [FFmpeg-devel] [PATCH 0/6 v2] avformat/movenc: normalize on AC-3 parser usage Jan Ekström
@ 2022-06-17 13:04 ` Jan Ekström
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 2/6] {configure, avformat/movenc}: enable AC-3 parser for movenc Jan Ekström
` (5 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Jan Ekström @ 2022-06-17 13:04 UTC (permalink / raw)
To: ffmpeg-devel
From: Jan Ekström <jan.ekstrom@24i.com>
This makes it possible to include it from libavformat
---
libavcodec/Makefile | 8 +++++---
libavcodec/ac3_bitrate_tab.c | 22 ++++++++++++++++++++++
libavcodec/ac3_bitrate_tab.h | 33 +++++++++++++++++++++++++++++++++
libavcodec/ac3tab.c | 6 ------
4 files changed, 60 insertions(+), 9 deletions(-)
create mode 100644 libavcodec/ac3_bitrate_tab.c
create mode 100644 libavcodec/ac3_bitrate_tab.h
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 3b8f7b5e01..10697d31f7 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -190,8 +190,9 @@ OBJS-$(CONFIG_AC3_DECODER) += ac3dec_float.o ac3dec_data.o ac3.o \
OBJS-$(CONFIG_AC3_FIXED_DECODER) += ac3dec_fixed.o ac3dec_data.o ac3.o \
kbdwin.o ac3tab.o ac3_channel_layout_tab.o
OBJS-$(CONFIG_AC3_ENCODER) += ac3enc_float.o ac3enc.o ac3tab.o \
- ac3.o kbdwin.o
-OBJS-$(CONFIG_AC3_FIXED_ENCODER) += ac3enc_fixed.o ac3enc.o ac3tab.o ac3.o kbdwin.o
+ ac3.o kbdwin.o ac3_bitrate_tab.o
+OBJS-$(CONFIG_AC3_FIXED_ENCODER) += ac3enc_fixed.o ac3enc.o ac3tab.o \
+ ac3.o kbdwin.o ac3_bitrate_tab.o
OBJS-$(CONFIG_AC3_MF_ENCODER) += mfenc.o mf_utils.o
OBJS-$(CONFIG_ACELP_KELVIN_DECODER) += g729dec.o lsp.o celp_math.o celp_filters.o acelp_filters.o acelp_pitch_delay.o acelp_vectors.o g729postfilter.o
OBJS-$(CONFIG_AGM_DECODER) += agm.o
@@ -1110,7 +1111,8 @@ OBJS-$(CONFIG_LIBZVBI_TELETEXT_DECODER) += libzvbi-teletextdec.o ass.o
OBJS-$(CONFIG_AAC_LATM_PARSER) += latm_parser.o
OBJS-$(CONFIG_AAC_PARSER) += aac_parser.o aac_ac3_parser.o
OBJS-$(CONFIG_AC3_PARSER) += aac_ac3_parser.o ac3tab.o \
- ac3_channel_layout_tab.o
+ ac3_channel_layout_tab.o \
+ ac3_bitrate_tab.o
OBJS-$(CONFIG_ADX_PARSER) += adx_parser.o adx.o
OBJS-$(CONFIG_AMR_PARSER) += amr_parser.o
OBJS-$(CONFIG_AV1_PARSER) += av1_parser.o
diff --git a/libavcodec/ac3_bitrate_tab.c b/libavcodec/ac3_bitrate_tab.c
new file mode 100644
index 0000000000..9ae8794a4c
--- /dev/null
+++ b/libavcodec/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 "ac3_bitrate_tab.h"
diff --git a/libavcodec/ac3_bitrate_tab.h b/libavcodec/ac3_bitrate_tab.h
new file mode 100644
index 0000000000..44fc5f59ec
--- /dev/null
+++ b/libavcodec/ac3_bitrate_tab.h
@@ -0,0 +1,33 @@
+/*
+ * 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
+ */
+
+#ifndef AVCODEC_AC3_BITRATE_TAB_H
+#define AVCODEC_AC3_BITRATE_TAB_H
+
+#include <stdint.h>
+
+/* possible bitrates */
+const uint16_t ff_ac3_bitrate_tab[19] = {
+ 32, 40, 48, 56, 64, 80, 96, 112, 128,
+ 160, 192, 224, 256, 320, 384, 448, 512, 576, 640
+};
+
+#endif
diff --git a/libavcodec/ac3tab.c b/libavcodec/ac3tab.c
index 48c89a8ba0..75865fd7d2 100644
--- a/libavcodec/ac3tab.c
+++ b/libavcodec/ac3tab.c
@@ -94,12 +94,6 @@ const uint8_t ff_ac3_dec_channel_map[8][2][6] = {
/* possible frequencies */
const int ff_ac3_sample_rate_tab[] = { 48000, 44100, 32000, 0 };
-/* possible bitrates */
-const uint16_t ff_ac3_bitrate_tab[19] = {
- 32, 40, 48, 56, 64, 80, 96, 112, 128,
- 160, 192, 224, 256, 320, 384, 448, 512, 576, 640
-};
-
/**
* Table of bin locations for rematrixing bands
* reference: Section 7.5.2 Rematrixing : Frequency Band Definitions
--
2.36.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 15+ messages in thread* [FFmpeg-devel] [PATCH 2/6] {configure, avformat/movenc}: enable AC-3 parser for movenc
2022-06-17 13:04 [FFmpeg-devel] [PATCH 0/6 v2] avformat/movenc: normalize on AC-3 parser usage Jan Ekström
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 1/6] avcodec: make AC-3 bit rate table available in a separate header Jan Ekström
@ 2022-06-17 13:04 ` Jan Ekström
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 3/6] avformat/movenc: enable handle_eac3 to handle AC-3 tracks Jan Ekström
` (4 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Jan Ekström @ 2022-06-17 13:04 UTC (permalink / raw)
To: ffmpeg-devel
From: Jan Ekström <jan.ekstrom@24i.com>
This simplifies the code to no longer have #ifs in a manner which
does not require handling avpriv_ac3_parse_header returning ENOSYS.
As an existing example, the MPEG-TS muxer already requires the AC-3
parser, and in order to fix existing issues with the current AC-3
movenc code, switching to use the AC-3 parser is required, so this
is an enabling change for that.
Signed-off-by: Jan Ekström <jan.ekstrom@24i.com>
---
configure | 2 +-
libavformat/movenc.c | 4 ----
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/configure b/configure
index 3dca1c4bd3..c535c4d4f1 100755
--- a/configure
+++ b/configure
@@ -3440,7 +3440,7 @@ mlp_demuxer_select="mlp_parser"
mmf_muxer_select="riffenc"
mov_demuxer_select="iso_media riffdec"
mov_demuxer_suggest="zlib"
-mov_muxer_select="iso_media riffenc rtpenc_chain vp9_superframe_bsf aac_adtstoasc_bsf"
+mov_muxer_select="iso_media riffenc rtpenc_chain vp9_superframe_bsf aac_adtstoasc_bsf ac3_parser"
mp3_demuxer_select="mpegaudio_parser"
mp3_muxer_select="mpegaudioheader"
mp4_muxer_select="mov_muxer"
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index b7b2f46a17..87cd066c95 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -408,7 +408,6 @@ struct eac3_info {
} substream[1]; /* TODO: support 8 independent substreams */
};
-#if CONFIG_AC3_PARSER
static int handle_eac3(MOVMuxContext *mov, AVPacket *pkt, MOVTrack *track)
{
AC3HeaderInfo *hdr = NULL;
@@ -549,7 +548,6 @@ end:
return ret;
}
-#endif
static int mov_write_eac3_tag(AVFormatContext *s, AVIOContext *pb, MOVTrack *track)
{
@@ -6046,7 +6044,6 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
}
}
-#if CONFIG_AC3_PARSER
} else if (par->codec_id == AV_CODEC_ID_EAC3) {
size = handle_eac3(mov, pkt, trk);
if (size < 0)
@@ -6054,7 +6051,6 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
else if (!size)
goto end;
avio_write(pb, pkt->data, size);
-#endif
} else if (par->codec_id == AV_CODEC_ID_EIA_608) {
size = 8;
--
2.36.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 15+ messages in thread* [FFmpeg-devel] [PATCH 3/6] avformat/movenc: enable handle_eac3 to handle AC-3 tracks
2022-06-17 13:04 [FFmpeg-devel] [PATCH 0/6 v2] avformat/movenc: normalize on AC-3 parser usage Jan Ekström
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 1/6] avcodec: make AC-3 bit rate table available in a separate header Jan Ekström
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 2/6] {configure, avformat/movenc}: enable AC-3 parser for movenc Jan Ekström
@ 2022-06-17 13:04 ` Jan Ekström
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 4/6] avformat/movenc: move eac3_info definition so that it can be used for AC-3 Jan Ekström
` (3 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Jan Ekström @ 2022-06-17 13:04 UTC (permalink / raw)
To: ffmpeg-devel
From: Jan Ekström <jan.ekstrom@24i.com>
Add the AC-3 frame type, as well as early exit from additional packet
parsing in case of AC-3, as only a single packet is required to get
the required information.
Signed-off-by: Jan Ekström <jan.ekstrom@24i.com>
---
libavformat/movenc.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 87cd066c95..c24b11cf5f 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -443,7 +443,8 @@ static int handle_eac3(MOVMuxContext *mov, AVPacket *pkt, MOVTrack *track)
/* this should always be the case, given that our AC-3 parser
* concatenates dependent frames to their independent parent */
- if (hdr->frame_type == EAC3_FRAME_TYPE_INDEPENDENT) {
+ if (hdr->frame_type == EAC3_FRAME_TYPE_INDEPENDENT ||
+ hdr->frame_type == EAC3_FRAME_TYPE_AC3_CONVERT) {
/* substream ids must be incremental */
if (hdr->substreamid > info->num_ind_sub + 1) {
ret = AVERROR(EINVAL);
@@ -475,6 +476,14 @@ static int handle_eac3(MOVMuxContext *mov, AVPacket *pkt, MOVTrack *track)
info->substream[hdr->substreamid].acmod = hdr->channel_mode;
info->substream[hdr->substreamid].lfeon = hdr->lfe_on;
+ if (track->par->codec_id == AV_CODEC_ID_AC3) {
+ // with AC-3 we only require the information of a single packet,
+ // so we can finish as soon as the basic values of the bit stream
+ // have been set to the track's informational structure.
+ info->ec3_done = 1;
+ goto concatenate;
+ }
+
/* Parse dependent substream(s), if any */
if (pkt->size != hdr->frame_size) {
int cumul_size = hdr->frame_size;
--
2.36.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 15+ messages in thread* [FFmpeg-devel] [PATCH 4/6] avformat/movenc: move eac3_info definition so that it can be used for AC-3
2022-06-17 13:04 [FFmpeg-devel] [PATCH 0/6 v2] avformat/movenc: normalize on AC-3 parser usage Jan Ekström
` (2 preceding siblings ...)
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 3/6] avformat/movenc: enable handle_eac3 to handle AC-3 tracks Jan Ekström
@ 2022-06-17 13:04 ` Jan Ekström
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 5/6] avformat/movenc: utilize existing AC-3 parsing workflow " Jan Ekström
` (2 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Jan Ekström @ 2022-06-17 13:04 UTC (permalink / raw)
To: ffmpeg-devel
From: Jan Ekström <jan.ekstrom@24i.com>
Signed-off-by: Jan Ekström <jan.ekstrom@24i.com>
---
libavformat/movenc.c | 64 ++++++++++++++++++++++----------------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index c24b11cf5f..103f958d75 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -328,6 +328,38 @@ static int mov_write_amr_tag(AVIOContext *pb, MOVTrack *track)
return 0x11;
}
+struct eac3_info {
+ AVPacket *pkt;
+ uint8_t ec3_done;
+ uint8_t num_blocks;
+
+ /* Layout of the EC3SpecificBox */
+ /* maximum bitrate */
+ uint16_t data_rate;
+ /* number of independent substreams */
+ uint8_t num_ind_sub;
+ struct {
+ /* sample rate code (see ff_ac3_sample_rate_tab) 2 bits */
+ uint8_t fscod;
+ /* bit stream identification 5 bits */
+ uint8_t bsid;
+ /* one bit reserved */
+ /* audio service mixing (not supported yet) 1 bit */
+ /* bit stream mode 3 bits */
+ uint8_t bsmod;
+ /* audio coding mode 3 bits */
+ uint8_t acmod;
+ /* sub woofer on 1 bit */
+ uint8_t lfeon;
+ /* 3 bits reserved */
+ /* number of dependent substreams associated with this substream 4 bits */
+ uint8_t num_dep_sub;
+ /* channel locations of the dependent substream(s), if any, 9 bits */
+ uint16_t chan_loc;
+ /* if there is no dependent substream, then one bit reserved instead */
+ } substream[1]; /* TODO: support 8 independent substreams */
+};
+
static int mov_write_ac3_tag(AVFormatContext *s, AVIOContext *pb, MOVTrack *track)
{
GetBitContext gbc;
@@ -376,38 +408,6 @@ static int mov_write_ac3_tag(AVFormatContext *s, AVIOContext *pb, MOVTrack *trac
return 11;
}
-struct eac3_info {
- AVPacket *pkt;
- uint8_t ec3_done;
- uint8_t num_blocks;
-
- /* Layout of the EC3SpecificBox */
- /* maximum bitrate */
- uint16_t data_rate;
- /* number of independent substreams */
- uint8_t num_ind_sub;
- struct {
- /* sample rate code (see ff_ac3_sample_rate_tab) 2 bits */
- uint8_t fscod;
- /* bit stream identification 5 bits */
- uint8_t bsid;
- /* one bit reserved */
- /* audio service mixing (not supported yet) 1 bit */
- /* bit stream mode 3 bits */
- uint8_t bsmod;
- /* audio coding mode 3 bits */
- uint8_t acmod;
- /* sub woofer on 1 bit */
- uint8_t lfeon;
- /* 3 bits reserved */
- /* number of dependent substreams associated with this substream 4 bits */
- uint8_t num_dep_sub;
- /* channel locations of the dependent substream(s), if any, 9 bits */
- uint16_t chan_loc;
- /* if there is no dependent substream, then one bit reserved instead */
- } substream[1]; /* TODO: support 8 independent substreams */
-};
-
static int handle_eac3(MOVMuxContext *mov, AVPacket *pkt, MOVTrack *track)
{
AC3HeaderInfo *hdr = NULL;
--
2.36.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 15+ messages in thread* [FFmpeg-devel] [PATCH 5/6] avformat/movenc: utilize existing AC-3 parsing workflow for AC-3
2022-06-17 13:04 [FFmpeg-devel] [PATCH 0/6 v2] avformat/movenc: normalize on AC-3 parser usage Jan Ekström
` (3 preceding siblings ...)
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 4/6] avformat/movenc: move eac3_info definition so that it can be used for AC-3 Jan Ekström
@ 2022-06-17 13:04 ` Jan Ekström
2022-06-20 9:10 ` Andreas Rheinhardt
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 6/6] avformat/movenc: handle OOM situations when parsing AC-3 headers Jan Ekström
2022-06-20 7:19 ` [FFmpeg-devel] [PATCH 0/6 v2] avformat/movenc: normalize on AC-3 parser usage Jan Ekström
6 siblings, 1 reply; 15+ messages in thread
From: Jan Ekström @ 2022-06-17 13:04 UTC (permalink / raw)
To: ffmpeg-devel
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
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;
--
2.36.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [FFmpeg-devel] [PATCH 5/6] avformat/movenc: utilize existing AC-3 parsing workflow for AC-3
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 5/6] avformat/movenc: utilize existing AC-3 parsing workflow " Jan Ekström
@ 2022-06-20 9:10 ` Andreas Rheinhardt
2022-06-20 14:26 ` Jan Ekström
0 siblings, 1 reply; 15+ messages in thread
From: Andreas Rheinhardt @ 2022-06-20 9:10 UTC (permalink / raw)
To: ffmpeg-devel
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;
_______________________________________________
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".
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [FFmpeg-devel] [PATCH 5/6] avformat/movenc: utilize existing AC-3 parsing workflow for AC-3
2022-06-20 9:10 ` Andreas Rheinhardt
@ 2022-06-20 14:26 ` Jan Ekström
2022-06-21 7:30 ` Andreas Rheinhardt
0 siblings, 1 reply; 15+ messages in thread
From: Jan Ekström @ 2022-06-20 14:26 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [FFmpeg-devel] [PATCH 5/6] avformat/movenc: utilize existing AC-3 parsing workflow for AC-3
2022-06-20 14:26 ` Jan Ekström
@ 2022-06-21 7:30 ` Andreas Rheinhardt
2022-06-21 14:34 ` Jan Ekström
0 siblings, 1 reply; 15+ messages in thread
From: Andreas Rheinhardt @ 2022-06-21 7:30 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [FFmpeg-devel] [PATCH 5/6] avformat/movenc: utilize existing AC-3 parsing workflow for AC-3
2022-06-21 7:30 ` Andreas Rheinhardt
@ 2022-06-21 14:34 ` Jan Ekström
2022-06-21 22:05 ` Andreas Rheinhardt
0 siblings, 1 reply; 15+ messages in thread
From: Jan Ekström @ 2022-06-21 14:34 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [FFmpeg-devel] [PATCH 5/6] avformat/movenc: utilize existing AC-3 parsing workflow for AC-3
2022-06-21 14:34 ` Jan Ekström
@ 2022-06-21 22:05 ` Andreas Rheinhardt
2022-06-22 6:54 ` Jan Ekström
0 siblings, 1 reply; 15+ messages in thread
From: Andreas Rheinhardt @ 2022-06-21 22:05 UTC (permalink / raw)
To: ffmpeg-devel
Jan Ekström:
> On Tue, Jun 21, 2022 at 10:30 AM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
>>
>> Jan Ekström:
>>> On Mon, Jun 20, 2022 at 12:10 PM Andreas Rheinhardt
>>> <andreas.rheinhardt@outlook.com> wrote:
>>>>
>>>> Jan Ekström:
>>>>> From: Jan Ekström <jan.ekstrom@24i.com>
>>>>>
>>>>> Signed-off-by: Jan Ekström <jan.ekstrom@24i.com>
>>>>> ---
>>>>> libavformat/Makefile | 1 +
>>>>> libavformat/ac3_bitrate_tab.c | 22 ++++++++++++++
>>>>> libavformat/movenc.c | 55 +++++++++++++++++------------------
>>>>> 3 files changed, 50 insertions(+), 28 deletions(-)
>>>>> create mode 100644 libavformat/ac3_bitrate_tab.c
>>>>>
>>>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>>>>> index 1416bf31bd..c71de95b2f 100644
>>>>> --- a/libavformat/Makefile
>>>>> +++ b/libavformat/Makefile
>>>>> @@ -699,6 +699,7 @@ SHLIBOBJS-$(CONFIG_FLV_MUXER) += mpeg4audio_sample_rates.o
>>>>> SHLIBOBJS-$(CONFIG_HLS_DEMUXER) += ac3_channel_layout_tab.o
>>>>> SHLIBOBJS-$(CONFIG_MATROSKA_DEMUXER) += mpeg4audio_sample_rates.o
>>>>> SHLIBOBJS-$(CONFIG_MOV_DEMUXER) += ac3_channel_layout_tab.o
>>>>> +SHLIBOBJS-$(CONFIG_MOV_MUXER) += ac3_bitrate_tab.o
>>>>> SHLIBOBJS-$(CONFIG_MP3_MUXER) += mpegaudiotabs.o
>>>>> SHLIBOBJS-$(CONFIG_MXF_MUXER) += golomb_tab.o
>>>>> SHLIBOBJS-$(CONFIG_NUT_MUXER) += mpegaudiotabs.o
>>>>
>>>> The above line only takes care of duplicating ac3_bitrate_tab.o into
>>>> lavf for shared builds; yet there needs to be a corresponding STLIBOBJS
>>>> entry in lavc/Makefile so that lavc/ac3_bitrate_tab.o is compiled in
>>>> case the mov muxer is enabled in a static build.
>>>> It would work either way in this case because #2 made the mov muxer
>>>> depend upon the ac3 parser which makes lavc/ac3_bitrate_tab.o be included.
>>>>
>>>
>>> OK, then I clearly missed something since I thought I was following
>>> how you had done things earlier for the other table. Will look into
>>> this tomorrow morning.
>>>
>>>>> diff --git a/libavformat/ac3_bitrate_tab.c b/libavformat/ac3_bitrate_tab.c
>>>>> new file mode 100644
>>>>> index 0000000000..57b6181511
>>>>> --- /dev/null
>>>>> +++ b/libavformat/ac3_bitrate_tab.c
>>>>> @@ -0,0 +1,22 @@
>>>>> +/*
>>>>> + * AC-3 bit rate table
>>>>> + * copyright (c) 2001 Fabrice Bellard
>>>>> + *
>>>>> + * This file is part of FFmpeg.
>>>>> + *
>>>>> + * FFmpeg is free software; you can redistribute it and/or
>>>>> + * modify it under the terms of the GNU Lesser General Public
>>>>> + * License as published by the Free Software Foundation; either
>>>>> + * version 2.1 of the License, or (at your option) any later version.
>>>>> + *
>>>>> + * FFmpeg is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>>>> + * Lesser General Public License for more details.
>>>>> + *
>>>>> + * You should have received a copy of the GNU Lesser General Public
>>>>> + * License along with FFmpeg; if not, write to the Free Software
>>>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>>>> + */
>>>>> +
>>>>> +#include "libavcodec/ac3_bitrate_tab.h"
>>>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>>>> index 103f958d75..a071f1cdd5 100644
>>>>> --- a/libavformat/movenc.c
>>>>> +++ b/libavformat/movenc.c
>>>>> @@ -36,6 +36,7 @@
>>>>> #include "av1.h"
>>>>> #include "avc.h"
>>>>> #include "libavcodec/ac3_parser_internal.h"
>>>>> +#include "libavcodec/ac3tab.h"
>>>>> #include "libavcodec/dnxhddata.h"
>>>>> #include "libavcodec/flac.h"
>>>>> #include "libavcodec/get_bits.h"
>>>>> @@ -362,44 +363,42 @@ struct eac3_info {
>>>>>
>>>>> static int mov_write_ac3_tag(AVFormatContext *s, AVIOContext *pb, MOVTrack *track)
>>>>> {
>>>>> - GetBitContext gbc;
>>>>> + struct eac3_info *info = track->eac3_priv;
>>>>> + int8_t ac3_bit_rate_code = -1;
>>>>> PutBitContext pbc;
>>>>> uint8_t buf[3];
>>>>> - int fscod, bsid, bsmod, acmod, lfeon, frmsizecod;
>>>>>
>>>>> - if (track->vos_len < 7) {
>>>>> + if (!info || !info->ec3_done) {
>>>>> av_log(s, AV_LOG_ERROR,
>>>>> "Cannot write moov atom before AC3 packets."
>>>>> " Set the delay_moov flag to fix this.\n");
>>>>> return AVERROR(EINVAL);
>>>>> }
>>>>>
>>>>> - avio_wb32(pb, 11);
>>>>> - ffio_wfourcc(pb, "dac3");
>>>>> + for (unsigned int i = 0; i < FF_ARRAY_ELEMS(ff_ac3_bitrate_tab); i++) {
>>>>> + if (info->data_rate == ff_ac3_bitrate_tab[i]) {
>>>>> + ac3_bit_rate_code = i;
>>>>
>>>> Anyway, I see that you use ff_ac3_bitrate_tab in lavf instead of adding
>>>> the frame_size_code to AC3HeaderInfo. Did it turn out to be easier this
>>>> way or what is the reason for this?
>>>
>>> I did partially go through the reasoning in the cover letter, but
>>> here's a short rewording:
>>>
>>> 1. This way doesn't extend the semi-public avpriv.
>>> 2. While reviewing I noticed that bsid 9, 10 streams (apparently some
>>> RealAudio fork of AC-3) actually have the sample/bit rate values bit
>>> shifted (divided by either 2 or 4). The table in the ETSI
>>> specification is based on the effective bit rate. While it is highly
>>> unlikely that someone would put that type of AC-3 into MP4, In that
>>> case I'd then have to re-index the value based on the effective bit
>>> rate instead of just taking the value from the bit stream in the
>>> parser. So as the effective bit rate is already available through the
>>> header parser, it just seems simpler to have the re-indexing happen on
>>> the using side in this case.
>>>
>>
>> I have to admit not to get this point at all. Let's work through an
>> example: You have bsid 10 and you use ff_ac3_bitrate_tab[17] (i.e. 576).
>> AC3HeaderInfo.bit_rate will then contain 144000 (i.e. the value is
>> already divided by four). Your data_rate will be set to 144 based upon
>> this (if there is no higher datarate already encountered). But there is
>> no entry for 144 in ff_ac3_bitrate_tab, so that one will enter the "No
>> valid AC3 bit rate code for data rate" branch. Is this intended?
>>
>
> In short, yes. Relevant specification for AC-3 in MP4 is
> https://www.etsi.org/deliver/etsi_ts/102300_102399/102366/01.04.01_60/ts_102366v010401p.pdf
> , Annex F. bit_rate_code is defined there, which defines a set of
> values based on the effective bit rate (albeit noted as derived from
> (the topmost bits of) "frmsizcod"). The specification limits itself to
> to bsid N..8, so technically the RealAudio bsid 9, 10 are out of
> scope.
>
> At the end of the day, it's a marking of the bit rate, and if you
> cannot convey the actual bit rate by this table, then I think it's
> valid to fail. With E-AC-3 the MP4 structure has the data_rate itself,
> and not some index value :) . Of course the alternative course of
> action is that you should just stick the original bits there, even if
> then you would be advertising an "incorrect" bit rate on the container
> level.
>
> At the end of the day, I'm fine with either the extension of the
> parser struct or the exposure of the table. It just felt like it's
> alright to map the value according to the list, as there are cases
> where the bits are not interpreted as-is by the parser (and it's
> better to fail in the muxer than in the parser if you want to find the
> index for the newly interpreted bit rate - which does not exist for
> many bit shifted values).
>
Lots of the values in ff_ac3_bitrate_tab have the property that the
value divided by 2 or 4 is also in ff_ac3_bitrate_tab. This means you
some AC-3 with bsid == 9 or bsid == 10 might pass through and be muxed,
yet it is my interpretation of the spec you linked that this is simply
out-of-spec. So you should not allow it. The best way for this is to
simply add bsid to AC3HeaderInfo. After having restricted yourself in
this way AC3HeaderInfo.bit_rate can be used as-is (if I am not mistaken).
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [FFmpeg-devel] [PATCH 5/6] avformat/movenc: utilize existing AC-3 parsing workflow for AC-3
2022-06-21 22:05 ` Andreas Rheinhardt
@ 2022-06-22 6:54 ` Jan Ekström
0 siblings, 0 replies; 15+ messages in thread
From: Jan Ekström @ 2022-06-22 6:54 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 15+ messages in thread
* [FFmpeg-devel] [PATCH 6/6] avformat/movenc: handle OOM situations when parsing AC-3 headers
2022-06-17 13:04 [FFmpeg-devel] [PATCH 0/6 v2] avformat/movenc: normalize on AC-3 parser usage Jan Ekström
` (4 preceding siblings ...)
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 5/6] avformat/movenc: utilize existing AC-3 parsing workflow " Jan Ekström
@ 2022-06-17 13:04 ` Jan Ekström
2022-06-20 7:19 ` [FFmpeg-devel] [PATCH 0/6 v2] avformat/movenc: normalize on AC-3 parser usage Jan Ekström
6 siblings, 0 replies; 15+ messages in thread
From: Jan Ekström @ 2022-06-17 13:04 UTC (permalink / raw)
To: ffmpeg-devel
From: Jan Ekström <jan.ekstrom@24i.com>
Signed-off-by: Jan Ekström <jan.ekstrom@24i.com>
---
libavformat/movenc.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index a071f1cdd5..58ba0bc545 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -420,7 +420,10 @@ static int handle_eac3(MOVMuxContext *mov, AVPacket *pkt, MOVTrack *track)
if (!info->pkt && !(info->pkt = av_packet_alloc()))
return AVERROR(ENOMEM);
- if (avpriv_ac3_parse_header(&hdr, pkt->data, pkt->size) < 0) {
+ if ((ret = avpriv_ac3_parse_header(&hdr, pkt->data, pkt->size) < 0)) {
+ if (ret == AVERROR(ENOMEM))
+ goto end;
+
/* drop the packets until we see a good one */
if (!track->entry) {
av_log(mov->fc, AV_LOG_WARNING, "Dropping invalid packet from start of the stream\n");
--
2.36.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [FFmpeg-devel] [PATCH 0/6 v2] avformat/movenc: normalize on AC-3 parser usage
2022-06-17 13:04 [FFmpeg-devel] [PATCH 0/6 v2] avformat/movenc: normalize on AC-3 parser usage Jan Ekström
` (5 preceding siblings ...)
2022-06-17 13:04 ` [FFmpeg-devel] [PATCH 6/6] avformat/movenc: handle OOM situations when parsing AC-3 headers Jan Ekström
@ 2022-06-20 7:19 ` Jan Ekström
6 siblings, 0 replies; 15+ messages in thread
From: Jan Ekström @ 2022-06-20 7:19 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Fri, Jun 17, 2022 at 4:04 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> The simplified parsing currently in `mov_write_ac3_tag` trusts the content
> of the packets a bit too much (the AC-3 parser returns all data fed to it,
> including any possible data before the start code), while the existing E-AC-3
> logic does proper header validation by utilizing the (E-)AC-3 parser.
>
> Thus, normalize on AC-3 parser usage for both AC-3 and E-AC-3.
>
> Difference to v1:
> * Instead of expanding the avpriv parser interface, make the bit rate table
> available to libavformat as well as libavcodec. This also theoretically
> allows the calculation based on data rate, thus allowing the correct
> bit rate to be signaled with bsids 9 and 10, which have their effective
> bit rate and sample rate values divided by 2 and 4 respectively.
>
I will move towards pulling this patch set in tomorrow unless there
are objections. The earlier version of this patch set has been
successfully working in various locations, and the effective result
from this version of the patch set seems to be the same.
Jan
_______________________________________________
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".
^ permalink raw reply [flat|nested] 15+ messages in thread