Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(11218)

#12866: Add support for 24-bit samples in the audioop module

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 4 months ago by peder.jorgensen
Modified:
5 years, 3 months ago
Reviewers:
pitrou, storchaka
CC:
ezio.melotti, eric.araujo, r.david.murray, devnull_psf.upfronthosting.co.za, peder.jorgensen_gmail.com, storchaka
Visibility:
Public.

Patch Set 1 #

Total comments: 12

Patch Set 2 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/audioop.rst View 1 3 chunks +11 lines, -8 lines 0 comments Download
Doc/whatsnew/3.4.rst View 1 1 chunk +6 lines, -0 lines 0 comments Download
Lib/test/test_audioop.py View 1 22 chunks +33 lines, -27 lines 0 comments Download
Modules/audioop.c View 1 49 chunks +200 lines, -223 lines 0 comments Download

Messages

Total messages: 2
AntoinePitrou
http://bugs.python.org/review/12866/diff/9185/Modules/audioop.c File Modules/audioop.c (right): http://bugs.python.org/review/12866/diff/9185/Modules/audioop.c#newcode301 Modules/audioop.c:301: ((unsigned char *)(cp) + (i))[2] + \ How about ...
5 years, 3 months ago #1
storchaka_gmail.com
5 years, 3 months ago #2
http://bugs.python.org/review/12866/diff/9185/Modules/audioop.c
File Modules/audioop.c (right):

http://bugs.python.org/review/12866/diff/9185/Modules/audioop.c#newcode301
Modules/audioop.c:301: ((unsigned char *)(cp) + (i))[2] + \
On 2013/10/15 16:23:51, AntoinePitrou wrote:
> How about (unsigned char *)(cp)[i+2] ?

Perhaps compiler will optimize common subexpression (cp+i). With cp+(i+2) there
is less chance.

http://bugs.python.org/review/12866/diff/9185/Modules/audioop.c#newcode312
Modules/audioop.c:312: #define SETINT8(cp, i, val) do {                \
On 2013/10/15 16:23:51, AntoinePitrou wrote:
> Why another do..while around SETINTX?

Well, this is not needed.

http://bugs.python.org/review/12866/diff/9185/Modules/audioop.c#newcode324
Modules/audioop.c:324: ((unsigned char *)(cp) + (i))[1] = (val);       \
On 2013/10/15 16:23:51, AntoinePitrou wrote:
> 2, not 1.

Thanks.

http://bugs.python.org/review/12866/diff/9185/Modules/audioop.c#newcode326
Modules/audioop.c:326: ((signed char *)(cp) + (i))[0] = (val) >> 16;   \
On 2013/10/15 16:23:51, AntoinePitrou wrote:
> I don't think the cast to "signed char *" makes a difference here?

Practically no (on supported platforms and with other assumptions in this
module), but it accents that high byte contains sign bit.

http://bugs.python.org/review/12866/diff/9185/Modules/audioop.c#newcode330
Modules/audioop.c:330: ((unsigned char *)(cp) + (i))[0] = (int)(val);          \
On 2013/10/15 16:23:51, AntoinePitrou wrote:
> Why the int cast? It's not in the big-endian code above.

val can be double. I should add (int) in the big-endian code too.

http://bugs.python.org/review/12866/diff/9185/Modules/audioop.c#newcode787
Modules/audioop.c:787: if ( !PyArg_ParseTuple(args, "s#i:cross", &cp, &len,
&size) )
On 2013/10/15 16:23:51, AntoinePitrou wrote:
> Not related to this patch, but I'm surprised the "s#" code was kept here. In
> Python 3, we should generally use "s*".

Issue #16685.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+