[Home]

Summary:ASTERISK-14695: [patch] Frame.c adjustment for Speex
Reporter:Nicholas Blasgen (nblasgen)Labels:
Date Opened:2009-08-23 13:02:19Date Closed:2010-06-11 13:32:13
Priority:MinorRegression?No
Status:Closed/CompleteComponents:General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) issue15672.patch
Description:In function speex_samples, there is a test for the number of remaining bits.

if ((len * 8 - bit) < 5) { ... }

The Speex docs (http://speex.org/docs/manual/speex-manual.ps) say in Section 5.5 that:

"In cases where the number of frames is not determined by an out-of-band mechanism, it is possible to include a terminator code. That terminator consists of the code 15 (decimal) encoded with 5 bits, as shown in Table 9.2. Note that as of version 1.0.2, calling speex_bits_write automatically inserts the terminator so as to fill the last byte. This doesn’t involves any overhead and makes sure Speex can always detect when there is no more frame in a packet."

Your code makes this terminator a requirement when 0 bits is also allowed, though it might not be suggested.  I propose to add an extra line saying the following:

if ((len * 8 - bit) == 0)
   break;
else if ((len * 8 - bit) < 5) {
   ...
}
Comments:By: Nicholas Blasgen (nblasgen) 2010-03-23 23:08:38

Guess this never made it into the 1.4 branch.
By: Paul Belanger (pabelanger) 2010-06-01 10:46:40

Upload patch based on reporters comments.  It seems like a redundant if statement to me.
By: Nicholas Blasgen (nblasgen) 2010-06-01 12:31:36

It's not redundant since the statement that checks for less than 5 ends up putting out an error message.  Since 0 bits isn't an error and should be handled the same way as a terminator, i proposed adding in a check for it.
By: Paul Belanger (pabelanger) 2010-06-01 12:57:51

Warning != Error

Either way, a developer will review the patch and make a decision.
By: Digium Subversion (svnbot) 2010-06-11 13:23:05

Repository: asterisk
Revision: 269960

U   branches/1.4/main/frame.c

------------------------------------------------------------------------
r269960 | tilghman | 2010-06-11 13:23:05 -0500 (Fri, 11 Jun 2010) | 8 lines

For SpeeX, 0 bits remaining is valid and does not need an emitted warning.

(closes issue ASTERISK-14695)
Reported by: nblasgen
Patches:
      issue15672.patch uploaded by pabelanger (license 224)
Tested by: nblasgen

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=269960
By: Digium Subversion (svnbot) 2010-06-11 13:31:14

Repository: asterisk
Revision: 269976

_U  trunk/
U   trunk/main/frame.c

------------------------------------------------------------------------
r269976 | tilghman | 2010-06-11 13:31:14 -0500 (Fri, 11 Jun 2010) | 15 lines

Merged revisions 269960 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r269960 | tilghman | 2010-06-11 13:23:05 -0500 (Fri, 11 Jun 2010) | 8 lines
 
 For SpeeX, 0 bits remaining is valid and does not need an emitted warning.
 
 (closes issue ASTERISK-14695)
  Reported by: nblasgen
  Patches:
        issue15672.patch uploaded by pabelanger (license 224)
  Tested by: nblasgen
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=269976
By: Digium Subversion (svnbot) 2010-06-11 13:32:13

Repository: asterisk
Revision: 269977

_U  branches/1.6.2/
U   branches/1.6.2/main/frame.c

------------------------------------------------------------------------
r269977 | tilghman | 2010-06-11 13:32:12 -0500 (Fri, 11 Jun 2010) | 22 lines

Merged revisions 269976 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r269976 | tilghman | 2010-06-11 13:31:14 -0500 (Fri, 11 Jun 2010) | 15 lines
 
 Merged revisions 269960 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r269960 | tilghman | 2010-06-11 13:23:05 -0500 (Fri, 11 Jun 2010) | 8 lines
   
   For SpeeX, 0 bits remaining is valid and does not need an emitted warning.
   
   (closes issue ASTERISK-14695)
    Reported by: nblasgen
    Patches:
          issue15672.patch uploaded by pabelanger (license 224)
    Tested by: nblasgen
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=269977