| Message ID | 20230213180936.815-3-nicolas.gaullier@cji.paris |
|---|---|
| State | New |
| Headers | show |
| Series | wavdev: s337m support | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
mån 2023-02-13 klockan 19:09 +0100 skrev Nicolas Gaullier: > Prepare the support of s337m in muxers other than raw (ex: wav). > For example, this forbids reading 16 bits DolbyE stream from a 24 bit > wav file. > --- > libavformat/s337m.c | 21 +++++++++++++++------ > libavformat/s337m.h | 3 ++- > 2 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/libavformat/s337m.c b/libavformat/s337m.c > index 582c8b3670..2a566e3cba 100644 > --- a/libavformat/s337m.c > +++ b/libavformat/s337m.c > @@ -35,7 +35,7 @@ > > static int s337m_get_offset_and_codec(void *avc, > uint64_t state, > - int data_type, int data_size, > + int data_type, int data_size, > int container_word_bits, > int *offset, enum AVCodecID > *codec) > { > int word_bits; > @@ -56,6 +56,12 @@ static int s337m_get_offset_and_codec(void *avc, > avpriv_report_missing_feature(avc, "Data type %#x in > SMPTE 337M", data_type & 0x1F); > return AVERROR_PATCHWELCOME; > } > + if (container_word_bits && > + !(container_word_bits == 16 && word_bits == 16) && > + !(container_word_bits == 24 && word_bits == 20) && I presume 20/24 is intentional. Does WAV not support signalling 20-bit? The rest looks OK enough /Tomas
>> @@ -56,6 +56,12 @@ static int s337m_get_offset_and_codec(void *avc, >> avpriv_report_missing_feature(avc, "Data type %#x in >> SMPTE 337M", data_type & 0x1F); >> return AVERROR_PATCHWELCOME; >> } >> + if (container_word_bits && >> + !(container_word_bits == 16 && word_bits == 16) && >> + !(container_word_bits == 24 && word_bits == 20) && > >I presume 20/24 is intentional. Does WAV not support signalling 20-bit? > >The rest looks OK enough > >/Tomas You are true, I meant "container_word_bits" as "block_size" rather than "valid bits per sample" and I think this should be clarified is the latter integration code in "wavdec: s337m support" patch where I use par->bits_per_coded_sample... But I should have rather coded "AV_CODEC_ID_PCM_S16LE ? 16 : 24" This is in case a wav file would be detected as 20 bits in a 24 bits container (I don't think it is supported yet, but it could as bitspersample and validbitpersample are two different fields in WAVE_FORMAT_EXTENSIBLE). Are you ok with this ? Nicolas
fre 2023-02-17 klockan 09:44 +0000 skrev Nicolas Gaullier: > > > @@ -56,6 +56,12 @@ static int s337m_get_offset_and_codec(void > > > *avc, > > > avpriv_report_missing_feature(avc, "Data type %#x in > > > SMPTE 337M", data_type & 0x1F); > > > return AVERROR_PATCHWELCOME; > > > } > > > + if (container_word_bits && > > > + !(container_word_bits == 16 && word_bits == 16) && > > > + !(container_word_bits == 24 && word_bits == 20) && > > > > I presume 20/24 is intentional. Does WAV not support signalling 20- > > bit? > > > > The rest looks OK enough > > > > /Tomas > > You are true, I meant "container_word_bits" as "block_size" rather > than "valid bits per sample" and > I think this should be clarified is the latter integration code in > "wavdec: s337m support" patch where > I use par->bits_per_coded_sample... > But I should have rather coded "AV_CODEC_ID_PCM_S16LE ? 16 : 24" > This is in case a wav file would be detected as 20 bits in a 24 bits > container (I don't think it is supported yet, but it could as > bitspersample and validbitpersample are two different fields in > WAVE_FORMAT_EXTENSIBLE). > Are you ok with this ? I haven't worked enough with S377m to really know, but I do know it's a mess. Is there a way to differentiate "regular" packed 20-bit audio from S377m in wav? /Tomas
>I haven't worked enough with S377m to really know, but I do know it's a mess. Is there a way to differentiate "regular" packed 20-bit audio from S377m in wav? > >/Tomas There is a relevant overview of S337m in this dolby paper: https://developer.dolby.com/globalassets/professional/dolby-e/dolby-e-tech-doc_1.2.pdf Page 25, one can read: SMPTE 337M is the primary method by which Dolby E is able to work in existing facilities and with existing devices. The standard is written such that the same AES3 interface can be shared with the normal PCM audio usage either by treating the AES3 channels independently or by alternating between data and linear PCM usage. Devices that are specifically designed for SMPTE 337M/Dolby E compatibility should be able to transition easily between both usages. The whole design is to not signal, not differentiate "normal PCM audio usage" from s337m. And indeed, manufacturers have implemented probing in all their hardware/software (be it linear/SDI input, or mxf/wav files input etc.). Note: ARD/ZDF mxf file format being "the" world exception here, as dolby_e/non-pcm must be signaled, made explicit: https://www.ard.de/ard/die-ard/ARD-ZDF-HDF02a-AVC-I-100-1080i-25-8-Audio-Tracks-100.pdf In ffmpeg, we already have spdif probing in wav, so there is nothing really new technically speaking. Quoting the previous dolby paper, about spdif (IEC61937): The IEC61937 format is similar to the SMPTE 337M format and can be considered a subset of SMPTE 337M for some data types (see SMPTE 337M Section 7), however there are significant differences in the two standards. In particular IEC61937 does not support the Dolby E data type. Nicolas
tis 2023-02-21 klockan 10:43 +0000 skrev Nicolas Gaullier: > > I haven't worked enough with S377m to really know, but I do know > > it's a mess. Is there a way to differentiate "regular" packed 20- > > bit audio from S377m in wav? > > > > /Tomas > > There is a relevant overview of S337m in this dolby paper: > https://developer.dolby.com/globalassets/professional/dolby-e/dolby-e-tech-doc_1.2.pdf > Page 25, one can read: > SMPTE 337M is the primary method by which Dolby E is able to work in > existing > facilities and with existing devices. The standard is written such > that the same AES3 > interface can be shared with the normal PCM audio usage either by > treating the AES3 > channels independently or by alternating between data and linear PCM > usage. > Devices that are specifically designed for SMPTE 337M/Dolby E > compatibility > should be able to transition easily between both usages. > > The whole design is to not signal, not differentiate "normal PCM > audio usage" from s337m. > And indeed, manufacturers have implemented probing in all their > hardware/software (be it linear/SDI input, or mxf/wav files input > etc.). Right, it's a deliberate mess. > Note: ARD/ZDF mxf file format being "the" world exception here, as > dolby_e/non-pcm must be signaled, made explicit: > > https://www.ard.de/ard/die-ard/ARD-ZDF-HDF02a-AVC-I-100-1080i-25-8-Audio-Tracks-100.pdf This is the correct approach and precisely what I would expect in the MXF space. You make it possible to probe in case you have stupid hardware, but you also explicitly signal it in the container for systems that are capable of carrying that over. I will note that Dolby-E support in mxfdec seems to have been in Baptiste's mind when he first committed the code: > //{ { > 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x02,0x02,0x02,0x03,0x02 > ,0x1C,0x00 }, 15, AV_CODEC_ID_DOLBY_E }, /* Dolby-E */ Since ccba07d12c. It remains commented out to this day. Anyway I don't think this has to hold up this patchset. If it works it works. We can always improve this later if necessary. /Tomas
diff --git a/libavformat/s337m.c b/libavformat/s337m.c index 582c8b3670..2a566e3cba 100644 --- a/libavformat/s337m.c +++ b/libavformat/s337m.c @@ -35,7 +35,7 @@ static int s337m_get_offset_and_codec(void *avc, uint64_t state, - int data_type, int data_size, + int data_type, int data_size, int container_word_bits, int *offset, enum AVCodecID *codec) { int word_bits; @@ -56,6 +56,12 @@ static int s337m_get_offset_and_codec(void *avc, avpriv_report_missing_feature(avc, "Data type %#x in SMPTE 337M", data_type & 0x1F); return AVERROR_PATCHWELCOME; } + if (container_word_bits && + !(container_word_bits == 16 && word_bits == 16) && + !(container_word_bits == 24 && word_bits == 20) && + !(container_word_bits == 24 && word_bits == 24)) { + return AVERROR_INVALIDDATA; + } if (codec) *codec = AV_CODEC_ID_DOLBY_E; @@ -105,7 +111,7 @@ static int s337m_probe(const AVProbeData *p) data_size = AV_RL24(buf + 3); } - if (s337m_get_offset_and_codec(NULL, state, data_type, data_size, &offset, NULL)) + if (s337m_get_offset_and_codec(NULL, state, data_type, data_size, 0, &offset, NULL)) continue; i = IS_16LE_MARKER(state) ? 0 : IS_20LE_MARKER(state) ? 1 : 2; @@ -143,13 +149,16 @@ static void bswap_buf24(uint8_t *data, int size) FFSWAP(uint8_t, data[0], data[2]); } -int ff_s337m_get_packet(AVIOContext *pb, AVPacket *pkt, int size, enum AVCodecID *codec, void *avc) +int ff_s337m_get_packet(AVIOContext *pb, AVPacket *pkt, int size, enum AVCodecID *codec, void *avc, int container_word_bits) { uint64_t state = 0; int ret, data_type, data_size, offset; int64_t orig_pos = avio_tell(pb); - while (!IS_LE_MARKER(state)) { + if (container_word_bits && container_word_bits != 16 && container_word_bits != 24) + return AVERROR_INVALIDDATA; + while ((container_word_bits == 24 || !IS_16LE_MARKER(state)) + && (container_word_bits == 16 || !IS_20LE_MARKER(state) && !IS_24LE_MARKER(state))) { state = (state << 8) | avio_r8(pb); if (avio_feof(pb)) return AVERROR_EOF; @@ -167,7 +176,7 @@ int ff_s337m_get_packet(AVIOContext *pb, AVPacket *pkt, int size, enum AVCodecID data_size = avio_rl24(pb); } - if ((ret = s337m_get_offset_and_codec(avc, state, data_type, data_size, &offset, codec)) < 0) + if ((ret = s337m_get_offset_and_codec(avc, state, data_type, data_size, container_word_bits, &offset, codec)) < 0) return ret; offset = FFMIN(offset, size - (avio_tell(pb) - orig_pos)); @@ -187,7 +196,7 @@ static int s337m_read_packet(AVFormatContext *s, AVPacket *pkt) enum AVCodecID codec; int ret; - if ((ret = ff_s337m_get_packet(s->pb, pkt, avio_size(s->pb), &codec, s)) < 0) + if ((ret = ff_s337m_get_packet(s->pb, pkt, avio_size(s->pb), &codec, s, 0)) < 0) return ret; if (!s->nb_streams) { diff --git a/libavformat/s337m.h b/libavformat/s337m.h index f7bd0c16f6..af2c4c85a3 100644 --- a/libavformat/s337m.h +++ b/libavformat/s337m.h @@ -30,8 +30,9 @@ * @param size Maximum IO read size available for reading at current position * @param codec Returns AV_CODEC_ID_DOLBY_E * @param avc For av_log + * @param container_word_bits 16,24, or 0 for autodetect * @return = 0 on success (an error is raised if no s337m was found) */ -int ff_s337m_get_packet(AVIOContext *pb, AVPacket *pkt, int size, enum AVCodecID *codec, void *avc); +int ff_s337m_get_packet(AVIOContext *pb, AVPacket *pkt, int size, enum AVCodecID *codec, void *avc, int container_word_bits); #endif /* AVFORMAT_S337M_H */