diff mbox series

[FFmpeg-devel,v4,3/3] avcodec/adpcm: Fixes output from Westwood ADPCM.

Message ID AM9PR09MB51702B2048EF6D1779A731E1C5449@AM9PR09MB5170.eurprd09.prod.outlook.com
State Accepted
Headers show
Series [FFmpeg-devel,v4,1/3] avcodec/adpcmenc: Adds encoder for Westwood ADPCM.
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate fail Make fate failed
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate warning Make fate failed

Commit Message

Aidan Richmond April 24, 2021, 10:42 p.m. UTC
Fixes bug #9198

Signed-off-by: Aidan Richmond <aidan.is@hotmail.co.uk>
---
 libavcodec/adpcm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Andreas Rheinhardt April 24, 2021, 11:40 p.m. UTC | #1
Aidan Richmond:
> Fixes bug #9198
> 
> Signed-off-by: Aidan Richmond <aidan.is@hotmail.co.uk>
> ---
>  libavcodec/adpcm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/adpcm.c b/libavcodec/adpcm.c
> index be14607eac..5ec9691001 100644
> --- a/libavcodec/adpcm.c
> +++ b/libavcodec/adpcm.c
> @@ -1400,16 +1400,16 @@ static int adpcm_decode_frame(AVCodecContext *avctx, void *data,
>  
>                  for (n = nb_samples / 2; n > 0; n--) {
>                      int v = bytestream2_get_byteu(&gb);
> -                    *smp++ = adpcm_ima_expand_nibble(&c->status[channel], v >> 4  , 3);
>                      *smp++ = adpcm_ima_expand_nibble(&c->status[channel], v & 0x0F, 3);
> +                    *smp++ = adpcm_ima_expand_nibble(&c->status[channel], v >> 4  , 3);
>                  }
>              }
>          } else {
>              for (n = nb_samples / 2; n > 0; n--) {
>                  for (channel = 0; channel < avctx->channels; channel++) {
>                      int v = bytestream2_get_byteu(&gb);
> -                    *samples++  = adpcm_ima_expand_nibble(&c->status[channel], v >> 4  , 3);
> -                    samples[st] = adpcm_ima_expand_nibble(&c->status[channel], v & 0x0F, 3);
> +                    *samples++  = adpcm_ima_expand_nibble(&c->status[channel], v & 0x0F, 3);
> +                    samples[st] = adpcm_ima_expand_nibble(&c->status[channel], v >> 4  , 3);
>                  }
>                  samples += avctx->channels;
>              }
> 
This format is covered by FATE (namely the adpcm-ima-ws test) and so you
need to update the checksums in your patch; of course you should verify
that your change is correct. See https://ffmpeg.org/fate.html for more.

- Andreas
Zane van Iperen April 24, 2021, 11:49 p.m. UTC | #2
On 25/4/21 9:40 am, Andreas Rheinhardt wrote:

> This format is covered by FATE (namely the adpcm-ima-ws test) and so you
> need to update the checksums in your patch; of course you should verify
> that your change is correct. See https://ffmpeg.org/fate.html for more.
> 
> - Andreas
> _______________________________________________

The attached file here https://trac.ffmpeg.org/ticket/9198 decodes
incorrectly without the patch. I'd be inclined to say that it's correct.

The issue isn't caught by FATE because it only tests a mono sample.


I'll merge this patch shortly if you have no objections.

After the encoder's added, I'll add an ENCDEC test which should cover the stereo case.
Andreas Rheinhardt April 25, 2021, 1:02 a.m. UTC | #3
Zane van Iperen:
> 
> 
> On 25/4/21 9:40 am, Andreas Rheinhardt wrote:
> 
>> This format is covered by FATE (namely the adpcm-ima-ws test) and so you
>> need to update the checksums in your patch; of course you should verify
>> that your change is correct. See https://ffmpeg.org/fate.html for more.
>>
>> - Andreas
>> _______________________________________________
> 
> The attached file here https://trac.ffmpeg.org/ticket/9198 decodes
> incorrectly without the patch. I'd be inclined to say that it's correct.
> 
> The issue isn't caught by FATE because it only tests a mono sample.
> 

If it weren't caught by FATE, the test wouldn't need an update.

Both the sample from the ticket as well as the fate-sample
(vqa/cc-demo1-partial.vqa) are 22050 Hz, mono, s16; the fate sample is
version 2, the new sample version 0. None of them test the version 3
codepath, so the change to it is untested.
I have tested a few files and the changes to the non-version 3 codepath
seem to be correct; I have also found out that your commit
c012f9b265e172de9c240c9dfab8665936fa3e83 made decoding
https://samples.ffmpeg.org/game-formats/vqa/Tiberian Sun
VQAs/startup.vqa (which is a version 3 format ima-ws file) crash.

> 
> I'll merge this patch shortly if you have no objections.
> 
> After the encoder's added, I'll add an ENCDEC test which should cover
> the stereo case.
Zane van Iperen April 25, 2021, 1:41 a.m. UTC | #4
On 25/4/21 11:02 am, Andreas Rheinhardt wrote:
> Zane van Iperen:
>>
>>
>> On 25/4/21 9:40 am, Andreas Rheinhardt wrote:
>>
>>> This format is covered by FATE (namely the adpcm-ima-ws test) and so you
>>> need to update the checksums in your patch; of course you should verify
>>> that your change is correct. See https://ffmpeg.org/fate.html for more.
>>>
>>> - Andreas
>>> _______________________________________________
>>
>> The attached file here https://trac.ffmpeg.org/ticket/9198 decodes
>> incorrectly without the patch. I'd be inclined to say that it's correct.
>>
>> The issue isn't caught by FATE because it only tests a mono sample.
>>
> 
> If it weren't caught by FATE, the test wouldn't need an update.
> 

Interesting, FATE didn't catch it for me. Which test is it?

$ make fate-adpcm fate-acodec fate-westwood-aud SAMPLES=fate-suite/

All pass for me.

> Both the sample from the ticket as well as the fate-sample
> (vqa/cc-demo1-partial.vqa) are 22050 Hz, mono, s16; the fate sample is
> version 2, the new sample version 0. None of them test the version 3
> codepath, so the change to it is untested.

Yeah, don't know where stereo came from. I need to sleep more...

What I meant was see this. Top is before, bottom is after:

https://0x0.st/-mex.png

> I have tested a few files and the changes to the non-version 3 codepath
> seem to be correct; I have also found out that your commit
> c012f9b265e172de9c240c9dfab8665936fa3e83 made decoding
> https://samples.ffmpeg.org/game-formats/vqa/Tiberian Sun
> VQAs/startup.vqa (which is a version 3 format ima-ws file) crash.
> 

I've fixed the crash, see https://ffmpeg.org/pipermail/ffmpeg-devel/2021-April/279504.html
Zane van Iperen April 25, 2021, 4:48 a.m. UTC | #5
On 25/4/21 11:41 am, Zane van Iperen wrote:

> Interesting, FATE didn't catch it for me. Which test is it?
> 
> $ make fate-adpcm fate-acodec fate-westwood-aud SAMPLES=fate-suite/
> 
> All pass for me.
> 

Never mind, found it.

>> Both the sample from the ticket as well as the fate-sample
>> (vqa/cc-demo1-partial.vqa) are 22050 Hz, mono, s16; the fate sample is
>> version 2, the new sample version 0. None of them test the version 3
>> codepath, so the change to it is untested.
> 
> Yeah, don't know where stereo came from. I need to sleep more...
> 
> What I meant was see this. Top is before, bottom is after:
> 
> https://0x0.st/-mex.png
> 

I've also confirmed all the "https://samples.ffmpeg.org/game-formats/vqa/Tiberian Sun VQAs" samples
exhibit the same behaviour without the patch.

>> I have tested a few files and the changes to the non-version 3 codepath
>> seem to be correct; I have also found out that your commit
>> c012f9b265e172de9c240c9dfab8665936fa3e83 made decoding
>> https://samples.ffmpeg.org/game-formats/vqa/Tiberian Sun
>> VQAs/startup.vqa (which is a version 3 format ima-ws file) crash.
>>
> 
> I've fixed the crash, see https://ffmpeg.org/pipermail/ffmpeg-devel/2021-April/279504.html
> 
> 

Once the patch (and my crash fix) are merged, I'll submit a test case for v3 VQA files so this doesn't happen again.
diff mbox series

Patch

diff --git a/libavcodec/adpcm.c b/libavcodec/adpcm.c
index be14607eac..5ec9691001 100644
--- a/libavcodec/adpcm.c
+++ b/libavcodec/adpcm.c
@@ -1400,16 +1400,16 @@  static int adpcm_decode_frame(AVCodecContext *avctx, void *data,
 
                 for (n = nb_samples / 2; n > 0; n--) {
                     int v = bytestream2_get_byteu(&gb);
-                    *smp++ = adpcm_ima_expand_nibble(&c->status[channel], v >> 4  , 3);
                     *smp++ = adpcm_ima_expand_nibble(&c->status[channel], v & 0x0F, 3);
+                    *smp++ = adpcm_ima_expand_nibble(&c->status[channel], v >> 4  , 3);
                 }
             }
         } else {
             for (n = nb_samples / 2; n > 0; n--) {
                 for (channel = 0; channel < avctx->channels; channel++) {
                     int v = bytestream2_get_byteu(&gb);
-                    *samples++  = adpcm_ima_expand_nibble(&c->status[channel], v >> 4  , 3);
-                    samples[st] = adpcm_ima_expand_nibble(&c->status[channel], v & 0x0F, 3);
+                    *samples++  = adpcm_ima_expand_nibble(&c->status[channel], v & 0x0F, 3);
+                    samples[st] = adpcm_ima_expand_nibble(&c->status[channel], v >> 4  , 3);
                 }
                 samples += avctx->channels;
             }