diff mbox series

[FFmpeg-devel] avcodec/adpcm_argo: simplify and move duplicated logic into a function

Message ID 20200201065954.27106-1-zane@zanevaniperen.com
State Accepted
Headers show
Series [FFmpeg-devel] avcodec/adpcm_argo: simplify and move duplicated logic into a function
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Zane van Iperen Feb. 1, 2020, 6:59 a.m. UTC
Signed-off-by: Zane van Iperen <zane@zanevaniperen.com>
---
 libavcodec/adpcm.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

Comments

Michael Niedermayer Feb. 1, 2020, 6:04 p.m. UTC | #1
On Sat, Feb 01, 2020 at 06:59:59AM +0000, Zane van Iperen wrote:
> Signed-off-by: Zane van Iperen <zane@zanevaniperen.com>
> ---
>  libavcodec/adpcm.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)

it seems theres no fate test for adpcm_argo
this patch looks ok but the codepath seems untested, maybe you can add a test ?

thx

[...]
Zane van Iperen Feb. 1, 2020, 11:27 p.m. UTC | #2
2/2/20 4:04 am, Michael Niedermayer пишет:
> 
> On Sat, Feb 01, 2020 at 06:59:59AM +0000, Zane van Iperen wrote:
>> Signed-off-by: Zane van Iperen <zane@zanevaniperen.com>
>> ---
>>   libavcodec/adpcm.c | 40 ++++++++++++++++++----------------------
>>   1 file changed, 18 insertions(+), 22 deletions(-)
> 
> it seems theres no fate test for adpcm_argo
> this patch looks ok but the codepath seems untested, maybe you can add a test ?
> 

I can. Would it best be send as a v2, or as a separate patch completely?

In the meantime, this patch doesn't break anything, as it passes my usual checks.
These are the files that gave the most grief when writing the decoder and I
know their expected hashes.

$ ./ffmpeg -loglevel error -i OUTRO.ASF -map 0:a -f md5 -
MD5=e0428a6c55382f48502f04b3a7f7b94b

$ ./ffmpeg -loglevel error -i CBK2.asf -map 0:a -f md5 -
MD5=589f3f3c518e5d58aa404b7576db5b70



> thx
> 
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> I am the wisest man alive, for I know one thing, and that is that I know
> nothing. -- Socrates
> _______________________________________________
> 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".
> 

Zane
Michael Niedermayer Feb. 2, 2020, 10:35 a.m. UTC | #3
On Sat, Feb 01, 2020 at 11:27:47PM +0000, Zane van Iperen wrote:
> 2/2/20 4:04 am, Michael Niedermayer пишет:
> > 
> > On Sat, Feb 01, 2020 at 06:59:59AM +0000, Zane van Iperen wrote:
> >> Signed-off-by: Zane van Iperen <zane@zanevaniperen.com>
> >> ---
> >>   libavcodec/adpcm.c | 40 ++++++++++++++++++----------------------
> >>   1 file changed, 18 insertions(+), 22 deletions(-)
> > 
> > it seems theres no fate test for adpcm_argo
> > this patch looks ok but the codepath seems untested, maybe you can add a test ?
> > 
> 
> I can. Would it best be send as a v2, or as a separate patch completely?

whatever you prefer

thx

[...]
Michael Niedermayer Feb. 2, 2020, 4:05 p.m. UTC | #4
On Sat, Feb 01, 2020 at 11:27:47PM +0000, Zane van Iperen wrote:
> 2/2/20 4:04 am, Michael Niedermayer пишет:
> > 
> > On Sat, Feb 01, 2020 at 06:59:59AM +0000, Zane van Iperen wrote:
> >> Signed-off-by: Zane van Iperen <zane@zanevaniperen.com>
> >> ---
> >>   libavcodec/adpcm.c | 40 ++++++++++++++++++----------------------
> >>   1 file changed, 18 insertions(+), 22 deletions(-)
> > 
> > it seems theres no fate test for adpcm_argo
> > this patch looks ok but the codepath seems untested, maybe you can add a test ?
> > 
> 
> I can. Would it best be send as a v2, or as a separate patch completely?
> 
> In the meantime, this patch doesn't break anything, as it passes my usual checks.
> These are the files that gave the most grief when writing the decoder and I
> know their expected hashes.
> 
> $ ./ffmpeg -loglevel error -i OUTRO.ASF -map 0:a -f md5 -
> MD5=e0428a6c55382f48502f04b3a7f7b94b
> 
> $ ./ffmpeg -loglevel error -i CBK2.asf -map 0:a -f md5 -
> MD5=589f3f3c518e5d58aa404b7576db5b70

will apply

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/adpcm.c b/libavcodec/adpcm.c
index dad3da28d3..9a42353351 100644
--- a/libavcodec/adpcm.c
+++ b/libavcodec/adpcm.c
@@ -552,9 +552,21 @@  static void adpcm_swf_decode(AVCodecContext *avctx, const uint8_t *buf, int buf_
     }
 }
 
-static inline int16_t adpcm_argo_expand_nibble(int nibble, int shift, int16_t prev0, int16_t prev1)
+static inline int16_t adpcm_argo_expand_nibble(ADPCMChannelStatus *cs, int nibble, int control, int shift)
 {
-    return ((8 * prev0) - (4 * prev1) + (nibble * (1 << shift))) >> 2;
+    int sample = nibble * (1 << shift);
+
+    if (control & 0x04)
+        sample += (8 * cs->sample1) - (4 * cs->sample2);
+    else
+        sample += 4 * cs->sample1;
+
+    sample = av_clip_int16(sample >> 2);
+
+    cs->sample2 = cs->sample1;
+    cs->sample1 = sample;
+
+    return sample;
 }
 
 /**
@@ -1805,7 +1817,7 @@  static int adpcm_decode_frame(AVCodecContext *avctx, void *data,
          * They should be 0 initially.
          */
         for (channel = 0; channel < avctx->channels; channel++) {
-            int control, shift, sample, nibble;
+            int control, shift;
 
             samples = samples_p[channel];
             cs = c->status + channel;
@@ -1815,25 +1827,9 @@  static int adpcm_decode_frame(AVCodecContext *avctx, void *data,
             shift = (control >> 4) + 2;
 
             for (n = 0; n < nb_samples / 2; n++) {
-                sample = bytestream2_get_byteu(&gb);
-
-                nibble = sign_extend(sample >> 4, 4);
-                if (control & 0x04)
-                    *samples = adpcm_argo_expand_nibble(nibble, shift, cs->sample1, cs->sample2);
-                else
-                    *samples = adpcm_argo_expand_nibble(nibble, shift, cs->sample1, cs->sample1);
-
-                cs->sample2 = cs->sample1;
-                cs->sample1 = *samples++;
-
-                nibble = sign_extend(sample >> 0, 4);
-                if (control & 0x04)
-                    *samples = adpcm_argo_expand_nibble(nibble, shift, cs->sample1, cs->sample2);
-                else
-                    *samples = adpcm_argo_expand_nibble(nibble, shift, cs->sample1, cs->sample1);
-
-                cs->sample2 = cs->sample1;
-                cs->sample1 = *samples++;
+                int sample = bytestream2_get_byteu(&gb);
+                *samples++ = adpcm_argo_expand_nibble(cs, sign_extend(sample >> 4, 4), control, shift);
+                *samples++ = adpcm_argo_expand_nibble(cs, sign_extend(sample >> 0, 4), control, shift);
             }
         }
         break;