19/1/20 2:18 pm, Andreas Rheinhardt пишет:
>
> On Sun, Jan 19, 2020 at 4:12 AM Zane van Iperen <z...@zanevaniperen.com>
> wrote:
>
>> Adds support for the custom ASF container used by some Argonaut Games'
>> games, such as 'Croc! Legend of the Gobbos', and 'Croc 2'.
>>
>> Can also handle the sample files in:
>> https://samples.ffmpeg.org/game-formats/brender/part2.zip
>>
>> Signed-off-by: Zane van Iperen <z...@zanevaniperen.com>
>> ---
>> Changelog | 1 +
>> libavformat/Makefile | 1 +
>> libavformat/allformats.c | 1 +
>> libavformat/argo_asf.c | 254 +++++++++++++++++++++++++++++++++++++++
>> libavformat/version.h | 4 +-
>> 5 files changed, 259 insertions(+), 2 deletions(-)
>> create mode 100644 libavformat/argo_asf.c
>>
>> diff --git a/Changelog b/Changelog
>> index e26320c0ce..bc1593bfd1 100644
>> --- a/Changelog
>> +++ b/Changelog
>> @@ -31,6 +31,7 @@ version <next>:
>> - thistogram filter
>> - freezeframes filter
>> - Argonaut Games ADPCM decoder
>> +- Argonaut Games ASF demuxer
>>
>> version 4.2:
>> - tpad filter
>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>> index 52f29b1a6d..ba6ea8c4a6 100644
>> --- a/libavformat/Makefile
>> +++ b/libavformat/Makefile
>> @@ -99,6 +99,7 @@ OBJS-$(CONFIG_APTX_MUXER) += rawenc.o
>> OBJS-$(CONFIG_APTX_HD_DEMUXER) += aptxdec.o rawdec.o
>> OBJS-$(CONFIG_APTX_HD_MUXER) += rawenc.o
>> OBJS-$(CONFIG_AQTITLE_DEMUXER) += aqtitledec.o subtitles.o
>> +OBJS-$(CONFIG_ARGO_ASF_DEMUXER) += argo_asf.o
>> OBJS-$(CONFIG_ASF_DEMUXER) += asfdec_f.o asf.o asfcrypt.o \
>> avlanguage.o
>> OBJS-$(CONFIG_ASF_O_DEMUXER) += asfdec_o.o asf.o asfcrypt.o \
>> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
>> index ff9bdb906f..fe74a32e47 100644
>> --- a/libavformat/allformats.c
>> +++ b/libavformat/allformats.c
>> @@ -60,6 +60,7 @@ extern AVOutputFormat ff_aptx_muxer;
>> extern AVInputFormat ff_aptx_hd_demuxer;
>> extern AVOutputFormat ff_aptx_hd_muxer;
>> extern AVInputFormat ff_aqtitle_demuxer;
>> +extern AVInputFormat ff_argo_asf_demuxer;
>> extern AVInputFormat ff_asf_demuxer;
>> extern AVOutputFormat ff_asf_muxer;
>> extern AVInputFormat ff_asf_o_demuxer;
>> diff --git a/libavformat/argo_asf.c b/libavformat/argo_asf.c
>> new file mode 100644
>> index 0000000000..ab8a0955ab
>> --- /dev/null
>> +++ b/libavformat/argo_asf.c
>> @@ -0,0 +1,254 @@
>> +/*
>> + * Argonaut Games ASF demuxer
>> + *
>> + * Copyright (C) 2020 Zane van Iperen (z...@zanevaniperen.com)
>> + *
>> + * 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 "avformat.h"
>> +#include "internal.h"
>> +#include "libavutil/internal.h"
>> +#include "libavutil/bswap.h"
>> +#include "libavutil/avassert.h"
>> +
>> +#pragma pack(push, 1)
>> +typedef struct ArgoASFFileHeader {
>> + uint8_t magic[4]; /*< Magic Number, {'A', 'S', 'F', '\0'} */
>> + uint16_t version_major; /*< File Major Version. */
>> + uint16_t version_minor; /*< File Minor Version. */
>> + uint32_t num_chunks; /*< No. chunks in the file. */
>> + uint32_t chunk_offset; /*< Offset to the first chunk from the
>> start of the file. */
>> + int8_t name[8]; /*< Name. */
>> +} ArgoASFFileHeader;
>> +
>> +typedef struct ArgoASFChunkHeader {
>> + uint32_t num_blocks; /*< No. blocks in the chunk. */
>> + uint32_t num_samples; /*< No. samples per channel in a block. */
>> + uint32_t unk1; /*< Unknown */
>> + uint16_t sample_rate; /*< Sample rate. */
>> + uint16_t unk2; /*< Unknown. */
>> + uint32_t flags; /*< Stream flags. */
>> +} ArgoASFChunkHeader;
>> +#pragma pack(pop)
>> +
>>
>
> These pragmas should be removed.
>
>
Done.
>> +enum {
>> + ASF_CF_BITS_PER_SAMPLE = (1 << 0), /*< 16-bit if set, 8 otherwise.
>> */
>> + ASF_CF_STEREO = (1 << 1), /*< Stereo if set, mono
>> otherwise. */
>> + ASF_CF_ALWAYS1_1 = (1 << 2), /*< Unknown, always seems to be
>> set. */
>> + ASF_CF_ALWAYS1_2 = (1 << 3), /*< Unknown, always seems to be
>> set. */
>> +
>> + ASF_CF_ALWAYS1 = ASF_CF_ALWAYS1_1 | ASF_CF_ALWAYS1_2,
>> + ASF_CF_ALWAYS0 = ~(ASF_CF_BITS_PER_SAMPLE | ASF_CF_STEREO |
>> ASF_CF_ALWAYS1)
>> +};
>> +
>> +typedef struct ArgoASFDemuxContext
>> +{
>> + ArgoASFFileHeader fhdr;
>> + ArgoASFChunkHeader ckhdr;
>> + uint32_t blocks_read;
>> +} ArgoASFDemuxContext;
>> +
>> +static void argo_asf_fix_header_endianness(ArgoASFFileHeader *hdr)
>> +{
>> + hdr->version_major = av_le2ne16(hdr->version_major);
>> + hdr->version_minor = av_le2ne16(hdr->version_minor);
>> + hdr->num_chunks = av_le2ne32(hdr->num_chunks);
>> + hdr->chunk_offset = av_le2ne32(hdr->chunk_offset);
>> +}
>> +
>> +static void argo_asf_fix_chunk_endianness(ArgoASFChunkHeader *ck)
>> +{
>> + ck->num_blocks = av_le2ne32(ck->num_blocks);
>> + ck->num_samples = av_le2ne32(ck->num_samples);
>> + ck->unk1 = av_le2ne32(ck->unk1);
>> + ck->sample_rate = av_le2ne16(ck->sample_rate);
>> + ck->unk2 = av_le2ne32(ck->unk2);
>> + ck->flags = av_le2ne32(ck->flags);
>> +}
>> +
>> +/*
>> + * Known versions:
>> + * 1.1: The sample files in /game-formats/brender/part2.zip
>> + * 1.2: Croc! Legend of the Gobbos
>> + * 2.1: Croc 2
>> + */
>> +static int argo_asf_is_known_version(const ArgoASFFileHeader *hdr)
>> +{
>> + return (hdr->version_major == 1 && hdr->version_minor == 1) ||
>> + (hdr->version_major == 1 && hdr->version_minor == 2) ||
>> + (hdr->version_major == 2 && hdr->version_minor == 1);
>> +}
>> +
>> +static int argo_asf_probe(const AVProbeData *p)
>> +{
>> + int score;
>> + ArgoASFFileHeader hdr;
>> +
>> + av_assert0(sizeof(ArgoASFFileHeader) == 24);
>> + av_assert0(sizeof(ArgoASFChunkHeader) == 20);
>> +
>>
>
> These asserts should be removed, too.
Done.
>
>> + if (p->buf_size < sizeof(ArgoASFFileHeader))
>>
>
> Instead of relying on the compiler not adding padding you should rather use
> a #define for the size of the header as it exists in the file and compare
> with that.
>
Done.
>
>> + return AVPROBE_SCORE_RETRY;
>> +
>> + if (p->buf[0] != 'A' || p->buf[1] != 'S' ||
>> + p->buf[2] != 'F' || p->buf[3] != '\0')
>> + return 0;
>>
>
> I am sure this can be simplified with one of our macros for creating tags.
>
Changed to use MKTAG()
>
>> +
>> + score = 0;
>> + if (av_match_ext(p->filename, "asf"))
>> + score += AVPROBE_SCORE_EXTENSION;
>> +
>> + hdr = *(ArgoASFFileHeader*)p->buf;
>> + argo_asf_fix_header_endianness(&hdr);
>> +
>>
>
> You are modifying the probe buffer. I don't think you are supposed to do
> that (although the AVProbeData,buf is not a pointer to const uint8_t).
> Instead read what you need with AV_RL from the buffer.
>
hdr is a stack variable. I'm simply assigning it from the buffer, so nothing's
being changed.
It's a moot point anyway, since I've changed it to use AV_RL.
>
>> + if (argo_asf_is_known_version(&hdr))
>> + score += 25;
>> +
>> + /* Have only ever seen these with 1 chunk. */
>> + if (hdr.num_chunks == 1)
>> + score += 25;
>> +
>> + if (score > AVPROBE_SCORE_MAX)
>> + score = AVPROBE_SCORE_MAX;
>> +
>> + return score;
>> +}
>> +
>> +static int argo_asf_read_header(AVFormatContext *s)
>> +{
>> + int ret;
>> + AVIOContext *pb = s->pb;
>> + AVStream *st;
>> + ArgoASFDemuxContext *asf = s->priv_data;
>> +
>> + st = avformat_new_stream(s, NULL);
>> + if (!st)
>> + return AVERROR(ENOMEM);
>> +
>> + if (avio_read(pb, (unsigned char*)&asf->fhdr, sizeof(asf->fhdr)) !=
>> sizeof(asf->fhdr))
>> + return AVERROR(EIO);
>> +
>> + argo_asf_fix_header_endianness(&asf->fhdr);
>>
>
> Instead of relying on the availability of the pack pragma and fixing up the
> endianness afterwards you should read into a scratch buffer on the stack
> and then populate the header struct by reading the buffer with AV_RLxx,
>
Done.
>
>
>> +
>> + if (!argo_asf_is_known_version(&asf->fhdr)) {
>> + avpriv_request_sample(s, "Version %hu.%hu",
>> + asf->fhdr.version_major, asf->fhdr.version_minor
>> + );
>> + return AVERROR_PATCHWELCOME;
>> + }
>> +
>> + if ((ret = avio_seek(pb, asf->fhdr.chunk_offset, SEEK_SET)) < 0)
>>
>
> avio_seek() returns an int64_t. If the chunk_offset > INT_MAX (it is an
> uint32_t after all), the seek could succeed, yet it would appear as failure
> after conversion to an int like ret. (Btw: You appear to rely on the
> AVIOContext to be at offset zero initially. A relative seek would not
> depend on this.)
>
Fixed, and done.
> (Furthermore, maybe it would be a good idea to decrease the score in the
> probing function if this offset here is gigantic?)
>
Good idea, done.
>
>> + return ret;
>> +
>> + argo_asf_fix_chunk_endianness(&asf->ckhdr);
>>
>
> Putting this here is useless as nothing has been read into the chunk header
> at all at this moment.
> Of course, you should not move this below, but rather use AV_RL to parse
> what you have read.
Oops, that was meant to be below. I'll change it to use AV_RL.
>
>> +
>> + if (avio_read(pb, (unsigned char*)&asf->ckhdr, sizeof(asf->ckhdr)) !=
>> sizeof(asf->ckhdr))
>>
>
> As above: #define instead of sizeof and don't read directly into asf->ckhdr.
>
Done.
>>
>> + return AVERROR(EIO);
>> +
>> + if ((asf->ckhdr.flags & ASF_CF_ALWAYS1) != ASF_CF_ALWAYS1 ||
>> (asf->ckhdr.flags & ASF_CF_ALWAYS0) != 0) {
>> + avpriv_request_sample(s, "Nonstandard flags (0x%08X)",
>> asf->ckhdr.flags);
>> + return AVERROR_PATCHWELCOME;
>> + }
>> +
>> + st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>> + st->codecpar->codec_id = AV_CODEC_ID_ADPCM_ARGO;
>> + st->codecpar->format = AV_SAMPLE_FMT_S16;
>> +
>> + if (asf->ckhdr.flags & ASF_CF_STEREO) {
>> + st->codecpar->channel_layout = AV_CH_LAYOUT_STEREO;
>> + st->codecpar->channels = 2;
>> + } else {
>> + st->codecpar->channel_layout = AV_CH_LAYOUT_MONO;
>> + st->codecpar->channels = 1;
>> + }
>> +
>> + st->codecpar->sample_rate = asf->ckhdr.sample_rate;
>> +
>> + st->codecpar->bits_per_coded_sample = 4;
>> +
>> + if (asf->ckhdr.flags & ASF_CF_BITS_PER_SAMPLE)
>> + st->codecpar->bits_per_raw_sample = 16;
>> + else
>> + st->codecpar->bits_per_raw_sample = 8;
>> +
>> + if (st->codecpar->bits_per_raw_sample != 16) {
>> + /* The header allows for these, but I've never seen any files
>> with them. */
>> + avpriv_request_sample(s, "Non 16-bit samples");
>> + return AVERROR_PATCHWELCOME;
>> + }
>> +
>> + /*
>> + * (nchannel control bytes) + ((bytes_per_channel) * nchannel)
>> + * For mono, this is 17. For stereo, this is 34.
>> + */
>> + st->codecpar->frame_size = st->codecpar->channels +
>> + (asf->ckhdr.num_samples / 2) *
>> + st->codecpar->channels;
>> +
>> + st->codecpar->block_align = st->codecpar->frame_size;
>> +
>> + st->codecpar->bit_rate = st->codecpar->channels *
>> + st->codecpar->sample_rate *
>> +
>> st->codecpar->bits_per_coded_sample;
>> +
>> + avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
>> + st->start_time = 0;
>> + st->duration = asf->ckhdr.num_blocks * asf->ckhdr.num_samples;
>> + st->nb_frames = asf->ckhdr.num_blocks;
>> +
>> + asf->blocks_read = 0;
>>
>
> Unnecessary as asf will have been zeroed before this function.
>
Done.
>
>> + return 0;
>> +}
>> +
>> +static int argo_asf_read_packet(AVFormatContext *s, AVPacket *pkt)
>> +{
>> + ArgoASFDemuxContext *asf = s->priv_data;
>> +
>> + AVStream *st = s->streams[0];
>> + AVIOContext *pb = s->pb;
>> + int ret;
>> +
>> + if (asf->blocks_read >= asf->ckhdr.num_blocks)
>> + return AVERROR_EOF;
>> +
>> + ret = av_get_packet(pb, pkt, st->codecpar->frame_size);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (ret != st->codecpar->frame_size)
>> + return AVERROR_INVALIDDATA;
>> +
>> + pkt->stream_index = st->index;
>> + pkt->duration = asf->ckhdr.num_samples;
>> +
>> + ++asf->blocks_read;
>> + return ret;
>>
>
> You should just return 0.
Done
>
>> +}
>> +
>> +/*
>> + * Not actually sure what ASF stands for.
>> + * - Argonaut Sound File?
>> + * - Audio Stream File?
>> + */
>> +AVInputFormat ff_argo_asf_demuxer = {
>> + .name = "argo_asf",
>> + .long_name = NULL_IF_CONFIG_SMALL("Argonaut Games ASF"),
>> + .priv_data_size = sizeof(ArgoASFDemuxContext),
>> + .read_probe = argo_asf_probe,
>> + .read_header = argo_asf_read_header,
>> + .read_packet = argo_asf_read_packet
>> +};
>> \ No newline at end of file
>>
>
> Never ever seen this message here. I presume it is from git diff (or git
> format-patch?) or so. IIRC C source files need to end with a newline, so
> just add it.
>
Yep, that's my bad. Fixed.
>
>> diff --git a/libavformat/version.h b/libavformat/version.h
>> index 0a79868663..f72fb9478a 100644
>> --- a/libavformat/version.h
>> +++ b/libavformat/version.h
>> @@ -32,8 +32,8 @@
>> // Major bumping may affect Ticket5467, 5421, 5451(compatibility with
>> Chromium)
>> // Also please add any ticket numbers that you believe might be affected
>> here
>> #define LIBAVFORMAT_VERSION_MAJOR 58
>> -#define LIBAVFORMAT_VERSION_MINOR 35
>> -#define LIBAVFORMAT_VERSION_MICRO 103
>> +#define LIBAVFORMAT_VERSION_MINOR 36
>> +#define LIBAVFORMAT_VERSION_MICRO 100
>>
>> #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR,
>> \
>> LIBAVFORMAT_VERSION_MINOR,
>> \
>>
>>
> - 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-requ...@ffmpeg.org with subject "unsubscribe".
>
Do I need to submit only the revised commit or do I resend the entire series as
a "v2"?
Also, should I include any additional headers such as "Reviewed-by"?
Thanks,
Zane
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
