[FFmpeg-devel] avformat/aea: Add aea muxer

Empty Message

asivery via ffmpeg-devel:
> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> index b04b43cab3..1a50181447 100644
> --- a/libavformat/allformats.c
> +++ b/libavformat/allformats.c
> @@ -46,6 +46,7 @@ extern const FFOutputFormat ff_adts_muxer;
>  extern const AVInputFormat  ff_adx_demuxer;
>  extern const FFOutputFormat ff_adx_muxer;
>  extern const AVInputFormat  ff_aea_demuxer;
> +extern const FFOutputFormat ff_aea_muxer;
>  extern const AVInputFormat  ff_afc_demuxer;
>  extern const AVInputFormat  ff_aiff_demuxer;
>  extern const FFOutputFormat ff_aiff_muxer;
> -- 

Resend the patch rebased on current master.

- Andreas

Apologies for my oversight.
I've attached the new patch. 

- asivery

On Friday, March 8th, 2024 at 12:22 PM, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:

> asivery via ffmpeg-devel:
> 
> > diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> > index b04b43cab3..1a50181447 100644
> > --- a/libavformat/allformats.c
> > +++ b/libavformat/allformats.c
> > @@ -46,6 +46,7 @@ extern const FFOutputFormat ff_adts_muxer;
> > extern const AVInputFormat ff_adx_demuxer;
> > extern const FFOutputFormat ff_adx_muxer;
> > extern const AVInputFormat ff_aea_demuxer;
> > +extern const FFOutputFormat ff_aea_muxer;
> > extern const AVInputFormat ff_afc_demuxer;
> > extern const AVInputFormat ff_aiff_demuxer;
> > extern const FFOutputFormat ff_aiff_muxer;
> > --
> 
> 
> Resend the patch rebased on current master.
> 
> - 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".

asivery via ffmpeg-devel:
> +typedef struct {
> +    int block_count;
> +} AeaMuxerContext;
> +
> +static int aea_init(AVFormatContext *s)
> +{
> +    AeaMuxerContext *c = s->priv_data;
> +    c->block_count = 0;

Unnecessary: A muxer's priv_data is always pre-zeroed.

> +    
> +    return 0;
> +}
> +
> +static int aea_write_header(AVFormatContext *s)
> +{
> +    AVDictionaryEntry *title_entry;

const

> +    int title_length = 0, i;
> +    char* title_contents;
> +    AVStream *st;
> +
> +
> +    if (s->nb_streams > 1) {
> +        av_log(s, AV_LOG_WARNING, "Got more than one stream to encode, they will be ignored.\n");

We don't ignore extraneous streams, we normally error out if something
like this happens.

> +    }
> +    
> +    st = s->streams[0];
> +    if (st->codecpar->ch_layout.nb_channels != 1 && st->codecpar->ch_layout.nb_channels != 2) {
> +        av_log(s, AV_LOG_ERROR, "Invalid amount of channels to mux (%d).\n", st->codecpar->ch_layout.nb_channels);
> +        return AVERROR(EINVAL);
> +    }
> +
> +    if (st->codecpar->sample_rate != 44100) {
> +        av_log(s, AV_LOG_ERROR, "Invalid sample rate (%d) AEA only supports 44.1kHz.\n", st->codecpar->sample_rate);
> +        return AVERROR(EINVAL);
> +    }
> +
> +    /* Write magic */
> +    avio_wl32(s->pb, 2048);
> +    
> +    /* Write AEA title */
> +    title_entry = av_dict_get(st->metadata, "title", NULL, 0);
> +    if (title_entry) {
> +        title_contents = title_entry->value;
> +        title_length = FFMIN(256, strlen(title_contents));

Don't use strlen() in FFMIN (like many macros, it can evaluate its
argument multiple times).

> +    }
> +
> +    if (title_length) {
> +        avio_write(s->pb, title_contents, title_length);
> +    }

No need for this branch, just call this inside the "if (title_entry)"
block above.

> +
> +    for (i = 0; i<(256 - title_length); i++) {
> +        avio_w8(s->pb, 0);
> +    }

ffio_fill(). Same below.

> +
> +    /* Write number of frames (zero at header-writing time, will seek later), number of channels */
> +    avio_wl32(s->pb, 0);
> +    avio_w8(s->pb, st->codecpar->ch_layout.nb_channels);
> +    avio_w8(s->pb, 0);
> +
> +    /* Write flags (meaning unknown) */ 
> +    for(i = 0; i<11; i++) {
> +        avio_wl32(s->pb, 0);
> +    }
> +
> +    /* Pad the header to 2048 bytes */
> +    for(i = 0; i<1738; i++) avio_w8(s->pb, 0);
> +
> +    return 0;
> +}
> +
> +static int aea_write_packet(struct AVFormatContext *s, AVPacket *pkt)
> +{
> +    AeaMuxerContext *c = s->priv_data;
> +    c->block_count++;
> +
> +    avio_write(s->pb, pkt->data, pkt->size);
> +    return 0;

You can avoid this and the whole muxer context by using
ff_raw_write_packet() and relying on AVStream.nb_frames.

> +}
> +
> +static int aea_write_trailer(struct AVFormatContext *s)
> +{
> +    AVIOContext *pb = s->pb;
> +    AeaMuxerContext *c = s->priv_data;
> +    if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
> +        /* Seek to rewrite the block count. */
> +        avio_seek(pb, 260, SEEK_SET);
> +        avio_wl32(pb, c->block_count * s->streams[0]->codecpar->ch_layout.nb_channels);
> +    } else {
> +        av_log(s, AV_LOG_WARNING, "unable to rewrite AEA header.\n");
> +    }
> +
> +    return 0;
> +}
> +
> +const FFOutputFormat ff_aea_muxer = {
> +    .p.name           = "aea",
> +    .p.long_name      = NULL_IF_CONFIG_SMALL("MD STUDIO audio"),
> +    .priv_data_size   = sizeof(AeaMuxerContext),
> +    .p.extensions     = "aea",
> +    .p.audio_codec    = AV_CODEC_ID_ATRAC1,
> +    .init             = aea_init,
> +
> +    .write_header     = aea_write_header,
> +    .write_packet     = aea_write_packet,
> +    .write_trailer    = aea_write_trailer,
> +    .p.flags          = AVFMT_GENERIC_INDEX,

Makes no sense for a muxer.

> +};

Thank you for your incredibly thorough and fast response. I've applied all the corrections you requested. Please let me know if there's anything else wrong with my patch, and thank you for your time.

- asivery

On Friday, March 8th, 2024 at 1:06 PM, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:

> asivery via ffmpeg-devel:
> 
> > +typedef struct {
> > + int block_count;
> > +} AeaMuxerContext;
> > +
> > +static int aea_init(AVFormatContext *s)
> > +{
> > + AeaMuxerContext *c = s->priv_data;
> > + c->block_count = 0;
> 
> 
> Unnecessary: A muxer's priv_data is always pre-zeroed.
> 
> > +
> > + return 0;
> > +}
> > +
> > +static int aea_write_header(AVFormatContext *s)
> > +{
> > + AVDictionaryEntry *title_entry;
> 
> 
> const
> 
> > + int title_length = 0, i;
> > + char* title_contents;
> > + AVStream *st;
> > +
> > +
> > + if (s->nb_streams > 1) {
> > + av_log(s, AV_LOG_WARNING, "Got more than one stream to encode, they will be ignored.\n");
> 
> 
> We don't ignore extraneous streams, we normally error out if something
> like this happens.
> 
> > + }
> > +
> > + st = s->streams[0];
> > + if (st->codecpar->ch_layout.nb_channels != 1 && st->codecpar->ch_layout.nb_channels != 2) {
> > + av_log(s, AV_LOG_ERROR, "Invalid amount of channels to mux (%d).\n", st->codecpar->ch_layout.nb_channels);
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + if (st->codecpar->sample_rate != 44100) {
> > + av_log(s, AV_LOG_ERROR, "Invalid sample rate (%d) AEA only supports 44.1kHz.\n", st->codecpar->sample_rate);
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + /* Write magic /
> > + avio_wl32(s->pb, 2048);
> > +
> > + / Write AEA title */
> > + title_entry = av_dict_get(st->metadata, "title", NULL, 0);
> > + if (title_entry) {
> > + title_contents = title_entry->value;
> > + title_length = FFMIN(256, strlen(title_contents));
> 
> 
> Don't use strlen() in FFMIN (like many macros, it can evaluate its
> argument multiple times).
> 
> > + }
> > +
> > + if (title_length) {
> > + avio_write(s->pb, title_contents, title_length);
> > + }
> 
> 
> No need for this branch, just call this inside the "if (title_entry)"
> block above.
> 
> > +
> > + for (i = 0; i<(256 - title_length); i++) {
> > + avio_w8(s->pb, 0);
> > + }
> 
> 
> ffio_fill(). Same below.
> 
> > +
> > + /* Write number of frames (zero at header-writing time, will seek later), number of channels /
> > + avio_wl32(s->pb, 0);
> > + avio_w8(s->pb, st->codecpar->ch_layout.nb_channels);
> > + avio_w8(s->pb, 0);
> > +
> > + / Write flags (meaning unknown) /
> > + for(i = 0; i<11; i++) {
> > + avio_wl32(s->pb, 0);
> > + }
> > +
> > + / Pad the header to 2048 bytes */
> > + for(i = 0; i<1738; i++) avio_w8(s->pb, 0);
> > +
> > + return 0;
> > +}
> > +
> > +static int aea_write_packet(struct AVFormatContext *s, AVPacket *pkt)
> > +{
> > + AeaMuxerContext *c = s->priv_data;
> > + c->block_count++;
> > +
> > + avio_write(s->pb, pkt->data, pkt->size);
> > + return 0;
> 
> 
> You can avoid this and the whole muxer context by using
> ff_raw_write_packet() and relying on AVStream.nb_frames.
> 
> > +}
> > +
> > +static int aea_write_trailer(struct AVFormatContext *s)
> > +{
> > + AVIOContext *pb = s->pb;
> > + AeaMuxerContext c = s->priv_data;
> > + if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
> > + / Seek to rewrite the block count. */
> > + avio_seek(pb, 260, SEEK_SET);
> > + avio_wl32(pb, c->block_count * s->streams[0]->codecpar->ch_layout.nb_channels);
> > + } else {
> > + av_log(s, AV_LOG_WARNING, "unable to rewrite AEA header.\n");
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +const FFOutputFormat ff_aea_muxer = {
> > + .p.name = "aea",
> > + .p.long_name = NULL_IF_CONFIG_SMALL("MD STUDIO audio"),
> > + .priv_data_size = sizeof(AeaMuxerContext),
> > + .p.extensions = "aea",
> > + .p.audio_codec = AV_CODEC_ID_ATRAC1,
> > + .init = aea_init,
> > +
> > + .write_header = aea_write_header,
> > + .write_packet = aea_write_packet,
> > + .write_trailer = aea_write_trailer,
> > + .p.flags = AVFMT_GENERIC_INDEX,
> 
> 
> Makes no sense for a muxer.
> 
> > +};
> 
> 
> _______________________________________________
> 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".

On date Friday 2024-03-08 13:50:59 +0000, ffmpeg-devel Mailing List wrote:
> Thank you for your incredibly thorough and fast response. I've applied all the corrections you requested. Please let me know if there's anything else wrong with my patch, and thank you for your time.
> 
> - asivery

[...]

> From d2163e7d943a5ba53148aa73a813a28d346124a4 Mon Sep 17 00:00:00 2001
> From: asivery <asivery@protonmail.com>
> Date: Fri, 8 Mar 2024 14:45:02 +0100
> Subject: [PATCH] avformat/aea: Add aea muxer
> 
> Signed-off-by: asivery <asivery@protonmail.com>
> ---
>  libavformat/Makefile            |   3 +-
>  libavformat/{aea.c => aeadec.c} |   0
>  libavformat/aeaenc.c            | 103 ++++++++++++++++++++++++++++++++
>  libavformat/allformats.c        |   1 +

Please add some information in doc/muxers.texi, even if just a short
notice about where this format is expected to be used or a quick
historical intro. Also you might mention that the title metadata is
used to fill the container title.

>  4 files changed, 106 insertions(+), 1 deletion(-)
>  rename libavformat/{aea.c => aeadec.c} (100%)
>  create mode 100644 libavformat/aeaenc.c
> 
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index 8811a0ffc9..70d56f391f 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -91,7 +91,8 @@ OBJS-$(CONFIG_ADTS_MUXER)                += adtsenc.o apetag.o img2.o \
>                                              id3v2enc.o
>  OBJS-$(CONFIG_ADX_DEMUXER)               += adxdec.o
>  OBJS-$(CONFIG_ADX_MUXER)                 += rawenc.o
> -OBJS-$(CONFIG_AEA_DEMUXER)               += aea.o pcm.o
> +OBJS-$(CONFIG_AEA_DEMUXER)               += aeadec.o pcm.o
> +OBJS-$(CONFIG_AEA_MUXER)                 += aeaenc.o rawenc.o
>  OBJS-$(CONFIG_AFC_DEMUXER)               += afc.o
>  OBJS-$(CONFIG_AIFF_DEMUXER)              += aiffdec.o aiff.o pcm.o \
>                                              mov_chan.o replaygain.o
> diff --git a/libavformat/aea.c b/libavformat/aeadec.c
> similarity index 100%
> rename from libavformat/aea.c
> rename to libavformat/aeadec.c
> diff --git a/libavformat/aeaenc.c b/libavformat/aeaenc.c
> new file mode 100644
> index 0000000000..e01fabcace
> --- /dev/null
> +++ b/libavformat/aeaenc.c
> @@ -0,0 +1,103 @@
> +/*
> + * MD STUDIO audio muxer
> + *
> + * Copyright (c) 2024 asivery
> + *
> + * 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/channel_layout.h"
> +#include "libavutil/intreadwrite.h"
> +#include "libavutil/avstring.h"
> +#include "avformat.h"
> +#include "avio_internal.h"
> +#include "rawenc.h"
> +#include "mux.h"
> +
> +static int aea_write_header(AVFormatContext *s)
> +{
> +    const AVDictionaryEntry *title_entry;
> +    int title_length = 0;
> +    char* title_contents;
> +    AVStream *st;
> +
> +    if (s->nb_streams > 1) {

> +        av_log(s, AV_LOG_WARNING, "Got more than one stream to encode. This is not supported.\n");

ERROR

> +        return AVERROR(EINVAL);
> +    }
> +    

> +    st = s->streams[0];

do you also need to check the stream type (probably this is handled by
the librarye already)?

> +    if (st->codecpar->ch_layout.nb_channels != 1 && st->codecpar->ch_layout.nb_channels != 2) {

> +        av_log(s, AV_LOG_ERROR, "Invalid amount of channels to mux (%d).\n", st->codecpar->ch_layout.nb_channels);

this can be made more explicit to aid users:
Only maximum 2 channels are supported in the audio stream, %d channels were found

> +        return AVERROR(EINVAL);
> +    }
> +
> +    if (st->codecpar->sample_rate != 44100) {
> +        av_log(s, AV_LOG_ERROR, "Invalid sample rate (%d) AEA only supports 44.1kHz.\n", st->codecpar->sample_rate);
> +        return AVERROR(EINVAL);
> +    }
> +
> +    /* Write magic */
> +    avio_wl32(s->pb, 2048);
> +    
> +    /* Write AEA title */
> +    title_entry = av_dict_get(st->metadata, "title", NULL, 0);
> +    if (title_entry) {
> +        title_contents = title_entry->value;
> +        title_length = strlen(title_contents);
> +        title_length = FFMIN(256, title_length);
> +        avio_write(s->pb, title_contents, title_length);
> +    }
> +
> +    ffio_fill(s->pb, 0, 256 - title_length);
> +
> +    /* Write number of frames (zero at header-writing time, will seek later), number of channels */
> +    avio_wl32(s->pb, 0);
> +    avio_w8(s->pb, st->codecpar->ch_layout.nb_channels);
> +    avio_w8(s->pb, 0);
> +
> +    /* Pad the header to 2048 bytes */
> +    ffio_fill(s->pb, 0, 1782);
> +
> +    return 0;
> +}
> +
> +static int aea_write_trailer(struct AVFormatContext *s)
> +{
> +    AVIOContext *pb = s->pb;
> +    AVStream *st = s->streams[0];
> +    if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
> +        /* Seek to rewrite the block count. */
> +        avio_seek(pb, 260, SEEK_SET);
> +        avio_wl32(pb, st->nb_frames * st->codecpar->ch_layout.nb_channels);
> +    } else {

> +        av_log(s, AV_LOG_WARNING, "unable to rewrite AEA header.\n");

nit: Unable

asivery via ffmpeg-devel:
> +#include "libavutil/intreadwrite.h"
> +#include "libavutil/avstring.h"

These two headers seem unused.

> +#include "avformat.h"
> +#include "avio_internal.h"
> +#include "rawenc.h"
> +#include "mux.h"
> +
> +static int aea_write_header(AVFormatContext *s)
> +{
> +    const AVDictionaryEntry *title_entry;
> +    int title_length = 0;
> +    char* title_contents;

const please. Also we put the * to the variable (because "char* foo,
bar" still only declares one pointer to char). Furthermore, the scope
for this should be the block below.

> +    AVStream *st;
> +
> +    if (s->nb_streams > 1) {
> +        av_log(s, AV_LOG_WARNING, "Got more than one stream to encode. This is not supported.\n");
> +        return AVERROR(EINVAL);
> +    }
> +    
> +    st = s->streams[0];
> +    if (st->codecpar->ch_layout.nb_channels != 1 && st->codecpar->ch_layout.nb_channels != 2) {
> +        av_log(s, AV_LOG_ERROR, "Invalid amount of channels to mux (%d).\n", st->codecpar->ch_layout.nb_channels);
> +        return AVERROR(EINVAL);
> +    }
> +
> +    if (st->codecpar->sample_rate != 44100) {
> +        av_log(s, AV_LOG_ERROR, "Invalid sample rate (%d) AEA only supports 44.1kHz.\n", st->codecpar->sample_rate);
> +        return AVERROR(EINVAL);
> +    }
> +
> +    /* Write magic */
> +    avio_wl32(s->pb, 2048);
> +    
> +    /* Write AEA title */
> +    title_entry = av_dict_get(st->metadata, "title", NULL, 0);
> +    if (title_entry) {
> +        title_contents = title_entry->value;
> +        title_length = strlen(title_contents);

Possible truncation here.

> +        title_length = FFMIN(256, title_length);
> +        avio_write(s->pb, title_contents, title_length);
> +    }
> +
> +    ffio_fill(s->pb, 0, 256 - title_length);
> +
> +    /* Write number of frames (zero at header-writing time, will seek later), number of channels */
> +    avio_wl32(s->pb, 0);
> +    avio_w8(s->pb, st->codecpar->ch_layout.nb_channels);
> +    avio_w8(s->pb, 0);
> +
> +    /* Pad the header to 2048 bytes */
> +    ffio_fill(s->pb, 0, 1782);
> +
> +    return 0;
> +}
> +
> +static int aea_write_trailer(struct AVFormatContext *s)
> +{
> +    AVIOContext *pb = s->pb;
> +    AVStream *st = s->streams[0];
> +    if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
> +        /* Seek to rewrite the block count. */
> +        avio_seek(pb, 260, SEEK_SET);
> +        avio_wl32(pb, st->nb_frames * st->codecpar->ch_layout.nb_channels);

I don't see anything guaranteeing that the result fits into 32 bits.

> +    } else {
> +        av_log(s, AV_LOG_WARNING, "unable to rewrite AEA header.\n");
> +    }
> +
> +    return 0;
> +}
> +
> +const FFOutputFormat ff_aea_muxer = {
> +    .p.name           = "aea",
> +    .p.long_name      = NULL_IF_CONFIG_SMALL("MD STUDIO audio"),
> +    .p.extensions     = "aea",
> +    .p.audio_codec    = AV_CODEC_ID_ATRAC1,
> +
> +    .write_header     = aea_write_header,
> +    .write_packet     = ff_raw_write_packet,
> +    .write_trailer    = aea_write_trailer,
> +};

Thank you both for the suggestions. I've updated the code as requested, and I apologize for the AV_LOG_WARNING instead of AV_LOG_ERROR - it was an oversight on my part.
I have also added the stream codec check, and it did get triggered when I tried to feed it audio that was not ATRAC1, so it seems it is required.
Regarding titles being truncated - that was my intention. However I've now added a warning if it was going to happen.
As for the block count in the header - none of the modern software which uses AEA reads that field, but for the older software, it will now be truncated to UINT32_MAX if needed.
Is there anything else that needs changes?

- asivery

On Saturday, March 9th, 2024 at 1:06 PM, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:

> asivery via ffmpeg-devel:
> 
> > +#include "libavutil/intreadwrite.h"
> > +#include "libavutil/avstring.h"
> 
> 
> These two headers seem unused.
> 
> > +#include "avformat.h"
> > +#include "avio_internal.h"
> > +#include "rawenc.h"
> > +#include "mux.h"
> > +
> > +static int aea_write_header(AVFormatContext *s)
> > +{
> > + const AVDictionaryEntry title_entry;
> > + int title_length = 0;
> > + char title_contents;
> 
> 
> const please. Also we put the * to the variable (because "char* foo,
> bar" still only declares one pointer to char). Furthermore, the scope
> for this should be the block below.
> 
> > + AVStream st;
> > +
> > + if (s->nb_streams > 1) {
> > + av_log(s, AV_LOG_WARNING, "Got more than one stream to encode. This is not supported.\n");
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + st = s->streams[0];
> > + if (st->codecpar->ch_layout.nb_channels != 1 && st->codecpar->ch_layout.nb_channels != 2) {
> > + av_log(s, AV_LOG_ERROR, "Invalid amount of channels to mux (%d).\n", st->codecpar->ch_layout.nb_channels);
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + if (st->codecpar->sample_rate != 44100) {
> > + av_log(s, AV_LOG_ERROR, "Invalid sample rate (%d) AEA only supports 44.1kHz.\n", st->codecpar->sample_rate);
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + / Write magic /
> > + avio_wl32(s->pb, 2048);
> > +
> > + / Write AEA title */
> > + title_entry = av_dict_get(st->metadata, "title", NULL, 0);
> > + if (title_entry) {
> > + title_contents = title_entry->value;
> > + title_length = strlen(title_contents);
> 
> 
> Possible truncation here.
> 
> > + title_length = FFMIN(256, title_length);
> > + avio_write(s->pb, title_contents, title_length);
> > + }
> > +
> > + ffio_fill(s->pb, 0, 256 - title_length);
> > +
> > + /* Write number of frames (zero at header-writing time, will seek later), number of channels /
> > + avio_wl32(s->pb, 0);
> > + avio_w8(s->pb, st->codecpar->ch_layout.nb_channels);
> > + avio_w8(s->pb, 0);
> > +
> > + / Pad the header to 2048 bytes */
> > + ffio_fill(s->pb, 0, 1782);
> > +
> > + return 0;
> > +}
> > +
> > +static int aea_write_trailer(struct AVFormatContext *s)
> > +{
> > + AVIOContext *pb = s->pb;
> > + AVStream st = s->streams[0];
> > + if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
> > + / Seek to rewrite the block count. */
> > + avio_seek(pb, 260, SEEK_SET);
> > + avio_wl32(pb, st->nb_frames * st->codecpar->ch_layout.nb_channels);
> 
> 
> I don't see anything guaranteeing that the result fits into 32 bits.
> 
> > + } else {
> > + av_log(s, AV_LOG_WARNING, "unable to rewrite AEA header.\n");
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +const FFOutputFormat ff_aea_muxer = {
> > + .p.name = "aea",
> > + .p.long_name = NULL_IF_CONFIG_SMALL("MD STUDIO audio"),
> > + .p.extensions = "aea",
> > + .p.audio_codec = AV_CODEC_ID_ATRAC1,
> > +
> > + .write_header = aea_write_header,
> > + .write_packet = ff_raw_write_packet,
> > + .write_trailer = aea_write_trailer,
> > +};
> 
> 
> _______________________________________________
> 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".

asivery via ffmpeg-devel:
> +        const char *title_contents = title_entry->value;
> +        title_length = strlen(title_contents);
> +        if (title_length > 256) {
> +            av_log(s, AV_LOG_WARNING, "Title too long, truncated to 256 bytes.\n");
> +            title_length = 256;
> +        }

I actually meant that you should use size_t for title_length (so that
the output of strlen() is never truncated) instead of adding a warning
that will likely never be triggered in practice.

- Andreas

I see, I've changed title_length to size_t, but left the warning as is to inform the user in case truncation needs to take place.

On Saturday, March 9th, 2024 at 6:48 PM, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:

> asivery via ffmpeg-devel:
> 
> > + const char *title_contents = title_entry->value;
> > + title_length = strlen(title_contents);
> > + if (title_length > 256) {
> > + av_log(s, AV_LOG_WARNING, "Title too long, truncated to 256 bytes.\n");
> > + title_length = 256;
> > + }
> 
> 
> I actually meant that you should use size_t for title_length (so that
> the output of strlen() is never truncated) instead of adding a warning
> that will likely never be triggered in practice.
> 
> - 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".

On date Saturday 2024-03-09 17:20:49 +0000, ffmpeg-devel Mailing List wrote:
> Thank you both for the suggestions. I've updated the code as requested, and I apologize for the AV_LOG_WARNING instead of AV_LOG_ERROR - it was an oversight on my part.
> I have also added the stream codec check, and it did get triggered when I tried to feed it audio that was not ATRAC1, so it seems it is required.
> Regarding titles being truncated - that was my intention. However I've now added a warning if it was going to happen.
> As for the block count in the header - none of the modern software which uses AEA reads that field, but for the older software, it will now be truncated to UINT32_MAX if needed.
> Is there anything else that needs changes?
> 

> From ee1d4c93c66e729d9d0456b2e8e789f3f98389e3 Mon Sep 17 00:00:00 2001
> From: asivery <asivery@protonmail.com>
> Date: Fri, 8 Mar 2024 14:45:02 +0100
> Subject: [PATCH] avformat/aea: Add aea muxer
> 
> Signed-off-by: asivery <asivery@protonmail.com>
> ---
>  doc/muxers.texi                 |  10 +++
>  libavformat/Makefile            |   3 +-
>  libavformat/{aea.c => aeadec.c} |   0
>  libavformat/aeaenc.c            | 115 ++++++++++++++++++++++++++++++++
>  libavformat/allformats.c        |   1 +
>  5 files changed, 128 insertions(+), 1 deletion(-)
>  rename libavformat/{aea.c => aeadec.c} (100%)
>  create mode 100644 libavformat/aeaenc.c
> 
> diff --git a/doc/muxers.texi b/doc/muxers.texi
> index 2104cc4a95..a4df8f736d 100644
> --- a/doc/muxers.texi
> +++ b/doc/muxers.texi
> @@ -663,6 +663,16 @@ when enabled, write a CRC checksum for each packet to the output,
>  default is @code{false}
>  @end table
>  

> +@anchor{aea}
> +@section aea

nit: sort order (should go after adts)

> +MD STUDIO audio muxer.

out of my own curiosity, what is MD STUDIO?

[...]

You might also add an entry to the Changelog.
Looks good to me otherwise, thanks.

Great, thank you very much!
I'm attaching the (hopefully) final version of the patch.

MD studio was a piece of software created by a company called "EDL", which in combination with alternative firmware for the Sony MDH-10 MiniDisc recorder (https://www.minidisc.wiki/equipment/sony/misc/mdh-10) let people download raw ATRAC1 audio from their MiniDiscs onto computers, and (probably) put this audio back on their discs later.
Nowadays it's used by software like Web Minidisc Pro (which I maintain) to store ATRAC1 audio, as it (until recently) was the only way to store ATRAC1 so that it would be picked up by VLC. Now we have matroska support for ATRAC1, but I wanted to write a muxer for AEA before I phase it out from new pieces of software, so that the people who would like to use AEA instead of matroska have a way to go back to it.

> On date Saturday 2024-03-09 17:20:49 +0000, ffmpeg-devel Mailing List wrote:
> 
> > Thank you both for the suggestions. I've updated the code as requested, and I apologize for the AV_LOG_WARNING instead of AV_LOG_ERROR - it was an oversight on my part.
> > I have also added the stream codec check, and it did get triggered when I tried to feed it audio that was not ATRAC1, so it seems it is required.
> > Regarding titles being truncated - that was my intention. However I've now added a warning if it was going to happen.
> > As for the block count in the header - none of the modern software which uses AEA reads that field, but for the older software, it will now be truncated to UINT32_MAX if needed.
> > Is there anything else that needs changes?
> 
> > From ee1d4c93c66e729d9d0456b2e8e789f3f98389e3 Mon Sep 17 00:00:00 2001
> > From: asivery asivery@protonmail.com
> > Date: Fri, 8 Mar 2024 14:45:02 +0100
> > Subject: [PATCH] avformat/aea: Add aea muxer
> > 
> > Signed-off-by: asivery asivery@protonmail.com
> > ---
> > doc/muxers.texi | 10 +++
> > libavformat/Makefile | 3 +-
> > libavformat/{aea.c => aeadec.c} | 0
> > libavformat/aeaenc.c | 115 ++++++++++++++++++++++++++++++++
> > libavformat/allformats.c | 1 +
> > 5 files changed, 128 insertions(+), 1 deletion(-)
> > rename libavformat/{aea.c => aeadec.c} (100%)
> > create mode 100644 libavformat/aeaenc.c
> > 
> > diff --git a/doc/muxers.texi b/doc/muxers.texi
> > index 2104cc4a95..a4df8f736d 100644
> > --- a/doc/muxers.texi
> > +++ b/doc/muxers.texi
> > @@ -663,6 +663,16 @@ when enabled, write a CRC checksum for each packet to the output,
> > default is @code{false}
> > @end table
> 
> > +@anchor{aea}
> > +@section aea
> 
> 
> nit: sort order (should go after adts)
> 
> > +MD STUDIO audio muxer.
> 
> 
> out of my own curiosity, what is MD STUDIO?
> 
> [...]
> 
> You might also add an entry to the Changelog.
> Looks good to me otherwise, thanks.

On date Sunday 2024-03-10 14:20:12 +0000, ffmpeg-devel Mailing List wrote:
> Great, thank you very much!
> I'm attaching the (hopefully) final version of the patch.
> 

> MD studio was a piece of software created by a company called "EDL",
> which in combination with alternative firmware for the Sony MDH-10
> MiniDisc recorder
> (https://www.minidisc.wiki/equipment/sony/misc/mdh-10) let people
> download raw ATRAC1 audio from their MiniDiscs onto computers, and
> (probably) put this audio back on their discs later.  Nowadays it's
> used by software like Web Minidisc Pro (which I maintain) to store
> ATRAC1 audio, as it (until recently) was the only way to store
> ATRAC1 so that it would be picked up by VLC. Now we have matroska
> support for ATRAC1, but I wanted to write a muxer for AEA before I
> phase it out from new pieces of software, so that the people who
> would like to use AEA instead of matroska have a way to go back to
> it.

Thanks, probably part of this information could go to the doc to
provide some context (can be done as a separate patch), as this kind
of info is very difficult to retrieve otherwise (and one of the goals
of FFmpeg is digital preservation).

> From e67d2df52c65528fbbfe8d5268661c88a7ad225e Mon Sep 17 00:00:00 2001
> From: asivery <asivery@protonmail.com>
> Date: Fri, 8 Mar 2024 14:45:02 +0100
> Subject: [PATCH] avformat/aea: Add aea muxer
> 
> Signed-off-by: asivery <asivery@protonmail.com>
> ---
>  Changelog                       |   1 +
>  doc/muxers.texi                 |  10 +++
>  libavformat/Makefile            |   3 +-
>  libavformat/{aea.c => aeadec.c} |   0
>  libavformat/aeaenc.c            | 115 ++++++++++++++++++++++++++++++++
>  libavformat/allformats.c        |   1 +
>  6 files changed, 129 insertions(+), 1 deletion(-)
>  rename libavformat/{aea.c => aeadec.c} (100%)
>  create mode 100644 libavformat/aeaenc.c

Patch looks good to me, waiting for more feedback by Andreas.

Thanks.

On date Sunday 2024-03-10 16:10:15 +0100, Stefano Sabatini wrote:
> On date Sunday 2024-03-10 14:20:12 +0000, ffmpeg-devel Mailing List wrote:
> > Great, thank you very much!
> > I'm attaching the (hopefully) final version of the patch.
> > 
> 
> > MD studio was a piece of software created by a company called "EDL",
> > which in combination with alternative firmware for the Sony MDH-10
> > MiniDisc recorder
> > (https://www.minidisc.wiki/equipment/sony/misc/mdh-10) let people
> > download raw ATRAC1 audio from their MiniDiscs onto computers, and
> > (probably) put this audio back on their discs later.  Nowadays it's
> > used by software like Web Minidisc Pro (which I maintain) to store
> > ATRAC1 audio, as it (until recently) was the only way to store
> > ATRAC1 so that it would be picked up by VLC. Now we have matroska
> > support for ATRAC1, but I wanted to write a muxer for AEA before I
> > phase it out from new pieces of software, so that the people who
> > would like to use AEA instead of matroska have a way to go back to
> > it.
> 
> Thanks, probably part of this information could go to the doc to
> provide some context (can be done as a separate patch), as this kind
> of info is very difficult to retrieve otherwise (and one of the goals
> of FFmpeg is digital preservation).
> 
> > From e67d2df52c65528fbbfe8d5268661c88a7ad225e Mon Sep 17 00:00:00 2001
> > From: asivery <asivery@protonmail.com>
> > Date: Fri, 8 Mar 2024 14:45:02 +0100
> > Subject: [PATCH] avformat/aea: Add aea muxer
> > 
> > Signed-off-by: asivery <asivery@protonmail.com>
> > ---
> >  Changelog                       |   1 +
> >  doc/muxers.texi                 |  10 +++
> >  libavformat/Makefile            |   3 +-
> >  libavformat/{aea.c => aeadec.c} |   0
> >  libavformat/aeaenc.c            | 115 ++++++++++++++++++++++++++++++++
> >  libavformat/allformats.c        |   1 +
> >  6 files changed, 129 insertions(+), 1 deletion(-)
> >  rename libavformat/{aea.c => aeadec.c} (100%)
> >  create mode 100644 libavformat/aeaenc.c
> 
> Patch looks good to me, waiting for more feedback by Andreas.

Will apply it soon, thanks.

Thank you Stefano Sabatini and Andreas Rheinhardt - you guys have the patience of saints.

On Tuesday, March 12th, 2024 at 11:27 AM, Stefano Sabatini <stefasab@gmail.com> wrote:

> On date Sunday 2024-03-10 16:10:15 +0100, Stefano Sabatini wrote:
> 
> > On date Sunday 2024-03-10 14:20:12 +0000, ffmpeg-devel Mailing List wrote:
> > 
> > > Great, thank you very much!
> > > I'm attaching the (hopefully) final version of the patch.
> > 
> > > MD studio was a piece of software created by a company called "EDL",
> > > which in combination with alternative firmware for the Sony MDH-10
> > > MiniDisc recorder
> > > (https://www.minidisc.wiki/equipment/sony/misc/mdh-10) let people
> > > download raw ATRAC1 audio from their MiniDiscs onto computers, and
> > > (probably) put this audio back on their discs later. Nowadays it's
> > > used by software like Web Minidisc Pro (which I maintain) to store
> > > ATRAC1 audio, as it (until recently) was the only way to store
> > > ATRAC1 so that it would be picked up by VLC. Now we have matroska
> > > support for ATRAC1, but I wanted to write a muxer for AEA before I
> > > phase it out from new pieces of software, so that the people who
> > > would like to use AEA instead of matroska have a way to go back to
> > > it.
> > 
> > Thanks, probably part of this information could go to the doc to
> > provide some context (can be done as a separate patch), as this kind
> > of info is very difficult to retrieve otherwise (and one of the goals
> > of FFmpeg is digital preservation).
> > 
> > > From e67d2df52c65528fbbfe8d5268661c88a7ad225e Mon Sep 17 00:00:00 2001
> > > From: asivery asivery@protonmail.com
> > > Date: Fri, 8 Mar 2024 14:45:02 +0100
> > > Subject: [PATCH] avformat/aea: Add aea muxer
> > > 
> > > Signed-off-by: asivery asivery@protonmail.com
> > > ---
> > > Changelog | 1 +
> > > doc/muxers.texi | 10 +++
> > > libavformat/Makefile | 3 +-
> > > libavformat/{aea.c => aeadec.c} | 0
> > > libavformat/aeaenc.c | 115 ++++++++++++++++++++++++++++++++
> > > libavformat/allformats.c | 1 +
> > > 6 files changed, 129 insertions(+), 1 deletion(-)
> > > rename libavformat/{aea.c => aeadec.c} (100%)
> > > create mode 100644 libavformat/aeaenc.c
> > 
> > Patch looks good to me, waiting for more feedback by Andreas.
> 
> 
> Will apply it soon, thanks.