Skip to content

Commit

Permalink
Force serialization of render commands
Browse files Browse the repository at this point in the history
This avoids us to break ign-rendering ABI while also simplifying the
amount of work to be done

Serializing work is easier to maintain and debug
Only CPU-bound scenario would potentially benefit from parallel command
generation (in terms of UI responsiveness)
Parallel command generation can be added back later

Also fixed coding style

Refer to
gazebosim/gz-rendering#304 (comment)
for discussion

Affects ign-rendering#304

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
  • Loading branch information
darksylinc committed Apr 28, 2021
1 parent 0e5baa1 commit 4ea254b
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 11 deletions.
34 changes: 26 additions & 8 deletions src/gui/plugins/scene3d/Scene3D.cc
Original file line number Diff line number Diff line change
Expand Up @@ -449,23 +449,23 @@ using namespace gazebo;
QList<QThread *> RenderWindowItemPrivate::threads;

/////////////////////////////////////////////////
void RenderSync::requestQtThreadToBlock(std::unique_lock<std::mutex> &lock)
void RenderSync::RequestQtThreadToBlock(std::unique_lock<std::mutex> &lock)
{
this->renderStallState = RenderStallState::WorkerThreadRequested;
this->cv.wait(lock, [this]
{ return this->renderStallState == RenderStallState::QtThreadBlocked; });
}

/////////////////////////////////////////////////
void RenderSync::releaseQtThreadFromBlock(std::unique_lock<std::mutex> &lock)
void RenderSync::ReleaseQtThreadFromBlock(std::unique_lock<std::mutex> &lock)
{
this->renderStallState = RenderStallState::Unblocked;
lock.unlock();
this->cv.notify_one();
}

/////////////////////////////////////////////////
void RenderSync::waitForWorkerThread()
void RenderSync::WaitForWorkerThread()
{
{
std::unique_lock<std::mutex> lock(this->mutex);
Expand Down Expand Up @@ -526,10 +526,20 @@ void IgnRenderer::Render(RenderSync *renderSync)

IGN_PROFILE_THREAD_NAME("RenderThread");
IGN_PROFILE("IgnRenderer::Render");

std::unique_lock<std::mutex> lock(renderSync->mutex);
renderSync->RequestQtThreadToBlock(lock);

if (this->textureDirty)
{
std::unique_lock<std::mutex> lock(renderSync->mutex);
renderSync->requestQtThreadToBlock(lock);
// TODO(anyone) If SwapFromThread gets implemented,
// then we only need to lock when texture is dirty
// (but we still need to lock the whole routine if
// debugging from RenderDoc or if user is not willing
// to sacrifice VRAM)
//
// std::unique_lock<std::mutex> lock(renderSync->mutex);
// renderSync->RequestQtThreadToBlock(lock);
this->dataPtr->camera->SetImageWidth(this->textureSize.width());
this->dataPtr->camera->SetImageHeight(this->textureSize.height());
this->dataPtr->camera->SetAspectRatio(this->textureSize.width() /
Expand All @@ -540,7 +550,9 @@ void IgnRenderer::Render(RenderSync *renderSync)
this->dataPtr->camera->PreRender();
}
this->textureDirty = false;
renderSync->releaseQtThreadFromBlock(lock);

// TODO(anyone) See SwapFromThread comments
// renderSync->ReleaseQtThreadFromBlock(lock);
}

// texture id could change so get the value in every render update
Expand Down Expand Up @@ -899,7 +911,13 @@ void IgnRenderer::Render(RenderSync *renderSync)
// this notifes ECM to continue updating the scene
g_renderCv.notify_one();

this->dataPtr->camera->SwapFromThread();
// TODO(anyone) implement a SwapFromThread for parallel command generation
// See https://github.com/ignitionrobotics/ign-rendering/issues/304
// if( bForcedSerialization )
// this->dataPtr->camera->SwapFromThread();
// else
// renderSync->ReleaseQtThreadFromBlock(lock);
renderSync->ReleaseQtThreadFromBlock(lock);
}

/////////////////////////////////////////////////
Expand Down Expand Up @@ -2312,7 +2330,7 @@ void TextureNode::NewTexture(uint _id, const QSize &_size)
/////////////////////////////////////////////////
void TextureNode::PrepareNode()
{
renderSync.waitForWorkerThread();
renderSync.WaitForWorkerThread();
this->mutex.lock();
uint newId = this->id;
QSize sz = this->size;
Expand Down
6 changes: 3 additions & 3 deletions src/gui/plugins/scene3d/Scene3D.hh
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,12 @@ inline namespace IGNITION_GAZEBO_VERSION_NAMESPACE {

/// \brief Must be called from worker thread when we want to block
/// \param[in] lock Acquired lock. Must be based on this->mutex
public: void requestQtThreadToBlock(std::unique_lock<std::mutex> &lock);
public: void RequestQtThreadToBlock(std::unique_lock<std::mutex> &lock);
/// \brief Must be called from worker thread when we are done
/// \param[in] lock Acquired lock. Must be based on this->mutex
public: void releaseQtThreadFromBlock(std::unique_lock<std::mutex> &lock);
public: void ReleaseQtThreadFromBlock(std::unique_lock<std::mutex> &lock);
/// \brief Must be called from Qt thread periodically
public: void waitForWorkerThread();
public: void WaitForWorkerThread();
};

/// \brief Ign-rendering renderer.
Expand Down

0 comments on commit 4ea254b

Please sign in to comment.