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.

Sign up for GitHub

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

Better protect this->dataPtr->initialized with renderMutex. #1119

Merged
merged 2 commits into from
Oct 19, 2021
Merged

Better protect this->dataPtr->initialized with renderMutex. #1119

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0048276
Better protect this->dataPtr->initialized with renderMutex.
Oct 18, 2021
bf139a8
Merge branch 'ign-gazebo4' into lock
iche033 Oct 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions src/systems/sensors/Sensors.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ void Sensors::Update(const UpdateInfo &_info,
EntityComponentManager &_ecm)
{
IGN_PROFILE("Sensors::Update");
std::unique_lock<std::mutex> lock(this->dataPtr->renderMutex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is merged, but I just noticed this. If I understand correctly, having this mutex lock here could prevent sensors from updating in parallel with physics. This would happen if the Sensors system comes before the Physics system in the SDFormat file. Here's the sequence of events that could happen:

  1. In the Sensors::PostUpdate, a new render is triggered that takes a long time to complete. The render thread locks renderMutex and starts the render.
  2. In the next phase, Sensors::Update tries to acquire renderMutex and blocks because the render is not complete. This also blocks Physics::Update from being called because it comes after Sensors::Update.

Can some one verify this?

if (this->dataPtr->running && this->dataPtr->initialized)
{
this->dataPtr->renderUtil.UpdateECM(_info, _ecm);
Expand All @@ -437,17 +438,19 @@ void Sensors::PostUpdate(const UpdateInfo &_info,
<< "s]. System may not work properly." << std::endl;
}

if (!this->dataPtr->initialized &&
(_ecm.HasComponentType(components::Camera::typeId) ||
_ecm.HasComponentType(components::DepthCamera::typeId) ||
_ecm.HasComponentType(components::GpuLidar::typeId) ||
_ecm.HasComponentType(components::RgbdCamera::typeId) ||
_ecm.HasComponentType(components::ThermalCamera::typeId)))

{
igndbg << "Initialization needed" << std::endl;
std::unique_lock<std::mutex> lock(this->dataPtr->renderMutex);
this->dataPtr->doInit = true;
this->dataPtr->renderCv.notify_one();
if (!this->dataPtr->initialized &&
(_ecm.HasComponentType(components::Camera::typeId) ||
_ecm.HasComponentType(components::DepthCamera::typeId) ||
_ecm.HasComponentType(components::GpuLidar::typeId) ||
_ecm.HasComponentType(components::RgbdCamera::typeId) ||
_ecm.HasComponentType(components::ThermalCamera::typeId))) {
igndbg << "Initialization needed" << std::endl;
this->dataPtr->doInit = true;
this->dataPtr->renderCv.notify_one();
}
}

if (this->dataPtr->running && this->dataPtr->initialized)
Expand Down