Update links, fix bits, add security considerations
[opus.git] / doc / draft-ietf-codec-opus-update.xml
index 7414722..ad0d569 100644 (file)
@@ -10,8 +10,8 @@
 <?rfc inline="yes"?>
 <?rfc compact="yes"?>
 <?rfc subcompact="no"?>
 <?rfc inline="yes"?>
 <?rfc compact="yes"?>
 <?rfc subcompact="no"?>
-<rfc category="std" docName="draft-ietf-codec-opus-update-01"
-     ipr="trust200902">
+<rfc category="std" docName="draft-ietf-codec-opus-update-07"
+     ipr="trust200902" updates="6716">
   <front>
     <title abbrev="Opus Update">Updates to the Opus Audio Codec</title>
 
   <front>
     <title abbrev="Opus Update">Updates to the Opus Audio Codec</title>
 
 
 
 
 
 
 
-    <date day="4" month="September" year="2014" />
+    <date day="16" month="July" year="2017" />
 
     <abstract>
       <t>This document addresses minor issues that were found in the specification
 
     <abstract>
       <t>This document addresses minor issues that were found in the specification
-      of the Opus audio codec in <xref target="RFC6716">RFC 6716</xref>.</t>
+      of the Opus audio codec in RFC 6716.</t>
     </abstract>
   </front>
 
     </abstract>
   </front>
 
       implementation of the Opus codec that serves as the specification in
       <xref target="RFC6716">RFC 6716</xref>. Only issues affecting the decoder are
       listed here. An up-to-date implementation of the Opus encoder can be found at
       implementation of the Opus codec that serves as the specification in
       <xref target="RFC6716">RFC 6716</xref>. Only issues affecting the decoder are
       listed here. An up-to-date implementation of the Opus encoder can be found at
-      http://opus-codec.org/. The updated specification remains fully compatible with
-      the original specification and only one of the changes results in any difference
-      in the audio output.
-      </t>
+      <eref target="https://opus-codec.org/"/>.</t>
+    <t>
+      Some of the changes in this document update normative behaviour in a way that requires
+      new test vectors. The English text of the specification is unaffected, only
+      the C implementation is. The updated specification remains fully compatible with
+      the original specification.
+    </t>
+
+    <t>
+    Note: due to RFC formatting conventions, lines exceeding the column width
+    in the patch are split using a backslash character. The backslashes
+    at the end of a line and the white space at the beginning
+    of the following line are not part of the patch. A properly formatted patch
+    including all changes is available at
+    <eref target="https://www.ietf.org/proceedings/98/slides/materials-98-codec-opus-update-00.patch"/>.
+    </t>
+
     </section>
 
     <section title="Terminology">
     </section>
 
     <section title="Terminology">
       during a mode switch. The old stereo memory can produce a brief impulse
       (i.e. single sample) in the decoded audio. This can be fixed by changing
       silk/dec_API.c at line 72:
       during a mode switch. The old stereo memory can produce a brief impulse
       (i.e. single sample) in the decoded audio. This can be fixed by changing
       silk/dec_API.c at line 72:
-      <figure>
-      <artwork><![CDATA[
+    </t>
+<figure>
+<artwork><![CDATA[
+<CODE BEGINS>
      for( n = 0; n < DECODER_NUM_CHANNELS; n++ ) {
          ret  = silk_init_decoder( &channel_state[ n ] );
      }
      for( n = 0; n < DECODER_NUM_CHANNELS; n++ ) {
          ret  = silk_init_decoder( &channel_state[ n ] );
      }
  
      return ret;
  }
  
      return ret;
  }
+<CODE ENDS>
 ]]></artwork>
 </figure>
 ]]></artwork>
 </figure>
-     This change affects the normative part of the decoder. Fortunately,
-     the modified decoder is still compliant with the original specification because
-     it still easily passes the testvectors. For example, for the float decoder
-     at 48 kHz, the opus_compare (arbitrary) "quality score" changes from
-     from 99.9333% to 99.925%.
+     <t>
+     This change affects the normative part of the decoder, although the
+     amount of change is too small to make a significant impact on testvectors.
       </t>
     </section>
 
       </t>
     </section>
 
       This is due to an integer overflow if the signaled padding exceeds 2^31-1 bytes
       (the actual packet may be smaller). The code can be fixed by applying the following
       changes at line 596 of src/opus_decoder.c:
       This is due to an integer overflow if the signaled padding exceeds 2^31-1 bytes
       (the actual packet may be smaller). The code can be fixed by applying the following
       changes at line 596 of src/opus_decoder.c:
-      <figure>
-      <artwork><![CDATA[
+    </t>
+<figure>
+<artwork><![CDATA[
+<CODE BEGINS>
        /* Padding flag is bit 6 */
        if (ch&0x40)
        {
        /* Padding flag is bit 6 */
        if (ch&0x40)
        {
           } while (p==255);
 -         len -= padding;
        }
           } while (p==255);
 -         len -= padding;
        }
+<CODE ENDS>
 ]]></artwork>
 </figure>
 ]]></artwork>
 </figure>
-      </t>
       <t>This packet parsing issue is limited to reading memory up
          to about 60 kB beyond the compressed buffer. This can only be triggered
          by a compressed packet more than about 16 MB long, so it's not a problem
       <t>This packet parsing issue is limited to reading memory up
          to about 60 kB beyond the compressed buffer. This can only be triggered
          by a compressed packet more than about 16 MB long, so it's not a problem
      the batch sizes are reasonable enough that none of the issues above
      was ever a problem. However, proving that is non-obvious.
     </t>
      the batch sizes are reasonable enough that none of the issues above
      was ever a problem. However, proving that is non-obvious.
     </t>
-    <t>The code can be fixed by applying the following changes to line 70 of silk/resampler_private_IIR_FIR.c:
+    <t>The code can be fixed by applying the following changes to line 78 of silk/resampler_private_IIR_FIR.c:
+    </t>
 <figure>
 <artwork><![CDATA[
 <figure>
 <artwork><![CDATA[
+<CODE BEGINS>
  )
  {
      silk_resampler_state_struct *S = \
  )
  {
      silk_resampler_state_struct *S = \
@@ -212,18 +230,168 @@ RESAMPLER_ORDER_FIR_12 * sizeof( opus_int32 ) );
 +    silk_memcpy( S->sFIR, &buf[ nSamplesIn << 1 ], \
 RESAMPLER_ORDER_FIR_12 * sizeof( opus_int16 ) );
  }
 +    silk_memcpy( S->sFIR, &buf[ nSamplesIn << 1 ], \
 RESAMPLER_ORDER_FIR_12 * sizeof( opus_int16 ) );
  }
+<CODE ENDS>
+]]></artwork>
+</figure>
+    </section>
+
+    <section title="Integer wrap-around in inverse gain computation">
+      <t>
+        It was discovered through decoder fuzzing that some bitstreams could produce
+        integer values exceeding 32-bits in LPC_inverse_pred_gain_QA(), causing
+        a wrap-around. Although the error is harmless in practice, the C standard considers
+        the behavior as undefined, so the following patch to line 87 of silk/LPC_inv_pred_gain.c
+        detects values that do not fit in a 32-bit integer and considers the corresponding filters unstable:
+      </t>
+<figure>
+<artwork><![CDATA[
+<CODE BEGINS>
+         /* Update AR coefficient */
+         for( n = 0; n < k; n++ ) {
+-            tmp_QA = Aold_QA[ n ] - MUL32_FRAC_Q( \
+Aold_QA[ k - n - 1 ], rc_Q31, 31 );
+-            Anew_QA[ n ] = MUL32_FRAC_Q( tmp_QA, rc_mult2 , mult2Q );
++            opus_int64 tmp64;
++            tmp_QA = silk_SUB_SAT32( Aold_QA[ n ], MUL32_FRAC_Q( \
+Aold_QA[ k - n - 1 ], rc_Q31, 31 ) );
++            tmp64 = silk_RSHIFT_ROUND64( silk_SMULL( tmp_QA, \
+rc_mult2 ), mult2Q);
++            if( tmp64 > silk_int32_MAX || tmp64 < silk_int32_MIN ) {
++               return 0;
++            }
++            Anew_QA[ n ] = ( opus_int32 )tmp64;
+         }
+<CODE ENDS>
 ]]></artwork>
 </figure>
 ]]></artwork>
 </figure>
-    Note: due to RFC formatting conventions, lines exceeding the column width
-    in the patch above are split using a backslash character. The backslashes
-    at the end of a line and the white space at the beginning
-    of the following line are not part of the patch. A properly formatted patch
-    including the three changes above is available at
-    <eref target="http://jmvalin.ca/misc_stuff/opus_update.patch"/>.
-    </t>
     </section>
 
     </section>
 
-    <section title="Downmix to Mono">
+    <section title="Integer wrap-around in LSF decoding">
+      <t>
+        It was discovered -- also from decoder fuzzing -- that an integer wrap-around could
+        occur when decoding line spectral frequency coefficients from extreme bitstreams.
+        The end result of the wrap-around is an illegal read access on the stack, which
+        the authors do not believe is exploitable but should nonetheless be fixed. The following
+        patch to line 137 of silk/NLSF_stabilize.c prevents the problem:
+      </t>
+<figure>
+<artwork><![CDATA[
+<CODE BEGINS>
+           /* Keep delta_min distance between the NLSFs */
+         for( i = 1; i < L; i++ )
+-            NLSF_Q15[i] = silk_max_int( NLSF_Q15[i], \
+NLSF_Q15[i-1] + NDeltaMin_Q15[i] );
++            NLSF_Q15[i] = silk_max_int( NLSF_Q15[i], \
+silk_ADD_SAT16( NLSF_Q15[i-1], NDeltaMin_Q15[i] ) );
+         /* Last NLSF should be no higher than 1 - NDeltaMin[L] */
+<CODE ENDS>
+]]></artwork>
+</figure>
+
+    </section>
+
+    <section title="Cap on Band Energy">
+      <t>On extreme bit-streams, it is possible for log-domain band energy levels
+        to exceed the maximum single-precision floating point value once converted
+        to a linear scale. This would later cause the decoded values to be NaN,
+        possibly causing problems in the software using the PCM values. This can be
+        avoided with the following patch to line 552 of celt/quant_bands.c:
+      </t>
+<figure>
+<artwork><![CDATA[
+<CODE BEGINS>
+       {
+          opus_val16 lg = ADD16(oldEBands[i+c*m->nbEBands],
+                          SHL16((opus_val16)eMeans[i],6));
++         lg = MIN32(QCONST32(32.f, 16), lg);
+          eBands[i+c*m->nbEBands] = PSHR32(celt_exp2(lg),4);
+       }
+       for (;i<m->nbEBands;i++)
+<CODE ENDS>
+]]></artwork>
+</figure>
+    </section>
+
+    <section title="Hybrid Folding" anchor="folding">
+      <t>When encoding in hybrid mode at low bitrate, we sometimes only have
+        enough bits to code a single CELT band (8 - 9.6 kHz). When that happens,
+        the second band (CELT band 18, from 9.6 to 12 kHz) cannot use folding
+        because it is wider than the amount already coded, and falls back to
+        LCG noise. Because it can also happen on transients (e.g. stops), it
+        can cause audible pre-echo.
+      </t>
+      <t>
+        To address the issue, we change the folding behavior so that it is
+        never forced to fall back to LCG due to the first band not containing
+        enough coefficients to fold onto the second band. This
+        is achieved by simply repeating part of the first band in the folding
+        of the second band. This changes the code in celt/bands.c around line 1237:
+      </t>
+<figure>
+<artwork><![CDATA[
+<CODE BEGINS>
+          b = 0;
+       }
+-      if (resynth && M*eBands[i]-N >= M*eBands[start] && \
+(update_lowband || lowband_offset==0))
++      if (resynth && (M*eBands[i]-N >= M*eBands[start] || \
+i==start+1) && (update_lowband || lowband_offset==0))
+             lowband_offset = i;
++      if (i == start+1)
++      {
++         int n1, n2;
++         int offset;
++         n1 = M*(eBands[start+1]-eBands[start]);
++         n2 = M*(eBands[start+2]-eBands[start+1]);
++         offset = M*eBands[start];
++         /* Duplicate enough of the first band folding data to \
+be able to fold the second band.
++            Copies no data for CELT-only mode. */
++         OPUS_COPY(&norm[offset+n1], &norm[offset+2*n1 - n2], n2-n1);
++         if (C==2)
++            OPUS_COPY(&norm2[offset+n1], &norm2[offset+2*n1 - n2], \
+n2-n1);
++      }
++
+       tf_change = tf_res[i];
+       if (i>=m->effEBands)
+       {
+<CODE ENDS>
+]]></artwork>
+</figure>
+
+      <t>
+       as well as line 1260:
+      </t>
+
+<figure>
+<artwork><![CDATA[
+<CODE BEGINS>
+          fold_start = lowband_offset;
+          while(M*eBands[--fold_start] > effective_lowband);
+          fold_end = lowband_offset-1;
+-         while(M*eBands[++fold_end] < effective_lowband+N);
++         while(++fold_end < i && M*eBands[fold_end] < \
+effective_lowband+N);
+          x_cm = y_cm = 0;
+          fold_i = fold_start; do {
+            x_cm |= collapse_masks[fold_i*C+0];
+
+<CODE ENDS>
+]]></artwork>
+</figure>
+      <t>
+        The fix does not impact compatibility, because the improvement does
+        not depend on the encoder doing anything special. There is also no
+        reasonable way for an encoder to use the original behavior to
+        improve quality over the proposed change.
+      </t>
+    </section>
+
+    <section title="Downmix to Mono" anchor="stereo">
       <t>The last issue is not strictly a bug, but it is an issue that has been reported
       when downmixing an Opus decoded stream to mono, whether this is done inside the decoder
       or as a post-processing step on the stereo decoder output. Opus intensity stereo allows
       <t>The last issue is not strictly a bug, but it is an issue that has been reported
       when downmixing an Opus decoded stream to mono, whether this is done inside the decoder
       or as a post-processing step on the stereo decoder output. Opus intensity stereo allows
@@ -237,6 +405,34 @@ RESAMPLER_ORDER_FIR_12 * sizeof( opus_int16 ) );
       outside of the decoder).
       </t>
     </section>
       outside of the decoder).
       </t>
     </section>
+
+
+    <section title="New Test Vectors">
+      <t>Changes in <xref target="folding"/> and <xref target="stereo"/> have
+        sufficient impact on the testvectors to make them fail. For this reason,
+        this document also updates the Opus test vectors. The new test vectors now
+        include two decoded outputs for the same bitstream. The outputs with
+        suffix 'm' do not apply the CELT 180-degree phase shift as allowed in
+        <xref target="stereo"/>, while the outputs without the suffix do. An
+        implementation is compliant as long as it passes either set of vectors.
+      </t>
+      <t>
+        In addition, any Opus implementation
+        that passes the original test vectors from <xref target="RFC6716">RFC 6716</xref>
+        is still compliant with the Opus specification. However, newer implementations
+        SHOULD be based on the new test vectors rather than the old ones.
+      </t>
+      <t>The new test vectors are located at
+        <eref target="https://www.ietf.org/proceedings/98/slides/materials-98-codec-opus-newvectors-00.tar.gz"/>.
+      </t>
+    </section>
+
+    <section anchor="security" title="Security Considerations">
+      <t>This document adds no new security considerations on top of
+        <xref target="RFC6716">RFC 6716</xref>.
+      </t>
+    </section>
+
     <section anchor="IANA" title="IANA Considerations">
       <t>This document makes no request of IANA.</t>
 
     <section anchor="IANA" title="IANA Considerations">
       <t>This document makes no request of IANA.</t>
 
@@ -246,12 +442,13 @@ RESAMPLER_ORDER_FIR_12 * sizeof( opus_int16 ) );
 
     <section anchor="Acknowledgements" title="Acknowledgements">
       <t>We would like to thank Juri Aedla for reporting the issue with the parsing of
 
     <section anchor="Acknowledgements" title="Acknowledgements">
       <t>We would like to thank Juri Aedla for reporting the issue with the parsing of
-      the Opus padding.</t>
+      the Opus padding. Also, thanks to Jonathan Lennox and Mark Harris for their
+      feedback on this document.</t>
     </section>
   </middle>
 
   <back>
     </section>
   </middle>
 
   <back>
-    <references title="References">
+    <references title="Normative References">
       <?rfc include="http://xml.resource.org/public/rfc/bibxml/reference.RFC.2119.xml"?>
       <?rfc include="http://xml.resource.org/public/rfc/bibxml/reference.RFC.6716.xml"?>
 
       <?rfc include="http://xml.resource.org/public/rfc/bibxml/reference.RFC.2119.xml"?>
       <?rfc include="http://xml.resource.org/public/rfc/bibxml/reference.RFC.6716.xml"?>