Use ByteBuffer for sending audio

[MinnDevelopment]

Pull Request Etiquette

• I have checked the PRs for upcoming features/bug fixes.
• I have read the contributing guidelines.

Changes

• Internal code
• Library interface (affecting end-user code)
• Documentation
• Other: _____

Closes Issue: NaN

Description

This change allows send handlers to re-use a single buffer for all packets.

Example Usage

public class AudioSender implements AudioSendHandler {
    private AudioPlayer player;
    private AudioFrame frame;
    private ByteBuffer buffer = ByteBuffer.allocate(1024);

    public AudioSender(AudioPlayer player) {
        this.player = player;
    }

    @Override
    public boolean canProvide() {
        frame = player.provide();
        return frame != null;
    }

    @Override
    public ByteBuffer provide20MsAudio() {
        frame.getData(buffer.array(), frame.getDataLength()); // lavaplayer still had the old broken behavior where it used offset as length in arraycopy
        buffer.position(0);
        buffer.limit(frame.getDataLength());
        return buffer;
    }

    @Override
    public boolean isOpus() {
        return true;
    }
}

[MinnDevelopment]

Both receive and send seem to be working as expected after these changes. This implies encoding/decoding of opus and encryption of audio are unaffected and work just like before.

The conversion for v3 users is actually a one-liner:

-    return data;
+    return data == null ? null : ByteBuffer.wrap(data);

[DV8FromTheWorld]

Can you provide a full example of a v3 vs v4 implementation of AudioSendHandler?

Also, why does this not impact the AudioReceiveHandler?

[MinnDevelopment]

Can you provide a full example of a v3 vs v4 implementation of AudioSendHandler?

Its exactly the same as v3 except that provide20MsAudio should return a ByteBuffer with position pointing at the read-head of the buffer. For proper usage you would use a MutableAudioFrame in lavaplayer which re-uses the buffer for each frame and populates it.
player.provide(mutableFrame) returning true if something was written to the frame, else false.
This can be used for canProvide(). Then in provide20MsAudio you simply return the buffer after calling flip().

private final MutableAudioFrame frame = new MutableAudioFrame();
private final ByteBuffer buffer = ByteBuffer.allocate(1024);

public MyConstructor() {
    this.frame.setBuffer(buffer);
}

public ByteBuffer provide20MsAudio() {
    return buffer.flip();
}

public boolean canProvide() {
    return player.provide(frame);
}

The difference is that in v3 you have to return a fitting byte array instead which requires re-allocation for every frame.

Also, why does this not impact the AudioReceiveHandler?

Because audio receive is not affected by changes to the send system.

[DV8FromTheWorld]

Because audio receive is not affected by changes to the send system.

Let me rephrase: Would there be any benefit we could eke out by using a buffer of some sort for the receiving system as well?

[MinnDevelopment]

The receive system is generally not very memory optimized. Additionally, I only want to make changes to receive required to keep it working for now.

[natanbc]

Currently, lavaplayer might change the buffer in MutableAudioFrame, so a safer approach for the send handler would be something like this (reflection is needed because there is no other way of knowing whether or not the buffer was replaced)

public class MySendHandler implements AudioSendHandler {
    private static final Function<MutableAudioFrame, ByteBuffer> BUFFER_GETTER;

    static {
        try {
            Field f = MutableAudioFrame.class.getDeclaredField("frameBuffer");
            f.setAccessible(true);
            BUFFER_GETTER = frame -> {
                try {
                    return (ByteBuffer)f.get(frame);
                } catch(Exception impossible) {
                    throw new AssertionError(impossible);
                }
            };
        } catch(Exception e) {
            throw new ExceptionInInitializerError(e);
        }
    }

    //other fields and methods

    @Override
    public ByteBuffer provide20MsAudio() {
        if(BUFFER_GETTER.apply(frame) != buffer) {
            frame.getData(buffer.array(), frame.getDataLength());
            frame.setBuffer(buffer.limit(buffer.capacity()));
        }
        return buffer.position(0).limit(frame.getDataLength());
    }
}

[MinnDevelopment]

@natanbc can't you just use getData(buffer.array(), 0) instead?

[natanbc]

That would work as well, but would copy the data every time, this approach only copies when the buffer is replaced. After setting the buffer to the original one, new data will be written to it until LP changes the buffer again

[MinnDevelopment]

It would be ideal if MutableAudioFrame would have a getter for the buffer it uses. Something like provideBuffer(): ByteBuffer or similar which would be safe to use.

[MinnDevelopment]

In any case, this doesn't really matter for our interface change. We only need to provide a way to re-use one buffer properly. That should be possible regardless of lavaplayer usage.

[DV8FromTheWorld]

The AudioEcho example is a quality addition. Nice work.

[DV8FromTheWorld]

LGTM

[schnapster]

Shouldn't this method have a ByteBuffer as an argument that the audio data should be written to?
Returning a buffer looks off because how does the implementor know when the buffer it returns here is safe to be used again? Or am I on the wrong track about how this would be used in an implementation with the goal of minimal allocations?

[MinnDevelopment]

If you would do provide(ByteBuffer) you get additional overhead for copying over data. It would also severely limit usability since the buffer could not be re-allocated correctly. The buffer should be updated by the next provide cycle. Meaning canProvide signals the buffer was sent and the next one will be requested. If you are worried about concurrency you can always use buffer.duplicate() instead or have a queue of buffers. Since we immediately read the buffer and write the encrypted data into a different buffer of the packet provider it should almost immediately be usable again.

[MinnDevelopment]

Additionally, the goal of these changes was not to force users into single-allocation which your suggestion would imply. The goal was to make it possible in the first place. Since returning a buffer with a position and limit you can create a fitting requirement without re-allocation compared to using a byte[]. This is due to the fact that byte[] needs to be allocated with the exact size needed for the audio frame while the buffer can be adjusted for it without any allocations. To achieve 0 allocations you simply need to have one buffer that is filled by canProvide() and then returned by provide() as was shown by the examples in the comments of this PR.

[schnapster]

Thanks for clarifying!