Audaspace: Load CoreAudio device on demand #107607

Richard Antalik · 2023-05-04T10:09:37+02:00

Richard Antalik commented

2023-05-04 10:09:37 +02:00

Refactor CoreAudioDevice code to support on demand device opening and
closing when sound is not playing.

Currently device is opened when AUD_init() is called and then closed
immediately. This is because it seems, that this is causing issues with
synchronizer and causes AV sync to fail.

Anothe issue seems to be usage of audio scrubbing. This seems to confuse the device and it fails to play audio at all after some time. For WASAPI this was also problematic and a solution could be to create a timer in new thread, that would delay closing of the device.

Refactor CoreAudioDevice code to support on demand device opening and closing when sound is not playing. Currently device is opened when AUD_init() is called and then closed immediately. This is because it seems, that this is causing issues with synchronizer and causes AV sync to fail. Anothe issue seems to be usage of audio scrubbing. This seems to confuse the device and it fails to play audio at all after some time. For WASAPI this was also problematic and a solution could be to create a timer in new thread, that would delay closing of the device.

Richard Antalik added 1 commit 2023-05-04 10:09:47 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

0b834f3573 Audaspace: Load CoreAudio device on demand

Refactor CoreAudioDevice code to support on demand device opening and
closing when sound is not playing.

Currently device is opened when AUD_init() is called and then closed
immediately. This is because it seems, that this is causing issues with
synchronizer and causes AV sync to fail.

Richard Antalik commented

2023-05-04 10:10:47 +02:00

@blender-bot package

Blender Bot commented

2023-05-04 10:10:50 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR107607) when ready.

Joerg Mueller requested changes 2023-05-04 16:47:35 +02:00

Joerg Mueller left a comment

Please follow the code style of audaspace and do not apply Blender's clang format in this file. This leads to way too many changes in this file. Audaspace has it's own clang format changes, but you should only format actually changed code.

I'd also name the methods simply open and close, the Device is superfluous and the open function is not on demand, it just opens.

Apart from that, the changes look good as far as I can tell. Cannot try it, since I lack the hardware though.

Please follow the code style of audaspace and do not apply Blender's clang format in this file. This leads to way too many changes in this file. Audaspace has it's own clang format changes, but you should only format actually changed code. I'd also name the methods simply `open` and `close`, the Device is superfluous and the open function is not on demand, it just opens. Apart from that, the changes look good as far as I can tell. Cannot try it, since I lack the hardware though.

Richard Antalik added 1 commit 2023-06-20 20:42:29 +02:00

dbd56306b5 continue testing of audio stuff

Sergey Sharybin referenced this pull request

2023-07-05 11:32:11 +02:00

Audio missing in rendered Video file using pip bpy module 3.5 #109524

Richard Antalik commented

2023-09-05 01:08:18 +02:00

@neXyon Hi, this should be ready to review,.Can you have a look?

Joerg Mueller commented

2023-09-07 12:03:14 +02:00

Please fix the formatting first as I mentioned before, otherwise it's hard to read the diff.

Richard Antalik commented

2023-09-14 23:30:15 +02:00

@neXyon Sorry I was pretty sure I did that, but I was wrong. Would you in future accept plain cleanup patch to format the file? Otherwise I have to undo unintended changes manually which is bit tedious.

Joerg Mueller commented

2023-09-15 09:24:11 +02:00

Not sure what you mean? Do you mean running clang-format as auto-formatter? Clang format can also be run on just the changes in a diff which would be ideal here. So, can you apply the cleanup patch here already?

Richard Antalik commented

2023-09-15 13:45:15 +02:00

Not sure what you mean?

I meant to make the files conform to format rules before I make any changes to files. I don't have IDE setup to work on Audaspace code, and MSVC is ignoring clang format currently anyway. Not sure if I broke the addon somehow. It's just in Blender we like to keep our code formatted at all times, so I thought about doing same thing in this case.

So, can you apply the cleanup patch here already?

OMG I forgot to commit changes before pushing :D

> Not sure what you mean? I meant to make the files conform to format rules before I make any changes to files. I don't have IDE setup to work on Audaspace code, and MSVC is ignoring clang format currently anyway. Not sure if I broke the addon somehow. It's just in Blender we like to keep our code formatted at all times, so I thought about doing same thing in this case. > So, can you apply the cleanup patch here already? OMG I forgot to commit changes before pushing :D

Joerg Mueller reviewed 2023-09-23 16:02:46 +02:00

Joerg Mueller left a comment

Overall changes look good now! I've added some comments inline of what I'd like to see changed still and additionally:

The threaded functionality is very similar to what ThreadedDevice is doing so it is a bit of code duplication if I did not miss anything and that code can be reused (m_device_opened and m_delayed_close_finished correspond to m_playing, m_stop in some sense and m_delayed_close_thread corresponds to m_thread ofc). Additionally, the automatic opening and closing could be used by other backends as well. Therefore, please create a class OpenCloseDevice or so, derived from ThreadedDevice that adds abstract/purely virtual methods for open/close and does all the functionality that can be shared, i.e., implementing ThreadedDevice's runMixingThread with what closeAfterDelay does.

Overall changes look good now! I've added some comments inline of what I'd like to see changed still and additionally: The threaded functionality is very similar to what `ThreadedDevice` is doing so it is a bit of code duplication if I did not miss anything and that code can be reused (`m_device_opened` and `m_delayed_close_finished` correspond to `m_playing`, `m_stop` in some sense and `m_delayed_close_thread` corresponds to `m_thread` ofc). Additionally, the automatic opening and closing could be used by other backends as well. Therefore, please create a class `OpenCloseDevice` or so, derived from `ThreadedDevice` that adds abstract/purely virtual methods for `open`/`close` and does all the functionality that can be shared, i.e., implementing `ThreadedDevice`'s `runMixingThread` with what `closeAfterDelay` does.

extern/audaspace/plugins/coreaudio/CoreAudioDevice.cpp Outdated

						
				@ -170,0 +196,4 @@

				{

					for(;;)

					{

						std::this_thread::sleep_for(std::chrono::microseconds(5));

Joerg Mueller commented

2023-09-23 15:51:24 +02:00

I dislike the hardcoding of this value, especially since it reappears just a few lines below again. Maybe you could turn this into a proper variable and even base it on some other value that makes sense (see e.g. sleep_duration in WASAPIDevice.cpp:95).

I dislike the hardcoding of this value, especially since it reappears just a few lines below again. Maybe you could turn this into a proper variable and even base it on some other value that makes sense (see e.g. `sleep_duration` in `WASAPIDevice.cpp:95`).

Richard Antalik commented

2024-01-19 17:49:51 +01:00

Sure, makes sense to use variables here, but not sure if I can base this on something else. My goal was to have multiple seconds until device is closed. If these delays would not be constant, some users may have issues and some not, or they would have trouble providing reproducible case.

extern/audaspace/plugins/coreaudio/CoreAudioDevice.cpp Outdated

						
				@ -170,0 +198,4 @@

					{

						std::this_thread::sleep_for(std::chrono::microseconds(5));

						if(m_playback || m_playback_stopped_time == 0)

							time(&m_playback_stopped_time);

Joerg Mueller commented

2023-09-23 15:49:18 +02:00

Please don't mix std::chrono code with ctime code, just use std::chrono everywhere!

Please don't mix `std::chrono` code with `ctime` code, just use `std::chrono` everywhere!

extern/audaspace/plugins/coreaudio/CoreAudioDevice.h Outdated

						
				@ -29,7 +29,9 @@

				#include "CoreAudioSynchronizer.h"

				#include "devices/SoftwareDevice.h"

				#include <ctime>

Joerg Mueller commented

2023-09-23 15:52:32 +02:00

Please use chrono instead of ctime.

Please use `chrono` instead of `ctime`.

Francesco Siddi commented

2023-10-31 10:15:43 +01:00

@iss what is the status of this patch? Can this be wrapped up?

Richard Antalik commented

2023-11-01 01:42:43 +01:00

@fsiddi Will prioritize this. I focused most of my efforts on retiming and bugfixing, so did revisit this when I got time to spare.

Richard Antalik commented

2024-01-17 20:51:48 +01:00

Overall changes look good now! I've added some comments inline of what I'd like to see changed still and additionally:

The threaded functionality is very similar to what ThreadedDevice is doing so it is a bit of code duplication if I did not miss anything and that code can be reused (m_device_opened and m_delayed_close_finished correspond to m_playing, m_stop in some sense and m_delayed_close_thread corresponds to m_thread ofc). Additionally, the automatic opening and closing could be used by other backends as well. Therefore, please create a class OpenCloseDevice or so, derived from ThreadedDevice that adds abstract/purely virtual methods for open/close and does all the functionality that can be shared, i.e., implementing ThreadedDevice's runMixingThread with what closeAfterDelay does.

Sorry for delay, I have allocated some time to finish this now. Also sorry if I am plain wrong here, I have still limited understanding of cpp abstraction in this case.

OpenCloseDevice would have to have also start and stop virtual functions, since it needs to use playing function to control when the device should be opened and closed.
Then looking at ThreadedDevice this also implements playing function, but it would have to be overridden, which I can do. In that case, I would be able to reuse ThreadedDevice::start() function, but to me it seems I would have to do quite a bit of gymnastics to make it work. To me this solution looks highly confusing, and I would propose to derive from SoftwareDevice class. The threading that is used is super simple anyway.

If I am wrong, I would still probably do that as I think it is better starting point for code optimization.

> Overall changes look good now! I've added some comments inline of what I'd like to see changed still and additionally: > > The threaded functionality is very similar to what `ThreadedDevice` is doing so it is a bit of code duplication if I did not miss anything and that code can be reused (`m_device_opened` and `m_delayed_close_finished` correspond to `m_playing`, `m_stop` in some sense and `m_delayed_close_thread` corresponds to `m_thread` ofc). Additionally, the automatic opening and closing could be used by other backends as well. Therefore, please create a class `OpenCloseDevice` or so, derived from `ThreadedDevice` that adds abstract/purely virtual methods for `open`/`close` and does all the functionality that can be shared, i.e., implementing `ThreadedDevice`'s `runMixingThread` with what `closeAfterDelay` does. Sorry for delay, I have allocated some time to finish this now. Also sorry if I am plain wrong here, I have still limited understanding of cpp abstraction in this case. `OpenCloseDevice` would have to have also `start` and `stop` virtual functions, since it needs to use `playing` function to control when the device should be opened and closed. Then looking at `ThreadedDevice` this also implements playing function, but it would have to be overridden, which I can do. In that case, I would be able to reuse `ThreadedDevice::start()` function, but to me it seems I would have to do quite a bit of gymnastics to make it work. To me this solution looks highly confusing, and I would propose to derive from `SoftwareDevice` class. The threading that is used is super simple anyway. If I am wrong, I would still probably do that as I think it is better starting point for code optimization.

Richard Antalik force-pushed core-audio-on-demand from b5b5b95fda to 8f65025a11

2024-01-18 21:53:46 +01:00

Compare

Richard Antalik added 1 commit 2024-01-19 18:00:23 +01:00

ec85a92532 Use std::chrono for time

Richard Antalik added 2 commits 2024-01-19 18:13:07 +01:00

ace1906761 Merge branch 'main' into core-audio-on-demand

e9af744b55 Clang format

Richard Antalik requested review from Joerg Mueller 2024-01-19 18:14:11 +01:00

Richard Antalik commented

2024-01-19 18:36:41 +01:00

@neXyon Do you have policy on how copyright blocks should be updated?

I have thought a bit more about implications of previous message, and currently I think there would be some issues if you needed a class derived from both OpenCloseDevice and ThreadedDevice

@neXyon Do you have policy on how copyright blocks should be updated? I have thought a bit more about implications of previous message, and currently I think there would be some issues if you needed a class derived from both `OpenCloseDevice` and `ThreadedDevice`

Joerg Mueller commented

2024-02-29 23:25:23 +01:00

Sorry for taking some time to respond, I'm as busy as usual. The patch looks good and you're right, the code probably doesn't get much better if you derive from ThreadedDevice. I've made minor fixes (attached), please add those and then I think this is ready to merge. It would be great if you could also open a PR for this at upstream audaspace (https://github.com/audaspace/audaspace/) as mentioned before. And no I don't have a policy for the copyright blocks. You can put your name and current dates if you wish :)

Sorry for taking some time to respond, I'm as busy as usual. The patch looks good and you're right, the code probably doesn't get much better if you derive from `ThreadedDevice`. I've made minor fixes (attached), please add those and then I think this is ready to merge. It would be great if you could also open a PR for this at upstream audaspace (https://github.com/audaspace/audaspace/) as mentioned before. And no I don't have a policy for the copyright blocks. You can put your name and current dates if you wish :)

openclosedevice.diff

3.1 KiB

Richard Antalik commented

2024-03-01 20:36:12 +01:00

Thanks for checking, will apply your changes and post PR in audaspace repo. As far as copyright goes, I will then just update year. I don't believe in intellectual property anyway :D