pb_bss icon indicating copy to clipboard operation
pb_bss copied to clipboard

Switch to alternative pesq implementation

Open jonashaag opened this issue 3 years ago • 12 comments

It's a fork of the original pesq repo that does proper error handling (rather than simply exiting with exit(1); from C code).

cc FYI @mpariente

jonashaag avatar Oct 12 '20 12:10 jonashaag

Recommended by original author here https://github.com/ludlows/python-pesq/issues/12#issuecomment-645097746

jonashaag avatar Oct 12 '20 12:10 jonashaag

@jonashaag Thanks for the PR. Do you know why @ludlows does not merge that Repo? It would be cleaner when the PyPI version will be updated.

boeddeker avatar Oct 12 '20 12:10 boeddeker

No idea, sorry

jonashaag avatar Oct 12 '20 12:10 jonashaag

Or simply switch to https://github.com/vBaiCai/python-pesq, which I have worked with in the past and didn’t cause any trouble

jonashaag avatar Oct 14 '20 08:10 jonashaag

When we added pesq, we considered three implementations:

  • Our own implementation that used subprosess
    • + Zero modifications of original code
    • - Need file system
    • + Can run in Parallel
  • ludlows
    • + Python wrapper
    • + Supports all features
    • - Modified PESQ source code
    • - Cannot run in Parallel
  • vBaiCai
    • + Python wrapper
    • - Modified PESQ source code
    • - Disabled wideband
    • - Cannot run in Parallel

Since vBaiCai does not support wideband it is not an option that I want to choose. I am not sure in which source code it was, but in one someone changed the PESQ algorithm. The intention was most likely to stabilize, but it changes the actual values. This disables the ability of the comparison between papers and I want to have the score that the original PESQ produced.

We decided against our own implementation, because of the file system overhead.

boeddeker avatar Oct 14 '20 08:10 boeddeker

You're right, I just checked, the vBaiCai version has modified PESQ source.

Maybe I'll create my own wrapper... I mean the wrapper source codes look really simple

jonashaag avatar Oct 14 '20 09:10 jonashaag

What are we going to do with this problem?

jonashaag avatar Dec 12 '20 08:12 jonashaag

I was hoping that https://github.com/ludlows/python-pesq fixes this problem. It looks like a huge modification of the source code is necessary and I don't know when they fix it. It is problematic, that the owner does no longer use the code.

One solution would be to release our internal code that uses subprocess. But this introduces a file writing and subprocess overhead, so I am not sure, if it is better. Also, it needs at the moment gcc.

boeddeker avatar Dec 14 '20 12:12 boeddeker

it is a little complicated to change the code to a thread-safe version which supports multi-threads as well. right now, I am not sure the priority among thread-safe version and the one supporting to error handling yet.

ludlows avatar Dec 14 '20 16:12 ludlows

As far as I know, your pesq code is thread safe, because the python GIL is not released. Letting the code release the GIL may be difficult, because the original code uses global states.

boeddeker avatar Dec 14 '20 16:12 boeddeker

yes. but it means the code cannot running in parallel since the GIL exists. the easy way to accelarate computing for pesq in python is subprocessing. so handling the errors well seems more important, right?

ludlows avatar Dec 14 '20 16:12 ludlows

Yes, subprocess allows to execute the computation in parallel. An another alternative is multiprocessing. I personally use most of the time MPI, where I calculate PESQ, there I don't care about the GIL.

Subprosess also handles the problem with the error handling. At least partially.

For the error handling it would be nice, when an exception is thrown. That simplifies the bug search

boeddeker avatar Dec 14 '20 16:12 boeddeker