[v2,1/5] replaygain: correctly parse peak values

Message ID 1395829505-4123-2-git-send-email-alessandro@ghedini.me
State New
Headers show

Commit Message

Alessandro Ghedini March 26, 2014, 10:25 a.m.
According to the ReplayGain spec, the peak amplitude may overflow and may result
in peak amplitude values greater than 1.0 with psychoacoustically coded audio,
such as MP3. Fully compliant decoders must allow peak overflows.

Additionally, having peak values in the 0<->UINT32_MAX scale makes it more
difficult for applications to actually use the peak values (e.g. when
implementing clipping prevention) since values have to be rescaled down.

This patch corrects the peak parsing by removing the rescaling of the decoded
values between 0 and UINT32_MAX and the 1.0 upper limit.
---
 libavformat/replaygain.c | 35 +++++++++++++++--------------------
 libavutil/replaygain.h   |  3 +--
 2 files changed, 16 insertions(+), 22 deletions(-)

Comments

Justin Ruggles March 30, 2014, 7:01 p.m. | #1
On 03/26/2014 06:25 AM, Alessandro Ghedini wrote:
> According to the ReplayGain spec, the peak amplitude may overflow and may result
> in peak amplitude values greater than 1.0 with psychoacoustically coded audio,
> such as MP3. Fully compliant decoders must allow peak overflows.
> 
> Additionally, having peak values in the 0<->UINT32_MAX scale makes it more
> difficult for applications to actually use the peak values (e.g. when
> implementing clipping prevention) since values have to be rescaled down.
> 
> This patch corrects the peak parsing by removing the rescaling of the decoded
> values between 0 and UINT32_MAX and the 1.0 upper limit.
> ---
>  libavformat/replaygain.c | 35 +++++++++++++++--------------------
>  libavutil/replaygain.h   |  3 +--
>  2 files changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/libavformat/replaygain.c b/libavformat/replaygain.c
> index cf4dbf8..d4d3d1a 100644
> --- a/libavformat/replaygain.c
> +++ b/libavformat/replaygain.c
> @@ -64,34 +64,29 @@ static int32_t parse_gain(const char *gain)
>  
>  static uint32_t parse_peak(const uint8_t *peak)
>  {
> -    int64_t val = 0;
> -    int64_t scale = 1;
> +    char *fraction;
> +    unsigned int  scale = 10000;
> +    uint32_t mb = 0;
> +    unsigned int db;
>  
>      if (!peak)
>          return 0;
>  
>      peak += strspn(peak, " \t");
>  
> -    if (peak[0] == '1' && peak[1] == '.')
> -        return UINT32_MAX;
> -    else if (!(peak[0] == '0' && peak[1] == '.'))
> -        return 0;
> -
> -    peak += 2;
> -
> -    while (av_isdigit(*peak)) {
> -        int digit = *peak - '0';
> -
> -        if (scale > INT64_MAX / 10)
> -            break;
> -
> -        val    = 10 * val + digit;
> -        scale *= 10;
> -
> -        peak++;
> +    db = strtol(peak, &fraction, 0);
> +    if (*fraction++ == '.') {
> +        while (av_isdigit(*fraction) && scale) {
> +            mb += scale * (*fraction - '0');
> +            scale /= 10;
> +            fraction++;
> +        }
>      }
>  
> -    return av_rescale(val, UINT32_MAX, scale);
> +    if (db > (UINT32_MAX - mb) / 100000)
> +        return 0;
> +
> +    return db * 100000 + mb;
>  }
>  

This looks pretty darn close to parse_gain(), just without the sign.
Maybe the two functions could be merged.

>  static int replaygain_export(AVStream *st,
> diff --git a/libavutil/replaygain.h b/libavutil/replaygain.h
> index 8447703..00d2873 100644
> --- a/libavutil/replaygain.h
> +++ b/libavutil/replaygain.h
> @@ -34,8 +34,7 @@ typedef struct AVReplayGain {
>       */
>      int32_t track_gain;
>      /**
> -     * Peak track amplitude, with UINT32_MAX representing full scale. 0 when
> -     * unknown.
> +     * Peak track amplitude. 0 when unknown.
>       */
>      uint32_t track_peak;
>      /**

This is ok in general, but please specify the scale in the
documentation. Also this needs a libavutil minor bump, plus an entry in
APIchanges noting the change.

Thanks,
Justin

Patch

diff --git a/libavformat/replaygain.c b/libavformat/replaygain.c
index cf4dbf8..d4d3d1a 100644
--- a/libavformat/replaygain.c
+++ b/libavformat/replaygain.c
@@ -64,34 +64,29 @@  static int32_t parse_gain(const char *gain)
 
 static uint32_t parse_peak(const uint8_t *peak)
 {
-    int64_t val = 0;
-    int64_t scale = 1;
+    char *fraction;
+    unsigned int  scale = 10000;
+    uint32_t mb = 0;
+    unsigned int db;
 
     if (!peak)
         return 0;
 
     peak += strspn(peak, " \t");
 
-    if (peak[0] == '1' && peak[1] == '.')
-        return UINT32_MAX;
-    else if (!(peak[0] == '0' && peak[1] == '.'))
-        return 0;
-
-    peak += 2;
-
-    while (av_isdigit(*peak)) {
-        int digit = *peak - '0';
-
-        if (scale > INT64_MAX / 10)
-            break;
-
-        val    = 10 * val + digit;
-        scale *= 10;
-
-        peak++;
+    db = strtol(peak, &fraction, 0);
+    if (*fraction++ == '.') {
+        while (av_isdigit(*fraction) && scale) {
+            mb += scale * (*fraction - '0');
+            scale /= 10;
+            fraction++;
+        }
     }
 
-    return av_rescale(val, UINT32_MAX, scale);
+    if (db > (UINT32_MAX - mb) / 100000)
+        return 0;
+
+    return db * 100000 + mb;
 }
 
 static int replaygain_export(AVStream *st,
diff --git a/libavutil/replaygain.h b/libavutil/replaygain.h
index 8447703..00d2873 100644
--- a/libavutil/replaygain.h
+++ b/libavutil/replaygain.h
@@ -34,8 +34,7 @@  typedef struct AVReplayGain {
      */
     int32_t track_gain;
     /**
-     * Peak track amplitude, with UINT32_MAX representing full scale. 0 when
-     * unknown.
+     * Peak track amplitude. 0 when unknown.
      */
     uint32_t track_peak;
     /**