diff mbox series

[v5,3/4] audio: Add functions to initialize buffers

Message ID 20250123-coreaudio-v5-3-6873df4215a0@daynix.com (mailing list archive)
State New
Headers show
Series coreaudio fixes | expand

Commit Message

Akihiko Odaki Jan. 23, 2025, 7:18 a.m. UTC 
These functions can be used to re-initialize buffers when hardware
parameters change due to device hotplug, for example.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 audio/audio_int.h |  2 ++
 audio/audio.c     | 24 ++++++++++++++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)

Comments

Philippe Mathieu-Daudé Jan. 23, 2025, 8:43 a.m. UTC | #1 
Hi Akihiko,

On 23/1/25 08:18, Akihiko Odaki wrote:
> These functions can be used to re-initialize buffers when hardware
> parameters change due to device hotplug, for example.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>   audio/audio_int.h |  2 ++
>   audio/audio.c     | 24 ++++++++++++++++++------
>   2 files changed, 20 insertions(+), 6 deletions(-)


> index 87b4e9b6f2f356b6e5e972eabc100cf270fcbc29..17c6bbd0ae9e6ff810c0989dbfa1710ef674ff0a 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1407,12 +1407,18 @@ void audio_run(AudioState *s, const char *msg)
>   #endif
>   }
>   
> +void audio_generic_initialize_buffer_in(HWVoiceIn *hw)
> +{
> +    g_free(hw->buf_emul);
> +    hw->size_emul = hw->samples * hw->info.bytes_per_frame;
> +    hw->buf_emul = g_malloc(hw->size_emul);

What about using g_realloc()? Otherwise LGTM.

> +    hw->pos_emul = hw->pending_emul = 0;
> +}
Akihiko Odaki Jan. 24, 2025, 4:58 a.m. UTC | #2 
On 2025/01/23 17:43, Philippe Mathieu-Daudé wrote:
> Hi Akihiko,
> 
> On 23/1/25 08:18, Akihiko Odaki wrote:
>> These functions can be used to re-initialize buffers when hardware
>> parameters change due to device hotplug, for example.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
>> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>> ---
>>   audio/audio_int.h |  2 ++
>>   audio/audio.c     | 24 ++++++++++++++++++------
>>   2 files changed, 20 insertions(+), 6 deletions(-)
> 
> 
>> index 
>> 87b4e9b6f2f356b6e5e972eabc100cf270fcbc29..17c6bbd0ae9e6ff810c0989dbfa1710ef674ff0a 100644
>> --- a/audio/audio.c
>> +++ b/audio/audio.c
>> @@ -1407,12 +1407,18 @@ void audio_run(AudioState *s, const char *msg)
>>   #endif
>>   }
>> +void audio_generic_initialize_buffer_in(HWVoiceIn *hw)
>> +{
>> +    g_free(hw->buf_emul);
>> +    hw->size_emul = hw->samples * hw->info.bytes_per_frame;
>> +    hw->buf_emul = g_malloc(hw->size_emul);
> 
> What about using g_realloc()? Otherwise LGTM.

g_realloc() will copy the content, which is not necessary here.

> 
>> +    hw->pos_emul = hw->pending_emul = 0;
>> +}
>
Philippe Mathieu-Daudé Jan. 25, 2025, 6:25 p.m. UTC | #3 
On 24/1/25 05:58, Akihiko Odaki wrote:
> On 2025/01/23 17:43, Philippe Mathieu-Daudé wrote:
>> Hi Akihiko,
>>
>> On 23/1/25 08:18, Akihiko Odaki wrote:
>>> These functions can be used to re-initialize buffers when hardware
>>> parameters change due to device hotplug, for example.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
>>> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>>> ---
>>>   audio/audio_int.h |  2 ++
>>>   audio/audio.c     | 24 ++++++++++++++++++------
>>>   2 files changed, 20 insertions(+), 6 deletions(-)
>>
>>
>>> index 
>>> 87b4e9b6f2f356b6e5e972eabc100cf270fcbc29..17c6bbd0ae9e6ff810c0989dbfa1710ef674ff0a 100644
>>> --- a/audio/audio.c
>>> +++ b/audio/audio.c
>>> @@ -1407,12 +1407,18 @@ void audio_run(AudioState *s, const char *msg)
>>>   #endif
>>>   }
>>> +void audio_generic_initialize_buffer_in(HWVoiceIn *hw)
>>> +{
>>> +    g_free(hw->buf_emul);
>>> +    hw->size_emul = hw->samples * hw->info.bytes_per_frame;
>>> +    hw->buf_emul = g_malloc(hw->size_emul);
>>
>> What about using g_realloc()? Otherwise LGTM.
> 
> g_realloc() will copy the content, which is not necessary here.

Oh I missed that! Maybe worth mentioning? Anyway, just nitpicking...

hw->size_emul = hw->samples * hw->info.bytes_per_frame;
g_free(hw->buf_emul); /* Discard previous content, no need to realloc */
hw->buf_emul = g_malloc(hw->size_emul);
Akihiko Odaki Jan. 26, 2025, 4:02 a.m. UTC | #4 
On 2025/01/26 3:25, Philippe Mathieu-Daudé wrote:
> On 24/1/25 05:58, Akihiko Odaki wrote:
>> On 2025/01/23 17:43, Philippe Mathieu-Daudé wrote:
>>> Hi Akihiko,
>>>
>>> On 23/1/25 08:18, Akihiko Odaki wrote:
>>>> These functions can be used to re-initialize buffers when hardware
>>>> parameters change due to device hotplug, for example.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
>>>> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>>>> ---
>>>>   audio/audio_int.h |  2 ++
>>>>   audio/audio.c     | 24 ++++++++++++++++++------
>>>>   2 files changed, 20 insertions(+), 6 deletions(-)
>>>
>>>
>>>> index 
>>>> 87b4e9b6f2f356b6e5e972eabc100cf270fcbc29..17c6bbd0ae9e6ff810c0989dbfa1710ef674ff0a 100644
>>>> --- a/audio/audio.c
>>>> +++ b/audio/audio.c
>>>> @@ -1407,12 +1407,18 @@ void audio_run(AudioState *s, const char *msg)
>>>>   #endif
>>>>   }
>>>> +void audio_generic_initialize_buffer_in(HWVoiceIn *hw)
>>>> +{
>>>> +    g_free(hw->buf_emul);
>>>> +    hw->size_emul = hw->samples * hw->info.bytes_per_frame;
>>>> +    hw->buf_emul = g_malloc(hw->size_emul);
>>>
>>> What about using g_realloc()? Otherwise LGTM.
>>
>> g_realloc() will copy the content, which is not necessary here.
> 
> Oh I missed that! Maybe worth mentioning? Anyway, just nitpicking...
> 
> hw->size_emul = hw->samples * hw->info.bytes_per_frame;
> g_free(hw->buf_emul); /* Discard previous content, no need to realloc */
> hw->buf_emul = g_malloc(hw->size_emul);

I think we can just leave it as is. This function is named 
audio_generic_initialize_buffer_in() so it is not expected to keep the 
state.
diff mbox series

Patch

diff --git a/audio/audio_int.h b/audio/audio_int.h
index 2d079d00a2596e6bce20e1f52abf01a88c4e0012..9ba4a144d571659ad2d32a4d6b2919ad0db89e25 100644
--- a/audio/audio_int.h
+++ b/audio/audio_int.h
@@ -187,9 +187,11 @@  struct audio_pcm_ops {
     void   (*volume_in)(HWVoiceIn *hw, Volume *vol);
 };
 
+void audio_generic_initialize_buffer_in(HWVoiceIn *hw);
 void audio_generic_run_buffer_in(HWVoiceIn *hw);
 void *audio_generic_get_buffer_in(HWVoiceIn *hw, size_t *size);
 void audio_generic_put_buffer_in(HWVoiceIn *hw, void *buf, size_t size);
+void audio_generic_initialize_buffer_out(HWVoiceOut *hw);
 void audio_generic_run_buffer_out(HWVoiceOut *hw);
 size_t audio_generic_buffer_get_free(HWVoiceOut *hw);
 void *audio_generic_get_buffer_out(HWVoiceOut *hw, size_t *size);
diff --git a/audio/audio.c b/audio/audio.c
index 87b4e9b6f2f356b6e5e972eabc100cf270fcbc29..17c6bbd0ae9e6ff810c0989dbfa1710ef674ff0a 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1407,12 +1407,18 @@  void audio_run(AudioState *s, const char *msg)
 #endif
 }
 
+void audio_generic_initialize_buffer_in(HWVoiceIn *hw)
+{
+    g_free(hw->buf_emul);
+    hw->size_emul = hw->samples * hw->info.bytes_per_frame;
+    hw->buf_emul = g_malloc(hw->size_emul);
+    hw->pos_emul = hw->pending_emul = 0;
+}
+
 void audio_generic_run_buffer_in(HWVoiceIn *hw)
 {
     if (unlikely(!hw->buf_emul)) {
-        hw->size_emul = hw->samples * hw->info.bytes_per_frame;
-        hw->buf_emul = g_malloc(hw->size_emul);
-        hw->pos_emul = hw->pending_emul = 0;
+        audio_generic_initialize_buffer_in(hw);
     }
 
     while (hw->pending_emul < hw->size_emul) {
@@ -1446,6 +1452,14 @@  void audio_generic_put_buffer_in(HWVoiceIn *hw, void *buf, size_t size)
     hw->pending_emul -= size;
 }
 
+void audio_generic_initialize_buffer_out(HWVoiceOut *hw)
+{
+    g_free(hw->buf_emul);
+    hw->size_emul = hw->samples * hw->info.bytes_per_frame;
+    hw->buf_emul = g_malloc(hw->size_emul);
+    hw->pos_emul = hw->pending_emul = 0;
+}
+
 size_t audio_generic_buffer_get_free(HWVoiceOut *hw)
 {
     if (hw->buf_emul) {
@@ -1477,9 +1491,7 @@  void audio_generic_run_buffer_out(HWVoiceOut *hw)
 void *audio_generic_get_buffer_out(HWVoiceOut *hw, size_t *size)
 {
     if (unlikely(!hw->buf_emul)) {
-        hw->size_emul = hw->samples * hw->info.bytes_per_frame;
-        hw->buf_emul = g_malloc(hw->size_emul);
-        hw->pos_emul = hw->pending_emul = 0;
+        audio_generic_initialize_buffer_out(hw);
     }
 
     *size = MIN(hw->size_emul - hw->pending_emul,