diff mbox series

[FFmpeg-devel,v5,1/6] avformat/apm: use new extradata format

Message ID 20200706092347.102557-2-zane@zanevaniperen.com
State Superseded
Headers show
Series adpcm_ima_apm encoder + apm muxer
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Zane van Iperen July 6, 2020, 9:24 a.m. UTC
Signed-off-by: Zane van Iperen <zane@zanevaniperen.com>
---
 libavformat/Makefile |  2 +-
 libavformat/apm.c    | 62 ++++++++++++++++++++++++--------------------
 2 files changed, 35 insertions(+), 29 deletions(-)

Comments

Michael Niedermayer July 6, 2020, 4:29 p.m. UTC | #1
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

[...]
Zane van Iperen July 7, 2020, 9:46 a.m. UTC | #2
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
Paul B Mahol July 7, 2020, 12:08 p.m. UTC | #3
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".
Michael Niedermayer July 7, 2020, 12:33 p.m. UTC | #4
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

[...]
Paul B Mahol July 7, 2020, 12:52 p.m. UTC | #5
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
>
Michael Niedermayer July 7, 2020, 1:46 p.m. UTC | #6
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 mbox series

Patch

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;