Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design: Add ability to create sound from string buffer (DEF-1967) #3289

Closed
britzl opened this issue May 23, 2019 · 14 comments · Fixed by #5675
Closed

Design: Add ability to create sound from string buffer (DEF-1967) #3289

britzl opened this issue May 23, 2019 · 14 comments · Fixed by #5675
Assignees
Labels
engine Issues related to the Defold engine feature request A suggestion for a new feature sound Issues related to sound handling
Projects
Milestone

Comments

@britzl
Copy link
Contributor

britzl commented May 23, 2019

Similarly to what can be done with loading textures dynamically using gui.new_texture it would be nice to be able to do sound.new_sound

Requested on the forum: (LINK REMOVED)

@britzl britzl added bug Something is not working as expected engine Issues related to the Defold engine feature request A suggestion for a new feature and removed bug Something is not working as expected feature request A suggestion for a new feature labels May 23, 2019
@britzl britzl added this to the Sound milestone Sep 21, 2019
@britzl britzl added the sound Issues related to sound handling label Oct 29, 2019
@otsakir
Copy link
Contributor

otsakir commented Feb 17, 2021

Starting working on this...

@otsakir
Copy link
Contributor

otsakir commented Feb 23, 2021

@britzl, does it make sense to support OGG raw data, or is it only about WAV ?

@britzl
Copy link
Contributor Author

britzl commented Feb 23, 2021

@otsakir for an MVP I'd say only WAV but I think we'd like support for both.

@otsakir
Copy link
Contributor

otsakir commented Feb 23, 2021

ok.

MVP ?

@britzl
Copy link
Contributor Author

britzl commented Feb 23, 2021

MVP ?

Minimum Viable Product. The least amount of functionality that we can release with. If the feature is released with .wav support only it would still be useful. Adding .ogg support could be done in a second step.

@otsakir
Copy link
Contributor

otsakir commented Feb 23, 2021

@britzl , @JCash , before discussing details of the implementation, the summary of my plan is the following:

  • Provide, new lua api calls for creating/tearing down the dynamic sound.

  • Creating, initialize and link the following sound data structures:

    • dmSound::SoundData - holds the raw data of the sound and is added to g_SoundSystem.m_SoundData array container.

    • dmGameSystem::Sound - represents the sound resource and links to the SoundData created above. There is no file behind it. It's created and populated manually. It's kept in the sound component below. SoundComponent.m_Resource property.

    • dmGameSystem::SoundComponent - i guess we need a sound component to play sounds. comp_sound.cpp:CompSoundOnMessage() relies on a proper sound component and a proper Sound resource to work its magic. I'm not really sure how to create this though.

Having all these set up, should theoretically allow us to play the sound with sound.play(). It will be the responsibility of the user to create and release the dynamic sound.

Does this make any sense to you ? Any easier approach ? I also checked gui.new_texture but it uses the Scene to keep track of dynamic textures and didn't find many analogies that would help me.

Thanks

@JCash
Copy link
Contributor

JCash commented Feb 25, 2021

Provide, new lua api calls for creating/tearing down the dynamic sound.

Sounds good.
Just a heads up (for the implementation): Lua is always run on the Lua (main) thread. Sound is threaded.
So any communication between the two needs to respect that.

Creating, initialize and link the following sound data structures:
dmSound::SoundData - holds the raw data of the sound and is added to g_SoundSystem.m_SoundData array container.
dmGameSystem::Sound - represents the sound resource and links to the SoundData created above. There is no file behind it. It's created and populated manually. It's kept in the sound component below. SoundComponent.m_Resource property.

Although there is no actual file representing this dynamic sound, I believe it should go through our resource system.
That way it can be treated as any other sounds/resources.
Multiple reasons:

  • Resource management: we should keep our resources in one place
  • Ref counting: we have that system already in place in our resource system
  • Profiling: we list the current resources and their cost in our web profiler
  • Resource properties: A feature we have is that you can use resource properties to assign/reassign resources on components.
  • No extra types

For an example of how to create resources, checkout res_sound.cpp, and also script_resource.cpp (SetTexture).

Also, if we have the sound as a proper resource, we can use the dynamic sound data interchangeably with the other code. (i.e. no need for another type dmGameSystem::Sound). See comp_sprite.cpp (GetMaterial) for an example of this.

dmGameSystem::SoundComponent - i guess we need a sound component to play sounds. comp_sound.cpp:CompSoundOnMessage() relies on a proper sound component and a proper Sound resource to work its magic. I'm not really sure how to create this though.

I think it's reasonable to expect to have a sound component in order to play the sounds.

It will be the responsibility of the user to create and release the dynamic sound.

While I agree, we also have our paradigm of just unloading a collection. This is how things are designed.
It's not technically difficult to handle this case though, as we have all the events/data we need to deallocate/stop things when the comp_sound world is destroyed.

Given the fire-and-forget nature of our sounds, I wonder how we handle the refcounting/destruction on those resources when we unload a collection when the sounds are still playing.

Overall, I think what you have here is a good strategy moving forwards.

@otsakir
Copy link
Contributor

otsakir commented Mar 1, 2021

@JCash, thanks for the suggestiong. I went through them and resource related code. I also tried to better understand defold's bootstrapping wrt resources using gdb on a simple project with a sample ogg sound. Let me describe the mechanism as i've observed and understood it. I think it will save us some time later from clearing up misunderstandings.

  1. Projects resources are loaded in a tree like fashion, starting from the root and recurring in increasing depths while parsing 'files' like, getting child nodes, parsing those etc. All loaded resources are kept in dmEnding::Engine.m_Factory.m_Resources. They are later accessed from there by their hash.
  2. The primary functions for access or load a resource are resource.cpp:Get()/DoGet(). When asking for a resource from Get(), it will first try to get an existing one by looking it up in ...m_Factory.m_Resources and fallback to loading from some sort of storage if it is missing. By 'storage' i mean either loading it from disk or over http.
  3. There are actually two types of resources we need. 'soundc' and the 'wavc'. In conventional data, the first links to the second.

Having that in place, and after considering your suggestions i'm sharing my view on the obstacles i'm facing:

  • Relying on functionality that ends up invoking to DoGet() to load the resources makes no sense. DoGet() will try to load stuff from disk/network. Such functionality is res_sound.cpp:ResSoundCreate() -> AcquireResources() -> resource.cpp:Get() -> DoGet().
  • script_resource.cpp:SetTexture() replaces the texture_image of an existing resource. It does not create a new one.

Instead, wrt to the core of the implementation i'm suggesting the following approach:

  1. Use resource.cpp:InsertResource() the way it's used in DoGet(). Create resources for both 'soundc' and 'wavc' resources. i.e. create an SResourceDescriptor, populate it, pass it to InsertResources().

https://github.com/defold/defold/blob/dev/engine/resource/src/resource.cpp#L1101
https://github.com/defold/defold/blob/dev/engine/resource/src/resource.cpp#L1245

  1. Use comp_sound.cpp:CompSoundCreate() to create the component. Give it the 'soundc' resource created above.

I won't get into the details of the rest of the implementation now. I believe we can settle these later as the feature gets together.

JCash, tell me if this makes any sense to you.

cc @britzl

@JCash
Copy link
Contributor

JCash commented Mar 1, 2021

Hi @otsakir !

Projects resources are loaded in a tree like fashion, starting from the root and recurring in increasing depths while parsing 'files' like, getting child nodes, parsing those etc. All loaded resources are kept in dmEnding::Engine.m_Factory.m_Resources. They are later accessed from there by their hash.
The primary functions for access or load a resource are resource.cpp:Get()/DoGet(). When asking for a resource from Get(), it will first try to get an existing one by looking it up in ...m_Factory.m_Resources and fallback to loading from some sort of storage if it is missing. By 'storage' i mean either loading it from disk or over http.

I think this is a good summary of how we're dealing with resources

There are actually two types of resources we need. 'soundc' and the 'wavc'. In conventional data, the first links to the second.

The .soundc is a resource for a sound component. Since we're going to use an existing sound component, we don't need to create a new resource of that type.

The .wavc and .oggc resources are the ones that we want to be able to generate.

Relying on functionality that ends up invoking to DoGet() to load the resources makes no sense. DoGet() will try to load stuff from disk/network. Such functionality is res_sound.cpp:ResSoundCreate() -> AcquireResources() -> resource.cpp:Get() -> DoGet().
script_resource.cpp:SetTexture() replaces the texture_image of an existing resource. It does not create a new one.

Use resource.cpp:InsertResource() the way it's used in DoGet(). Create resources for both 'soundc' and 'wavc' resources. i.e. create an SResourceDescriptor, populate it, pass it to InsertResources().

Correct. Our current example is dmResource::SetTexture(), which allows us to update an existing resource.
I expect us to get a similar function for creating a new resource (E.g. InsertResource) and in our case fill it with sound data.
No file I/O will be required since we'll get a cache hit (we just added the resource!)

Use comp_sound.cpp:CompSoundCreate() to create the component. Give it the 'soundc' resource created above.

Yes. We'll require the developer to add a sound component in the editor, then use our Lua script and perhaps extension to create and update the sound data resource.
A good example is comp_mesh.cpp, where we use a .bufferc resource (from res_mesh.cpp), or if it's overridden by the CompMeshSetProperty() from Lua.

@otsakir
Copy link
Contributor

otsakir commented Mar 1, 2021

@JCash, thanks.

Updating an existing sound soundc resource instead of creating it simplifies things a lot.

So, as far as i understand, the developer will be required to create a sound component and provide some sort of sound for it initially. Then, he/she will be able to update it using some sort of new lua api call like sound.set_sound(). This will trigger the actual update of the resource using dmResource::SetResource(factory, resource_hash, new_buffer, new_buffer_size).

One final point. The update will affect the data of the wavc/oggc resource. Not the soundc resource. So, we need the wavc/oggc path/hash. Thus, we either ask the developer to provide the path in the lua api call, or we let him/her give the soundc resource path/hash and somehow extract the wavc/oggc from it.

Thoughts ?

@otsakir
Copy link
Contributor

otsakir commented Mar 1, 2021

Btw, I was wondering about the actual format of the raw data provided by the user in the lua api call when updating the wavc/oggc resource content. From my understanding it should follow the format of a valid .wav/.ogg file. Does this stand ?

@britzl
Copy link
Contributor Author

britzl commented Mar 2, 2021

Yes, the data should be in the format of a wav or ogg file

@JCash
Copy link
Contributor

JCash commented Mar 2, 2021

So, as far as i understand, the developer will be required to create a sound component and provide some sort of sound for it initially. Then, he/she will be able to update it using some sort of new lua api call like sound.set_sound(). This will trigger the actual update of the resource using dmResource::SetResource(factory, resource_hash, new_buffer, new_buffer_size).

Correct. We currently have no way to specify "no resource", and I'm not sure we want it (it's a bit of a separate discussion).

One final point. The update will affect the data of the wavc/oggc resource. Not the soundc resource. So, we need the wavc/oggc path/hash. Thus, we either ask the developer to provide the path in the lua api call, or we let him/her give the soundc resource path/hash and somehow extract the wavc/oggc from it.

Correct. The path is required to update a resource. Example is resource.set_texture(path, data).

To get the path of a resource, we need to implement it in the sound component type. See comp_mesh.cpp/CompMeshGetProperty() for how to get the (potentially overridden) vertices, textureN properties.
We should also implement the setter, to that we can reassign sounds too (this can be done in MVP2).

Btw, I was wondering about the actual format of the raw data provided by the user in the lua api call when updating the wavc/oggc resource content. From my understanding it should follow the format of a valid .wav/.ogg file. Does this stand ?

As @britzl mentioned, the raw data we want in our case is .wav/.ogg.

It may be beneficial however to know that most often our internal formats need to be stored in our own data formats.
Example is script_resource.cpp/SetTexture() that takes the raw data from the user, and wraps that in our own internal
format from graphics_ddf.proto/graphics_ddf.h.

@otsakir
Copy link
Contributor

otsakir commented Mar 3, 2021

@JCash , thanks for the feedback.

I've already created sample code that updates the sound resource on the fly and seems to work. Let me draft the implementation, based on the texture example presented here:

https://defold.com/ref/resource/#resource.set_texture:path-table-buffer

Lua api

Below, a preview of the lua code that updates the internal .wav resource of a sound component. /go#sound is the url of the component. sound is the name of the new property of the sound component that refers to the internal wavc/oggc resource.

local f = io.open("....../audio1.wav")
local c  = f:read("*all")
f:close()

local resource_path = go.get("/go#sound", "sound")
resource.set_sound( resource_path, c )

Implementation

script_resource.cpp

New static int SetSound(lua_State* L) function that after taking care of the lua invocation plumbing calls dmResource::SetResource(). The published lua function name is set_sound.

comp_sound.cpp:GetSoundGetProperty

Provide support for a new property named sound. It will return the path to the internal resource.

Concerns

A sound component that initially has a 'wavc' resource bound to it, will throw a decoder error if it's updated to an 'oggc' type of resource. Are we ok with that ? Maybe do some forward check and provide a more informative error message to the developer.

otsakir added a commit to otsakir/defold that referenced this issue Mar 5, 2021
* 'Published' sound resource property in sound component.
  Now accessible from lua resource.get()
* Support for sound-data resource update from lua string
otsakir added a commit to otsakir/defold that referenced this issue Mar 15, 2021
otsakir added a commit to otsakir/defold that referenced this issue Mar 18, 2021
otsakir added a commit to otsakir/defold that referenced this issue Mar 22, 2021
otsakir added a commit to otsakir/defold that referenced this issue Mar 23, 2021
otsakir added a commit to otsakir/defold that referenced this issue Mar 27, 2021
otsakir added a commit to otsakir/defold that referenced this issue Mar 29, 2021
…3289

Commit: "More robust test for dynamic sound update - ref defold#3289"
This reverts commit 9a57d6a.
@britzl britzl linked a pull request Mar 30, 2021 that will close this issue
@britzl britzl added this to To do in 1.2.182 via automation Mar 30, 2021
1.2.182 automation moved this from To do to Done Mar 30, 2021
britzl pushed a commit that referenced this issue Mar 30, 2021
* Update sound resource from buffer - ref #3289

* 'Published' sound resource property in sound component.
  Now accessible from lua resource.get()

* Support for sound-data resource update from lua string

* Test case for stringbuffer sound update - ref #3289
pull bot pushed a commit to proteanblank/defold that referenced this issue Jul 28, 2021
* Update sound resource from buffer - ref defold#3289

* 'Published' sound resource property in sound component.
  Now accessible from lua resource.get()

* Support for sound-data resource update from lua string

* Test case for stringbuffer sound update - ref defold#3289
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engine Issues related to the Defold engine feature request A suggestion for a new feature sound Issues related to sound handling
Projects
No open projects
1.2.182
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants