Re: [FFmpeg-devel] [PATCH 3/4] opus: Add Special Hybrid Folding (per RFC8251)
On Wed Dec 6 21:42:48 EET 2017, James Almer
wrote:
> Valgrind apparently complains about this commit, as i mentioned in
> another reply to this thread
Hi James, thanks for reporting the issue. There's indeed a problem
with patch #3 (use-of-uninitialized-memory in norm[] and norm2[]).
libopus organizes the norm[] variable into bands with *room for* 8
MDCT coefficients in each band (even though the actual number of
coefficients per band is 1, 2, 4, or 8 for 2.5ms, 5ms, 10ms, or 20ms
CELT frames, respectively).
Whereas Libav/FFmpeg changes the organization of norm[] slightly and
packs the bands contiguously (i.e. each band is allocated the
necessary amount of 1 << f->size MDCT coefficients, and no more),
leaving some unused space at the end of norm[].
Hence, every instance of "8 * xxx" in my patch should really be
"xxx << f->size". That got rid of all errors in valgrind/MSAN.
Rostislav has applied the change to master with Commit
4678339e745dac8fa428 ("opus: fix hybrid folding indexing during band
quantization"), so the bug should be fixed now. Thanks for catching
it!
Andrew
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/4] opus: Add Special Hybrid Folding (per RFC8251)
This decoder-side change, introduced in RFC 8251 (section 9), slightly
improves the decoded quality of 16kbps speech in Hybrid Mode.
Differences can be seen/heard in testvector05.bit, testvector06.bit,
and testvector12.bit in the RFC 6716/8251 testvectors found here:
https://people.xiph.org/~greg/opus_testvectors/
Signed-off-by: Andrew D'Addesio
---
libavcodec/opus_celt.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/libavcodec/opus_celt.c b/libavcodec/opus_celt.c
index ff56041..2bbb96b 100644
--- a/libavcodec/opus_celt.c
+++ b/libavcodec/opus_celt.c
@@ -712,10 +712,22 @@ static void celt_decode_bands(CeltFrame *f,
OpusRangeCoder *rc)
b = av_clip_uintp2(FFMIN(f->remaining2 + 1, f->pulses[i] +
curr_balance), 14);
}
-if (ff_celt_freq_bands[i] - ff_celt_freq_range[i] >=
ff_celt_freq_bands[f->start_band] &&
-(update_lowband || lowband_offset == 0))
+if ((ff_celt_freq_bands[i] - ff_celt_freq_range[i] >=
ff_celt_freq_bands[f->start_band] ||
+i == f->start_band + 1) && (update_lowband || lowband_offset == 0))
lowband_offset = i;
+if (i == f->start_band + 1) {
+/* Special Hybrid Folding (RFC 8251 section 9). Copy the first
band into
+the second to ensure the second band never has to use the LCG. */
+int offset = 8 * ff_celt_freq_bands[i];
+int count = 8 * (ff_celt_freq_range[i] - ff_celt_freq_range[i-1]);
+
+memcpy(&norm[offset], &norm[offset - count], count *
sizeof(float));
+
+if (f->channels == 2)
+memcpy(&norm2[offset], &norm2[offset - count], count *
sizeof(float));
+}
+
/* Get a conservative estimate of the collapse_mask's for the bands
we're
going to be folding from. */
if (lowband_offset != 0 && (f->spread != CELT_SPREAD_AGGRESSIVE ||
@@ -728,7 +740,7 @@ static void celt_decode_bands(CeltFrame *f, OpusRangeCoder
*rc)
foldstart = lowband_offset;
while (ff_celt_freq_bands[--foldstart] > effective_lowband);
foldend = lowband_offset - 1;
-while (ff_celt_freq_bands[++foldend] < effective_lowband +
ff_celt_freq_range[i]);
+while (++foldend < i && ff_celt_freq_bands[foldend] <
effective_lowband + ff_celt_freq_range[i]);
cm[0] = cm[1] = 0;
for (j = foldstart; j < foldend; j++) {
--
2.15.1.windows.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 4/4] opus: Don't invert phase when downmixing to mono (per RFC8251)
When decoding to downmixed mono, don't put the channels out of phase,
as they will cancel out and create audible artifacts. (See
RFC 8251 sec. 10.)
Signed-off-by: Andrew D'Addesio
---
libavcodec/opus_pvq.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/libavcodec/opus_pvq.c b/libavcodec/opus_pvq.c
index 2f7aa74..f18c010 100644
--- a/libavcodec/opus_pvq.c
+++ b/libavcodec/opus_pvq.c
@@ -643,7 +643,13 @@ static av_always_inline uint32_t
quant_band_template(CeltPVQ *pvq, CeltFrame *f,
}
} else {
inv = (b > 2 << 3 && f->remaining2 > 2 << 3) ?
ff_opus_rc_dec_log(rc, 2) : 0;
+
+/* Don't put the channels out of phase if we are decoding to
downmixed
+ * mono as this subjectively sounds bad (RFC 8251 section 10).
*/
+if (f->channels == 1)
+inv = 0;
}
+
itheta = 0;
}
qalloc = opus_rc_tell_frac(rc) - tell;
--
2.15.1.windows.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/4] libavutil: Add saturating subtraction functions
Add av_sat_sub32 and av_sat_dsub32 as the subtraction analogues to
av_sat_add32/av_sat_dadd32.
Also clarify the formulas for dadd32/dsub32.
Signed-off-by: Andrew D'Addesio
---
libavutil/arm/intmath.h | 16
libavutil/common.h | 32 +++-
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/libavutil/arm/intmath.h b/libavutil/arm/intmath.h
index 65e42c5..5311a7d 100644
--- a/libavutil/arm/intmath.h
+++ b/libavutil/arm/intmath.h
@@ -94,6 +94,22 @@ static av_always_inline int av_sat_dadd32_arm(int a, int b)
return r;
}
+#define av_sat_sub32 av_sat_sub32_arm
+static av_always_inline int av_sat_sub32_arm(int a, int b)
+{
+int r;
+__asm__ ("qsub %0, %1, %2" : "=r"(r) : "r"(a), "r"(b));
+return r;
+}
+
+#define av_sat_dsub32 av_sat_dsub32_arm
+static av_always_inline int av_sat_dsub32_arm(int a, int b)
+{
+int r;
+__asm__ ("qdsub %0, %1, %2" : "=r"(r) : "r"(a), "r"(b));
+return r;
+}
+
#endif /* HAVE_ARMV6_INLINE */
#if HAVE_ASM_MOD_Q
diff --git a/libavutil/common.h b/libavutil/common.h
index 8142b31..5e03828 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -260,13 +260,37 @@ static av_always_inline int av_sat_add32_c(int a, int b)
*
* @param a first value
* @param b value doubled and added to a
- * @return sum with signed saturation
+ * @return sum sat(a + sat(2*b)) with signed saturation
*/
static av_always_inline int av_sat_dadd32_c(int a, int b)
{
return av_sat_add32(a, av_sat_add32(b, b));
}
+/**
+ * Subtract two signed 32-bit values with saturation.
+ *
+ * @param a one value
+ * @param b another value
+ * @return difference with signed saturation
+ */
+static av_always_inline int av_sat_sub32_c(int a, int b)
+{
+return av_clipl_int32((int64_t)a - b);
+}
+
+/**
+ * Subtract a doubled value from another value with saturation at both stages.
+ *
+ * @param a first value
+ * @param b value doubled and subtracted from a
+ * @return difference sat(a - sat(2*b)) with signed saturation
+ */
+static av_always_inline int av_sat_dsub32_c(int a, int b)
+{
+return av_sat_sub32(a, av_sat_add32(b, b));
+}
+
/**
* Clip a float value into the amin-amax range.
* @param a value to clip
@@ -513,6 +537,12 @@ static av_always_inline av_const int av_parity_c(uint32_t
v)
#ifndef av_sat_dadd32
# define av_sat_dadd32av_sat_dadd32_c
#endif
+#ifndef av_sat_sub32
+# define av_sat_sub32 av_sat_sub32_c
+#endif
+#ifndef av_sat_dsub32
+# define av_sat_dsub32av_sat_dsub32_c
+#endif
#ifndef av_clipf
# define av_clipf av_clipf_c
#endif
--
2.15.1.windows.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 4/4] opus: Don't invert phase when downmixing to mono (per RFC8251)
When decoding to downmixed mono, don't put the channels out of phase,
as they will cancel out and create audible artifacts. (See
RFC 8251 sec. 10.)
Signed-off-by: Andrew D'Addesio
---
libavcodec/opus_pvq.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/libavcodec/opus_pvq.c b/libavcodec/opus_pvq.c
index 2f7aa74..f18c010 100644
--- a/libavcodec/opus_pvq.c
+++ b/libavcodec/opus_pvq.c
@@ -643,7 +643,13 @@ static av_always_inline uint32_t
quant_band_template(CeltPVQ *pvq, CeltFrame *f,
}
} else {
inv = (b > 2 << 3 && f->remaining2 > 2 << 3) ?
ff_opus_rc_dec_log(rc, 2) : 0;
+
+/* Don't put the channels out of phase if we are decoding to
downmixed
+ * mono as this subjectively sounds bad (RFC 8251 section 10).
*/
+if (f->channels == 1)
+inv = 0;
}
+
itheta = 0;
}
qalloc = opus_rc_tell_frac(rc) - tell;
--
2.15.1.windows.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/4] opus: Fix arithmetic overflows (per RFC8251)
The relevant sections from the RFC are:
Sec.6. Integer Wrap-Around in Inverse Gain Computation
32-bit integer overflow in Levinson recursion. Affects
silk_is_lpc_stable().
Sec.8. Cap on Band Energy
NaN due to large log-energy value. Affects celt_denormalize().
Signed-off-by: Andrew D'Addesio
---
libavcodec/opus_celt.c | 3 ++-
libavcodec/opus_silk.c | 11 +--
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/libavcodec/opus_celt.c b/libavcodec/opus_celt.c
index 84d4847..ff56041 100644
--- a/libavcodec/opus_celt.c
+++ b/libavcodec/opus_celt.c
@@ -481,7 +481,8 @@ static void celt_denormalize(CeltFrame *f, CeltBlock
*block, float *data)
for (i = f->start_band; i < f->end_band; i++) {
float *dst = data + (ff_celt_freq_bands[i] << f->size);
-float norm = exp2f(block->energy[i] + ff_celt_mean_energy[i]);
+float log_norm = block->energy[i] + ff_celt_mean_energy[i];
+float norm = exp2f(FFMIN(log_norm, 32.0f));
for (j = 0; j < ff_celt_freq_range[i] << f->size; j++)
dst[j] *= norm;
diff --git a/libavcodec/opus_silk.c b/libavcodec/opus_silk.c
index 3c9c849..344333c 100644
--- a/libavcodec/opus_silk.c
+++ b/libavcodec/opus_silk.c
@@ -185,8 +185,15 @@ static inline int silk_is_lpc_stable(const int16_t
lpc[16], int order)
row = lpc32[k & 1];
for (j = 0; j < k; j++) {
-int x = prevrow[j] - ROUND_MULL(prevrow[k - j - 1], rc, 31);
-row[j] = ROUND_MULL(x, gain, fbits);
+int x = av_sat_sub32(prevrow[j], ROUND_MULL(prevrow[k - j - 1],
rc, 31));
+int64_t tmp = ROUND_MULL(x, gain, fbits);
+
+/* per RFC 8251 section 6, if this calculation overflows, the
filter
+ is considered unstable. */
+if (tmp < INT32_MIN || tmp > INT32_MAX)
+return 0;
+
+row[j] = (int32_t)tmp;
}
}
}
--
2.15.1.windows.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/4] opus: Add Special Hybrid Folding (per RFC8251)
This decoder-side change, introduced in RFC 8251 (section 9), slightly
improves the decoded quality of 16kbps speech in Hybrid Mode.
Differences can be seen/heard in testvector05.bit, testvector06.bit,
and testvector12.bit in the RFC 6716/8251 testvectors found here:
https://people.xiph.org/~greg/opus_testvectors/
Signed-off-by: Andrew D'Addesio
---
libavcodec/opus_celt.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/libavcodec/opus_celt.c b/libavcodec/opus_celt.c
index ff56041..2bbb96b 100644
--- a/libavcodec/opus_celt.c
+++ b/libavcodec/opus_celt.c
@@ -712,10 +712,22 @@ static void celt_decode_bands(CeltFrame *f,
OpusRangeCoder *rc)
b = av_clip_uintp2(FFMIN(f->remaining2 + 1, f->pulses[i] +
curr_balance), 14);
}
-if (ff_celt_freq_bands[i] - ff_celt_freq_range[i] >=
ff_celt_freq_bands[f->start_band] &&
-(update_lowband || lowband_offset == 0))
+if ((ff_celt_freq_bands[i] - ff_celt_freq_range[i] >=
ff_celt_freq_bands[f->start_band] ||
+i == f->start_band + 1) && (update_lowband || lowband_offset == 0))
lowband_offset = i;
+if (i == f->start_band + 1) {
+/* Special Hybrid Folding (RFC 8251 section 9). Copy the first
band into
+the second to ensure the second band never has to use the LCG. */
+int offset = 8 * ff_celt_freq_bands[i];
+int count = 8 * (ff_celt_freq_range[i] - ff_celt_freq_range[i-1]);
+
+memcpy(&norm[offset], &norm[offset - count], count *
sizeof(float));
+
+if (f->channels == 2)
+memcpy(&norm2[offset], &norm2[offset - count], count *
sizeof(float));
+}
+
/* Get a conservative estimate of the collapse_mask's for the bands
we're
going to be folding from. */
if (lowband_offset != 0 && (f->spread != CELT_SPREAD_AGGRESSIVE ||
@@ -728,7 +740,7 @@ static void celt_decode_bands(CeltFrame *f, OpusRangeCoder
*rc)
foldstart = lowband_offset;
while (ff_celt_freq_bands[--foldstart] > effective_lowband);
foldend = lowband_offset - 1;
-while (ff_celt_freq_bands[++foldend] < effective_lowband +
ff_celt_freq_range[i]);
+while (++foldend < i && ff_celt_freq_bands[foldend] <
effective_lowband + ff_celt_freq_range[i]);
cm[0] = cm[1] = 0;
for (j = foldstart; j < foldend; j++) {
--
2.15.1.windows.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/4] opus: Fix arithmetic overflows (per RFC8251)
The relevant sections from the RFC are:
Sec.6. Integer Wrap-Around in Inverse Gain Computation
32-bit integer overflow in Levinson recursion. Affects
silk_is_lpc_stable().
Sec.8. Cap on Band Energy
NaN due to large log-energy value. Affects celt_denormalize().
Signed-off-by: Andrew D'Addesio
---
libavcodec/opus_celt.c | 3 ++-
libavcodec/opus_silk.c | 11 +--
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/libavcodec/opus_celt.c b/libavcodec/opus_celt.c
index 84d4847..ff56041 100644
--- a/libavcodec/opus_celt.c
+++ b/libavcodec/opus_celt.c
@@ -481,7 +481,8 @@ static void celt_denormalize(CeltFrame *f, CeltBlock
*block, float *data)
for (i = f->start_band; i < f->end_band; i++) {
float *dst = data + (ff_celt_freq_bands[i] << f->size);
-float norm = exp2f(block->energy[i] + ff_celt_mean_energy[i]);
+float log_norm = block->energy[i] + ff_celt_mean_energy[i];
+float norm = exp2f(FFMIN(log_norm, 32.0f));
for (j = 0; j < ff_celt_freq_range[i] << f->size; j++)
dst[j] *= norm;
diff --git a/libavcodec/opus_silk.c b/libavcodec/opus_silk.c
index 3c9c849..344333c 100644
--- a/libavcodec/opus_silk.c
+++ b/libavcodec/opus_silk.c
@@ -185,8 +185,15 @@ static inline int silk_is_lpc_stable(const int16_t
lpc[16], int order)
row = lpc32[k & 1];
for (j = 0; j < k; j++) {
-int x = prevrow[j] - ROUND_MULL(prevrow[k - j - 1], rc, 31);
-row[j] = ROUND_MULL(x, gain, fbits);
+int x = av_sat_sub32(prevrow[j], ROUND_MULL(prevrow[k - j - 1],
rc, 31));
+int64_t tmp = ROUND_MULL(x, gain, fbits);
+
+/* per RFC 8251 section 6, if this calculation overflows, the
filter
+ is considered unstable. */
+if (tmp < INT32_MIN || tmp > INT32_MAX)
+return 0;
+
+row[j] = (int32_t)tmp;
}
}
}
--
2.15.1.windows.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/4] libavutil: Add saturating subtraction functions
Add av_sat_sub32 and av_sat_dsub32 as the subtraction analogues to
av_sat_add32/av_sat_dadd32.
Also clarify the formulas for dadd32/dsub32.
Signed-off-by: Andrew D'Addesio
---
libavutil/arm/intmath.h | 16
libavutil/common.h | 32 +++-
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/libavutil/arm/intmath.h b/libavutil/arm/intmath.h
index 65e42c5..5311a7d 100644
--- a/libavutil/arm/intmath.h
+++ b/libavutil/arm/intmath.h
@@ -94,6 +94,22 @@ static av_always_inline int av_sat_dadd32_arm(int a, int b)
return r;
}
+#define av_sat_sub32 av_sat_sub32_arm
+static av_always_inline int av_sat_sub32_arm(int a, int b)
+{
+int r;
+__asm__ ("qsub %0, %1, %2" : "=r"(r) : "r"(a), "r"(b));
+return r;
+}
+
+#define av_sat_dsub32 av_sat_dsub32_arm
+static av_always_inline int av_sat_dsub32_arm(int a, int b)
+{
+int r;
+__asm__ ("qdsub %0, %1, %2" : "=r"(r) : "r"(a), "r"(b));
+return r;
+}
+
#endif /* HAVE_ARMV6_INLINE */
#if HAVE_ASM_MOD_Q
diff --git a/libavutil/common.h b/libavutil/common.h
index 8142b31..5e03828 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -260,13 +260,37 @@ static av_always_inline int av_sat_add32_c(int a, int b)
*
* @param a first value
* @param b value doubled and added to a
- * @return sum with signed saturation
+ * @return sum sat(a + sat(2*b)) with signed saturation
*/
static av_always_inline int av_sat_dadd32_c(int a, int b)
{
return av_sat_add32(a, av_sat_add32(b, b));
}
+/**
+ * Subtract two signed 32-bit values with saturation.
+ *
+ * @param a one value
+ * @param b another value
+ * @return difference with signed saturation
+ */
+static av_always_inline int av_sat_sub32_c(int a, int b)
+{
+return av_clipl_int32((int64_t)a - b);
+}
+
+/**
+ * Subtract a doubled value from another value with saturation at both stages.
+ *
+ * @param a first value
+ * @param b value doubled and subtracted from a
+ * @return difference sat(a - sat(2*b)) with signed saturation
+ */
+static av_always_inline int av_sat_dsub32_c(int a, int b)
+{
+return av_sat_sub32(a, av_sat_add32(b, b));
+}
+
/**
* Clip a float value into the amin-amax range.
* @param a value to clip
@@ -513,6 +537,12 @@ static av_always_inline av_const int av_parity_c(uint32_t
v)
#ifndef av_sat_dadd32
# define av_sat_dadd32av_sat_dadd32_c
#endif
+#ifndef av_sat_sub32
+# define av_sat_sub32 av_sat_sub32_c
+#endif
+#ifndef av_sat_dsub32
+# define av_sat_dsub32av_sat_dsub32_c
+#endif
#ifndef av_clipf
# define av_clipf av_clipf_c
#endif
--
2.15.1.windows.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
