Skip to content

bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) #18608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 11, 2020
Merged

bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) #18608

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
42b2a61
Port audioop extension module to multiphase initialization(PEP 489)
shihai1991 Feb 22, 2020
981bac9
📜🤖 Added by blurb_it.
blurb-it[bot] Feb 22, 2020
b776cb1
port _audioopstate() macro to inline function
shihai1991 Feb 25, 2020
8434b61
Merge branch 'bpo_1635741_audioop' of https://github.com/shihai1991/c…
shihai1991 Feb 25, 2020
1a1b56e
Update Modules/audioop.c
shihai1991 Mar 1, 2020
cfa67a7
using the var name module to replace m
shihai1991 Mar 1, 2020
5364b74
using the var name module to replace m in get_audioop_state()
shihai1991 Mar 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
port _audioopstate() macro to inline function
  • Loading branch information
@shihai1991
shihai1991 committed Feb 25, 2020
commit b776cb15040b364de6103a34a1324a1fd8c1399b
76 changes: 50 additions & 26 deletions Modules/audioop.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,15 +375,22 @@ static PyModuleDef audioopmodule;

typedef struct {
PyObject *AudioopError;
} _audioopstate;
} audioop_state;

#define _audioopstate(o) ((_audioopstate *)PyModule_GetState(o))
static inline audioop_state *
get_audioop_state(PyObject *m)
{
void *state = PyModule_GetState(m);
assert(state != NULL);
return (audioop_state *)state;
}

static int
audioop_check_size(PyObject *module, int size)
{
if (size < 1 || size > 4) {
PyErr_SetString(_audioopstate(module)->AudioopError, "Size should be 1, 2, 3 or 4");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"Size should be 1, 2, 3 or 4");
return 0;
}
else
Expand All @@ -396,7 +403,8 @@ audioop_check_parameters(PyObject *module, Py_ssize_t len, int size)
if (!audioop_check_size(module, size))
return 0;
if (len % size != 0) {
PyErr_SetString(_audioopstate(module)->AudioopError, "not a whole number of frames");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"not a whole number of frames");
return 0;
}
return 1;
Expand Down Expand Up @@ -428,7 +436,8 @@ audioop_getsample_impl(PyObject *module, Py_buffer *fragment, int width,
if (!audioop_check_parameters(module, fragment->len, width))
return NULL;
if (index < 0 || index >= fragment->len/width) {
PyErr_SetString(_audioopstate(module)->AudioopError, "Index out of range");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"Index out of range");
return NULL;
}
val = GETRAWSAMPLE(width, fragment->buf, index*width);
Expand Down Expand Up @@ -619,7 +628,8 @@ audioop_findfit_impl(PyObject *module, Py_buffer *fragment,
double sum_ri_2, sum_aij_2, sum_aij_ri, result, best_result, factor;

if (fragment->len & 1 || reference->len & 1) {
PyErr_SetString(_audioopstate(module)->AudioopError, "Strings should be even-sized");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"Strings should be even-sized");
return NULL;
}
cp1 = (const int16_t *)fragment->buf;
Expand All @@ -628,7 +638,8 @@ audioop_findfit_impl(PyObject *module, Py_buffer *fragment,
len2 = reference->len >> 1;

if (len1 < len2) {
PyErr_SetString(_audioopstate(module)->AudioopError, "First sample should be longer");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"First sample should be longer");
return NULL;
}
sum_ri_2 = _sum2(cp2, cp2, len2);
Expand Down Expand Up @@ -686,11 +697,13 @@ audioop_findfactor_impl(PyObject *module, Py_buffer *fragment,
double sum_ri_2, sum_aij_ri, result;

if (fragment->len & 1 || reference->len & 1) {
PyErr_SetString(_audioopstate(module)->AudioopError, "Strings should be even-sized");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"Strings should be even-sized");
return NULL;
}
if (fragment->len != reference->len) {
PyErr_SetString(_audioopstate(module)->AudioopError, "Samples should be same size");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"Samples should be same size");
return NULL;
}
cp1 = (const int16_t *)fragment->buf;
Expand Down Expand Up @@ -730,14 +743,16 @@ audioop_findmax_impl(PyObject *module, Py_buffer *fragment,
double result, best_result;

if (fragment->len & 1) {
PyErr_SetString(_audioopstate(module)->AudioopError, "Strings should be even-sized");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"Strings should be even-sized");
return NULL;
}
cp1 = (const int16_t *)fragment->buf;
len1 = fragment->len >> 1;

if (length < 0 || len1 < length) {
PyErr_SetString(_audioopstate(module)->AudioopError, "Input sample should be longer");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"Input sample should be longer");
return NULL;
}

Expand Down Expand Up @@ -969,7 +984,8 @@ audioop_tomono_impl(PyObject *module, Py_buffer *fragment, int width,
if (!audioop_check_parameters(module, len, width))
return NULL;
if (((len / width) & 1) != 0) {
PyErr_SetString(_audioopstate(module)->AudioopError, "not a whole number of frames");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"not a whole number of frames");
return NULL;
}

Expand Down Expand Up @@ -1064,7 +1080,8 @@ audioop_add_impl(PyObject *module, Py_buffer *fragment1,
if (!audioop_check_parameters(module, fragment1->len, width))
return NULL;
if (fragment1->len != fragment2->len) {
PyErr_SetString(_audioopstate(module)->AudioopError, "Lengths should be the same");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"Lengths should be the same");
return NULL;
}

Expand Down Expand Up @@ -1310,7 +1327,8 @@ audioop_ratecv_impl(PyObject *module, Py_buffer *fragment, int width,
if (!audioop_check_size(module, width))
return NULL;
if (nchannels < 1) {
PyErr_SetString(_audioopstate(module)->AudioopError, "# of channels should be >= 1");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"# of channels should be >= 1");
return NULL;
}
if (width > INT_MAX / nchannels) {
Expand All @@ -1323,17 +1341,19 @@ audioop_ratecv_impl(PyObject *module, Py_buffer *fragment, int width,
}
bytes_per_frame = width * nchannels;
if (weightA < 1 || weightB < 0) {
PyErr_SetString(_audioopstate(module)->AudioopError,
PyErr_SetString(get_audioop_state(module)->AudioopError,
"weightA should be >= 1, weightB should be >= 0");
return NULL;
}
assert(fragment->len >= 0);
if (fragment->len % bytes_per_frame != 0) {
PyErr_SetString(_audioopstate(module)->AudioopError, "not a whole number of frames");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"not a whole number of frames");
return NULL;
}
if (inrate <= 0 || outrate <= 0) {
PyErr_SetString(_audioopstate(module)->AudioopError, "sampling rate not > 0");
PyErr_SetString(get_audioop_state(module)->AudioopError,
"sampling rate not > 0");
return NULL;
}
/* divide inrate and outrate by their greatest common divisor */
Expand Down Expand Up @@ -1374,7 +1394,7 @@ audioop_ratecv_impl(PyObject *module, Py_buffer *fragment, int width,
&d, &PyTuple_Type, &samps))
goto exit;
if (PyTuple_Size(samps) != nchannels) {
PyErr_SetString(_audioopstate(module)->AudioopError,
PyErr_SetString(get_audioop_state(module)->AudioopError,
"illegal state argument");
goto exit;
}
Expand Down Expand Up @@ -1903,18 +1923,22 @@ static PyMethodDef audioop_methods[] = {
};

static int
audioop_traverse(PyObject *m, visitproc visit, void *arg) {
_audioopstate *state = _audioopstate(m);
if (state != NULL)
audioop_traverse(PyObject *m, visitproc visit, void *arg)
{
audioop_state *state = (audioop_state *)PyModule_GetState(m);
if (state) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really possible that state can be NULL when this function is called?

Py_VISIT(state->AudioopError);
}
return 0;
}

static int
audioop_clear(PyObject *m) {
_audioopstate *state = _audioopstate(m);
if (state != NULL)
audioop_clear(PyObject *m)
{
audioop_state *state = (audioop_state *)PyModule_GetState(m);
if (state) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really possible that state can be NULL when this function is called?

module_clear() doesn't touch module->md_state value. Only module_dealloc() calls PyMem_FREE(m->md_state). But audioop_clear() must not be called on a deallocated module object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to see this question answered before approving the PR.

Copy link
Member

@encukou encukou Mar 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth us to change this logic as victor said?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created https://bugs.python.org/issue39824 to propose to change the Python behavior, so modules would not have to handle md_state=NULL anymore.

Py_CLEAR(state->AudioopError);
}
return 0;
}

Expand All @@ -1926,7 +1950,7 @@ audioop_free(void *m) {
static int
audioop_exec(PyObject* m)
{
_audioopstate *state = _audioopstate(m);
audioop_state *state = get_audioop_state(m);

state->AudioopError = PyErr_NewException("audioop.error", NULL, NULL);
if (state->AudioopError == NULL) {
Expand All @@ -1950,7 +1974,7 @@ static struct PyModuleDef audioopmodule = {
PyModuleDef_HEAD_INIT,
"audioop",
NULL,
sizeof(_audioopstate),
sizeof(audioop_state),
audioop_methods,
audioop_slots,
audioop_traverse,
Expand Down