| 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. | expand |
| 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 |
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
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.
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.
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
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 --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; }
Fixes bug #9198 Signed-off-by: Aidan Richmond <aidan.is@hotmail.co.uk> --- libavcodec/adpcm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)