| Message ID | 20210410081606.557527-1-Shulyaka@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] IEC61937_EAC3 decoding support | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
Am Sa., 10. Apr. 2021 um 10:46 Uhr schrieb Denis Shulyaka <shulyaka@gmail.com>: > > This patch adds support to decode IEC61937_EAC3 (aka dolby digital plus). > > Signed-off-by: Denis Shulyaka <Shulyaka@gmail.com> > --- > libavformat/spdifdec.c | 47 ++++++++++++++++++++++++++++++++++-------- > 1 file changed, 38 insertions(+), 9 deletions(-) > > diff --git a/libavformat/spdifdec.c b/libavformat/spdifdec.c > index 1808fa9d65..58841e7775 100644 > --- a/libavformat/spdifdec.c > +++ b/libavformat/spdifdec.c > @@ -93,6 +93,10 @@ static int spdif_get_offset_and_codec(AVFormatContext *s, > *offset = 8192; > *codec = AV_CODEC_ID_DTS; > break; > + case IEC61937_EAC3: > + *offset = AC3_FRAME_SIZE << 4; > + *codec = AV_CODEC_ID_EAC3; > + break; > default: > if (s) { /* be silent during a probe */ > avpriv_request_sample(s, "Data type 0x%04x in IEC 61937", > @@ -103,9 +107,34 @@ static int spdif_get_offset_and_codec(AVFormatContext *s, > return 0; > } > > -/* Largest offset between bursts we currently handle, i.e. AAC with > - samples = 4096 */ > -#define SPDIF_MAX_OFFSET 16384 > +static int spdif_read_burst_payload_length(AVFormatContext *s, > + enum IEC61937DataType data_type) > +{ > + AVIOContext *pb = s->pb; > + int pkt_size_bits, pkt_size_bytes; > + > + switch (data_type & 0xff) { > + case IEC61937_EAC3: > + pkt_size_bytes = avio_rl16(pb); > + > + if (pkt_size_bytes % 2) Please use "&" instead of "%". > + avpriv_request_sample(s, "Packet not ending at a 16-bit boundary"); > + > + return FFALIGN(pkt_size_bytes, 2); > + break; Please remove this line. > + default: > + pkt_size_bits = avio_rl16(pb); > + > + if (pkt_size_bits % 16) See above. > + avpriv_request_sample(s, "Packet not ending at a 16-bit boundary"); > + > + return FFALIGN(pkt_size_bits, 16) >> 3; > + } > + return 0; Please remove this line or the default branch above. > +} > + > +/* Largest offset between bursts we currently handle, i.e. E-AC-3 */ > +#define SPDIF_MAX_OFFSET 24576 > > static int spdif_probe(const AVProbeData *p) > { > @@ -140,7 +169,7 @@ int ff_spdif_probe(const uint8_t *p_buf, int buf_size, enum AVCodecID *codec) > break; > > /* continue probing to find more sync codes */ > - probe_end = FFMIN(buf + SPDIF_MAX_OFFSET, p_buf + buf_size - 1); > + probe_end = FFMIN(buf + SPDIF_MAX_OFFSET + 1, p_buf + buf_size - 1); Is this an unrelated fix? > > /* skip directly to the next sync code */ > if (!spdif_get_offset_and_codec(NULL, (buf[2] << 8) | buf[1], > @@ -176,7 +205,7 @@ int ff_spdif_read_packet(AVFormatContext *s, AVPacket *pkt) > enum IEC61937DataType data_type; > enum AVCodecID codec_id; > uint32_t state = 0; > - int pkt_size_bits, offset, ret; > + int pkt_size, offset, ret; > > while (state != (AV_BSWAP16C(SYNCWORD1) << 16 | AV_BSWAP16C(SYNCWORD2))) { > state = (state << 8) | avio_r8(pb); > @@ -185,12 +214,12 @@ int ff_spdif_read_packet(AVFormatContext *s, AVPacket *pkt) > } > > data_type = avio_rl16(pb); > - pkt_size_bits = avio_rl16(pb); > + pkt_size = spdif_read_burst_payload_length(s, data_type); > > - if (pkt_size_bits % 16) > - avpriv_request_sample(s, "Packet not ending at a 16-bit boundary"); > + if (!pkt_size) > + return AVERROR_BUG; This looks wrong or do I miss something? Carl Eugen
Hi Carl, Thanks for the review. сб, 10 апр. 2021 г. в 18:05, Carl Eugen Hoyos <ceffmpeg@gmail.com>: > Please use "&" instead of "%". > Fixed Please remove this line. > Fixed See above. > Fixed Please remove this line or the default branch above. > Fixed. It was there as a safety feature in case someone adds a new condition to the switch which wouldn't return. Is this an unrelated fix? > Yes, you are right, will submit as a separate patch. > + if (!pkt_size) > > + return AVERROR_BUG; > > This looks wrong or do I miss something? > This is also just for safety to verify that future modifications of spdif_read_burst_payload_length() work correctly. Should I remove it?
Hi all, This is a kind reminder. Best regards, Denis Shulyaka вс, 11 апр. 2021 г. в 13:21, Denis Shulyaka <shulyaka@gmail.com>: > Hi Carl, > > Thanks for the review. > > сб, 10 апр. 2021 г. в 18:05, Carl Eugen Hoyos <ceffmpeg@gmail.com>: > >> Please use "&" instead of "%". >> > > Fixed > > Please remove this line. >> > > Fixed > > See above. >> > > Fixed > > Please remove this line or the default branch above. >> > > Fixed. It was there as a safety feature in case someone adds a new > condition to the switch which wouldn't return. > > Is this an unrelated fix? >> > > Yes, you are right, will submit as a separate patch. > > > + if (!pkt_size) >> > + return AVERROR_BUG; >> >> This looks wrong or do I miss something? >> > > This is also just for safety to verify that future modifications of > spdif_read_burst_payload_length() work correctly. Should I remove it? > > -- > Best regards, > Denis Shulyaka >
Hi all, Is there anything I could do to kindly ask to review and accept the patch? Best regards, Denis Shulyaka вт, 28 сент. 2021 г. в 17:53, Denis Shulyaka <shulyaka@gmail.com>: > Hi all, > > This is a kind reminder. > > Best regards, > Denis Shulyaka > > вс, 11 апр. 2021 г. в 13:21, Denis Shulyaka <shulyaka@gmail.com>: > >> Hi Carl, >> >> Thanks for the review. >> >> сб, 10 апр. 2021 г. в 18:05, Carl Eugen Hoyos <ceffmpeg@gmail.com>: >> >>> Please use "&" instead of "%". >>> >> >> Fixed >> >> Please remove this line. >>> >> >> Fixed >> >> See above. >>> >> >> Fixed >> >> Please remove this line or the default branch above. >>> >> >> Fixed. It was there as a safety feature in case someone adds a new >> condition to the switch which wouldn't return. >> >> Is this an unrelated fix? >>> >> >> Yes, you are right, will submit as a separate patch. >> >> > + if (!pkt_size) >>> > + return AVERROR_BUG; >>> >>> This looks wrong or do I miss something? >>> >> >> This is also just for safety to verify that future modifications of >> spdif_read_burst_payload_length() work correctly. Should I remove it? >> >> -- >> Best regards, >> Denis Shulyaka >> > > > -- > Best regards, > Denis Shulyaka >
Quoting Denis Shulyaka (2022-01-11 16:52:42) > Hi all, > > Is there anything I could do to kindly ask to review and accept the patch? Could you add a FATE test, so this code can be regression-tested automatically?
Sure, will try to figure out how to do it! ср, 12 янв. 2022 г. в 12:46, Anton Khirnov <anton@khirnov.net>: > Quoting Denis Shulyaka (2022-01-11 16:52:42) > > Hi all, > > > > Is there anything I could do to kindly ask to review and accept the > patch? > > Could you add a FATE test, so this code can be regression-tested > automatically? > > -- > Anton Khirnov > _______________________________________________ > 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". >
diff --git a/libavformat/spdifdec.c b/libavformat/spdifdec.c index 1808fa9d65..58841e7775 100644 --- a/libavformat/spdifdec.c +++ b/libavformat/spdifdec.c @@ -93,6 +93,10 @@ static int spdif_get_offset_and_codec(AVFormatContext *s, *offset = 8192; *codec = AV_CODEC_ID_DTS; break; + case IEC61937_EAC3: + *offset = AC3_FRAME_SIZE << 4; + *codec = AV_CODEC_ID_EAC3; + break; default: if (s) { /* be silent during a probe */ avpriv_request_sample(s, "Data type 0x%04x in IEC 61937", @@ -103,9 +107,34 @@ static int spdif_get_offset_and_codec(AVFormatContext *s, return 0; } -/* Largest offset between bursts we currently handle, i.e. AAC with - samples = 4096 */ -#define SPDIF_MAX_OFFSET 16384 +static int spdif_read_burst_payload_length(AVFormatContext *s, + enum IEC61937DataType data_type) +{ + AVIOContext *pb = s->pb; + int pkt_size_bits, pkt_size_bytes; + + switch (data_type & 0xff) { + case IEC61937_EAC3: + pkt_size_bytes = avio_rl16(pb); + + if (pkt_size_bytes % 2) + avpriv_request_sample(s, "Packet not ending at a 16-bit boundary"); + + return FFALIGN(pkt_size_bytes, 2); + break; + default: + pkt_size_bits = avio_rl16(pb); + + if (pkt_size_bits % 16) + avpriv_request_sample(s, "Packet not ending at a 16-bit boundary"); + + return FFALIGN(pkt_size_bits, 16) >> 3; + } + return 0; +} + +/* Largest offset between bursts we currently handle, i.e. E-AC-3 */ +#define SPDIF_MAX_OFFSET 24576 static int spdif_probe(const AVProbeData *p) { @@ -140,7 +169,7 @@ int ff_spdif_probe(const uint8_t *p_buf, int buf_size, enum AVCodecID *codec) break; /* continue probing to find more sync codes */ - probe_end = FFMIN(buf + SPDIF_MAX_OFFSET, p_buf + buf_size - 1); + probe_end = FFMIN(buf + SPDIF_MAX_OFFSET + 1, p_buf + buf_size - 1); /* skip directly to the next sync code */ if (!spdif_get_offset_and_codec(NULL, (buf[2] << 8) | buf[1], @@ -176,7 +205,7 @@ int ff_spdif_read_packet(AVFormatContext *s, AVPacket *pkt) enum IEC61937DataType data_type; enum AVCodecID codec_id; uint32_t state = 0; - int pkt_size_bits, offset, ret; + int pkt_size, offset, ret; while (state != (AV_BSWAP16C(SYNCWORD1) << 16 | AV_BSWAP16C(SYNCWORD2))) { state = (state << 8) | avio_r8(pb); @@ -185,12 +214,12 @@ int ff_spdif_read_packet(AVFormatContext *s, AVPacket *pkt) } data_type = avio_rl16(pb); - pkt_size_bits = avio_rl16(pb); + pkt_size = spdif_read_burst_payload_length(s, data_type); - if (pkt_size_bits % 16) - avpriv_request_sample(s, "Packet not ending at a 16-bit boundary"); + if (!pkt_size) + return AVERROR_BUG; - ret = av_new_packet(pkt, FFALIGN(pkt_size_bits, 16) >> 3); + ret = av_new_packet(pkt, pkt_size); if (ret) return ret;
This patch adds support to decode IEC61937_EAC3 (aka dolby digital plus). Signed-off-by: Denis Shulyaka <Shulyaka@gmail.com> --- libavformat/spdifdec.c | 47 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 9 deletions(-)