diff mbox

[FFmpeg-devel,v2] avformat/westwood_aud: Adds PCM format demux.

Message ID AM4PR1001MB13462CFA70D76258E02D0294C5400@AM4PR1001MB1346.EURPRD10.PROD.OUTLOOK.COM
State New
Headers show

Commit Message

Aidan Richmond March 19, 2019, 10:53 p.m. UTC 
PCM format AUD files are found in Westwood's Blade Runner game.
---
 libavformat/westwood_aud.c | 83 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 66 insertions(+), 17 deletions(-)

Comments

Tomas Härdin March 20, 2019, 1:26 p.m. UTC | #1 
tis 2019-03-19 klockan 22:53 +0000 skrev Aidan R:
> @@ -69,13 +75,25 @@ static int wsaud_probe(AVProbeData *p)
>      if (p->buf[10] & 0xFC)
>          return 0;
>  
> -    if (p->buf[11] != 99 && p->buf[11] != 1)
> +    /* valid format values are 99 == adpcm, 1 == snd1 and 0 == pcm */
> +    if (p->buf[11] != 99 && p->buf[11] != 1 && p->buf[11] != 0)
>          return 0;
>  
> -    /* read ahead to the first audio chunk and validate the first header signature */
> -    if (AV_RL32(&p->buf[16]) != AUD_CHUNK_SIGNATURE)
> +    /* read ahead to the first audio chunk and validate the first header
> +     * signature pcm format does not use a chunk format, so don't check

Missing a period between "pcm" and "format"?

> @@ -130,20 +161,24 @@ static int wsaud_read_packet(AVFormatContext *s,
>                               AVPacket *pkt)
>  {
>      AVIOContext *pb = s->pb;
> +    AUDDemuxContext *aud = s->priv_data;
>      unsigned char preamble[AUD_CHUNK_PREAMBLE_SIZE];
> -    unsigned int chunk_size;
> +    unsigned int chunk_size, bytes_per_sample;
>      int ret = 0;
>      AVStream *st = s->streams[0];
>  
> -    if (avio_read(pb, preamble, AUD_CHUNK_PREAMBLE_SIZE) !=
> -        AUD_CHUNK_PREAMBLE_SIZE)
> -        return AVERROR(EIO);
> +    /* AUD files don't store PCM audio in chunks */
> +    if (st->codecpar->codec_id != AV_CODEC_ID_PCM_S16LE) {

What about AV_CODEC_ID_PCM_U8?

A sample + FATE test for this would be nice

/Tomas
Aidan Richmond March 20, 2019, 3:56 p.m. UTC | #2 
tis 2019-03-19 klockan 22:53 +0000 skrev Aidan R:
> @@ -69,13 +75,25 @@ static int wsaud_probe(AVProbeData *p)
>>      if (p->buf[10] & 0xFC)
>>          return 0;
>>  
>> -    if (p->buf[11] != 99 && p->buf[11] != 1)
>> +    /* valid format values are 99 == adpcm, 1 == snd1 and 0 == pcm */
>> +    if (p->buf[11] != 99 && p->buf[11] != 1 && p->buf[11] != 0)
>>          return 0;
>>  
>> -    /* read ahead to the first audio chunk and validate the first header signature */
>> -    if (AV_RL32(&p->buf[16]) != AUD_CHUNK_SIGNATURE)
>> +    /* read ahead to the first audio chunk and validate the first header
>> +     * signature pcm format does not use a chunk format, so don't check
>
> Missing a period between "pcm" and "format"?

Missing after signature after re-reading it myself.

>
>> @@ -130,20 +161,24 @@ static int wsaud_read_packet(AVFormatContext *s,
>>                               AVPacket *pkt)
>>  {
>>      AVIOContext *pb = s->pb;
>> +    AUDDemuxContext *aud = s->priv_data;
>>      unsigned char preamble[AUD_CHUNK_PREAMBLE_SIZE];
>> -    unsigned int chunk_size;
>> +    unsigned int chunk_size, bytes_per_sample;
>>      int ret = 0;
>>      AVStream *st = s->streams[0];
>>  
>> -    if (avio_read(pb, preamble, AUD_CHUNK_PREAMBLE_SIZE) !=
>> -        AUD_CHUNK_PREAMBLE_SIZE)
>> -        return AVERROR(EIO);
>> +    /* AUD files don't store PCM audio in chunks */
>> +    if (st->codecpar->codec_id != AV_CODEC_ID_PCM_S16LE) {
>
>What about AV_CODEC_ID_PCM_U8?

Good catch, I don't think any actually exist in the wild, but I have supported
the possibility in wsaud_read_header and from looking at the audio engine it
looks to support it in theory.

>
>A sample + FATE test for this would be nice

Some guidance on where to start for that would be most welcome. I have short
examples from Blade Runner that could be used but wouldn't distributing them
violate copyright?
Tomas Härdin March 20, 2019, 4:11 p.m. UTC | #3 
ons 2019-03-20 klockan 15:56 +0000 skrev Aidan R:
> tis 2019-03-19 klockan 22:53 +0000 skrev Aidan R:
> > > @@ -130,20 +161,24 @@ static int wsaud_read_packet(AVFormatContext *s,
> > >                               AVPacket *pkt)
> > >  {
> > >      AVIOContext *pb = s->pb;
> > > +    AUDDemuxContext *aud = s->priv_data;
> > >      unsigned char preamble[AUD_CHUNK_PREAMBLE_SIZE];
> > > -    unsigned int chunk_size;
> > > +    unsigned int chunk_size, bytes_per_sample;
> > >      int ret = 0;
> > >      AVStream *st = s->streams[0];
> > >  
> > > -    if (avio_read(pb, preamble, AUD_CHUNK_PREAMBLE_SIZE) !=
> > > -        AUD_CHUNK_PREAMBLE_SIZE)
> > > -        return AVERROR(EIO);
> > > +    /* AUD files don't store PCM audio in chunks */
> > > +    if (st->codecpar->codec_id != AV_CODEC_ID_PCM_S16LE) {
> > 
> > What about AV_CODEC_ID_PCM_U8?
> 
> Good catch, I don't think any actually exist in the wild, but I have supported
> the possibility in wsaud_read_header and from looking at the audio engine it
> looks to support it in theory.

Given the recent 24-bit ZMBV discussion on this list, I think we should
not implement support for things which we don't have samples for. Using
it in the probe function is probably fine, but we should error out in
wsaud_read_header() until we have an 8-bit sample. The U8
implementation in this patch can be dummied out with #ifdefs until
then, for convenient un-#ifdefing

> > 
> > A sample + FATE test for this would be nice
> 
> Some guidance on where to start for that would be most welcome. I have short
> examples from Blade Runner that could be used but wouldn't distributing them
> violate copyright?

Cutting them down to a second or two should be fine I think. If we want
to be paranoid we could zero out the sample data as well, but I doubt
anyone cares. There's plenty of such samples in FATE. We could do this
in a separate thread, it doesn't have to hold up this patch

/Tomas
Michael Niedermayer March 20, 2019, 7:08 p.m. UTC | #4 
On Tue, Mar 19, 2019 at 10:53:11PM +0000, Aidan R wrote:
> PCM format AUD files are found in Westwood's Blade Runner game.
> ---
>  libavformat/westwood_aud.c | 83 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 66 insertions(+), 17 deletions(-)
> 
> diff --git a/libavformat/westwood_aud.c b/libavformat/westwood_aud.c
> index 9c2d35cb8a..b25d299bf0 100644
> --- a/libavformat/westwood_aud.c
> +++ b/libavformat/westwood_aud.c
> @@ -41,6 +41,12 @@
>  #define AUD_HEADER_SIZE 12
>  #define AUD_CHUNK_PREAMBLE_SIZE 8
>  #define AUD_CHUNK_SIGNATURE 0x0000DEAF
> +#define AUD_PCM_CHUNK_SIZE 2048 /* arbitrary size to pull out of PCM data*/
> +
> +typedef struct AUDDemuxContext {
> +    int size;
> +    int pos;
> +} AUDDemuxContext;
>  
>  static int wsaud_probe(AVProbeData *p)
>  {
> @@ -50,10 +56,10 @@ static int wsaud_probe(AVProbeData *p)
>       * so perform sanity checks on various header parameters:
>       *   8000 <= sample rate (16 bits) <= 48000  ==> 40001 acceptable numbers
>       *   flags <= 0x03 (2 LSBs are used)         ==> 4 acceptable numbers
> -     *   compression type (8 bits) = 1 or 99     ==> 2 acceptable numbers
> +     *   compression type (8 bits) = 0, 1 or 99  ==> 3 acceptable numbers
>       *   first audio chunk signature (32 bits)   ==> 1 acceptable number
> -     * The number space contains 2^64 numbers. There are 40001 * 4 * 2 * 1 =
> -     * 320008 acceptable number combinations.
> +     * The number space contains 2^64 numbers. There are 40001 * 4 * 3 * 1 =
> +     * 480012 acceptable number combinations.
>       */
>  
>      if (p->buf_size < AUD_HEADER_SIZE + AUD_CHUNK_PREAMBLE_SIZE)
> @@ -69,13 +75,25 @@ static int wsaud_probe(AVProbeData *p)
>      if (p->buf[10] & 0xFC)
>          return 0;
>  
> -    if (p->buf[11] != 99 && p->buf[11] != 1)
> +    /* valid format values are 99 == adpcm, 1 == snd1 and 0 == pcm */
> +    if (p->buf[11] != 99 && p->buf[11] != 1 && p->buf[11] != 0)
>          return 0;
>  
> -    /* read ahead to the first audio chunk and validate the first header signature */
> -    if (AV_RL32(&p->buf[16]) != AUD_CHUNK_SIGNATURE)
> +    /* read ahead to the first audio chunk and validate the first header
> +     * signature pcm format does not use a chunk format, so don't check
> +     * for that codec */
> +    if (p->buf[11] != 0 && AV_RL32(&p->buf[16]) != AUD_CHUNK_SIGNATURE)
>          return 0;
>  
> +    /* if we have pcm format then compressed size should equal
> +     * uncompressed size */
> +    if (p->buf[11] == 0)  {
> +        if (AV_RL32(&p->buf[2]) != AV_RL32(&p->buf[6]))
> +            return 0;
> +        if (AV_RL32(&p->buf[2]) + AUD_HEADER_SIZE < p->buf_size)
> +            return 0;
> +    }
> +
>      /* return 1/2 certainty since this file check is a little sketchy */
>      return AVPROBE_SCORE_EXTENSION;
>  }

fails probetest:


tools/probetest 256 4096
testing size=1
testing size=2
testing size=4
testing size=8
testing size=16
testing size=32
Failure of wsaud probing code with score=50 type=3 p=2D8 size=32
testing size=64
testing size=128
testing size=256
testing size=512
testing size=1024
testing size=2048

[...]
Aidan Richmond March 20, 2019, 10:20 p.m. UTC | #5 
From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> on behalf of Michael Niedermayer <michael@niedermayer.cc>
Sent: 20 March 2019 19:08
To: FFmpeg development discussions and patches
Subject: Re: [FFmpeg-devel] [PATCH v2] avformat/westwood_aud: Adds PCM format demux.

> fails probetest:
> 
> 
> tools/probetest 256 4096
> testing size=1
> testing size=2
> testing size=4
> testing size=8
> testing size=16
> testing size=32
> Failure of wsaud probing code with score=50 type=3 p=2D8 size=32
> testing size=64
> testing size=128
> testing size=256
> testing size=512
> testing size=1024
> testing size=2048

What is the appropriate way to resolve this if the format doesn't provide enough information to identify it uniquely? Return AVPROBE_SCORE_RETRY when we know we can't identify a file with a high enough confidence?
Carl Eugen Hoyos March 20, 2019, 10:44 p.m. UTC | #6 
2019-03-20 23:20 GMT+01:00, Aidan R <aidan.is@hotmail.co.uk>:
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> on behalf of Michael
> Niedermayer <michael@niedermayer.cc>
> Sent: 20 March 2019 19:08
> To: FFmpeg development discussions and patches
> Subject: Re: [FFmpeg-devel] [PATCH v2] avformat/westwood_aud: Adds PCM
> format demux.
>
>> fails probetest:
>>
>>
>> tools/probetest 256 4096
>> testing size=1
>> testing size=2
>> testing size=4
>> testing size=8
>> testing size=16
>> testing size=32
>> Failure of wsaud probing code with score=50 type=3 p=2D8 size=32
>> testing size=64
>> testing size=128
>> testing size=256
>> testing size=512
>> testing size=1024
>> testing size=2048
>
> What is the appropriate way to resolve this if the format doesn't
> provide enough information to identify it uniquely? Return
> AVPROBE_SCORE_RETRY when we know we can't identify
> a file with a high enough confidence?

Another possibility would be to commit the patch without
the change to the probe function (you can still force the
format) and then think about the probe function independently.

Carl Eugen
diff mbox

Patch

diff --git a/libavformat/westwood_aud.c b/libavformat/westwood_aud.c
index 9c2d35cb8a..b25d299bf0 100644
--- a/libavformat/westwood_aud.c
+++ b/libavformat/westwood_aud.c
@@ -41,6 +41,12 @@ 
 #define AUD_HEADER_SIZE 12
 #define AUD_CHUNK_PREAMBLE_SIZE 8
 #define AUD_CHUNK_SIGNATURE 0x0000DEAF
+#define AUD_PCM_CHUNK_SIZE 2048 /* arbitrary size to pull out of PCM data*/
+
+typedef struct AUDDemuxContext {
+    int size;
+    int pos;
+} AUDDemuxContext;
 
 static int wsaud_probe(AVProbeData *p)
 {
@@ -50,10 +56,10 @@  static int wsaud_probe(AVProbeData *p)
      * so perform sanity checks on various header parameters:
      *   8000 <= sample rate (16 bits) <= 48000  ==> 40001 acceptable numbers
      *   flags <= 0x03 (2 LSBs are used)         ==> 4 acceptable numbers
-     *   compression type (8 bits) = 1 or 99     ==> 2 acceptable numbers
+     *   compression type (8 bits) = 0, 1 or 99  ==> 3 acceptable numbers
      *   first audio chunk signature (32 bits)   ==> 1 acceptable number
-     * The number space contains 2^64 numbers. There are 40001 * 4 * 2 * 1 =
-     * 320008 acceptable number combinations.
+     * The number space contains 2^64 numbers. There are 40001 * 4 * 3 * 1 =
+     * 480012 acceptable number combinations.
      */
 
     if (p->buf_size < AUD_HEADER_SIZE + AUD_CHUNK_PREAMBLE_SIZE)
@@ -69,13 +75,25 @@  static int wsaud_probe(AVProbeData *p)
     if (p->buf[10] & 0xFC)
         return 0;
 
-    if (p->buf[11] != 99 && p->buf[11] != 1)
+    /* valid format values are 99 == adpcm, 1 == snd1 and 0 == pcm */
+    if (p->buf[11] != 99 && p->buf[11] != 1 && p->buf[11] != 0)
         return 0;
 
-    /* read ahead to the first audio chunk and validate the first header signature */
-    if (AV_RL32(&p->buf[16]) != AUD_CHUNK_SIGNATURE)
+    /* read ahead to the first audio chunk and validate the first header
+     * signature pcm format does not use a chunk format, so don't check
+     * for that codec */
+    if (p->buf[11] != 0 && AV_RL32(&p->buf[16]) != AUD_CHUNK_SIGNATURE)
         return 0;
 
+    /* if we have pcm format then compressed size should equal
+     * uncompressed size */
+    if (p->buf[11] == 0)  {
+        if (AV_RL32(&p->buf[2]) != AV_RL32(&p->buf[6]))
+            return 0;
+        if (AV_RL32(&p->buf[2]) + AUD_HEADER_SIZE < p->buf_size)
+            return 0;
+    }
+
     /* return 1/2 certainty since this file check is a little sketchy */
     return AVPROBE_SCORE_EXTENSION;
 }
@@ -83,16 +101,20 @@  static int wsaud_probe(AVProbeData *p)
 static int wsaud_read_header(AVFormatContext *s)
 {
     AVIOContext *pb = s->pb;
+    AUDDemuxContext *aud = s->priv_data;
     AVStream *st;
     unsigned char header[AUD_HEADER_SIZE];
-    int sample_rate, channels, codec;
+    int sample_rate, channels, codec, bits_per_sample;
 
     if (avio_read(pb, header, AUD_HEADER_SIZE) != AUD_HEADER_SIZE)
         return AVERROR(EIO);
 
     sample_rate = AV_RL16(&header[0]);
     channels    = (header[10] & 0x1) + 1;
+    bits_per_sample = (header[10] & 0x2) ? 16 : 8;
     codec       = header[11];
+    aud->size   = AV_RL32(&header[2]);
+    aud->pos    = 0; /* used to track position in a PCM stream */
 
     /* initialize the audio decoder stream */
     st = avformat_new_stream(s, NULL);
@@ -100,6 +122,15 @@  static int wsaud_read_header(AVFormatContext *s)
         return AVERROR(ENOMEM);
 
     switch (codec) {
+    case  0:
+        if (bits_per_sample == 8) {
+            st->codecpar->codec_id = AV_CODEC_ID_PCM_U8;
+        } else {
+            st->codecpar->codec_id = AV_CODEC_ID_PCM_S16LE;
+        }
+        st->codecpar->bits_per_coded_sample = bits_per_sample;
+        st->codecpar->bit_rate = channels * sample_rate * bits_per_sample;
+        break;
     case  1:
         if (channels != 1) {
             avpriv_request_sample(s, "Stereo WS-SND1");
@@ -130,20 +161,24 @@  static int wsaud_read_packet(AVFormatContext *s,
                              AVPacket *pkt)
 {
     AVIOContext *pb = s->pb;
+    AUDDemuxContext *aud = s->priv_data;
     unsigned char preamble[AUD_CHUNK_PREAMBLE_SIZE];
-    unsigned int chunk_size;
+    unsigned int chunk_size, bytes_per_sample;
     int ret = 0;
     AVStream *st = s->streams[0];
 
-    if (avio_read(pb, preamble, AUD_CHUNK_PREAMBLE_SIZE) !=
-        AUD_CHUNK_PREAMBLE_SIZE)
-        return AVERROR(EIO);
+    /* AUD files don't store PCM audio in chunks */
+    if (st->codecpar->codec_id != AV_CODEC_ID_PCM_S16LE) {
+        if (avio_read(pb, preamble, AUD_CHUNK_PREAMBLE_SIZE) !=
+            AUD_CHUNK_PREAMBLE_SIZE)
+            return AVERROR(EIO);
 
-    /* validate the chunk */
-    if (AV_RL32(&preamble[4]) != AUD_CHUNK_SIGNATURE)
-        return AVERROR_INVALIDDATA;
+        /* validate the chunk */
+        if (AV_RL32(&preamble[4]) != AUD_CHUNK_SIGNATURE)
+            return AVERROR_INVALIDDATA;
 
-    chunk_size = AV_RL16(&preamble[0]);
+        chunk_size = AV_RL16(&preamble[0]);
+    }
 
     if (st->codecpar->codec_id == AV_CODEC_ID_WESTWOOD_SND1) {
         /* For Westwood SND1 audio we need to add the output size and input
@@ -159,7 +194,7 @@  static int wsaud_read_packet(AVFormatContext *s,
         AV_WL16(&pkt->data[2], chunk_size);
 
         pkt->duration = out_size;
-    } else {
+    } else if (st->codecpar->codec_id == AV_CODEC_ID_ADPCM_IMA_WS) {
         ret = av_get_packet(pb, pkt, chunk_size);
         if (ret != chunk_size)
             return AVERROR(EIO);
@@ -172,7 +207,20 @@  static int wsaud_read_packet(AVFormatContext *s,
 
         /* 2 samples/byte, 1 or 2 samples per frame depending on stereo */
         pkt->duration = (chunk_size * 2) / st->codecpar->channels;
-    }
+    } else {
+        chunk_size = FFMIN(aud->size - aud->pos, AUD_PCM_CHUNK_SIZE);
+        if(chunk_size <= 0) 
+            return AVERROR_EOF;
+
+        aud->pos += chunk_size;
+        ret = av_get_packet(pb, pkt, chunk_size);
+        if (ret != chunk_size)
+            return AVERROR(EIO);
+
+        bytes_per_sample = st->codecpar->bits_per_coded_sample / 8;
+        pkt->duration = (chunk_size / bytes_per_sample) /
+                         st->codecpar->channels;
+    } 
     pkt->stream_index = st->index;
 
     return ret;
@@ -181,6 +229,7 @@  static int wsaud_read_packet(AVFormatContext *s,
 AVInputFormat ff_wsaud_demuxer = {
     .name           = "wsaud",
     .long_name      = NULL_IF_CONFIG_SMALL("Westwood Studios audio"),
+    .priv_data_size = sizeof(AUDDemuxContext),
     .read_probe     = wsaud_probe,
     .read_header    = wsaud_read_header,
     .read_packet    = wsaud_read_packet,