ludlows / PESQ

PESQ (Perceptual Evaluation of Speech Quality) Wrapper for Python Users (narrow band and wide band)
https://github.com/ludlows/PESQ
MIT License
518 stars 97 forks source link

Improved error handling #16

Closed Rafagd closed 3 years ago

Rafagd commented 3 years ago
ludlows commented 3 years ago

thanks for your work.

could you add correspoding testing functions for the new error throwing feature inside the folder tests .

jonashaag commented 3 years ago

Does the library support multi threaded usage? If so, I think it would be better to at least use thread locals for error reporting so that error reporting is thread safe as well.

boeddeker commented 3 years ago

I once tried to release the GIL in PESQ and it turned out, that PESQ uses some global statements (It either broke or returned wrong values, I don't remember). In my code I currently always assume that PESQ is not thread safe.

Maybe that would be a point for the future: Release the GIL and make the code thread safe.

boeddeker commented 3 years ago

Is there a reason, why you changed so many code lines? It looks like many parts of the old code did the same. Since PESQ has an official source code, I think it would be nice to be as close as possible to this.

Rafagd commented 3 years ago

Is there a reason, why you changed so many code lines? It looks like many parts of the old code did the same. Since PESQ has an official source code, I think it would be nice to be as close as possible to this.

It was the only way to reliably return the errors using the Error_Flag that was already present instead of having 2 different error returning mechanisms like in the other PR. Previously there was no way I could know what values would be returned, as each function inside the pesq_measure does a different thing with the Error_Flag and pesq_measure was just returning the same value to the outside world.

That said, I think I'll double check for leaks.

Ps.: Most of the lines are reindentation as well.

boeddeker commented 3 years ago

Then I probably don't see what you changed. Github shows too much deltas. I see that you removed many

if ((*Error_Flag) == 0)
{

and this changed the indent, so the github diff is difficult to read.

Rafagd commented 3 years ago

So, the way the function was written, it checks for the Error_Flag then it calls the next function that may change the flag. To make sure I'm always talking about the last Error_Flag, I do the thing first, then I check the flag and return early. Otherwise I would need to double check the flag multiple times. I may need to add a goto there to do some clean-up on failure.

jonashaag commented 3 years ago

and this changed the indent, so the github diff is difficult to read.

There is an option to hide all white space changes.

Rafagd commented 3 years ago

There is an option to hide all white space changes.

TIL. OMG, that is so much better to read!

ludlows commented 3 years ago

hi @Rafagd, thanks for your contribution. would you like to send this PR to the dev branch established by me 3 days ago?

Rafagd commented 3 years ago

Sorry for the delay, just did it now.