Fix #121015: Abnormal termination on exit after playback on MacOS #121242

Richard Antalik · 2024-04-30T05:11:19+02:00

Richard Antalik commented

2024-04-30 05:11:19 +02:00

Caused by thread being joinable when class was destroyed.

Join thread in destructor without trying to close the audio device,
since this is done in it's destructor.

Caused by thread being joinable when class was destroyed. Join thread in destructor without trying to close the audio device, since this is done in it's destructor.

Richard Antalik added 1 commit 2024-04-30 05:11:31 +02:00

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

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

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

3a87b17b36 Grammar and minor adjustments

Richard Antalik commented

2024-04-30 05:13:32 +02:00

@blender-bot build

Richard Antalik referenced this pull request

2024-04-30 05:24:14 +02:00

macOS: Abnormal termination on exit after playback #121015

Richard Antalik added 1 commit 2024-04-30 05:27:03 +02:00

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

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

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

d93c997c83 More grammar

Richard Antalik commented

2024-04-30 05:27:40 +02:00

@blender-bot build

Aras Pranckevicius reviewed 2024-04-30 08:36:44 +02:00

extern/audaspace/include/devices/OpenCloseDevice.h Outdated

						
				@ -51,2 +51,4 @@

					bool m_delayed_close_finished{false};

					/**

					 * Terminate thread without closing devide. Used when class is destroyed.

Aras Pranckevicius commented

2024-04-30 08:36:44 +02:00

Minor: "device", not "devide"

iss marked this conversation as resolved

Sergey Sharybin commented

2024-04-30 15:15:55 +02:00

This approach introduces up to 1 sec delay before Blender can be closed, which is undesirable. In a more ideal implementation destructor will notify the thread that it needs to be stopped. One of the ways to do so is to use conditional_variable. Some PoC:

class AUD_PLUGIN_API OpenCloseDevice : public SoftwareDevice
{
private:
    ...
	std::condition_variable stop_condition;
	std::mutex stop_condition_mutex;
    ...
};

OpenCloseDevice::~OpenCloseDevice()
{
	if(m_delayed_close_thread.joinable())
	{
		stop_condition.notify_one();
		m_delayed_close_thread.join();
	}
}

void OpenCloseDevice::closeAfterDelay()
{
	for(;;)
	{
		std::unique_lock<std::mutex> lock(stop_condition_mutex);
		if (stop_condition.wait_until(lock, std::chrono::steady_clock::now() + m_device_close_delay / 10) != std::cv_status::timeout)
		{
			return;
		}
        ...
    }
}

Perhaps this / 10 logic can also be removed then, and potentially it could become more explicit what the thread is being notified about (in the PoC notification means "please stop ASAP").

This approach introduces up to 1 sec delay before Blender can be closed, which is undesirable. In a more ideal implementation destructor will notify the thread that it needs to be stopped. One of the ways to do so is to use `conditional_variable`. Some PoC: ``` class AUD_PLUGIN_API OpenCloseDevice : public SoftwareDevice { private: ... std::condition_variable stop_condition; std::mutex stop_condition_mutex; ... }; OpenCloseDevice::~OpenCloseDevice() { if(m_delayed_close_thread.joinable()) { stop_condition.notify_one(); m_delayed_close_thread.join(); } } void OpenCloseDevice::closeAfterDelay() { for(;;) { std::unique_lock<std::mutex> lock(stop_condition_mutex); if (stop_condition.wait_until(lock, std::chrono::steady_clock::now() + m_device_close_delay / 10) != std::cv_status::timeout) { return; } ... } } ``` Perhaps this `/ 10` logic can also be removed then, and potentially it could become more explicit what the thread is being notified about (in the PoC notification means "please stop ASAP").

Richard Antalik commented

2024-05-01 05:41:06 +02:00

@Sergey Nice, did not know that you could timeout wait on condition. More elegant than bool definitely. I did not observe any delays here, but also wasn't looking for them, so there definitely could be.

Richard Antalik added 1 commit 2024-05-01 05:58:05 +02:00

d0c44597b4 Use condition mutex to suspend the thread

Richard Antalik added 1 commit 2024-05-01 06:49:12 +02:00

2e6539caac Misunderstood how lock should have been managed

Sergey Sharybin commented

2024-05-01 09:53:07 +02:00

Seems you've Blender-specific code style to the audaspace code. Please restore the original formatting, as it is something that would need to be done to accept the patch to upstream, and would also make it much easier to see the actual functional functional changes.

Sergey Sharybin commented

2024-05-01 09:54:06 +02:00

Forgot to mention, it is also usually a very good idea to look at your own patches after submossion or update, to verify nothing unforeseen happened, and the patch includes exactly what you intent.

Richard Antalik commented

2024-05-01 09:56:43 +02:00

I did apply formatting on upstream patch. Do you want to merge this patch to our main? If so, I will update the patch, otherwise I can close it.

Sergey Sharybin changed title from ~~Fix 121015: Abnormal termination on exit after playback on MacOS~~ to Fix #121015: Abnormal termination on exit after playback on MacOS

2024-05-01 10:08:54 +02:00

Sergey Sharybin commented

2024-05-01 10:13:03 +02:00

I did apply formatting on upstream patch.

I am not sure what do you mean by that.

The audaspace upstream used tabs for indentation, and { on a new line. I do not see this changed in neither of upstream version of audaspace on Github, nor in the bundled version of it in our blender.git. This patch does replace tabs with spaces, moves { to the same line as class/function, and half-indents visibility modifier. I do not think this is intended, and if it really is, it should not be done as part of a bugfix, and should not be done for a single file.

Do you want to merge this patch to our main? If so, I will update the patch, otherwise I can close it.

If by this patch you mean this specific PR (#121242), then I can not answer you that just yet, as I can not give proper review.

> I did apply formatting on upstream patch. I am not sure what do you mean by that. The audaspace upstream used tabs for indentation, and `{` on a new line. I do not see this changed in neither of upstream version of audaspace on Github, nor in the bundled version of it in our blender.git. This patch does replace tabs with spaces, moves `{` to the same line as class/function, and half-indents visibility modifier. I do not think this is intended, and if it really is, it should not be done as part of a bugfix, and should not be done for a single file. > Do you want to merge this patch to our main? If so, I will update the patch, otherwise I can close it. If by this patch you mean this specific PR (#121242), then I can not answer you that just yet, as I can not give proper review.

Richard Antalik added 2 commits 2024-05-01 10:18:43 +02:00

22c431659d Add comment

82d60a4dd6 Apply formatting

Richard Antalik commented

2024-05-01 10:23:03 +02:00

I did apply formatting on upstream patch.

I am not sure what do you mean by that.

That I have applied formatting in https://github.com/audaspace/audaspace/pull/23

I don't have good workflow when working on Audaspace code in our repository. In this case I copy files to their repo and apply clang format there. I am paranoid, that if I copy their clang format to our repo I will accidentally add it commit and push.

I've heared, that clang format should support specifying path to format file in some future release, perhaps future is now, but did not check.

> > I did apply formatting on upstream patch. > > I am not sure what do you mean by that. That I have applied formatting in https://github.com/audaspace/audaspace/pull/23 I don't have good workflow when working on Audaspace code in our repository. In this case I copy files to their repo and apply clang format there. I am paranoid, that if I copy their clang format to our repo I will accidentally add it commit and push. I've heared, that clang format should support specifying path to format file in some future release, perhaps future is now, but did not check.

Richard Antalik requested review from Sergey Sharybin 2024-05-01 10:23:53 +02:00

Sergey Sharybin commented

2024-05-01 10:44:05 +02:00

You should be able to copy audaspace's .clang-format to our extern/audaspace. This will allow editors to pick it up. For make format it might need to be added explicitly, as I don't think any of the extern libraries are covered there. Still unclear why blender-specific style appeared in a file in extern though.

For the review it is a bit strange to have same code submitted to two repositories. Unless there are bottlenecks in the upstream review we can just move the review there.

You should be able to copy audaspace's `.clang-format` to our `extern/audaspace`. This will allow editors to pick it up. For `make format` it might need to be added explicitly, as I don't think any of the extern libraries are covered there. Still unclear why blender-specific style appeared in a file in `extern` though. For the review it is a bit strange to have same code submitted to two repositories. Unless there are bottlenecks in the upstream review we can just move the review there.

Sergey Sharybin reviewed 2024-05-01 10:46:27 +02:00

extern/audaspace/include/devices/OpenCloseDevice.h Outdated

						
				@ -51,2 +52,4 @@

					bool m_delayed_close_finished{false};

					/**

					 * Thread condition to terminate immediately. Used when class is destroyed.

Sergey Sharybin commented

2024-05-01 10:46:24 +02:00

It does not help that much to document when something is used, as it quite easy to see by checking usage of a variable. What is less obvious to figure out is what something is used for. So here it will be more clear to say Used when class is destroyed to notify thread that it needs to stop as soon as possible.

It does not help that much to document when something is used, as it quite easy to see by checking usage of a variable. What is less obvious to figure out is what something is used for. So here it will be more clear to say `Used when class is destroyed to notify thread that it needs to stop as soon as possible.`

iss marked this conversation as resolved

Richard Antalik commented

2024-05-01 10:53:25 +02:00

For the review it is a bit strange to have same code submitted to two repositories. Unless there are bottlenecks in the upstream review we can just move the review there.

Well it's not high prio issue, but if is too annoying, we may want to fix it in our repo. Ideally I would only make patch for upstream, but not sure when it will be merged.

> For the review it is a bit strange to have same code submitted to two repositories. Unless there are bottlenecks in the upstream review we can just move the review there. Well it's not high prio issue, but if is too annoying, we may want to fix it in our repo. Ideally I would only make patch for upstream, but not sure when it will be merged.

Sergey Sharybin commented

2024-05-01 12:32:46 +02:00

Even though the report is not officially marked as High Priority it is quite a bad one. Seeing a "Blender Crashed" message every time you close Blender is not something we should be allowing for much longer.

Richard Antalik added 1 commit 2024-05-02 12:57:13 +02:00

9e7b8d8f04 Address review

Sergey Sharybin approved these changes 2024-05-02 16:23:28 +02:00

Sergey Sharybin left a comment

Thanks for the updates. Seems to work good!

Sergey Sharybin commented

2024-05-06 15:30:48 +02:00

With the a5ba4032e0 landed I think we can close this. The fixe from Joerg is more complete to keep everything thread-safe.

With the a5ba4032e0 landed I think we can close this. The fixe from Joerg is more complete to keep everything thread-safe.

Sergey Sharybin closed this pull request

2024-05-06 15:30:51 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #121015: Abnormal termination on exit after playback on MacOS #121242

Pull request closed