volume: Fix handling of volume>=4.0 for 8 and 16 bit integer formats

Also add a unit test for this. Previously volumes bigger than 4.0
would have resulted in overflows in the fixed point processing.

Fixes bug #649642.

diff --git a/gst/volume/gstvolume.c b/gst/volume/gstvolume.c
index 03145bf..152943d 100644 (file)
--- a/gst/volume/gstvolume.c
+++ b/gst/volume/gstvolume.c
@@ -63,12 +63,12 @@
 /* the volume factor is a range from 0.0 to (arbitrary) VOLUME_MAX_DOUBLE = 10.0
  * we map 1.0 to VOLUME_UNITY_INT*
  */
-#define VOLUME_UNITY_INT8            32 /* internal int for unity 2^(8-3) */
-#define VOLUME_UNITY_INT8_BIT_SHIFT  5  /* number of bits to shift for unity */
-#define VOLUME_UNITY_INT16           8192       /* internal int for unity 2^(16-3) */
-#define VOLUME_UNITY_INT16_BIT_SHIFT 13 /* number of bits to shift for unity */
-#define VOLUME_UNITY_INT24           2097152    /* internal int for unity 2^(24-3) */
-#define VOLUME_UNITY_INT24_BIT_SHIFT 21 /* number of bits to shift for unity */
+#define VOLUME_UNITY_INT8            8  /* internal int for unity 2^(8-5) */
+#define VOLUME_UNITY_INT8_BIT_SHIFT  3  /* number of bits to shift for unity */
+#define VOLUME_UNITY_INT16           2048       /* internal int for unity 2^(16-5) */
+#define VOLUME_UNITY_INT16_BIT_SHIFT 11 /* number of bits to shift for unity */
+#define VOLUME_UNITY_INT24           524288     /* internal int for unity 2^(24-5) */
+#define VOLUME_UNITY_INT24_BIT_SHIFT 19 /* number of bits to shift for unity */
 #define VOLUME_UNITY_INT32           134217728  /* internal int for unity 2^(32-5) */
 #define VOLUME_UNITY_INT32_BIT_SHIFT 27
 #define VOLUME_MAX_DOUBLE            10.0
@@ -702,7 +702,7 @@ volume_process_int16 (GstVolume * self, gpointer bytes, guint n_bytes)
   guint num_samples = n_bytes / sizeof (gint16);
 
   /* hard coded in volume.orc */
-  g_assert (VOLUME_UNITY_INT16_BIT_SHIFT == 13);
+  g_assert (VOLUME_UNITY_INT16_BIT_SHIFT == 11);
 
   orc_process_int16 (data, self->current_vol_i16, num_samples);
 }
@@ -714,7 +714,7 @@ volume_process_int16_clamp (GstVolume * self, gpointer bytes, guint n_bytes)
   guint num_samples = n_bytes / sizeof (gint16);
 
   /* hard coded in volume.orc */
-  g_assert (VOLUME_UNITY_INT16_BIT_SHIFT == 13);
+  g_assert (VOLUME_UNITY_INT16_BIT_SHIFT == 11);
 
   orc_process_int16_clamp (data, self->current_vol_i16, num_samples);
 }
@@ -750,7 +750,7 @@ volume_process_int8 (GstVolume * self, gpointer bytes, guint n_bytes)
   guint num_samples = n_bytes / sizeof (gint8);
 
   /* hard coded in volume.orc */
-  g_assert (VOLUME_UNITY_INT8_BIT_SHIFT == 5);
+  g_assert (VOLUME_UNITY_INT8_BIT_SHIFT == 3);
 
   orc_process_int8 (data, self->current_vol_i8, num_samples);
 }
@@ -762,7 +762,7 @@ volume_process_int8_clamp (GstVolume * self, gpointer bytes, guint n_bytes)
   guint num_samples = n_bytes / sizeof (gint8);
 
   /* hard coded in volume.orc */
-  g_assert (VOLUME_UNITY_INT8_BIT_SHIFT == 5);
+  g_assert (VOLUME_UNITY_INT8_BIT_SHIFT == 3);
 
   orc_process_int8_clamp (data, self->current_vol_i8, num_samples);
 }

diff --git a/gst/volume/gstvolumeorc.orc b/gst/volume/gstvolumeorc.orc
index aaea6f5..5fdf174 100644 (file)
--- a/gst/volume/gstvolumeorc.orc
+++ b/gst/volume/gstvolumeorc.orc
@@ -37,7 +37,7 @@ convsssql d1, t1
 .temp 4 t1
 
 mulswl t1, d1, p1
-shrsl t1, t1, 13
+shrsl t1, t1, 11
 convlw d1, t1
 
 
@@ -47,7 +47,7 @@ convlw d1, t1
 .temp 4 t1
 
 mulswl t1, d1, p1
-shrsl t1, t1, 13
+shrsl t1, t1, 11
 convssslw d1, t1
 
 .function orc_process_int8
@@ -56,7 +56,7 @@ convssslw d1, t1
 .temp 2 t1
 
 mulsbw t1, d1, p1
-shrsw t1, t1, 5
+shrsw t1, t1, 3
 convwb d1, t1
 
 
@@ -66,7 +66,7 @@ convwb d1, t1
 .temp 2 t1
 
 mulsbw t1, d1, p1
-shrsw t1, t1, 5
+shrsw t1, t1, 3
 convssswb d1, t1
 
 .function orc_memset_f64

diff --git a/tests/check/elements/volume.c b/tests/check/elements/volume.c
index d59a96f..3d2e146 100644 (file)
--- a/tests/check/elements/volume.c
+++ b/tests/check/elements/volume.c
@@ -309,6 +309,53 @@ GST_START_TEST (test_double_s8)
 
 GST_END_TEST;
 
+GST_START_TEST (test_ten_s8)
+{
+  GstElement *volume;
+  GstBuffer *inbuffer;
+  GstBuffer *outbuffer;
+  GstCaps *caps;
+  gint8 in[2] = { 64, -10 };
+  gint8 out[2] = { 127, -100 }; /* notice the clamped sample */
+  gint8 *res;
+
+  volume = setup_volume ();
+  g_object_set (G_OBJECT (volume), "volume", 10.0, NULL);
+  fail_unless (gst_element_set_state (volume,
+          GST_STATE_PLAYING) == GST_STATE_CHANGE_SUCCESS,
+      "could not set to playing");
+
+  inbuffer = gst_buffer_new_and_alloc (2);
+  memcpy (GST_BUFFER_DATA (inbuffer), in, 2);
+  fail_unless (memcmp (GST_BUFFER_DATA (inbuffer), in, 2) == 0);
+  caps = gst_caps_from_string (VOLUME_CAPS_STRING_S8);
+  gst_buffer_set_caps (inbuffer, caps);
+  gst_caps_unref (caps);
+  ASSERT_BUFFER_REFCOUNT (inbuffer, "inbuffer", 1);
+  /* FIXME: reffing the inbuffer should make the transformation not be
+   * inplace
+   gst_buffer_ref (inbuffer);
+   */
+
+  /* pushing gives away my reference ... */
+  fail_unless (gst_pad_push (mysrcpad, inbuffer) == GST_FLOW_OK);
+  /* ... but it ends up being modified inplace and
+   * collected on the global buffer list */
+  ASSERT_BUFFER_REFCOUNT (inbuffer, "inbuffer", 1);
+  fail_unless_equals_int (g_list_length (buffers), 1);
+  fail_if ((outbuffer = (GstBuffer *) buffers->data) == NULL);
+  fail_unless (inbuffer == outbuffer);
+  res = (gint8 *) GST_BUFFER_DATA (outbuffer);
+  GST_INFO ("expected %+5d %+5d  real %+5d %+5d", out[0], out[1], res[0],
+      res[1]);
+  fail_unless (memcmp (GST_BUFFER_DATA (outbuffer), out, 2) == 0);
+
+  /* cleanup */
+  cleanup_volume (volume);
+}
+
+GST_END_TEST;
+
 GST_START_TEST (test_mute_s8)
 {
   GstElement *volume;
@@ -487,6 +534,53 @@ GST_START_TEST (test_double_s16)
 
 GST_END_TEST;
 
+GST_START_TEST (test_ten_s16)
+{
+  GstElement *volume;
+  GstBuffer *inbuffer;
+  GstBuffer *outbuffer;
+  GstCaps *caps;
+  gint16 in[2] = { 16384, -10 };
+  gint16 out[2] = { 32767, -100 };      /* notice the clamped sample */
+  gint16 *res;
+
+  volume = setup_volume ();
+  g_object_set (G_OBJECT (volume), "volume", 10.0, NULL);
+  fail_unless (gst_element_set_state (volume,
+          GST_STATE_PLAYING) == GST_STATE_CHANGE_SUCCESS,
+      "could not set to playing");
+
+  inbuffer = gst_buffer_new_and_alloc (4);
+  memcpy (GST_BUFFER_DATA (inbuffer), in, 4);
+  fail_unless (memcmp (GST_BUFFER_DATA (inbuffer), in, 4) == 0);
+  caps = gst_caps_from_string (VOLUME_CAPS_STRING_S16);
+  gst_buffer_set_caps (inbuffer, caps);
+  gst_caps_unref (caps);
+  ASSERT_BUFFER_REFCOUNT (inbuffer, "inbuffer", 1);
+  /* FIXME: reffing the inbuffer should make the transformation not be
+   * inplace
+   gst_buffer_ref (inbuffer);
+   */
+
+  /* pushing gives away my reference ... */
+  fail_unless (gst_pad_push (mysrcpad, inbuffer) == GST_FLOW_OK);
+  /* ... but it ends up being modified inplace and
+   * collected on the global buffer list */
+  ASSERT_BUFFER_REFCOUNT (inbuffer, "inbuffer", 1);
+  fail_unless_equals_int (g_list_length (buffers), 1);
+  fail_if ((outbuffer = (GstBuffer *) buffers->data) == NULL);
+  fail_unless (inbuffer == outbuffer);
+  res = (gint16 *) GST_BUFFER_DATA (outbuffer);
+  GST_INFO ("expected %+5d %+5d  real %+5d %+5d", out[0], out[1], res[0],
+      res[1]);
+  fail_unless (memcmp (GST_BUFFER_DATA (outbuffer), out, 4) == 0);
+
+  /* cleanup */
+  cleanup_volume (volume);
+}
+
+GST_END_TEST;
+
 
 GST_START_TEST (test_mute_s16)
 {
@@ -703,6 +797,61 @@ GST_START_TEST (test_double_s24)
 
 GST_END_TEST;
 
+GST_START_TEST (test_ten_s24)
+{
+  GstElement *volume;
+  GstBuffer *inbuffer;
+  GstBuffer *outbuffer;
+  GstCaps *caps;
+  gint32 in_32[2] = { 4194304, -10 };
+  guint8 in[6];
+  guint8 *res;
+  gint32 res_32[2];
+  gint32 out_32[2] = { 8388607, -100 }; /* notice the clamped sample */
+
+  write_unaligned_u24 (in, in_32[0]);
+  write_unaligned_u24 (in + 3, in_32[1]);
+
+  volume = setup_volume ();
+  g_object_set (G_OBJECT (volume), "volume", 10.0, NULL);
+  fail_unless (gst_element_set_state (volume,
+          GST_STATE_PLAYING) == GST_STATE_CHANGE_SUCCESS,
+      "could not set to playing");
+
+  inbuffer = gst_buffer_new_and_alloc (6);
+  memcpy (GST_BUFFER_DATA (inbuffer), in, 6);
+  fail_unless (memcmp (GST_BUFFER_DATA (inbuffer), in, 6) == 0);
+  caps = gst_caps_from_string (VOLUME_CAPS_STRING_S24);
+  gst_buffer_set_caps (inbuffer, caps);
+  gst_caps_unref (caps);
+  ASSERT_BUFFER_REFCOUNT (inbuffer, "inbuffer", 1);
+  /* FIXME: reffing the inbuffer should make the transformation not be
+   * inplace
+   gst_buffer_ref (inbuffer);
+   */
+
+  /* pushing gives away my reference ... */
+  fail_unless (gst_pad_push (mysrcpad, inbuffer) == GST_FLOW_OK);
+  /* ... but it ends up being modified inplace and
+   * collected on the global buffer list */
+  ASSERT_BUFFER_REFCOUNT (inbuffer, "inbuffer", 1);
+  fail_unless_equals_int (g_list_length (buffers), 1);
+  fail_if ((outbuffer = (GstBuffer *) buffers->data) == NULL);
+  fail_unless (inbuffer == outbuffer);
+  res = GST_BUFFER_DATA (outbuffer);
+
+  res_32[0] = get_unaligned_i24 (res);
+  res_32[1] = get_unaligned_i24 ((res + 3));
+
+  GST_INFO ("expected %+5d %+5d  real %+5d %+5d", out_32[0], out_32[1],
+      res_32[0], res_32[1]);
+  fail_unless (memcmp (res_32, out_32, 8) == 0);
+
+  /* cleanup */
+  cleanup_volume (volume);
+}
+
+GST_END_TEST;
 
 GST_START_TEST (test_mute_s24)
 {
@@ -892,6 +1041,52 @@ GST_START_TEST (test_double_s32)
 
 GST_END_TEST;
 
+GST_START_TEST (test_ten_s32)
+{
+  GstElement *volume;
+  GstBuffer *inbuffer;
+  GstBuffer *outbuffer;
+  GstCaps *caps;
+  gint32 in[2] = { 1073741824, -10 };
+  gint32 out[2] = { 2147483647, -100 }; /* notice the clamped sample */
+  gint32 *res;
+
+  volume = setup_volume ();
+  g_object_set (G_OBJECT (volume), "volume", 10.0, NULL);
+  fail_unless (gst_element_set_state (volume,
+          GST_STATE_PLAYING) == GST_STATE_CHANGE_SUCCESS,
+      "could not set to playing");
+
+  inbuffer = gst_buffer_new_and_alloc (8);
+  memcpy (GST_BUFFER_DATA (inbuffer), in, 8);
+  fail_unless (memcmp (GST_BUFFER_DATA (inbuffer), in, 8) == 0);
+  caps = gst_caps_from_string (VOLUME_CAPS_STRING_S32);
+  gst_buffer_set_caps (inbuffer, caps);
+  gst_caps_unref (caps);
+  ASSERT_BUFFER_REFCOUNT (inbuffer, "inbuffer", 1);
+  /* FIXME: reffing the inbuffer should make the transformation not be
+   * inplace
+   gst_buffer_ref (inbuffer);
+   */
+
+  /* pushing gives away my reference ... */
+  fail_unless (gst_pad_push (mysrcpad, inbuffer) == GST_FLOW_OK);
+  /* ... but it ends up being modified inplace and
+   * collected on the global buffer list */
+  ASSERT_BUFFER_REFCOUNT (inbuffer, "inbuffer", 1);
+  fail_unless_equals_int (g_list_length (buffers), 1);
+  fail_if ((outbuffer = (GstBuffer *) buffers->data) == NULL);
+  fail_unless (inbuffer == outbuffer);
+  res = (gint32 *) GST_BUFFER_DATA (outbuffer);
+  GST_INFO ("expected %+5d %+5d  real %+5d %+5d", out[0], out[1], res[0],
+      res[1]);
+  fail_unless (memcmp (GST_BUFFER_DATA (outbuffer), out, 8) == 0);
+
+  /* cleanup */
+  cleanup_volume (volume);
+}
+
+GST_END_TEST;
 
 GST_START_TEST (test_mute_s32)
 {
@@ -1075,6 +1270,54 @@ GST_START_TEST (test_double_f32)
 
 GST_END_TEST;
 
+GST_START_TEST (test_ten_f32)
+{
+  GstElement *volume;
+  GstBuffer *inbuffer;
+  GstBuffer *outbuffer;
+  GstCaps *caps;
+  gfloat in[2] = { 0.75, -0.25 };
+  gfloat out[2] = { 7.5, -2.5 };        /* nothing is clamped */
+  gfloat *res;
+
+  volume = setup_volume ();
+  g_object_set (G_OBJECT (volume), "volume", 10.0, NULL);
+  fail_unless (gst_element_set_state (volume,
+          GST_STATE_PLAYING) == GST_STATE_CHANGE_SUCCESS,
+      "could not set to playing");
+
+  inbuffer = gst_buffer_new_and_alloc (8);
+  memcpy (GST_BUFFER_DATA (inbuffer), in, 8);
+  fail_unless (memcmp (GST_BUFFER_DATA (inbuffer), in, 8) == 0);
+  caps = gst_caps_from_string (VOLUME_CAPS_STRING_F32);
+  gst_buffer_set_caps (inbuffer, caps);
+  gst_caps_unref (caps);
+  ASSERT_BUFFER_REFCOUNT (inbuffer, "inbuffer", 1);
+  /* FIXME: reffing the inbuffer should make the transformation not be
+   * inplace
+   gst_buffer_ref (inbuffer);
+   */
+
+  /* pushing gives away my reference ... */
+  fail_unless (gst_pad_push (mysrcpad, inbuffer) == GST_FLOW_OK);
+  /* ... but it ends up being modified inplace and
+   * collected on the global buffer list */
+  ASSERT_BUFFER_REFCOUNT (inbuffer, "inbuffer", 1);
+  fail_unless_equals_int (g_list_length (buffers), 1);
+  fail_if ((outbuffer = (GstBuffer *) buffers->data) == NULL);
+  fail_unless (inbuffer == outbuffer);
+  res = (gfloat *) GST_BUFFER_DATA (outbuffer);
+  GST_INFO ("expected %+1.4f %+1.4f  real %+1.4f %+1.4f", out[0], out[1],
+      res[0], res[1]);
+  fail_unless_equals_float (res[0], out[0]);
+  fail_unless_equals_float (res[1], out[1]);
+
+  /* cleanup */
+  cleanup_volume (volume);
+}
+
+GST_END_TEST;
+
 
 GST_START_TEST (test_mute_f32)
 {
@@ -1259,6 +1502,54 @@ GST_START_TEST (test_double_f64)
 
 GST_END_TEST;
 
+GST_START_TEST (test_ten_f64)
+{
+  GstElement *volume;
+  GstBuffer *inbuffer;
+  GstBuffer *outbuffer;
+  GstCaps *caps;
+  gdouble in[2] = { 0.75, -0.25 };
+  gdouble out[2] = { 7.5, -2.5 };       /* nothing is clamped */
+  gdouble *res;
+
+  volume = setup_volume ();
+  g_object_set (G_OBJECT (volume), "volume", 10.0, NULL);
+  fail_unless (gst_element_set_state (volume,
+          GST_STATE_PLAYING) == GST_STATE_CHANGE_SUCCESS,
+      "could not set to playing");
+
+  inbuffer = gst_buffer_new_and_alloc (16);
+  memcpy (GST_BUFFER_DATA (inbuffer), in, 16);
+  fail_unless (memcmp (GST_BUFFER_DATA (inbuffer), in, 16) == 0);
+  caps = gst_caps_from_string (VOLUME_CAPS_STRING_F64);
+  gst_buffer_set_caps (inbuffer, caps);
+  gst_caps_unref (caps);
+  ASSERT_BUFFER_REFCOUNT (inbuffer, "inbuffer", 1);
+  /* FIXME: reffing the inbuffer should make the transformation not be
+   * inplace
+   gst_buffer_ref (inbuffer);
+   */
+
+  /* pushing gives away my reference ... */
+  fail_unless (gst_pad_push (mysrcpad, inbuffer) == GST_FLOW_OK);
+  /* ... but it ends up being modified inplace and
+   * collected on the global buffer list */
+  ASSERT_BUFFER_REFCOUNT (inbuffer, "inbuffer", 1);
+  fail_unless_equals_int (g_list_length (buffers), 1);
+  fail_if ((outbuffer = (GstBuffer *) buffers->data) == NULL);
+  fail_unless (inbuffer == outbuffer);
+  res = (gdouble *) GST_BUFFER_DATA (outbuffer);
+  GST_INFO ("expected %+1.4f %+1.4f  real %+1.4f %+1.4f", out[0], out[1],
+      res[0], res[1]);
+  fail_unless_equals_float (res[0], out[0]);
+  fail_unless_equals_float (res[1], out[1]);
+
+  /* cleanup */
+  cleanup_volume (volume);
+}
+
+GST_END_TEST;
+
 
 GST_START_TEST (test_mute_f64)
 {
@@ -1493,26 +1784,32 @@ volume_suite (void)
   tcase_add_test (tc_chain, test_unity_s8);
   tcase_add_test (tc_chain, test_half_s8);
   tcase_add_test (tc_chain, test_double_s8);
+  tcase_add_test (tc_chain, test_ten_s8);
   tcase_add_test (tc_chain, test_mute_s8);
   tcase_add_test (tc_chain, test_unity_s16);
   tcase_add_test (tc_chain, test_half_s16);
   tcase_add_test (tc_chain, test_double_s16);
+  tcase_add_test (tc_chain, test_ten_s16);
   tcase_add_test (tc_chain, test_mute_s16);
   tcase_add_test (tc_chain, test_unity_s24);
   tcase_add_test (tc_chain, test_half_s24);
   tcase_add_test (tc_chain, test_double_s24);
+  tcase_add_test (tc_chain, test_ten_s24);
   tcase_add_test (tc_chain, test_mute_s24);
   tcase_add_test (tc_chain, test_unity_s32);
   tcase_add_test (tc_chain, test_half_s32);
   tcase_add_test (tc_chain, test_double_s32);
+  tcase_add_test (tc_chain, test_ten_s32);
   tcase_add_test (tc_chain, test_mute_s32);
   tcase_add_test (tc_chain, test_unity_f32);
   tcase_add_test (tc_chain, test_half_f32);
   tcase_add_test (tc_chain, test_double_f32);
+  tcase_add_test (tc_chain, test_ten_f32);
   tcase_add_test (tc_chain, test_mute_f32);
   tcase_add_test (tc_chain, test_unity_f64);
   tcase_add_test (tc_chain, test_half_f64);
   tcase_add_test (tc_chain, test_double_f64);
+  tcase_add_test (tc_chain, test_ten_f64);
   tcase_add_test (tc_chain, test_mute_f64);
   tcase_add_test (tc_chain, test_wrong_caps);
   tcase_add_test (tc_chain, test_passthrough);