Added ability to invert the phase of only one channel

[cruxicheiros]

What it says on the tin.
Since reversing the phase of only one channel requires use of audioop, I figured it would be better to extend the invert_phase function within pydub rather than add that functionality in an external program.

This shouldn't break any existing programs using Pydub since it will behave in exactly the same way as it did before if no arguments are passed.

[jiaaro]

This is cool.

I'd suggest recombining the channels with AudioSegment.from_mono_audiosegments(seg1, seg2, …, segn) – It's significantly more performant (and less code).

Also, if possible, could you add a few lines to the unit test for invert_phase that exercise new API you've added?

Finally, I'm not sure, but I think the existing code works on audio streams with more than 2 channels - it's good to support arbitrary channel counts where possible so that we can eventually have complete support for surround sound :)

[cruxicheiros]

How would you suggest implementing support for an arbitrary number of channels- **kwargs?

[cruxicheiros]

Fixed the unit test. I haven't been able to test the unit test in line with the rest of them since avconv throws an error when I run the testing code. So I recommend you run it yourself before pushing.

Also, another question, what is rms?

[jiaaro]

RMS is a method of measuring signal strength (stands for root mean squared), though I'd recommend using dBFS since it better matches how humans perceive loudness.

regarding multichannel, could be something like this (based on your code):

def invert_phase(seg, channels=None):
    if channels is None:
        inverted = audioop.mul(seg._data, seg.sample_width, -1.0)  
        return seg._spawn(data=inverted)

    if seg.channels != len(channels):
        raise ValueError("the \"channels\" kwarg must contain one value for each channel in the AudioSegment.")
    
    mono_segs = seg.split_to_mono()
    for mono_seg, phase_change in zip(mono_segs, channels):
        if phase_change == 1:
            mono_seg = mono_seg.invert_phase()
        mono_segs.append(mono_seg)

    return seg.from_mono_audiosegments(left, right)

[cruxicheiros]

Working on it 😺
I've approached it like this, but I want to clarify that the ability to pass a mono audio segment to a function and have it return a segment with more than one channel is functionality you want to preserve. If it isn't it might make more sense to implement some sort of mono_to_stereo function that returns a list.

def invert_phase(seg, channels=None):
    """
    channels- specifies which channel (left or right) to reverse the phase of.
    Note that mono AudioSegments will become stereo.
    """
    if channels == None:
        inverted = audioop.mul(seg._data, seg.sample_width, -1.0)  
        return seg._spawn(data=inverted)
    
    else:
        if seg.channels == len(channels):
            mono_segs = seg.split_to_mono()
            
            for i in range(len(channels)):
                if channels[i] == 1:
                    mono_segs[i] == mono_segs[i].invert_phase()

        elif seg.channels == 1 and len(channels) > seg.channels:
            mono_segs = []
            for i in range(len(channels)):
                mono_segs.append(seg)
                
                if channels[i] == 1:
                    mono_segs[i] == mono_segs[i].invert_phase()
            
        else:
            raise ValueError("the \"channels\" kwarg must contain one value for each channel in the AudioSegment.")
        
        return seg.from_mono_audiosegments(*mono_segs)`

edit: formatting is screwy and I'm not sure why :|

[jiaaro]

In that function, the first section (which deals with if channels == None), works with any number of channels and doesn't change the number of channels. Because split_to_mono() works on mono audio segments it shouldn't be necessary to have a special case for mono signals.

[cruxicheiros]

I've thought about this some and tested your hypothesis and came to the conclusion that there are a couple of different cases where you might want to be using a mono audio segment, and they both have to be handled differently:

Case 1, when you have a mono AudioSegment and want it to become an AudioSegment with multiple channels. A similar example in Pydub is that if you have a mono recording and want to pan it, the recording is going to go from mono to stereo.

Case 2, when you have a mono AudioSegment and you want it to stay mono.

By default, Case 2 should happen, because if you have a mono AudioSegment and don't specify that you want it to become stereo (or multi channel) then it definitely should not. But if a mono audio segment is passed and a number of channels is specified, then it should follow the pattern of other functions in pydub and convert it from mono to stereo.

So, three different cases need to be handled:

-Mono to mono (1 channel > 1 channel)
-Mono to multi channel (1 channel -> n channels)
-Multi channel to multi channel (n channels -> n channels)

The only case that shouldn't work is (n channels -> x channels) where x != n.

The other way to deal with this is to make it so that mono NEVER converts to stereo automatically and requiring an explicit statement (e.g. AudioSegment.mono_to_stereo(channels)) but it would be annoying to set up because it would break programs that already use Pydub. However it might in the long run make it so that blocks of logic like this aren't as necessary.

[jiaaro]

Seems to me that pan is an operation that specifically deals with the stereo aspect of a signal, where phase inversion does not. So pan doesn't really have any meaning except in the context of a stereo output. Phase inversion does though, and probably (I think) shouldn't change the number of channels.

I don't think it's too annoying to write:

stereo_sound = mono_sound.set_channels(2).invert_phase([1, 0])

[cruxicheiros]

Yeah I reckon you're right.
I ran into a new issue though while writing a unit test for >2 channels though: it's not going to be possible to split an AudioSegment containing multiple audio channels without rewriting audioop.tomono entirely.

I'm not sure whether that's a problem for a separate issue or not, since if there's one instance of assuming either stereo or mono, it's probably present throughout audioop and may warrant its own project to change it.

[jiaaro]

@cruxicheiros you can use audio_segment.split_to_mono() which returns a list of mono audio segments (one per channel, can be called on a mono segment and return a list with one item)

[cruxicheiros]

No, you can't.

audio_segment.split_to_mono() relies on audioop.tomono() and while it will accept AudioSegments with more than 2 channels, it will only ever return 2.

    def split_to_mono(self):
        if self.channels == 1:
            return [self]

        left_channel = audioop.tomono(self._data, self.sample_width, 1, 0)
        right_channel = audioop.tomono(self._data, self.sample_width, 0, 1)

        return [self._spawn(data=left_channel,
                            overrides={'channels': 1,
                                       'frame_width': self.sample_width}),
                self._spawn(data=right_channel,
                            overrides={'channels': 1,
                                       'frame_width': self.sample_width})]

[jiaaro]

ah, right you are. This is still outstanding: #165

edit: btw thanks for all your work on this ticket :)

[cruxicheiros]

And I'd like to thank you for being awesome to work with on it.

I reckon the next steps should be to:

• merge this ticket as it is right now since it is completely functional when you assume that there are going to be 1 or 2 channels (which the rest of the code does right now anyway)
• make a new branch (maybe named 'multi-channel') and work on converting the whole thing to work with multiple channels, since it's a thing that needs to be done and will probably require quite a lot of things to be changed. Eventually, it would be merged with pydub/master.

I'd be happy to contribute to that as well since this is a really useful repository and I'm currently using it quite a lot. 😸

[jiaaro]

works for me - my only concern is that invert_phase shouldn't implicitly change the number of channels. I can make that change during the merge unless you disagree (or prefer to do it)

[cruxicheiros]

This should do it.

edit: Ah pants, looks like I messed something up while 'fixing' it :| Gonna go find out why that build failed and sort it out

[cruxicheiros]

Okay, so this should be that problem solved, for real.