Skip to content

Commit

Permalink
Appleseed driver : Work around crash in Appleseed 2.1
Browse files Browse the repository at this point in the history
`DisplayTileCallbackFactory::create()` returned the same DisplayTileCallback instance every time, and up until now took responsibility for deleting it. This has always been sketchy because Appleseed holds the returned DisplayTileCallback's with `auto_release_ptr`, so `DisplayTileCallback::release()` must avoid double deletes by just doing nothing. This worked if `DisplayTileCallback::release()` was called before `DisplayTileCallbackFactory::release()`, but Appleseed 2.1 has reversed the call order so that the factory is destroyed before the callback is. This means `DisplayTileCallback::release()` is called on an already-deleted instance, and crashes, with a stack trace like so :

```
#0  0x0000000000000000 in ?? ()
#1  0x00007fffe18153a6 in foundation::auto_release_ptr<renderer::ITileCallback>::reset (this=0x7fff8c012148, ptr=0x0)
   at /disk1/john/dev/gafferDependencies/Appleseed/working/appleseed-2.1.0-beta/src/appleseed/foundation/utility/autoreleaseptr.h:187
ImageEngine#2  0x00007fffe181137f in renderer::(anonymous namespace)::ProgressiveFrameRenderer::~ProgressiveFrameRenderer (this=0x7fff8c0120a0, __in_chrg=<optimized out>)
   at /disk1/john/dev/gafferDependencies/Appleseed/working/appleseed-2.1.0-beta/src/appleseed/renderer/kernel/rendering/progressive/progressiveframerenderer.cpp:178
ImageEngine#3  0x00007fffe1811592 in renderer::(anonymous namespace)::ProgressiveFrameRenderer::~ProgressiveFrameRenderer (this=0x7fff8c0120a0, __in_chrg=<optimized out>)
   at /disk1/john/dev/gafferDependencies/Appleseed/working/appleseed-2.1.0-beta/src/appleseed/renderer/kernel/rendering/progressive/progressiveframerenderer.cpp:187
ImageEngine#4  0x00007fffe18115ca in renderer::(anonymous namespace)::ProgressiveFrameRenderer::release (this=0x7fff8c0120a0)
   at /disk1/john/dev/gafferDependencies/Appleseed/working/appleseed-2.1.0-beta/src/appleseed/renderer/kernel/rendering/progressive/progressiveframerenderer.cpp:191
ImageEngine#5  0x00007fffe1712fb7 in foundation::auto_release_ptr<renderer::IFrameRenderer>::~auto_release_ptr (this=0x7fff8c01f928, __in_chrg=<optimized out>)
   at /disk1/john/dev/gafferDependencies/Appleseed/working/appleseed-2.1.0-beta/src/appleseed/foundation/utility/autoreleaseptr.h:124
ImageEngine#6  0x00007fffe17121be in renderer::RendererComponents::~RendererComponents (this=0x7fff8c01f890, __in_chrg=<optimized out>)
   at /disk1/john/dev/gafferDependencies/Appleseed/working/appleseed-2.1.0-beta/src/appleseed/renderer/kernel/rendering/renderercomponents.h:73
ImageEngine#7  0x00007fffe1712288 in std::default_delete<renderer::RendererComponents>::operator() (this=0x7fff8c000ae0, __ptr=0x7fff8c01f890)
   at /opt/rh/devtoolset-6/root/usr/include/c++/6.3.1/bits/unique_ptr.h:76
ImageEngine#8  0x00007fffe1711e0d in std::unique_ptr<renderer::RendererComponents, std::default_delete<renderer::RendererComponents> >::reset (this=0x7fff8c000ae0,
   __p=0x7fff8c01f890) at /opt/rh/devtoolset-6/root/usr/include/c++/6.3.1/bits/unique_ptr.h:347
ImageEngine#9  0x00007fffe1710632 in renderer::CPURenderDevice::~CPURenderDevice (this=0x7fff8c0009b0, __in_chrg=<optimized out>)
   at /disk1/john/dev/gafferDependencies/Appleseed/working/appleseed-2.1.0-beta/src/appleseed/renderer/device/cpu/cpurenderdevice.cpp:112
ImageEngine#10 0x00007fffe1710988 in renderer::CPURenderDevice::~CPURenderDevice (this=0x7fff8c0009b0, __in_chrg=<optimized out>)
   at /disk1/john/dev/gafferDependencies/Appleseed/working/appleseed-2.1.0-beta/src/appleseed/renderer/device/cpu/cpurenderdevice.cpp:129
ImageEngine#11 0x00007fffe181f21c in std::default_delete<renderer::IRenderDevice>::operator() (this=0x3093868, __ptr=0x7fff8c0009b0)
   at /opt/rh/devtoolset-6/root/usr/include/c++/6.3.1/bits/unique_ptr.h:76
ImageEngine#12 0x00007fffe181eafd in std::unique_ptr<renderer::IRenderDevice, std::default_delete<renderer::IRenderDevice> >::~unique_ptr (this=0x3093868,
   __in_chrg=<optimized out>) at /opt/rh/devtoolset-6/root/usr/include/c++/6.3.1/bits/unique_ptr.h:239
ImageEngine#13 0x00007fffe181d545 in renderer::MasterRenderer::Impl::~Impl (this=0x3093820, __in_chrg=<optimized out>)
   at /disk1/john/dev/gafferDependencies/Appleseed/working/appleseed-2.1.0-beta/src/appleseed/renderer/kernel/rendering/masterrenderer.cpp:168
ImageEngine#14 0x00007fffe181cfdb in renderer::MasterRenderer::~MasterRenderer (this=0x3088d70, __in_chrg=<optimized out>)
   at /disk1/john/dev/gafferDependencies/Appleseed/working/appleseed-2.1.0-beta/src/appleseed/renderer/kernel/rendering/masterrenderer.cpp:584
ImageEngine#15 0x00007fffc67d0ae0 in (anonymous namespace)::AppleseedRenderer::~AppleseedRenderer() () from /disk1/john/dev/build/gaffer/lib/libGafferAppleseed.so
ImageEngine#16 0x00007fffd8a734b6 in GafferScene::InteractiveRender::stop() () from /disk1/john/dev/build/gaffer/lib/libGafferScene.so
ImageEngine#17 0x00007fffd8a735bf in GafferScene::InteractiveRender::update() () from /disk1/john/dev/build/gaffer/lib/libGafferScene.so
```

The solution here is to return a new DisplayTileCallback every time `DisplayTileCallbackFactory::create()` is called, and to destroy those the regular way in `DisplayTileCallback::release()`. This works fine for interactive renders, because Appleseed will only ever make a single DisplayTileCallback at a time. For batch renders it is completely broken though, because Appleseed will call `DisplayTileCallbackFactory::create()` once per thread, and every thread will end up writing to a different display driver. So far we've only used the display driver for interactive renders though, so maybe this is OK.
  • Loading branch information
johnhaddon committed Dec 20, 2019
1 parent 5c1b06a commit 305d142
Showing 1 changed file with 4 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
#include "renderer/api/log.h"
#include "renderer/api/rendering.h"
#include "renderer/api/utility.h"
#include "renderer/api/version.h"

#include <vector>

Expand Down Expand Up @@ -272,8 +271,7 @@ class DisplayTileCallback : public ProgressTileCallback

void release() override
{
// We don't need to do anything here.
// The tile callback factory deletes this instance.
delete this;
}

void on_tile_begin(const asr::Frame *frame, const size_t tileX, const size_t tileY) override
Expand Down Expand Up @@ -359,13 +357,12 @@ class DisplayTileCallbackFactory : public asr::ITileCallbackFactory
public:

explicit DisplayTileCallbackFactory( const asr::ParamArray &params )
: m_params( params )
{
m_callback = new DisplayTileCallback( params );
}

~DisplayTileCallbackFactory() override
{
delete m_callback;
}

// Delete this instance.
Expand All @@ -377,12 +374,12 @@ class DisplayTileCallbackFactory : public asr::ITileCallbackFactory
// Return a new tile callback instance.
asr::ITileCallback *create() override
{
return m_callback;
return new DisplayTileCallback( m_params );
}

private:

DisplayTileCallback *m_callback;
const asr::ParamArray m_params;

};

Expand Down

0 comments on commit 305d142

Please sign in to comment.