| Message ID | 20200706092347.102557-2-zane@zanevaniperen.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | adpcm_ima_apm encoder + apm muxer | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Mon, Jul 06, 2020 at 09:24:20AM +0000, Zane van Iperen wrote: > Signed-off-by: Zane van Iperen <zane@zanevaniperen.com> > --- > libavformat/Makefile | 2 +- > libavformat/apm.c | 62 ++++++++++++++++++++++++-------------------- > 2 files changed, 35 insertions(+), 29 deletions(-) I think the commit message should be more elaborate "avformat/apm: use new extradata format" doesnt really say what changes, why its changes [...] > @@ -95,24 +99,29 @@ static int apm_read_header(AVFormatContext *s) > int64_t ret; > AVStream *st; > APMVS12Chunk vs12; > - uint8_t buf[APM_VS12_CHUNK_SIZE]; > + uint8_t buf[APM_FILE_EXTRADATA_SIZE]; > > if (!(st = avformat_new_stream(s, NULL))) > return AVERROR(ENOMEM); > > - /* The header starts with a WAVEFORMATEX */ > - if ((ret = ff_get_wav_header(s, s->pb, st->codecpar, APM_FILE_HEADER_SIZE, 0)) < 0) > - return ret; > - > - if (st->codecpar->bits_per_coded_sample != 4) > + /* > + * This is 98% a WAVEFORMATEX, but there's something screwy with the extradata > + * that ff_get_wav_header() can't (and shouldn't) handle properly. > + */ > + if (avio_rl16(s->pb) != APM_TAG_CODEC) > return AVERROR_INVALIDDATA; > > - if (st->codecpar->codec_tag != APM_TAG_CODEC) > + st->codecpar->channels = avio_rl16(s->pb); > + st->codecpar->sample_rate = avio_rl32(s->pb); > + st->codecpar->bit_rate = avio_rl32(s->pb) * 8; This can overflow You also may want to add yourself to the MAINTAINERs file [...]
On Mon, 6 Jul 2020 18:29:57 +0200 "Michael Niedermayer" <michael@niedermayer.cc> wrote: > On Mon, Jul 06, 2020 at 09:24:20AM +0000, Zane van Iperen wrote: > > Signed-off-by: Zane van Iperen <zane@zanevaniperen.com> > > --- > > libavformat/Makefile | 2 +- > > libavformat/apm.c | 62 > > ++++++++++++++++++++++++-------------------- 2 files changed, 35 > > insertions(+), 29 deletions(-) > > I think the commit message should be more elaborate > "avformat/apm: use new extradata format" > doesnt really say what changes, why its changes > What's a nice way of saying "I made an error in the old format and had to change it in order to keep things simple in the muxer."? Essentially, I - Handle the dodgy WAVEFORMATEX (that I missed before) correctly, and - Pass through the entire APMState structure to the decoder instead of only some fields. > You also may want to add yourself to the MAINTAINERs file > Will do, I'll send it in a different patchset. Would this warrant commit access? Because I'm including a GPG key, I'd like to have the commit signed, and `git format-patch` doesn't like signatures. Zane
On 7/7/20, Zane van Iperen <zane@zanevaniperen.com> wrote: > On Mon, 6 Jul 2020 18:29:57 +0200 > "Michael Niedermayer" <michael@niedermayer.cc> wrote: > >> On Mon, Jul 06, 2020 at 09:24:20AM +0000, Zane van Iperen wrote: >> > Signed-off-by: Zane van Iperen <zane@zanevaniperen.com> >> > --- >> > libavformat/Makefile | 2 +- >> > libavformat/apm.c | 62 >> > ++++++++++++++++++++++++-------------------- 2 files changed, 35 >> > insertions(+), 29 deletions(-) >> >> I think the commit message should be more elaborate >> "avformat/apm: use new extradata format" >> doesnt really say what changes, why its changes >> > What's a nice way of saying "I made an error in the old format and > had to change it in order to keep things simple in the muxer."? > > Essentially, I > - Handle the dodgy WAVEFORMATEX (that I missed before) correctly, and > - Pass through the entire APMState structure to the decoder instead of > only some fields. > >> You also may want to add yourself to the MAINTAINERs file >> > Will do, I'll send it in a different patchset. > Would this warrant commit access? Because I'm including a GPG key, I'd > like to have the commit signed, and `git format-patch` doesn't like > signatures. For developing few really trivial codecs/formats? Sorry, but than anyone can have commit access. > > > 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-request@ffmpeg.org with subject "unsubscribe".
On Tue, Jul 07, 2020 at 09:46:27AM +0000, Zane van Iperen wrote: > On Mon, 6 Jul 2020 18:29:57 +0200 > "Michael Niedermayer" <michael@niedermayer.cc> wrote: > > > On Mon, Jul 06, 2020 at 09:24:20AM +0000, Zane van Iperen wrote: > > > Signed-off-by: Zane van Iperen <zane@zanevaniperen.com> [...] > > You also may want to add yourself to the MAINTAINERs file > > > Will do, I'll send it in a different patchset. > Would this warrant commit access? Because I'm including a GPG key, I'd > like to have the commit signed, and `git format-patch` doesn't like > signatures. One of the purposes of the MAINTAINERs file is to list the people with git write access. So normally, if someone is in the file, wants and needs git write, he/she can have git write. write access makes maintaince more easy and practical. thx [...]
On 7/7/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Jul 07, 2020 at 09:46:27AM +0000, Zane van Iperen wrote: >> On Mon, 6 Jul 2020 18:29:57 +0200 >> "Michael Niedermayer" <michael@niedermayer.cc> wrote: >> >> > On Mon, Jul 06, 2020 at 09:24:20AM +0000, Zane van Iperen wrote: >> > > Signed-off-by: Zane van Iperen <zane@zanevaniperen.com> > [...] >> > You also may want to add yourself to the MAINTAINERs file >> > >> Will do, I'll send it in a different patchset. >> Would this warrant commit access? Because I'm including a GPG key, I'd >> like to have the commit signed, and `git format-patch` doesn't like >> signatures. > > One of the purposes of the MAINTAINERs file is to list the people with > git write access. > So normally, if someone is in the file, wants and needs git write, > he/she can have git write. > write access makes maintaince more easy and practical. By this reasoning and ignorance you are indirectly confirming you are still FFmpeg leader. > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Nations do behave wisely once they have exhausted all other alternatives. > -- Abba Eban >
On Tue, Jul 07, 2020 at 02:52:23PM +0200, Paul B Mahol wrote: > On 7/7/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Tue, Jul 07, 2020 at 09:46:27AM +0000, Zane van Iperen wrote: > >> On Mon, 6 Jul 2020 18:29:57 +0200 > >> "Michael Niedermayer" <michael@niedermayer.cc> wrote: > >> > >> > On Mon, Jul 06, 2020 at 09:24:20AM +0000, Zane van Iperen wrote: > >> > > Signed-off-by: Zane van Iperen <zane@zanevaniperen.com> > > [...] > >> > You also may want to add yourself to the MAINTAINERs file > >> > > >> Will do, I'll send it in a different patchset. > >> Would this warrant commit access? Because I'm including a GPG key, I'd > >> like to have the commit signed, and `git format-patch` doesn't like > >> signatures. > > > > One of the purposes of the MAINTAINERs file is to list the people with > > git write access. > > So normally, if someone is in the file, wants and needs git write, > > he/she can have git write. > > write access makes maintaince more easy and practical. > > By this reasoning and ignorance you are indirectly confirming you are > still FFmpeg leader. The way the git access works is 1. Developer sends a patch to the MAINTAINERs file 2. people can object to that patch like to any other patch 3. patch is applied and the new maintainer is offered write access unless that is forgotten, the developer explicitly doesnt want write access or is too inexperienced with git. There is no leader involved in this process anywhere thx [...]
diff --git a/libavformat/Makefile b/libavformat/Makefile index 26af859a28..a4113fe644 100644 --- a/libavformat/Makefile +++ b/libavformat/Makefile @@ -93,7 +93,7 @@ OBJS-$(CONFIG_AMRWB_DEMUXER) += amr.o OBJS-$(CONFIG_ANM_DEMUXER) += anm.o OBJS-$(CONFIG_APC_DEMUXER) += apc.o OBJS-$(CONFIG_APE_DEMUXER) += ape.o apetag.o img2.o -OBJS-$(CONFIG_APM_DEMUXER) += apm.o riffdec.o +OBJS-$(CONFIG_APM_DEMUXER) += apm.o OBJS-$(CONFIG_APNG_DEMUXER) += apngdec.o OBJS-$(CONFIG_APNG_MUXER) += apngenc.o OBJS-$(CONFIG_APTX_DEMUXER) += aptxdec.o rawdec.o diff --git a/libavformat/apm.c b/libavformat/apm.c index dc59c16562..b51b9fcbe6 100644 --- a/libavformat/apm.c +++ b/libavformat/apm.c @@ -21,12 +21,13 @@ */ #include "avformat.h" #include "internal.h" -#include "riff.h" #include "libavutil/internal.h" #include "libavutil/intreadwrite.h" -#define APM_FILE_HEADER_SIZE 20 -#define APM_VS12_CHUNK_SIZE 76 +#define APM_FILE_HEADER_SIZE 18 +#define APM_FILE_EXTRADATA_SIZE 80 +#define APM_EXTRADATA_SIZE 28 + #define APM_MAX_READ_SIZE 4096 #define APM_TAG_CODEC 0x2000 @@ -51,6 +52,7 @@ typedef struct APMVS12Chunk { uint32_t unk2; APMState state; uint32_t pad[7]; + uint32_t data; } APMVS12Chunk; static void apm_parse_vs12(APMVS12Chunk *vs12, const uint8_t *buf) @@ -71,6 +73,8 @@ static void apm_parse_vs12(APMVS12Chunk *vs12, const uint8_t *buf) for (int i = 0; i < FF_ARRAY_ELEMS(vs12->pad); i++) vs12->pad[i] = AV_RL32(buf + 48 + (i * 4)); + + vs12->data = AV_RL32(buf + 76); } static int apm_probe(const AVProbeData *p) @@ -95,24 +99,29 @@ static int apm_read_header(AVFormatContext *s) int64_t ret; AVStream *st; APMVS12Chunk vs12; - uint8_t buf[APM_VS12_CHUNK_SIZE]; + uint8_t buf[APM_FILE_EXTRADATA_SIZE]; if (!(st = avformat_new_stream(s, NULL))) return AVERROR(ENOMEM); - /* The header starts with a WAVEFORMATEX */ - if ((ret = ff_get_wav_header(s, s->pb, st->codecpar, APM_FILE_HEADER_SIZE, 0)) < 0) - return ret; - - if (st->codecpar->bits_per_coded_sample != 4) + /* + * This is 98% a WAVEFORMATEX, but there's something screwy with the extradata + * that ff_get_wav_header() can't (and shouldn't) handle properly. + */ + if (avio_rl16(s->pb) != APM_TAG_CODEC) return AVERROR_INVALIDDATA; - if (st->codecpar->codec_tag != APM_TAG_CODEC) + st->codecpar->channels = avio_rl16(s->pb); + st->codecpar->sample_rate = avio_rl32(s->pb); + st->codecpar->bit_rate = avio_rl32(s->pb) * 8; + st->codecpar->block_align = avio_rl16(s->pb); + st->codecpar->bits_per_coded_sample = avio_rl16(s->pb); + + if (avio_rl16(s->pb) != APM_FILE_EXTRADATA_SIZE) return AVERROR_INVALIDDATA; - /* ff_get_wav_header() does most of the work, but we need to fix a few things. */ - st->codecpar->codec_id = AV_CODEC_ID_ADPCM_IMA_APM; - st->codecpar->codec_tag = 0; + if (st->codecpar->bits_per_coded_sample != 4) + return AVERROR_INVALIDDATA; if (st->codecpar->channels == 2) st->codecpar->channel_layout = AV_CH_LAYOUT_STEREO; @@ -121,38 +130,35 @@ static int apm_read_header(AVFormatContext *s) else return AVERROR_INVALIDDATA; + st->codecpar->codec_type = AVMEDIA_TYPE_AUDIO; + st->codecpar->codec_id = AV_CODEC_ID_ADPCM_IMA_APM; st->codecpar->format = AV_SAMPLE_FMT_S16; st->codecpar->bits_per_raw_sample = 16; - st->codecpar->bit_rate = st->codecpar->channels * - st->codecpar->sample_rate * - st->codecpar->bits_per_coded_sample; - if ((ret = avio_read(s->pb, buf, APM_VS12_CHUNK_SIZE)) < 0) + /* Now skip the pad that ruins everything. */ + if ((ret = avio_skip(s->pb, 2)) < 0) + return ret; + + if ((ret = avio_read(s->pb, buf, APM_FILE_EXTRADATA_SIZE)) < 0) return ret; - else if (ret != APM_VS12_CHUNK_SIZE) + else if (ret != APM_FILE_EXTRADATA_SIZE) return AVERROR(EIO); apm_parse_vs12(&vs12, buf); - if (vs12.magic != APM_TAG_VS12) { + if (vs12.magic != APM_TAG_VS12 || vs12.data != APM_TAG_DATA) return AVERROR_INVALIDDATA; - } if (vs12.state.has_saved) { avpriv_request_sample(s, "Saved Samples"); return AVERROR_PATCHWELCOME; } - if (avio_rl32(s->pb) != APM_TAG_DATA) - return AVERROR_INVALIDDATA; - - if ((ret = ff_alloc_extradata(st->codecpar, 16)) < 0) + if ((ret = ff_alloc_extradata(st->codecpar, APM_EXTRADATA_SIZE)) < 0) return ret; - AV_WL32(st->codecpar->extradata + 0, vs12.state.predictor_l); - AV_WL32(st->codecpar->extradata + 4, vs12.state.step_index_l); - AV_WL32(st->codecpar->extradata + 8, vs12.state.predictor_r); - AV_WL32(st->codecpar->extradata + 12, vs12.state.step_index_r); + /* Use the entire state as extradata. */ + memcpy(st->codecpar->extradata, buf + 20, APM_EXTRADATA_SIZE); avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate); st->start_time = 0;
Signed-off-by: Zane van Iperen <zane@zanevaniperen.com> --- libavformat/Makefile | 2 +- libavformat/apm.c | 62 ++++++++++++++++++++++++-------------------- 2 files changed, 35 insertions(+), 29 deletions(-)