diff mbox series

[FFmpeg-devel,2/3] avformat: add raw AC-4 demuxer

Message ID 20200304222633.12177-2-onemda@gmail.com
State New
Headers show
Series [FFmpeg-devel,RFC,WIP,1/3] avcodec: add AC-4 decoder | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork fail Make fate failed

Commit Message

Paul B Mahol March 4, 2020, 10:26 p.m. UTC
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavformat/Makefile     |   1 +
 libavformat/ac4dec.c     | 104 +++++++++++++++++++++++++++++++++++++++
 libavformat/allformats.c |   1 +
 3 files changed, 106 insertions(+)
 create mode 100644 libavformat/ac4dec.c

Comments

James Almer March 4, 2020, 10:45 p.m. UTC | #1
On 3/4/2020 7:26 PM, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavformat/Makefile     |   1 +
>  libavformat/ac4dec.c     | 104 +++++++++++++++++++++++++++++++++++++++
>  libavformat/allformats.c |   1 +
>  3 files changed, 106 insertions(+)
>  create mode 100644 libavformat/ac4dec.c
> 
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index e0681058a2..b4e8d20e65 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -70,6 +70,7 @@ OBJS-$(CONFIG_AA_DEMUXER)                += aadec.o
>  OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o img2.o rawdec.o
>  OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
>  OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
> +OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
>  OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
>  OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
>  OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
> diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
> new file mode 100644
> index 0000000000..8c6e539409
> --- /dev/null
> +++ b/libavformat/ac4dec.c
> @@ -0,0 +1,104 @@
> +/*
> + * RAW AC-4 demuxer
> + * Copyright (c) 2019 Paul B Mahol
> + *
> + * 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 "libavutil/avassert.h"
> +#include "libavutil/crc.h"
> +#include "avformat.h"
> +#include "rawdec.h"
> +
> +static int ac4_probe(const AVProbeData *p)
> +{
> +    const uint8_t *buf = p->buf;
> +    int left = p->buf_size;
> +    int max_frames = 0;
> +
> +    while (left > 7) {
> +        int size;
> +
> +        if (buf[0] == 0xAC &&
> +            (buf[1] == 0x40 ||
> +             buf[1] == 0x41)) {
> +            size = (buf[2] << 8) | buf[3];
> +            if (size == 0xFFFF)
> +                size = 3 + (buf[4] << 16) | (buf[5] << 8) | buf[6];
> +            size += 4;
> +            if (buf[1] == 0x41)
> +                size += 2;
> +            max_frames++;
> +            left -= size;
> +            buf += size;
> +        } else {
> +            break;
> +        }
> +    }
> +
> +    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
> +}
> +
> +static int ac4_read_header(AVFormatContext *s)
> +{
> +    AVStream *st;
> +
> +    st = avformat_new_stream(s, NULL);
> +    if (!st)
> +        return AVERROR(ENOMEM);
> +
> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
> +    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
> +
> +    return 0;
> +}
> +
> +static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    AVIOContext *pb = s->pb;
> +    int64_t pos;
> +    uint16_t sync;
> +    int ret, size;
> +
> +    if (avio_feof(s->pb))
> +        return AVERROR_EOF;
> +
> +    pos   = avio_tell(s->pb);
> +    sync = avio_rb16(pb);

If there are sync codes then it sounds like the proper thing to do is,
much like with AC3, writing a trivial parser to assemble frames and then
use ff_raw_audio_read_header() and ff_raw_read_partial_packet() here
instead of custom functions.

> +    size = avio_rb16(pb);
> +    if (size == 0xffff)
> +        size = avio_rb24(pb);
> +
> +    ret = av_get_packet(pb, pkt, size);
> +    pkt->pos = pos;
> +    pkt->stream_index = 0;
> +
> +    if (sync == 0xAC41)
> +        avio_skip(pb, 2);
> +
> +    return ret;
> +}
> +
> +AVInputFormat ff_ac4_demuxer = {
> +    .name           = "ac4",
> +    .long_name      = NULL_IF_CONFIG_SMALL("raw AC-4"),
> +    .read_probe     = ac4_probe,
> +    .read_header    = ac4_read_header,
> +    .read_packet    = ac4_read_packet,
> +    .flags          = AVFMT_GENERIC_INDEX,
> +    .extensions     = "ac4",
> +};
> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> index 0209bf0e30..d2afcb2bdd 100644
> --- a/libavformat/allformats.c
> +++ b/libavformat/allformats.c
> @@ -33,6 +33,7 @@ extern AVInputFormat  ff_aa_demuxer;
>  extern AVInputFormat  ff_aac_demuxer;
>  extern AVInputFormat  ff_ac3_demuxer;
>  extern AVOutputFormat ff_ac3_muxer;
> +extern AVInputFormat  ff_ac4_demuxer;
>  extern AVInputFormat  ff_acm_demuxer;
>  extern AVInputFormat  ff_act_demuxer;
>  extern AVInputFormat  ff_adf_demuxer;
>
Paul B Mahol March 4, 2020, 10:51 p.m. UTC | #2
On 3/4/20, James Almer <jamrial@gmail.com> wrote:
> On 3/4/2020 7:26 PM, Paul B Mahol wrote:
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavformat/Makefile     |   1 +
>>  libavformat/ac4dec.c     | 104 +++++++++++++++++++++++++++++++++++++++
>>  libavformat/allformats.c |   1 +
>>  3 files changed, 106 insertions(+)
>>  create mode 100644 libavformat/ac4dec.c
>>
>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>> index e0681058a2..b4e8d20e65 100644
>> --- a/libavformat/Makefile
>> +++ b/libavformat/Makefile
>> @@ -70,6 +70,7 @@ OBJS-$(CONFIG_AA_DEMUXER)                += aadec.o
>>  OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o img2.o
>> rawdec.o
>>  OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
>>  OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
>> +OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
>>  OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
>>  OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
>>  OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
>> diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
>> new file mode 100644
>> index 0000000000..8c6e539409
>> --- /dev/null
>> +++ b/libavformat/ac4dec.c
>> @@ -0,0 +1,104 @@
>> +/*
>> + * RAW AC-4 demuxer
>> + * Copyright (c) 2019 Paul B Mahol
>> + *
>> + * 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 "libavutil/avassert.h"
>> +#include "libavutil/crc.h"
>> +#include "avformat.h"
>> +#include "rawdec.h"
>> +
>> +static int ac4_probe(const AVProbeData *p)
>> +{
>> +    const uint8_t *buf = p->buf;
>> +    int left = p->buf_size;
>> +    int max_frames = 0;
>> +
>> +    while (left > 7) {
>> +        int size;
>> +
>> +        if (buf[0] == 0xAC &&
>> +            (buf[1] == 0x40 ||
>> +             buf[1] == 0x41)) {
>> +            size = (buf[2] << 8) | buf[3];
>> +            if (size == 0xFFFF)
>> +                size = 3 + (buf[4] << 16) | (buf[5] << 8) | buf[6];
>> +            size += 4;
>> +            if (buf[1] == 0x41)
>> +                size += 2;
>> +            max_frames++;
>> +            left -= size;
>> +            buf += size;
>> +        } else {
>> +            break;
>> +        }
>> +    }
>> +
>> +    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
>> +}
>> +
>> +static int ac4_read_header(AVFormatContext *s)
>> +{
>> +    AVStream *st;
>> +
>> +    st = avformat_new_stream(s, NULL);
>> +    if (!st)
>> +        return AVERROR(ENOMEM);
>> +
>> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>> +    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
>> +
>> +    return 0;
>> +}
>> +
>> +static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
>> +{
>> +    AVIOContext *pb = s->pb;
>> +    int64_t pos;
>> +    uint16_t sync;
>> +    int ret, size;
>> +
>> +    if (avio_feof(s->pb))
>> +        return AVERROR_EOF;
>> +
>> +    pos   = avio_tell(s->pb);
>> +    sync = avio_rb16(pb);
>
> If there are sync codes then it sounds like the proper thing to do is,
> much like with AC3, writing a trivial parser to assemble frames and then
> use ff_raw_audio_read_header() and ff_raw_read_partial_packet() here
> instead of custom functions.

That is over complication for simple parsing like here.
Every raw packet have exact frame size set in bitstream.

>
>> +    size = avio_rb16(pb);
>> +    if (size == 0xffff)
>> +        size = avio_rb24(pb);
>> +
>> +    ret = av_get_packet(pb, pkt, size);
>> +    pkt->pos = pos;
>> +    pkt->stream_index = 0;
>> +
>> +    if (sync == 0xAC41)
>> +        avio_skip(pb, 2);
>> +
>> +    return ret;
>> +}
>> +
>> +AVInputFormat ff_ac4_demuxer = {
>> +    .name           = "ac4",
>> +    .long_name      = NULL_IF_CONFIG_SMALL("raw AC-4"),
>> +    .read_probe     = ac4_probe,
>> +    .read_header    = ac4_read_header,
>> +    .read_packet    = ac4_read_packet,
>> +    .flags          = AVFMT_GENERIC_INDEX,
>> +    .extensions     = "ac4",
>> +};
>> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
>> index 0209bf0e30..d2afcb2bdd 100644
>> --- a/libavformat/allformats.c
>> +++ b/libavformat/allformats.c
>> @@ -33,6 +33,7 @@ extern AVInputFormat  ff_aa_demuxer;
>>  extern AVInputFormat  ff_aac_demuxer;
>>  extern AVInputFormat  ff_ac3_demuxer;
>>  extern AVOutputFormat ff_ac3_muxer;
>> +extern AVInputFormat  ff_ac4_demuxer;
>>  extern AVInputFormat  ff_acm_demuxer;
>>  extern AVInputFormat  ff_act_demuxer;
>>  extern AVInputFormat  ff_adf_demuxer;
>>
>
> _______________________________________________
> 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".
James Almer March 4, 2020, 11:05 p.m. UTC | #3
On 3/4/2020 7:51 PM, Paul B Mahol wrote:
> On 3/4/20, James Almer <jamrial@gmail.com> wrote:
>> On 3/4/2020 7:26 PM, Paul B Mahol wrote:
>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>> ---
>>>  libavformat/Makefile     |   1 +
>>>  libavformat/ac4dec.c     | 104 +++++++++++++++++++++++++++++++++++++++
>>>  libavformat/allformats.c |   1 +
>>>  3 files changed, 106 insertions(+)
>>>  create mode 100644 libavformat/ac4dec.c
>>>
>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>>> index e0681058a2..b4e8d20e65 100644
>>> --- a/libavformat/Makefile
>>> +++ b/libavformat/Makefile
>>> @@ -70,6 +70,7 @@ OBJS-$(CONFIG_AA_DEMUXER)                += aadec.o
>>>  OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o img2.o
>>> rawdec.o
>>>  OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
>>>  OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
>>> +OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
>>>  OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
>>>  OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
>>>  OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
>>> diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
>>> new file mode 100644
>>> index 0000000000..8c6e539409
>>> --- /dev/null
>>> +++ b/libavformat/ac4dec.c
>>> @@ -0,0 +1,104 @@
>>> +/*
>>> + * RAW AC-4 demuxer
>>> + * Copyright (c) 2019 Paul B Mahol
>>> + *
>>> + * 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 "libavutil/avassert.h"
>>> +#include "libavutil/crc.h"
>>> +#include "avformat.h"
>>> +#include "rawdec.h"
>>> +
>>> +static int ac4_probe(const AVProbeData *p)
>>> +{
>>> +    const uint8_t *buf = p->buf;
>>> +    int left = p->buf_size;
>>> +    int max_frames = 0;
>>> +
>>> +    while (left > 7) {
>>> +        int size;
>>> +
>>> +        if (buf[0] == 0xAC &&
>>> +            (buf[1] == 0x40 ||
>>> +             buf[1] == 0x41)) {
>>> +            size = (buf[2] << 8) | buf[3];
>>> +            if (size == 0xFFFF)
>>> +                size = 3 + (buf[4] << 16) | (buf[5] << 8) | buf[6];
>>> +            size += 4;
>>> +            if (buf[1] == 0x41)
>>> +                size += 2;
>>> +            max_frames++;
>>> +            left -= size;
>>> +            buf += size;
>>> +        } else {
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
>>> +}
>>> +
>>> +static int ac4_read_header(AVFormatContext *s)
>>> +{
>>> +    AVStream *st;
>>> +
>>> +    st = avformat_new_stream(s, NULL);
>>> +    if (!st)
>>> +        return AVERROR(ENOMEM);
>>> +
>>> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>>> +    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
>>> +{
>>> +    AVIOContext *pb = s->pb;
>>> +    int64_t pos;
>>> +    uint16_t sync;
>>> +    int ret, size;
>>> +
>>> +    if (avio_feof(s->pb))
>>> +        return AVERROR_EOF;
>>> +
>>> +    pos   = avio_tell(s->pb);
>>> +    sync = avio_rb16(pb);
>>
>> If there are sync codes then it sounds like the proper thing to do is,
>> much like with AC3, writing a trivial parser to assemble frames and then
>> use ff_raw_audio_read_header() and ff_raw_read_partial_packet() here
>> instead of custom functions.
> 
> That is over complication for simple parsing like here.
> Every raw packet have exact frame size set in bitstream.

So does AC3, judging by how its parser assembles frames.

An AVParser will let you resync after a bad seek, read frames in non
seekable input like a pipe, read frames within badly muxed files,
simplify the demuxer, etc, and is a matter of just looking for that
16bit sync code and assembling a frame. Essentially just re-implementing
what you already did in ac4_probe().

> 
>>
>>> +    size = avio_rb16(pb);
>>> +    if (size == 0xffff)
>>> +        size = avio_rb24(pb);
>>> +
>>> +    ret = av_get_packet(pb, pkt, size);
>>> +    pkt->pos = pos;
>>> +    pkt->stream_index = 0;
>>> +
>>> +    if (sync == 0xAC41)
>>> +        avio_skip(pb, 2);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +AVInputFormat ff_ac4_demuxer = {
>>> +    .name           = "ac4",
>>> +    .long_name      = NULL_IF_CONFIG_SMALL("raw AC-4"),
>>> +    .read_probe     = ac4_probe,
>>> +    .read_header    = ac4_read_header,
>>> +    .read_packet    = ac4_read_packet,
>>> +    .flags          = AVFMT_GENERIC_INDEX,
>>> +    .extensions     = "ac4",
>>> +};
>>> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
>>> index 0209bf0e30..d2afcb2bdd 100644
>>> --- a/libavformat/allformats.c
>>> +++ b/libavformat/allformats.c
>>> @@ -33,6 +33,7 @@ extern AVInputFormat  ff_aa_demuxer;
>>>  extern AVInputFormat  ff_aac_demuxer;
>>>  extern AVInputFormat  ff_ac3_demuxer;
>>>  extern AVOutputFormat ff_ac3_muxer;
>>> +extern AVInputFormat  ff_ac4_demuxer;
>>>  extern AVInputFormat  ff_acm_demuxer;
>>>  extern AVInputFormat  ff_act_demuxer;
>>>  extern AVInputFormat  ff_adf_demuxer;
>>>
>>
>> _______________________________________________
>> 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".
> _______________________________________________
> 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".
>
Andreas Rheinhardt March 4, 2020, 11:15 p.m. UTC | #4
Am 05.03.2020 um 00:05 schrieb James Almer:
> On 3/4/2020 7:51 PM, Paul B Mahol wrote:
>> On 3/4/20, James Almer <jamrial@gmail.com> wrote:
>>> On 3/4/2020 7:26 PM, Paul B Mahol wrote:
>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>> ---
>>>>   libavformat/Makefile     |   1 +
>>>>   libavformat/ac4dec.c     | 104 +++++++++++++++++++++++++++++++++++++++
>>>>   libavformat/allformats.c |   1 +
>>>>   3 files changed, 106 insertions(+)
>>>>   create mode 100644 libavformat/ac4dec.c
>>>>
>>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>>>> index e0681058a2..b4e8d20e65 100644
>>>> --- a/libavformat/Makefile
>>>> +++ b/libavformat/Makefile
>>>> @@ -70,6 +70,7 @@ OBJS-$(CONFIG_AA_DEMUXER)                += aadec.o
>>>>   OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o img2.o
>>>> rawdec.o
>>>>   OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
>>>>   OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
>>>> +OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
>>>>   OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
>>>>   OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
>>>>   OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
>>>> diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
>>>> new file mode 100644
>>>> index 0000000000..8c6e539409
>>>> --- /dev/null
>>>> +++ b/libavformat/ac4dec.c
>>>> @@ -0,0 +1,104 @@
>>>> +/*
>>>> + * RAW AC-4 demuxer
>>>> + * Copyright (c) 2019 Paul B Mahol
>>>> + *
>>>> + * 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 "libavutil/avassert.h"
>>>> +#include "libavutil/crc.h"
>>>> +#include "avformat.h"
>>>> +#include "rawdec.h"
>>>> +
>>>> +static int ac4_probe(const AVProbeData *p)
>>>> +{
>>>> +    const uint8_t *buf = p->buf;
>>>> +    int left = p->buf_size;
>>>> +    int max_frames = 0;
>>>> +
>>>> +    while (left > 7) {
>>>> +        int size;
>>>> +
>>>> +        if (buf[0] == 0xAC &&
>>>> +            (buf[1] == 0x40 ||
>>>> +             buf[1] == 0x41)) {
>>>> +            size = (buf[2] << 8) | buf[3];
>>>> +            if (size == 0xFFFF)
>>>> +                size = 3 + (buf[4] << 16) | (buf[5] << 8) | buf[6];
>>>> +            size += 4;
>>>> +            if (buf[1] == 0x41)
>>>> +                size += 2;
>>>> +            max_frames++;
>>>> +            left -= size;
>>>> +            buf += size;
>>>> +        } else {
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
>>>> +}
>>>> +
>>>> +static int ac4_read_header(AVFormatContext *s)
>>>> +{
>>>> +    AVStream *st;
>>>> +
>>>> +    st = avformat_new_stream(s, NULL);
>>>> +    if (!st)
>>>> +        return AVERROR(ENOMEM);
>>>> +
>>>> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>>>> +    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
>>>> +{
>>>> +    AVIOContext *pb = s->pb;
>>>> +    int64_t pos;
>>>> +    uint16_t sync;
>>>> +    int ret, size;
>>>> +
>>>> +    if (avio_feof(s->pb))
>>>> +        return AVERROR_EOF;
>>>> +
>>>> +    pos   = avio_tell(s->pb);
>>>> +    sync = avio_rb16(pb);
>>>
>>> If there are sync codes then it sounds like the proper thing to do is,
>>> much like with AC3, writing a trivial parser to assemble frames and then
>>> use ff_raw_audio_read_header() and ff_raw_read_partial_packet() here
>>> instead of custom functions.
>>
>> That is over complication for simple parsing like here.
>> Every raw packet have exact frame size set in bitstream.
> 
> So does AC3, judging by how its parser assembles frames.
> 
> An AVParser will let you resync after a bad seek, read frames in non
> seekable input like a pipe, read frames within badly muxed files,
> simplify the demuxer, etc, and is a matter of just looking for that
> 16bit sync code and assembling a frame. Essentially just re-implementing
> what you already did in ac4_probe().
> 

If it is intended that the actual AVPackets should not contain the 
sync code and the size field at all, then this should be dropped in 
the demuxer here (but then the demuxer actually needs to check for 
sync codes and needs to handle resyncing itself). This will also mean 
that memcpy of the data can be avoided; which is not so when relying 
on a parser as the packet size is not chosen adaptively for the content.

The decoder seems to allow both variants: With header and without 
header. Is there a packetization that strips this header away? (And 
what is actually contained in the last two bytes in case the sync code 
is 0xAC41?)

- Andreas
Paul B Mahol March 5, 2020, 10:27 a.m. UTC | #5
On 3/5/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> Am 05.03.2020 um 00:05 schrieb James Almer:
>> On 3/4/2020 7:51 PM, Paul B Mahol wrote:
>>> On 3/4/20, James Almer <jamrial@gmail.com> wrote:
>>>> On 3/4/2020 7:26 PM, Paul B Mahol wrote:
>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>>> ---
>>>>>   libavformat/Makefile     |   1 +
>>>>>   libavformat/ac4dec.c     | 104
>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>   libavformat/allformats.c |   1 +
>>>>>   3 files changed, 106 insertions(+)
>>>>>   create mode 100644 libavformat/ac4dec.c
>>>>>
>>>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>>>>> index e0681058a2..b4e8d20e65 100644
>>>>> --- a/libavformat/Makefile
>>>>> +++ b/libavformat/Makefile
>>>>> @@ -70,6 +70,7 @@ OBJS-$(CONFIG_AA_DEMUXER)                += aadec.o
>>>>>   OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o img2.o
>>>>> rawdec.o
>>>>>   OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
>>>>>   OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
>>>>> +OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
>>>>>   OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
>>>>>   OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
>>>>>   OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
>>>>> diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
>>>>> new file mode 100644
>>>>> index 0000000000..8c6e539409
>>>>> --- /dev/null
>>>>> +++ b/libavformat/ac4dec.c
>>>>> @@ -0,0 +1,104 @@
>>>>> +/*
>>>>> + * RAW AC-4 demuxer
>>>>> + * Copyright (c) 2019 Paul B Mahol
>>>>> + *
>>>>> + * 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 "libavutil/avassert.h"
>>>>> +#include "libavutil/crc.h"
>>>>> +#include "avformat.h"
>>>>> +#include "rawdec.h"
>>>>> +
>>>>> +static int ac4_probe(const AVProbeData *p)
>>>>> +{
>>>>> +    const uint8_t *buf = p->buf;
>>>>> +    int left = p->buf_size;
>>>>> +    int max_frames = 0;
>>>>> +
>>>>> +    while (left > 7) {
>>>>> +        int size;
>>>>> +
>>>>> +        if (buf[0] == 0xAC &&
>>>>> +            (buf[1] == 0x40 ||
>>>>> +             buf[1] == 0x41)) {
>>>>> +            size = (buf[2] << 8) | buf[3];
>>>>> +            if (size == 0xFFFF)
>>>>> +                size = 3 + (buf[4] << 16) | (buf[5] << 8) | buf[6];
>>>>> +            size += 4;
>>>>> +            if (buf[1] == 0x41)
>>>>> +                size += 2;
>>>>> +            max_frames++;
>>>>> +            left -= size;
>>>>> +            buf += size;
>>>>> +        } else {
>>>>> +            break;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
>>>>> +}
>>>>> +
>>>>> +static int ac4_read_header(AVFormatContext *s)
>>>>> +{
>>>>> +    AVStream *st;
>>>>> +
>>>>> +    st = avformat_new_stream(s, NULL);
>>>>> +    if (!st)
>>>>> +        return AVERROR(ENOMEM);
>>>>> +
>>>>> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>>>>> +    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
>>>>> +{
>>>>> +    AVIOContext *pb = s->pb;
>>>>> +    int64_t pos;
>>>>> +    uint16_t sync;
>>>>> +    int ret, size;
>>>>> +
>>>>> +    if (avio_feof(s->pb))
>>>>> +        return AVERROR_EOF;
>>>>> +
>>>>> +    pos   = avio_tell(s->pb);
>>>>> +    sync = avio_rb16(pb);
>>>>
>>>> If there are sync codes then it sounds like the proper thing to do is,
>>>> much like with AC3, writing a trivial parser to assemble frames and then
>>>> use ff_raw_audio_read_header() and ff_raw_read_partial_packet() here
>>>> instead of custom functions.
>>>
>>> That is over complication for simple parsing like here.
>>> Every raw packet have exact frame size set in bitstream.
>>
>> So does AC3, judging by how its parser assembles frames.
>>
>> An AVParser will let you resync after a bad seek, read frames in non
>> seekable input like a pipe, read frames within badly muxed files,
>> simplify the demuxer, etc, and is a matter of just looking for that
>> 16bit sync code and assembling a frame. Essentially just re-implementing
>> what you already did in ac4_probe().
>>
>
> If it is intended that the actual AVPackets should not contain the
> sync code and the size field at all, then this should be dropped in
> the demuxer here (but then the demuxer actually needs to check for
> sync codes and needs to handle resyncing itself). This will also mean
> that memcpy of the data can be avoided; which is not so when relying
> on a parser as the packet size is not chosen adaptively for the content.
>
> The decoder seems to allow both variants: With header and without
> header. Is there a packetization that strips this header away? (And
> what is actually contained in the last two bytes in case the sync code
> is 0xAC41?)

CRC word.

>
> - 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".
Paul B Mahol March 5, 2020, 10:28 a.m. UTC | #6
On 3/5/20, James Almer <jamrial@gmail.com> wrote:
> On 3/4/2020 7:51 PM, Paul B Mahol wrote:
>> On 3/4/20, James Almer <jamrial@gmail.com> wrote:
>>> On 3/4/2020 7:26 PM, Paul B Mahol wrote:
>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>> ---
>>>>  libavformat/Makefile     |   1 +
>>>>  libavformat/ac4dec.c     | 104 +++++++++++++++++++++++++++++++++++++++
>>>>  libavformat/allformats.c |   1 +
>>>>  3 files changed, 106 insertions(+)
>>>>  create mode 100644 libavformat/ac4dec.c
>>>>
>>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>>>> index e0681058a2..b4e8d20e65 100644
>>>> --- a/libavformat/Makefile
>>>> +++ b/libavformat/Makefile
>>>> @@ -70,6 +70,7 @@ OBJS-$(CONFIG_AA_DEMUXER)                += aadec.o
>>>>  OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o img2.o
>>>> rawdec.o
>>>>  OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
>>>>  OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
>>>> +OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
>>>>  OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
>>>>  OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
>>>>  OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
>>>> diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
>>>> new file mode 100644
>>>> index 0000000000..8c6e539409
>>>> --- /dev/null
>>>> +++ b/libavformat/ac4dec.c
>>>> @@ -0,0 +1,104 @@
>>>> +/*
>>>> + * RAW AC-4 demuxer
>>>> + * Copyright (c) 2019 Paul B Mahol
>>>> + *
>>>> + * 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 "libavutil/avassert.h"
>>>> +#include "libavutil/crc.h"
>>>> +#include "avformat.h"
>>>> +#include "rawdec.h"
>>>> +
>>>> +static int ac4_probe(const AVProbeData *p)
>>>> +{
>>>> +    const uint8_t *buf = p->buf;
>>>> +    int left = p->buf_size;
>>>> +    int max_frames = 0;
>>>> +
>>>> +    while (left > 7) {
>>>> +        int size;
>>>> +
>>>> +        if (buf[0] == 0xAC &&
>>>> +            (buf[1] == 0x40 ||
>>>> +             buf[1] == 0x41)) {
>>>> +            size = (buf[2] << 8) | buf[3];
>>>> +            if (size == 0xFFFF)
>>>> +                size = 3 + (buf[4] << 16) | (buf[5] << 8) | buf[6];
>>>> +            size += 4;
>>>> +            if (buf[1] == 0x41)
>>>> +                size += 2;
>>>> +            max_frames++;
>>>> +            left -= size;
>>>> +            buf += size;
>>>> +        } else {
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
>>>> +}
>>>> +
>>>> +static int ac4_read_header(AVFormatContext *s)
>>>> +{
>>>> +    AVStream *st;
>>>> +
>>>> +    st = avformat_new_stream(s, NULL);
>>>> +    if (!st)
>>>> +        return AVERROR(ENOMEM);
>>>> +
>>>> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>>>> +    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
>>>> +{
>>>> +    AVIOContext *pb = s->pb;
>>>> +    int64_t pos;
>>>> +    uint16_t sync;
>>>> +    int ret, size;
>>>> +
>>>> +    if (avio_feof(s->pb))
>>>> +        return AVERROR_EOF;
>>>> +
>>>> +    pos   = avio_tell(s->pb);
>>>> +    sync = avio_rb16(pb);
>>>
>>> If there are sync codes then it sounds like the proper thing to do is,
>>> much like with AC3, writing a trivial parser to assemble frames and then
>>> use ff_raw_audio_read_header() and ff_raw_read_partial_packet() here
>>> instead of custom functions.
>>
>> That is over complication for simple parsing like here.
>> Every raw packet have exact frame size set in bitstream.
>
> So does AC3, judging by how its parser assembles frames.
>
> An AVParser will let you resync after a bad seek, read frames in non
> seekable input like a pipe, read frames within badly muxed files,
> simplify the demuxer, etc, and is a matter of just looking for that
> 16bit sync code and assembling a frame. Essentially just re-implementing
> what you already did in ac4_probe().

Disagree, it is over complication for zero gain.

>
>>
>>>
>>>> +    size = avio_rb16(pb);
>>>> +    if (size == 0xffff)
>>>> +        size = avio_rb24(pb);
>>>> +
>>>> +    ret = av_get_packet(pb, pkt, size);
>>>> +    pkt->pos = pos;
>>>> +    pkt->stream_index = 0;
>>>> +
>>>> +    if (sync == 0xAC41)
>>>> +        avio_skip(pb, 2);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +AVInputFormat ff_ac4_demuxer = {
>>>> +    .name           = "ac4",
>>>> +    .long_name      = NULL_IF_CONFIG_SMALL("raw AC-4"),
>>>> +    .read_probe     = ac4_probe,
>>>> +    .read_header    = ac4_read_header,
>>>> +    .read_packet    = ac4_read_packet,
>>>> +    .flags          = AVFMT_GENERIC_INDEX,
>>>> +    .extensions     = "ac4",
>>>> +};
>>>> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
>>>> index 0209bf0e30..d2afcb2bdd 100644
>>>> --- a/libavformat/allformats.c
>>>> +++ b/libavformat/allformats.c
>>>> @@ -33,6 +33,7 @@ extern AVInputFormat  ff_aa_demuxer;
>>>>  extern AVInputFormat  ff_aac_demuxer;
>>>>  extern AVInputFormat  ff_ac3_demuxer;
>>>>  extern AVOutputFormat ff_ac3_muxer;
>>>> +extern AVInputFormat  ff_ac4_demuxer;
>>>>  extern AVInputFormat  ff_acm_demuxer;
>>>>  extern AVInputFormat  ff_act_demuxer;
>>>>  extern AVInputFormat  ff_adf_demuxer;
>>>>
>>>
>>> _______________________________________________
>>> 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".
>> _______________________________________________
>> 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".
>>
>
> _______________________________________________
> 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".
Andreas Rheinhardt March 5, 2020, 11:25 a.m. UTC | #7
Paul B Mahol:
> On 3/5/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>> Am 05.03.2020 um 00:05 schrieb James Almer:
>>> On 3/4/2020 7:51 PM, Paul B Mahol wrote:
>>>> On 3/4/20, James Almer <jamrial@gmail.com> wrote:
>>>>> On 3/4/2020 7:26 PM, Paul B Mahol wrote:
>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>>>> ---
>>>>>>   libavformat/Makefile     |   1 +
>>>>>>   libavformat/ac4dec.c     | 104
>>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>>   libavformat/allformats.c |   1 +
>>>>>>   3 files changed, 106 insertions(+)
>>>>>>   create mode 100644 libavformat/ac4dec.c
>>>>>>
>>>>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>>>>>> index e0681058a2..b4e8d20e65 100644
>>>>>> --- a/libavformat/Makefile
>>>>>> +++ b/libavformat/Makefile
>>>>>> @@ -70,6 +70,7 @@ OBJS-$(CONFIG_AA_DEMUXER)                += aadec.o
>>>>>>   OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o img2.o
>>>>>> rawdec.o
>>>>>>   OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
>>>>>>   OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
>>>>>> +OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
>>>>>>   OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
>>>>>>   OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
>>>>>>   OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
>>>>>> diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
>>>>>> new file mode 100644
>>>>>> index 0000000000..8c6e539409
>>>>>> --- /dev/null
>>>>>> +++ b/libavformat/ac4dec.c
>>>>>> @@ -0,0 +1,104 @@
>>>>>> +/*
>>>>>> + * RAW AC-4 demuxer
>>>>>> + * Copyright (c) 2019 Paul B Mahol
>>>>>> + *
>>>>>> + * 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 "libavutil/avassert.h"
>>>>>> +#include "libavutil/crc.h"
>>>>>> +#include "avformat.h"
>>>>>> +#include "rawdec.h"
>>>>>> +
>>>>>> +static int ac4_probe(const AVProbeData *p)
>>>>>> +{
>>>>>> +    const uint8_t *buf = p->buf;
>>>>>> +    int left = p->buf_size;
>>>>>> +    int max_frames = 0;
>>>>>> +
>>>>>> +    while (left > 7) {
>>>>>> +        int size;
>>>>>> +
>>>>>> +        if (buf[0] == 0xAC &&
>>>>>> +            (buf[1] == 0x40 ||
>>>>>> +             buf[1] == 0x41)) {
>>>>>> +            size = (buf[2] << 8) | buf[3];
>>>>>> +            if (size == 0xFFFF)
>>>>>> +                size = 3 + (buf[4] << 16) | (buf[5] << 8) | buf[6];
>>>>>> +            size += 4;
>>>>>> +            if (buf[1] == 0x41)
>>>>>> +                size += 2;
>>>>>> +            max_frames++;
>>>>>> +            left -= size;
>>>>>> +            buf += size;
>>>>>> +        } else {
>>>>>> +            break;
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>> +    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
>>>>>> +}
>>>>>> +
>>>>>> +static int ac4_read_header(AVFormatContext *s)
>>>>>> +{
>>>>>> +    AVStream *st;
>>>>>> +
>>>>>> +    st = avformat_new_stream(s, NULL);
>>>>>> +    if (!st)
>>>>>> +        return AVERROR(ENOMEM);
>>>>>> +
>>>>>> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>>>>>> +    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
>>>>>> +{
>>>>>> +    AVIOContext *pb = s->pb;
>>>>>> +    int64_t pos;
>>>>>> +    uint16_t sync;
>>>>>> +    int ret, size;
>>>>>> +
>>>>>> +    if (avio_feof(s->pb))
>>>>>> +        return AVERROR_EOF;
>>>>>> +
>>>>>> +    pos   = avio_tell(s->pb);
>>>>>> +    sync = avio_rb16(pb);
>>>>>
>>>>> If there are sync codes then it sounds like the proper thing to do is,
>>>>> much like with AC3, writing a trivial parser to assemble frames and then
>>>>> use ff_raw_audio_read_header() and ff_raw_read_partial_packet() here
>>>>> instead of custom functions.
>>>>
>>>> That is over complication for simple parsing like here.
>>>> Every raw packet have exact frame size set in bitstream.
>>>
>>> So does AC3, judging by how its parser assembles frames.
>>>
>>> An AVParser will let you resync after a bad seek, read frames in non
>>> seekable input like a pipe, read frames within badly muxed files,
>>> simplify the demuxer, etc, and is a matter of just looking for that
>>> 16bit sync code and assembling a frame. Essentially just re-implementing
>>> what you already did in ac4_probe().
>>>
>>
>> If it is intended that the actual AVPackets should not contain the
>> sync code and the size field at all, then this should be dropped in
>> the demuxer here (but then the demuxer actually needs to check for
>> sync codes and needs to handle resyncing itself). This will also mean
>> that memcpy of the data can be avoided; which is not so when relying
>> on a parser as the packet size is not chosen adaptively for the content.
>>
>> The decoder seems to allow both variants: With header and without
>> header. Is there a packetization that strips this header away? (And
>> what is actually contained in the last two bytes in case the sync code
>> is 0xAC41?)
> 
> CRC word.
> 
You seem to intend for there to be two forms of AC-4 packets (at least
your decoder tries to support both): The one with syncwords, size
field and CRC stripped away and another one where these elements are
still present. This immediately brings a few questions:

1. Can a stripped packet be mistaken for an unstripped one, i.e. is it
possible for the data after the header to begin with a syncword?
2. How reversible is stripping away the header and the CRC? Stripping
away the CRC is obviously irreversible unless we presume that the CRC
matches. But is stripping the size field away reversible (it is not
reversible if one is allowed to use the three byte size field if the
two byte size field would suffice)?
3. Are there any container that use the stripped version?

If the answer to the third question is no, then I don't think we
should create a new packetization of AC-4 and potentially incur the
first problem as well as the inability to retain CRCs for codec copy.

- Andreas
Paul B Mahol March 5, 2020, 11:53 a.m. UTC | #8
On 3/5/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> Paul B Mahol:
>> On 3/5/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>> Am 05.03.2020 um 00:05 schrieb James Almer:
>>>> On 3/4/2020 7:51 PM, Paul B Mahol wrote:
>>>>> On 3/4/20, James Almer <jamrial@gmail.com> wrote:
>>>>>> On 3/4/2020 7:26 PM, Paul B Mahol wrote:
>>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>>>>> ---
>>>>>>>   libavformat/Makefile     |   1 +
>>>>>>>   libavformat/ac4dec.c     | 104
>>>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>>>   libavformat/allformats.c |   1 +
>>>>>>>   3 files changed, 106 insertions(+)
>>>>>>>   create mode 100644 libavformat/ac4dec.c
>>>>>>>
>>>>>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>>>>>>> index e0681058a2..b4e8d20e65 100644
>>>>>>> --- a/libavformat/Makefile
>>>>>>> +++ b/libavformat/Makefile
>>>>>>> @@ -70,6 +70,7 @@ OBJS-$(CONFIG_AA_DEMUXER)                += aadec.o
>>>>>>>   OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o
>>>>>>> img2.o
>>>>>>> rawdec.o
>>>>>>>   OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
>>>>>>>   OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
>>>>>>> +OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
>>>>>>>   OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
>>>>>>>   OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
>>>>>>>   OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
>>>>>>> diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
>>>>>>> new file mode 100644
>>>>>>> index 0000000000..8c6e539409
>>>>>>> --- /dev/null
>>>>>>> +++ b/libavformat/ac4dec.c
>>>>>>> @@ -0,0 +1,104 @@
>>>>>>> +/*
>>>>>>> + * RAW AC-4 demuxer
>>>>>>> + * Copyright (c) 2019 Paul B Mahol
>>>>>>> + *
>>>>>>> + * 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 "libavutil/avassert.h"
>>>>>>> +#include "libavutil/crc.h"
>>>>>>> +#include "avformat.h"
>>>>>>> +#include "rawdec.h"
>>>>>>> +
>>>>>>> +static int ac4_probe(const AVProbeData *p)
>>>>>>> +{
>>>>>>> +    const uint8_t *buf = p->buf;
>>>>>>> +    int left = p->buf_size;
>>>>>>> +    int max_frames = 0;
>>>>>>> +
>>>>>>> +    while (left > 7) {
>>>>>>> +        int size;
>>>>>>> +
>>>>>>> +        if (buf[0] == 0xAC &&
>>>>>>> +            (buf[1] == 0x40 ||
>>>>>>> +             buf[1] == 0x41)) {
>>>>>>> +            size = (buf[2] << 8) | buf[3];
>>>>>>> +            if (size == 0xFFFF)
>>>>>>> +                size = 3 + (buf[4] << 16) | (buf[5] << 8) | buf[6];
>>>>>>> +            size += 4;
>>>>>>> +            if (buf[1] == 0x41)
>>>>>>> +                size += 2;
>>>>>>> +            max_frames++;
>>>>>>> +            left -= size;
>>>>>>> +            buf += size;
>>>>>>> +        } else {
>>>>>>> +            break;
>>>>>>> +        }
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int ac4_read_header(AVFormatContext *s)
>>>>>>> +{
>>>>>>> +    AVStream *st;
>>>>>>> +
>>>>>>> +    st = avformat_new_stream(s, NULL);
>>>>>>> +    if (!st)
>>>>>>> +        return AVERROR(ENOMEM);
>>>>>>> +
>>>>>>> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>>>>>>> +    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
>>>>>>> +
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
>>>>>>> +{
>>>>>>> +    AVIOContext *pb = s->pb;
>>>>>>> +    int64_t pos;
>>>>>>> +    uint16_t sync;
>>>>>>> +    int ret, size;
>>>>>>> +
>>>>>>> +    if (avio_feof(s->pb))
>>>>>>> +        return AVERROR_EOF;
>>>>>>> +
>>>>>>> +    pos   = avio_tell(s->pb);
>>>>>>> +    sync = avio_rb16(pb);
>>>>>>
>>>>>> If there are sync codes then it sounds like the proper thing to do is,
>>>>>> much like with AC3, writing a trivial parser to assemble frames and
>>>>>> then
>>>>>> use ff_raw_audio_read_header() and ff_raw_read_partial_packet() here
>>>>>> instead of custom functions.
>>>>>
>>>>> That is over complication for simple parsing like here.
>>>>> Every raw packet have exact frame size set in bitstream.
>>>>
>>>> So does AC3, judging by how its parser assembles frames.
>>>>
>>>> An AVParser will let you resync after a bad seek, read frames in non
>>>> seekable input like a pipe, read frames within badly muxed files,
>>>> simplify the demuxer, etc, and is a matter of just looking for that
>>>> 16bit sync code and assembling a frame. Essentially just re-implementing
>>>> what you already did in ac4_probe().
>>>>
>>>
>>> If it is intended that the actual AVPackets should not contain the
>>> sync code and the size field at all, then this should be dropped in
>>> the demuxer here (but then the demuxer actually needs to check for
>>> sync codes and needs to handle resyncing itself). This will also mean
>>> that memcpy of the data can be avoided; which is not so when relying
>>> on a parser as the packet size is not chosen adaptively for the content.
>>>
>>> The decoder seems to allow both variants: With header and without
>>> header. Is there a packetization that strips this header away? (And
>>> what is actually contained in the last two bytes in case the sync code
>>> is 0xAC41?)
>>
>> CRC word.
>>
> You seem to intend for there to be two forms of AC-4 packets (at least
> your decoder tries to support both): The one with syncwords, size
> field and CRC stripped away and another one where these elements are
> still present. This immediately brings a few questions:
>
> 1. Can a stripped packet be mistaken for an unstripped one, i.e. is it
> possible for the data after the header to begin with a syncword?
> 2. How reversible is stripping away the header and the CRC? Stripping
> away the CRC is obviously irreversible unless we presume that the CRC
> matches. But is stripping the size field away reversible (it is not
> reversible if one is allowed to use the three byte size field if the
> two byte size field would suffice)?
> 3. Are there any container that use the stripped version?
>
> If the answer to the third question is no, then I don't think we
> should create a new packetization of AC-4 and potentially incur the
> first problem as well as the inability to retain CRCs for codec copy.

Decoder should support both, but I do not remember of stripped files.
Paul B Mahol March 5, 2020, 11:57 a.m. UTC | #9
On 3/5/20, Paul B Mahol <onemda@gmail.com> wrote:
> On 3/5/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>> Paul B Mahol:
>>> On 3/5/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>>> Am 05.03.2020 um 00:05 schrieb James Almer:
>>>>> On 3/4/2020 7:51 PM, Paul B Mahol wrote:
>>>>>> On 3/4/20, James Almer <jamrial@gmail.com> wrote:
>>>>>>> On 3/4/2020 7:26 PM, Paul B Mahol wrote:
>>>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>>>>>> ---
>>>>>>>>   libavformat/Makefile     |   1 +
>>>>>>>>   libavformat/ac4dec.c     | 104
>>>>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>>>>   libavformat/allformats.c |   1 +
>>>>>>>>   3 files changed, 106 insertions(+)
>>>>>>>>   create mode 100644 libavformat/ac4dec.c
>>>>>>>>
>>>>>>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>>>>>>>> index e0681058a2..b4e8d20e65 100644
>>>>>>>> --- a/libavformat/Makefile
>>>>>>>> +++ b/libavformat/Makefile
>>>>>>>> @@ -70,6 +70,7 @@ OBJS-$(CONFIG_AA_DEMUXER)                +=
>>>>>>>> aadec.o
>>>>>>>>   OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o
>>>>>>>> img2.o
>>>>>>>> rawdec.o
>>>>>>>>   OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
>>>>>>>>   OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
>>>>>>>> +OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
>>>>>>>>   OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
>>>>>>>>   OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
>>>>>>>>   OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
>>>>>>>> diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000000..8c6e539409
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/libavformat/ac4dec.c
>>>>>>>> @@ -0,0 +1,104 @@
>>>>>>>> +/*
>>>>>>>> + * RAW AC-4 demuxer
>>>>>>>> + * Copyright (c) 2019 Paul B Mahol
>>>>>>>> + *
>>>>>>>> + * 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 "libavutil/avassert.h"
>>>>>>>> +#include "libavutil/crc.h"
>>>>>>>> +#include "avformat.h"
>>>>>>>> +#include "rawdec.h"
>>>>>>>> +
>>>>>>>> +static int ac4_probe(const AVProbeData *p)
>>>>>>>> +{
>>>>>>>> +    const uint8_t *buf = p->buf;
>>>>>>>> +    int left = p->buf_size;
>>>>>>>> +    int max_frames = 0;
>>>>>>>> +
>>>>>>>> +    while (left > 7) {
>>>>>>>> +        int size;
>>>>>>>> +
>>>>>>>> +        if (buf[0] == 0xAC &&
>>>>>>>> +            (buf[1] == 0x40 ||
>>>>>>>> +             buf[1] == 0x41)) {
>>>>>>>> +            size = (buf[2] << 8) | buf[3];
>>>>>>>> +            if (size == 0xFFFF)
>>>>>>>> +                size = 3 + (buf[4] << 16) | (buf[5] << 8) |
>>>>>>>> buf[6];
>>>>>>>> +            size += 4;
>>>>>>>> +            if (buf[1] == 0x41)
>>>>>>>> +                size += 2;
>>>>>>>> +            max_frames++;
>>>>>>>> +            left -= size;
>>>>>>>> +            buf += size;
>>>>>>>> +        } else {
>>>>>>>> +            break;
>>>>>>>> +        }
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int ac4_read_header(AVFormatContext *s)
>>>>>>>> +{
>>>>>>>> +    AVStream *st;
>>>>>>>> +
>>>>>>>> +    st = avformat_new_stream(s, NULL);
>>>>>>>> +    if (!st)
>>>>>>>> +        return AVERROR(ENOMEM);
>>>>>>>> +
>>>>>>>> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>>>>>>>> +    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
>>>>>>>> +
>>>>>>>> +    return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
>>>>>>>> +{
>>>>>>>> +    AVIOContext *pb = s->pb;
>>>>>>>> +    int64_t pos;
>>>>>>>> +    uint16_t sync;
>>>>>>>> +    int ret, size;
>>>>>>>> +
>>>>>>>> +    if (avio_feof(s->pb))
>>>>>>>> +        return AVERROR_EOF;
>>>>>>>> +
>>>>>>>> +    pos   = avio_tell(s->pb);
>>>>>>>> +    sync = avio_rb16(pb);
>>>>>>>
>>>>>>> If there are sync codes then it sounds like the proper thing to do
>>>>>>> is,
>>>>>>> much like with AC3, writing a trivial parser to assemble frames and
>>>>>>> then
>>>>>>> use ff_raw_audio_read_header() and ff_raw_read_partial_packet() here
>>>>>>> instead of custom functions.
>>>>>>
>>>>>> That is over complication for simple parsing like here.
>>>>>> Every raw packet have exact frame size set in bitstream.
>>>>>
>>>>> So does AC3, judging by how its parser assembles frames.
>>>>>
>>>>> An AVParser will let you resync after a bad seek, read frames in non
>>>>> seekable input like a pipe, read frames within badly muxed files,
>>>>> simplify the demuxer, etc, and is a matter of just looking for that
>>>>> 16bit sync code and assembling a frame. Essentially just
>>>>> re-implementing
>>>>> what you already did in ac4_probe().
>>>>>
>>>>
>>>> If it is intended that the actual AVPackets should not contain the
>>>> sync code and the size field at all, then this should be dropped in
>>>> the demuxer here (but then the demuxer actually needs to check for
>>>> sync codes and needs to handle resyncing itself). This will also mean
>>>> that memcpy of the data can be avoided; which is not so when relying
>>>> on a parser as the packet size is not chosen adaptively for the
>>>> content.
>>>>
>>>> The decoder seems to allow both variants: With header and without
>>>> header. Is there a packetization that strips this header away? (And
>>>> what is actually contained in the last two bytes in case the sync code
>>>> is 0xAC41?)
>>>
>>> CRC word.
>>>
>> You seem to intend for there to be two forms of AC-4 packets (at least
>> your decoder tries to support both): The one with syncwords, size
>> field and CRC stripped away and another one where these elements are
>> still present. This immediately brings a few questions:
>>
>> 1. Can a stripped packet be mistaken for an unstripped one, i.e. is it
>> possible for the data after the header to begin with a syncword?
>> 2. How reversible is stripping away the header and the CRC? Stripping
>> away the CRC is obviously irreversible unless we presume that the CRC
>> matches. But is stripping the size field away reversible (it is not
>> reversible if one is allowed to use the three byte size field if the
>> two byte size field would suffice)?
>> 3. Are there any container that use the stripped version?
>>
>> If the answer to the third question is no, then I don't think we
>> should create a new packetization of AC-4 and potentially incur the
>> first problem as well as the inability to retain CRCs for codec copy.
>
> Decoder should support both, but I do not remember of stripped files.
>

Actually mp4 dash files are stripped one.
Lynne March 5, 2020, 12:34 p.m. UTC | #10
Mar 5, 2020, 11:57 by onemda@gmail.com:

> On 3/5/20, Paul B Mahol <onemda@gmail.com> wrote:
>
>> On 3/5/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>
>>> Paul B Mahol:
>>>
>>>> On 3/5/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>>>
>>>>> Am 05.03.2020 um 00:05 schrieb James Almer:
>>>>>
>>>>>> On 3/4/2020 7:51 PM, Paul B Mahol wrote:
>>>>>>
>>>>>>> On 3/4/20, James Almer <jamrial@gmail.com> wrote:
>>>>>>>
>>>>>>>> On 3/4/2020 7:26 PM, Paul B Mahol wrote:
>>>>>>>>
>>>>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>>>>>>> ---
>>>>>>>>>  libavformat/Makefile     |   1 +
>>>>>>>>>  libavformat/ac4dec.c     | 104
>>>>>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>>>>>  libavformat/allformats.c |   1 +
>>>>>>>>>  3 files changed, 106 insertions(+)
>>>>>>>>>  create mode 100644 libavformat/ac4dec.c
>>>>>>>>>
>>>>>>>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>>>>>>>>> index e0681058a2..b4e8d20e65 100644
>>>>>>>>> --- a/libavformat/Makefile
>>>>>>>>> +++ b/libavformat/Makefile
>>>>>>>>> @@ -70,6 +70,7 @@ OBJS-$(CONFIG_AA_DEMUXER)                +=
>>>>>>>>> aadec.o
>>>>>>>>>  OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o
>>>>>>>>> img2.o
>>>>>>>>> rawdec.o
>>>>>>>>>  OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
>>>>>>>>>  OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
>>>>>>>>> +OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
>>>>>>>>>  OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
>>>>>>>>>  OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
>>>>>>>>>  OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
>>>>>>>>> diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000000..8c6e539409
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/libavformat/ac4dec.c
>>>>>>>>> @@ -0,0 +1,104 @@
>>>>>>>>> +/*
>>>>>>>>> + * RAW AC-4 demuxer
>>>>>>>>> + * Copyright (c) 2019 Paul B Mahol
>>>>>>>>> + *
>>>>>>>>> + * 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 "libavutil/avassert.h"
>>>>>>>>> +#include "libavutil/crc.h"
>>>>>>>>> +#include "avformat.h"
>>>>>>>>> +#include "rawdec.h"
>>>>>>>>> +
>>>>>>>>> +static int ac4_probe(const AVProbeData *p)
>>>>>>>>> +{
>>>>>>>>> +    const uint8_t *buf = p->buf;
>>>>>>>>> +    int left = p->buf_size;
>>>>>>>>> +    int max_frames = 0;
>>>>>>>>> +
>>>>>>>>> +    while (left > 7) {
>>>>>>>>> +        int size;
>>>>>>>>> +
>>>>>>>>> +        if (buf[0] == 0xAC &&
>>>>>>>>> +            (buf[1] == 0x40 ||
>>>>>>>>> +             buf[1] == 0x41)) {
>>>>>>>>> +            size = (buf[2] << 8) | buf[3];
>>>>>>>>> +            if (size == 0xFFFF)
>>>>>>>>> +                size = 3 + (buf[4] << 16) | (buf[5] << 8) |
>>>>>>>>> buf[6];
>>>>>>>>> +            size += 4;
>>>>>>>>> +            if (buf[1] == 0x41)
>>>>>>>>> +                size += 2;
>>>>>>>>> +            max_frames++;
>>>>>>>>> +            left -= size;
>>>>>>>>> +            buf += size;
>>>>>>>>> +        } else {
>>>>>>>>> +            break;
>>>>>>>>> +        }
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static int ac4_read_header(AVFormatContext *s)
>>>>>>>>> +{
>>>>>>>>> +    AVStream *st;
>>>>>>>>> +
>>>>>>>>> +    st = avformat_new_stream(s, NULL);
>>>>>>>>> +    if (!st)
>>>>>>>>> +        return AVERROR(ENOMEM);
>>>>>>>>> +
>>>>>>>>> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>>>>>>>>> +    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
>>>>>>>>> +
>>>>>>>>> +    return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
>>>>>>>>> +{
>>>>>>>>> +    AVIOContext *pb = s->pb;
>>>>>>>>> +    int64_t pos;
>>>>>>>>> +    uint16_t sync;
>>>>>>>>> +    int ret, size;
>>>>>>>>> +
>>>>>>>>> +    if (avio_feof(s->pb))
>>>>>>>>> +        return AVERROR_EOF;
>>>>>>>>> +
>>>>>>>>> +    pos   = avio_tell(s->pb);
>>>>>>>>> +    sync = avio_rb16(pb);
>>>>>>>>>
>>>>>>>>
>>>>>>>> If there are sync codes then it sounds like the proper thing to do
>>>>>>>> is,
>>>>>>>> much like with AC3, writing a trivial parser to assemble frames and
>>>>>>>> then
>>>>>>>> use ff_raw_audio_read_header() and ff_raw_read_partial_packet() here
>>>>>>>> instead of custom functions.
>>>>>>>>
>>>>>>>
>>>>>>> That is over complication for simple parsing like here.
>>>>>>> Every raw packet have exact frame size set in bitstream.
>>>>>>>
>>>>>>
>>>>>> So does AC3, judging by how its parser assembles frames.
>>>>>>
>>>>>> An AVParser will let you resync after a bad seek, read frames in non
>>>>>> seekable input like a pipe, read frames within badly muxed files,
>>>>>> simplify the demuxer, etc, and is a matter of just looking for that
>>>>>> 16bit sync code and assembling a frame. Essentially just
>>>>>> re-implementing
>>>>>> what you already did in ac4_probe().
>>>>>>
>>>>>
>>>>> If it is intended that the actual AVPackets should not contain the
>>>>> sync code and the size field at all, then this should be dropped in
>>>>> the demuxer here (but then the demuxer actually needs to check for
>>>>> sync codes and needs to handle resyncing itself). This will also mean
>>>>> that memcpy of the data can be avoided; which is not so when relying
>>>>> on a parser as the packet size is not chosen adaptively for the
>>>>> content.
>>>>>
>>>>> The decoder seems to allow both variants: With header and without
>>>>> header. Is there a packetization that strips this header away? (And
>>>>> what is actually contained in the last two bytes in case the sync code
>>>>> is 0xAC41?)
>>>>>
>>>>
>>>> CRC word.
>>>>
>>> You seem to intend for there to be two forms of AC-4 packets (at least
>>> your decoder tries to support both): The one with syncwords, size
>>> field and CRC stripped away and another one where these elements are
>>> still present. This immediately brings a few questions:
>>>
>>> 1. Can a stripped packet be mistaken for an unstripped one, i.e. is it
>>> possible for the data after the header to begin with a syncword?
>>> 2. How reversible is stripping away the header and the CRC? Stripping
>>> away the CRC is obviously irreversible unless we presume that the CRC
>>> matches. But is stripping the size field away reversible (it is not
>>> reversible if one is allowed to use the three byte size field if the
>>> two byte size field would suffice)?
>>> 3. Are there any container that use the stripped version?
>>>
>>> If the answer to the third question is no, then I don't think we
>>> should create a new packetization of AC-4 and potentially incur the
>>> first problem as well as the inability to retain CRCs for codec copy.
>>>
>>
>> Decoder should support both, but I do not remember of stripped files.
>>
>
> Actually mp4 dash files are stripped one.
>

I think the raw demuxer should strip the CRCs, that's a container-level thing IMO.
The decoder should only support one version of packets.
Paul B Mahol March 5, 2020, 12:42 p.m. UTC | #11
On 3/5/20, Lynne <dev@lynne.ee> wrote:
> Mar 5, 2020, 11:57 by onemda@gmail.com:
>
>> On 3/5/20, Paul B Mahol <onemda@gmail.com> wrote:
>>
>>> On 3/5/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>>
>>>> Paul B Mahol:
>>>>
>>>>> On 3/5/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>>>>
>>>>>> Am 05.03.2020 um 00:05 schrieb James Almer:
>>>>>>
>>>>>>> On 3/4/2020 7:51 PM, Paul B Mahol wrote:
>>>>>>>
>>>>>>>> On 3/4/20, James Almer <jamrial@gmail.com> wrote:
>>>>>>>>
>>>>>>>>> On 3/4/2020 7:26 PM, Paul B Mahol wrote:
>>>>>>>>>
>>>>>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>>  libavformat/Makefile     |   1 +
>>>>>>>>>>  libavformat/ac4dec.c     | 104
>>>>>>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>  libavformat/allformats.c |   1 +
>>>>>>>>>>  3 files changed, 106 insertions(+)
>>>>>>>>>>  create mode 100644 libavformat/ac4dec.c
>>>>>>>>>>
>>>>>>>>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>>>>>>>>>> index e0681058a2..b4e8d20e65 100644
>>>>>>>>>> --- a/libavformat/Makefile
>>>>>>>>>> +++ b/libavformat/Makefile
>>>>>>>>>> @@ -70,6 +70,7 @@ OBJS-$(CONFIG_AA_DEMUXER)                +=
>>>>>>>>>> aadec.o
>>>>>>>>>>  OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o
>>>>>>>>>> img2.o
>>>>>>>>>> rawdec.o
>>>>>>>>>>  OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
>>>>>>>>>>  OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
>>>>>>>>>> +OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
>>>>>>>>>>  OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
>>>>>>>>>>  OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
>>>>>>>>>>  OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
>>>>>>>>>> diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 0000000000..8c6e539409
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/libavformat/ac4dec.c
>>>>>>>>>> @@ -0,0 +1,104 @@
>>>>>>>>>> +/*
>>>>>>>>>> + * RAW AC-4 demuxer
>>>>>>>>>> + * Copyright (c) 2019 Paul B Mahol
>>>>>>>>>> + *
>>>>>>>>>> + * 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 "libavutil/avassert.h"
>>>>>>>>>> +#include "libavutil/crc.h"
>>>>>>>>>> +#include "avformat.h"
>>>>>>>>>> +#include "rawdec.h"
>>>>>>>>>> +
>>>>>>>>>> +static int ac4_probe(const AVProbeData *p)
>>>>>>>>>> +{
>>>>>>>>>> +    const uint8_t *buf = p->buf;
>>>>>>>>>> +    int left = p->buf_size;
>>>>>>>>>> +    int max_frames = 0;
>>>>>>>>>> +
>>>>>>>>>> +    while (left > 7) {
>>>>>>>>>> +        int size;
>>>>>>>>>> +
>>>>>>>>>> +        if (buf[0] == 0xAC &&
>>>>>>>>>> +            (buf[1] == 0x40 ||
>>>>>>>>>> +             buf[1] == 0x41)) {
>>>>>>>>>> +            size = (buf[2] << 8) | buf[3];
>>>>>>>>>> +            if (size == 0xFFFF)
>>>>>>>>>> +                size = 3 + (buf[4] << 16) | (buf[5] << 8) |
>>>>>>>>>> buf[6];
>>>>>>>>>> +            size += 4;
>>>>>>>>>> +            if (buf[1] == 0x41)
>>>>>>>>>> +                size += 2;
>>>>>>>>>> +            max_frames++;
>>>>>>>>>> +            left -= size;
>>>>>>>>>> +            buf += size;
>>>>>>>>>> +        } else {
>>>>>>>>>> +            break;
>>>>>>>>>> +        }
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int ac4_read_header(AVFormatContext *s)
>>>>>>>>>> +{
>>>>>>>>>> +    AVStream *st;
>>>>>>>>>> +
>>>>>>>>>> +    st = avformat_new_stream(s, NULL);
>>>>>>>>>> +    if (!st)
>>>>>>>>>> +        return AVERROR(ENOMEM);
>>>>>>>>>> +
>>>>>>>>>> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>>>>>>>>>> +    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
>>>>>>>>>> +
>>>>>>>>>> +    return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
>>>>>>>>>> +{
>>>>>>>>>> +    AVIOContext *pb = s->pb;
>>>>>>>>>> +    int64_t pos;
>>>>>>>>>> +    uint16_t sync;
>>>>>>>>>> +    int ret, size;
>>>>>>>>>> +
>>>>>>>>>> +    if (avio_feof(s->pb))
>>>>>>>>>> +        return AVERROR_EOF;
>>>>>>>>>> +
>>>>>>>>>> +    pos   = avio_tell(s->pb);
>>>>>>>>>> +    sync = avio_rb16(pb);
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If there are sync codes then it sounds like the proper thing to do
>>>>>>>>> is,
>>>>>>>>> much like with AC3, writing a trivial parser to assemble frames and
>>>>>>>>> then
>>>>>>>>> use ff_raw_audio_read_header() and ff_raw_read_partial_packet()
>>>>>>>>> here
>>>>>>>>> instead of custom functions.
>>>>>>>>>
>>>>>>>>
>>>>>>>> That is over complication for simple parsing like here.
>>>>>>>> Every raw packet have exact frame size set in bitstream.
>>>>>>>>
>>>>>>>
>>>>>>> So does AC3, judging by how its parser assembles frames.
>>>>>>>
>>>>>>> An AVParser will let you resync after a bad seek, read frames in non
>>>>>>> seekable input like a pipe, read frames within badly muxed files,
>>>>>>> simplify the demuxer, etc, and is a matter of just looking for that
>>>>>>> 16bit sync code and assembling a frame. Essentially just
>>>>>>> re-implementing
>>>>>>> what you already did in ac4_probe().
>>>>>>>
>>>>>>
>>>>>> If it is intended that the actual AVPackets should not contain the
>>>>>> sync code and the size field at all, then this should be dropped in
>>>>>> the demuxer here (but then the demuxer actually needs to check for
>>>>>> sync codes and needs to handle resyncing itself). This will also mean
>>>>>> that memcpy of the data can be avoided; which is not so when relying
>>>>>> on a parser as the packet size is not chosen adaptively for the
>>>>>> content.
>>>>>>
>>>>>> The decoder seems to allow both variants: With header and without
>>>>>> header. Is there a packetization that strips this header away? (And
>>>>>> what is actually contained in the last two bytes in case the sync code
>>>>>> is 0xAC41?)
>>>>>>
>>>>>
>>>>> CRC word.
>>>>>
>>>> You seem to intend for there to be two forms of AC-4 packets (at least
>>>> your decoder tries to support both): The one with syncwords, size
>>>> field and CRC stripped away and another one where these elements are
>>>> still present. This immediately brings a few questions:
>>>>
>>>> 1. Can a stripped packet be mistaken for an unstripped one, i.e. is it
>>>> possible for the data after the header to begin with a syncword?
>>>> 2. How reversible is stripping away the header and the CRC? Stripping
>>>> away the CRC is obviously irreversible unless we presume that the CRC
>>>> matches. But is stripping the size field away reversible (it is not
>>>> reversible if one is allowed to use the three byte size field if the
>>>> two byte size field would suffice)?
>>>> 3. Are there any container that use the stripped version?
>>>>
>>>> If the answer to the third question is no, then I don't think we
>>>> should create a new packetization of AC-4 and potentially incur the
>>>> first problem as well as the inability to retain CRCs for codec copy.
>>>>
>>>
>>> Decoder should support both, but I do not remember of stripped files.
>>>
>>
>> Actually mp4 dash files are stripped one.
>>
>
> I think the raw demuxer should strip the CRCs, that's a container-level
> thing IMO.
> The decoder should only support one version of packets.

I think .ts packets have it, so it would need to be stripped from .ts demuxer.
In specification it is explicitly written that decoder should support both.
James Almer March 5, 2020, 2:19 p.m. UTC | #12
On 3/5/2020 9:42 AM, Paul B Mahol wrote:
> On 3/5/20, Lynne <dev@lynne.ee> wrote:
>> Mar 5, 2020, 11:57 by onemda@gmail.com:
>>
>>> On 3/5/20, Paul B Mahol <onemda@gmail.com> wrote:
>>>
>>>> On 3/5/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>>>
>>>>> Paul B Mahol:
>>>>>
>>>>>> On 3/5/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>>>>>
>>>>>>> Am 05.03.2020 um 00:05 schrieb James Almer:
>>>>>>>
>>>>>>>> On 3/4/2020 7:51 PM, Paul B Mahol wrote:
>>>>>>>>
>>>>>>>>> On 3/4/20, James Almer <jamrial@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>>> On 3/4/2020 7:26 PM, Paul B Mahol wrote:
>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  libavformat/Makefile     |   1 +
>>>>>>>>>>>  libavformat/ac4dec.c     | 104
>>>>>>>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>  libavformat/allformats.c |   1 +
>>>>>>>>>>>  3 files changed, 106 insertions(+)
>>>>>>>>>>>  create mode 100644 libavformat/ac4dec.c
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>>>>>>>>>>> index e0681058a2..b4e8d20e65 100644
>>>>>>>>>>> --- a/libavformat/Makefile
>>>>>>>>>>> +++ b/libavformat/Makefile
>>>>>>>>>>> @@ -70,6 +70,7 @@ OBJS-$(CONFIG_AA_DEMUXER)                +=
>>>>>>>>>>> aadec.o
>>>>>>>>>>>  OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o
>>>>>>>>>>> img2.o
>>>>>>>>>>> rawdec.o
>>>>>>>>>>>  OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
>>>>>>>>>>>  OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
>>>>>>>>>>> +OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
>>>>>>>>>>>  OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
>>>>>>>>>>>  OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
>>>>>>>>>>>  OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
>>>>>>>>>>> diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
>>>>>>>>>>> new file mode 100644
>>>>>>>>>>> index 0000000000..8c6e539409
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++ b/libavformat/ac4dec.c
>>>>>>>>>>> @@ -0,0 +1,104 @@
>>>>>>>>>>> +/*
>>>>>>>>>>> + * RAW AC-4 demuxer
>>>>>>>>>>> + * Copyright (c) 2019 Paul B Mahol
>>>>>>>>>>> + *
>>>>>>>>>>> + * 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 "libavutil/avassert.h"
>>>>>>>>>>> +#include "libavutil/crc.h"
>>>>>>>>>>> +#include "avformat.h"
>>>>>>>>>>> +#include "rawdec.h"
>>>>>>>>>>> +
>>>>>>>>>>> +static int ac4_probe(const AVProbeData *p)
>>>>>>>>>>> +{
>>>>>>>>>>> +    const uint8_t *buf = p->buf;
>>>>>>>>>>> +    int left = p->buf_size;
>>>>>>>>>>> +    int max_frames = 0;
>>>>>>>>>>> +
>>>>>>>>>>> +    while (left > 7) {
>>>>>>>>>>> +        int size;
>>>>>>>>>>> +
>>>>>>>>>>> +        if (buf[0] == 0xAC &&
>>>>>>>>>>> +            (buf[1] == 0x40 ||
>>>>>>>>>>> +             buf[1] == 0x41)) {
>>>>>>>>>>> +            size = (buf[2] << 8) | buf[3];
>>>>>>>>>>> +            if (size == 0xFFFF)
>>>>>>>>>>> +                size = 3 + (buf[4] << 16) | (buf[5] << 8) |
>>>>>>>>>>> buf[6];
>>>>>>>>>>> +            size += 4;
>>>>>>>>>>> +            if (buf[1] == 0x41)
>>>>>>>>>>> +                size += 2;
>>>>>>>>>>> +            max_frames++;
>>>>>>>>>>> +            left -= size;
>>>>>>>>>>> +            buf += size;
>>>>>>>>>>> +        } else {
>>>>>>>>>>> +            break;
>>>>>>>>>>> +        }
>>>>>>>>>>> +    }
>>>>>>>>>>> +
>>>>>>>>>>> +    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static int ac4_read_header(AVFormatContext *s)
>>>>>>>>>>> +{
>>>>>>>>>>> +    AVStream *st;
>>>>>>>>>>> +
>>>>>>>>>>> +    st = avformat_new_stream(s, NULL);
>>>>>>>>>>> +    if (!st)
>>>>>>>>>>> +        return AVERROR(ENOMEM);
>>>>>>>>>>> +
>>>>>>>>>>> +    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>>>>>>>>>>> +    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
>>>>>>>>>>> +
>>>>>>>>>>> +    return 0;
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
>>>>>>>>>>> +{
>>>>>>>>>>> +    AVIOContext *pb = s->pb;
>>>>>>>>>>> +    int64_t pos;
>>>>>>>>>>> +    uint16_t sync;
>>>>>>>>>>> +    int ret, size;
>>>>>>>>>>> +
>>>>>>>>>>> +    if (avio_feof(s->pb))
>>>>>>>>>>> +        return AVERROR_EOF;
>>>>>>>>>>> +
>>>>>>>>>>> +    pos   = avio_tell(s->pb);
>>>>>>>>>>> +    sync = avio_rb16(pb);
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If there are sync codes then it sounds like the proper thing to do
>>>>>>>>>> is,
>>>>>>>>>> much like with AC3, writing a trivial parser to assemble frames and
>>>>>>>>>> then
>>>>>>>>>> use ff_raw_audio_read_header() and ff_raw_read_partial_packet()
>>>>>>>>>> here
>>>>>>>>>> instead of custom functions.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> That is over complication for simple parsing like here.
>>>>>>>>> Every raw packet have exact frame size set in bitstream.
>>>>>>>>>
>>>>>>>>
>>>>>>>> So does AC3, judging by how its parser assembles frames.
>>>>>>>>
>>>>>>>> An AVParser will let you resync after a bad seek, read frames in non
>>>>>>>> seekable input like a pipe, read frames within badly muxed files,
>>>>>>>> simplify the demuxer, etc, and is a matter of just looking for that
>>>>>>>> 16bit sync code and assembling a frame. Essentially just
>>>>>>>> re-implementing
>>>>>>>> what you already did in ac4_probe().
>>>>>>>>
>>>>>>>
>>>>>>> If it is intended that the actual AVPackets should not contain the
>>>>>>> sync code and the size field at all, then this should be dropped in
>>>>>>> the demuxer here (but then the demuxer actually needs to check for
>>>>>>> sync codes and needs to handle resyncing itself). This will also mean
>>>>>>> that memcpy of the data can be avoided; which is not so when relying
>>>>>>> on a parser as the packet size is not chosen adaptively for the
>>>>>>> content.
>>>>>>>
>>>>>>> The decoder seems to allow both variants: With header and without
>>>>>>> header. Is there a packetization that strips this header away? (And
>>>>>>> what is actually contained in the last two bytes in case the sync code
>>>>>>> is 0xAC41?)
>>>>>>>
>>>>>>
>>>>>> CRC word.
>>>>>>
>>>>> You seem to intend for there to be two forms of AC-4 packets (at least
>>>>> your decoder tries to support both): The one with syncwords, size
>>>>> field and CRC stripped away and another one where these elements are
>>>>> still present. This immediately brings a few questions:
>>>>>
>>>>> 1. Can a stripped packet be mistaken for an unstripped one, i.e. is it
>>>>> possible for the data after the header to begin with a syncword?
>>>>> 2. How reversible is stripping away the header and the CRC? Stripping
>>>>> away the CRC is obviously irreversible unless we presume that the CRC
>>>>> matches. But is stripping the size field away reversible (it is not
>>>>> reversible if one is allowed to use the three byte size field if the
>>>>> two byte size field would suffice)?
>>>>> 3. Are there any container that use the stripped version?
>>>>>
>>>>> If the answer to the third question is no, then I don't think we
>>>>> should create a new packetization of AC-4 and potentially incur the
>>>>> first problem as well as the inability to retain CRCs for codec copy.
>>>>>
>>>>
>>>> Decoder should support both, but I do not remember of stripped files.
>>>>
>>>
>>> Actually mp4 dash files are stripped one.
>>>
>>
>> I think the raw demuxer should strip the CRCs, that's a container-level
>> thing IMO.
>> The decoder should only support one version of packets.
> 
> I think .ts packets have it, so it would need to be stripped from .ts demuxer.
> In specification it is explicitly written that decoder should support both.

Looking at the specification, normal AC-4 frames are the so called "raw
frames", and that's what's used within mp4. An "AC-4 Sync Frame" is also
defined, which is an optional encapsulation with the aforementioned sync
word, size and optional CRC, what this demuxer is for in case they are
standalone, and what i assume would be used within .ts (No mention in
the spec about it).
I can't find anything about a decoder needing to handle it. It's not
meant to be part of the AC-4 bitstream per se, just an optional
encapsulation when the container has no proper framing delimitation, and
it's defined in an annex section, almost as an afterthought.

If that extra encapsulation is going to be propagated by this demuxer
(and potentially the .ts one), then the mp4 muxer must be updated to
remove it, and the mp4 demuxer to insert it.
In short, we need to decide what is propagated between modules and
libraries, either AC-4 Sync Frames or Raw AC-4 Frames, and write
demuxers and muxers accordingly. But preferably not both.
diff mbox series

Patch

diff --git a/libavformat/Makefile b/libavformat/Makefile
index e0681058a2..b4e8d20e65 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -70,6 +70,7 @@  OBJS-$(CONFIG_AA_DEMUXER)                += aadec.o
 OBJS-$(CONFIG_AAC_DEMUXER)               += aacdec.o apetag.o img2.o rawdec.o
 OBJS-$(CONFIG_AC3_DEMUXER)               += ac3dec.o rawdec.o
 OBJS-$(CONFIG_AC3_MUXER)                 += rawenc.o
+OBJS-$(CONFIG_AC4_DEMUXER)               += ac4dec.o
 OBJS-$(CONFIG_ACM_DEMUXER)               += acm.o rawdec.o
 OBJS-$(CONFIG_ACT_DEMUXER)               += act.o
 OBJS-$(CONFIG_ADF_DEMUXER)               += bintext.o sauce.o
diff --git a/libavformat/ac4dec.c b/libavformat/ac4dec.c
new file mode 100644
index 0000000000..8c6e539409
--- /dev/null
+++ b/libavformat/ac4dec.c
@@ -0,0 +1,104 @@ 
+/*
+ * RAW AC-4 demuxer
+ * Copyright (c) 2019 Paul B Mahol
+ *
+ * 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 "libavutil/avassert.h"
+#include "libavutil/crc.h"
+#include "avformat.h"
+#include "rawdec.h"
+
+static int ac4_probe(const AVProbeData *p)
+{
+    const uint8_t *buf = p->buf;
+    int left = p->buf_size;
+    int max_frames = 0;
+
+    while (left > 7) {
+        int size;
+
+        if (buf[0] == 0xAC &&
+            (buf[1] == 0x40 ||
+             buf[1] == 0x41)) {
+            size = (buf[2] << 8) | buf[3];
+            if (size == 0xFFFF)
+                size = 3 + (buf[4] << 16) | (buf[5] << 8) | buf[6];
+            size += 4;
+            if (buf[1] == 0x41)
+                size += 2;
+            max_frames++;
+            left -= size;
+            buf += size;
+        } else {
+            break;
+        }
+    }
+
+    return FFMIN(AVPROBE_SCORE_MAX, max_frames * 7);
+}
+
+static int ac4_read_header(AVFormatContext *s)
+{
+    AVStream *st;
+
+    st = avformat_new_stream(s, NULL);
+    if (!st)
+        return AVERROR(ENOMEM);
+
+    st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
+    st->codecpar->codec_id   = AV_CODEC_ID_AC4;
+
+    return 0;
+}
+
+static int ac4_read_packet(AVFormatContext *s, AVPacket *pkt)
+{
+    AVIOContext *pb = s->pb;
+    int64_t pos;
+    uint16_t sync;
+    int ret, size;
+
+    if (avio_feof(s->pb))
+        return AVERROR_EOF;
+
+    pos   = avio_tell(s->pb);
+    sync = avio_rb16(pb);
+    size = avio_rb16(pb);
+    if (size == 0xffff)
+        size = avio_rb24(pb);
+
+    ret = av_get_packet(pb, pkt, size);
+    pkt->pos = pos;
+    pkt->stream_index = 0;
+
+    if (sync == 0xAC41)
+        avio_skip(pb, 2);
+
+    return ret;
+}
+
+AVInputFormat ff_ac4_demuxer = {
+    .name           = "ac4",
+    .long_name      = NULL_IF_CONFIG_SMALL("raw AC-4"),
+    .read_probe     = ac4_probe,
+    .read_header    = ac4_read_header,
+    .read_packet    = ac4_read_packet,
+    .flags          = AVFMT_GENERIC_INDEX,
+    .extensions     = "ac4",
+};
diff --git a/libavformat/allformats.c b/libavformat/allformats.c
index 0209bf0e30..d2afcb2bdd 100644
--- a/libavformat/allformats.c
+++ b/libavformat/allformats.c
@@ -33,6 +33,7 @@  extern AVInputFormat  ff_aa_demuxer;
 extern AVInputFormat  ff_aac_demuxer;
 extern AVInputFormat  ff_ac3_demuxer;
 extern AVOutputFormat ff_ac3_muxer;
+extern AVInputFormat  ff_ac4_demuxer;
 extern AVInputFormat  ff_acm_demuxer;
 extern AVInputFormat  ff_act_demuxer;
 extern AVInputFormat  ff_adf_demuxer;