Expose some AudioStreamPlayback methods (namely mix_audio()).

[stechyo]

Pretty much the same as:

• Expose some AudioStreamPlayback methods #75065
• Allow directly reading audio data #81324

The former is archived, and the latter seems to go against the maintainers' wishes that start, stop, and mix should not be exposed outward. I added some shim functions to get around this, not sure if that's fine. There are several proposals with different use cases for this:

• Expose more audio methods to be able to generate waveform previews godot-proposals#127
• Expose AudioStreamPreview to GDscript godot-proposals#6466
• Add ability read audio directly from audio playback instead of using the recording effect godot-proposals#7601
• Expose AudioStream + AudioStreamPlayback methods to GDExtension godot-proposals#8609

My usecase is embedding steam-audio as a GDExtension, which requires me to have these functions available as bound methods.

[ellenhp]

We really should have a story here because a lot of users want to get raw pcm data from a stream and it is currently impossible without a gdextension. In gdext you can just call mix repeatedly on a playback but that's not reasonable to make every user do who wants this. It feels like every few weeks in the discord someone asks about it.

[stechyo]

We really should have a story here because a lot of users want to get raw pcm data from a stream and it is currently impossible without a gdextension. In gdext you can just call mix repeatedly on a playback but that's not reasonable to make every user do who wants this. It feels like every few weeks in the discord someone asks about it.

Just a note, correct me if I'm wrong: unless you're referring to something other than the _mix() method that gets generated for AudioStreamPlayback, I don't think that works. That method is virtual and, when called in GDExt for a native audio stream, does nothing (at least in my experience, I may have missed something of course).

[theraot]

I remember pushback on the lines of "GDScript is too slow for that", but we should be able to use GDScript to connect this to a mixing solution in GPU, for example with compute shaders. In fact, I was able to use the fragment shader in Godot 3.x to generate audio. I shared a proof of concept in discussions: godotengine/godot-proposals#6476 (comment)

Futhermore, Godot have integration with other lnaguages that are considered faster such as C# and rust.

I have a post by Juan Linietsky agreeing on exposing more audio internals: https://x.com/reduzio/status/1683126371013349378

Thus, I hope this does not get the "It won't work on GDScript" treatment.

[stechyo]

I dislike the fact that I'm exposing this to GDScript at all but "should some things only be exposed on GDExt and not C#/GDS" is a much larger and possibly controversial discussion. This PR is not the ideal solution (I've learned this with time, see stechyo/godot-steam-audio#39), but I'm not aware of any efforts to do something similar but in a cleaner way (again, "knowing what is being worked on" is sometimes an issue in Godot, but again, that's another discussion).

[reduz]

Looks good to me other than what I mentioned.

[AThousandShips]

Since much of the code and especially the documentation is the same as my original PR I have to ask if you salvaged that one?

[stechyo]

Yeah, as I said in the description it's pretty much the same as two other PRs. Is there some "salvaging etiquette" I'm not aware of?

[AThousandShips]

I'd say that explicitly stating it would be good in the future as it's not clear from "essentially the same", copying code is something you need to be explicit about (doing so without being clear would usually be called plagiarism)

I'd ask that you add me as a co-author in that case, by adding:

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>

To your commit (the empty line is important)

Thank you!

[stechyo]

The XML description was certainly salvaged, so it's more than reasonable to add you as a co-author. Sorry for not having done so previously!

[MasterDingo]

Any chances to see this PR merged into 4.3?

[AThousandShips]

4.3 is in final release phase so this will be not be merged before 4.4

[stechyo]

Now that 4.3 has released, when could we merge this so that it's ready for 4.4? It seems there's no issues remaining with the code.

[AThousandShips]

It needs an approval by the audio team

[adamscott]

Just wondering, before approving the PR.

1. Is it just to tweak the PackedVector2Array in order to playback the data afterwards?
2. What about surround systems/mixing?

[stechyo]

1. My use case is for tweaking the audio frame array, yes. I believe there are also use cases for audio visualisers and the like.
2. I haven’t considered surround systems.

[mk56-spn]

1. My use case is for tweaking the audio frame array, yes. I believe there are also use cases for audio visualisers and the like.
2. I haven’t considered surround systems.

To add a bit to this. It was specifically static ( so audio visualizers that show the whole songs graph ( think audacity ) as opposed to the ones you'd see on a music player) that were a major pain to do beforehand. So yeah this is very much appreciated for that sort of thing.

[stechyo]

@adamscott do these answers block your approval? Just wondering

[adamscott]

@adamscott do these answers block your approval? Just wondering

@stechyo I'm investigating. I'm gonna give you an answer shortly.

[adamscott]

I haven’t considered surround systems.

It seems that surround is only done at the server level when adjusting volume, so no need for a special case for surround systems. Sorry, I mixed up stuff.

[adamscott]

Small changes IMO, but your PR is close to be merged!

[stechyo]

I think all changes should be addressed :) could someone approve the workflow?

[stechyo]

@adamscott is anything missing to merge? Just checking, idk what the etiquette is on this repo (if the commenter needs to resolve conversations, or if it's the PR author who does so).

[adamscott]

Seems good to me!

[stechyo]

Awesome! @reduz I think your requested change should be addressed as well, can we merge this?

[stechyo]

@reduz @AThousandShips my apologies for double-pinging here, but this PR has been approved by one engine maintainer and the comment of another maintainer has been addressed for a while now. Is there anything I can do to help merge this? I would love to get this in before the 4.4 freeze begins, and it seems to be ready to merge.

[adamscott]

I set the PR in the merge queue, so it should be merged as soon as possible. Thanks again for the PR!

[Repiteo]

Thanks, and congratulations on your first contribution! 🎉