Confusion with unique_ptr and saving audio settings

I have old code (probably 10 years or so) that has worked well. But I recently updated juce from about 2016 to the latest and had to make changes due to unique_ptr.

Here is the original code in the destructor of my settings windows:

SettingsWindow::~SettingsWindow()
{
	XmlElement *xml = MainContentComponent::getSharedAudioDeviceManager(device).createStateXml();
	if (xml == nullptr) return;
	String xxxx("");
	if (device > 0) xxxx = String(device);
	File outputFile(g_cCurrentPath + "/AudioDevice" + xxxx + ".xml");
	ScopedPointer<FileOutputStream> stream(outputFile.createOutputStream());
	Thread::sleep(250);
	if (stream != NULL)
	{
		stream->setPosition(0);
		stream->truncate();
		xml->writeToStream(*stream, juce::String::empty, false, true);
	}
	delete xml;
}

I changed it to this:

SettingsWindow::~SettingsWindow()
{
	XmlElement *xml = MainContentComponent::getSharedAudioDeviceManager(device).createStateXml().get();
	if (xml == nullptr) return;
	String xxxx("");
	if (device > 0) xxxx = String(device);
	File outputFile(g_cCurrentPath + "/AudioDevice" + xxxx + ".xml");
	FileOutputStream stream(outputFile);
	Thread::sleep(250);
	if (stream.openOk())
	{
		stream.setPosition(0);
		stream.truncate();
		xml->writeTo(outputFile);
	}
}

No other changes needed to be made in the SettingsWindow to compile.

This code crashed on the writeTo line while empy string is being tested. The output says this->data was 0xFFFFFFFFFFF… The debugger the pointer to the data is 0xdddddd… common for uninitialized data in debug windows code. It seems to me like the createStateXml().get() returns a pointer and then is deleted, but I think that should happen at the ending brace when it goes out of scope. (correct me if I am wrong)

I am new to this unique_ptr concept. Can anyone see why this does not work?

Sometimes it pay to walk away, after about 10 minutes, VS underled the createStateXml line in green with a warning:
C2681:The pointer is dangling because it points at a temporary instance that was destroyed. If that is the case what use is the get function

So I modified the code again. I commented out all the xml references, and changed the xml->writeto line like this:

`MainContentComponent::getSharedAudioDeviceManager(device).createStateXml().get()->writeTo(outputFile);`

Now it works.

I spoke too soon. It no longer crashes. Now the file gets written but the file is empty. So for some reason the createStateXml is not working. So I cannot save the device settings. What am I missing?

You could try :

auto xml = MainContentComponent::getSharedAudioDeviceManager(device).createStateXml();

Note the missing .get() in that.

I’ll try it. But then wont I have to put the .get() when I use xml later? Then wouldn’t that give me the same result?

As the message tells you, you’re creating a temporary unique_ptr object (by returning it from a function) which you don’t bind to any variable, so it’s destroyed just after it’s created. When you call writeTo(), the XmlElement object no longer exists. You need to keep the unique_ptr object alive to then be able to access the actual pointer inside of it, which is what happens when you use auto xml = ..., where auto is resolved to unique_ptr<XmlElement>. In the second case, the immediate call to get() makes the destruction be postponed, but then writeTo() is not called on the unique_ptr but on its XmlElement* member, so the unique_ptr is destroyed before. You don’t need get() to call writeTo(): just call operator-> on the unique_ptr itself. You only need get() to pass around the pointer without transferring its ownership. Just in case, see RAII.

Ok, that makes sense. I tried it and I get the same result. It compiles and does not crash, but I get a blank file written. I did a breakpoint on the writeTo line and inspected the xml object. and it is empty. So my earlier asumption that the createStateXml is not working still seems to be the issue. I can change my setting and they work so I know the audiodeviceselector is working.

Would you mind to share your entire class in current state?
I know you already said it seems to be createStateXml but just in case you don‘t want to trust the debugger and need some more ideas:

  • Are you sure the destructor is a good/correct place to do the saving?
  • Does the file get overwritten when you write to it? I got bitten by this more than once so deleting that file first might be a good step non the less.

The destructor worked before the update. I set a breakpoint in the destructor of the settingswindow class and it is executed before the settingspanel class destructor so I think the answer is yes.

Yes the file is overwritten.

I looked closer at xml with the debugger and at the time of writeto there is some data in the attributes link list. (I did not see this before)

The class is:

#include "MainComponent.h"
#include "SettingsPanel.h"
#include "SettingsWindow.h"

extern int DefaultBackGroundColor;
extern String g_cCurrentPath;

static char RCSid[] = "@(#) $Source: C:/CVS/Exostat/Mixer-2/Source/SettingsWindow.cpp,v $ $Revision: 1.2 $ $Date: 2017/06/17 23:07:58 $";
/* REFERENCED */
static char *RCSid_ptr = RCSid;


SettingsWindow::SettingsWindow (int dev)
: DialogWindow (TRANS("Settings"),
                Colour (DefaultBackGroundColor),
                true,
                false) // do not add to desktop yet
{
  SettingsPanel* contentComp = new SettingsPanel(dev);
  device = dev;
  setOpaque (true);
  //setDropShadowEnabled (false);

  // iOS doesn't have a native title bar
  //setUsingNativeTitleBar (true);

  // must happen AFTER setUsingNativeTitleBar()
  Component::addToDesktop (getDesktopWindowStyleFlags());

  // must happen after addToDesktop()
  setContentOwned (contentComp, true);

  centreWithSize (getWidth(), getHeight());
  setVisible (true);

  enterModalState ();
}

SettingsWindow::~SettingsWindow()
{
	auto xml = MainContentComponent::getSharedAudioDeviceManager(device).createStateXml();
	if (xml == nullptr) return;
	String xxxx("");
	if (device > 0) xxxx = String(device);
	File outputFile(g_cCurrentPath + "/AudioDevice" + xxxx + ".xml");
	FileOutputStream stream(outputFile);
	Thread::sleep(250);
	if (stream.openedOk())
	{
		stream.setPosition(0);
		stream.truncate();
		//xml->writeToStream(stream, juce::String(), false, true);
		xml->writeTo(outputFile);
		//xml->writeTo(outputFile);
	}
	//delete xml;
}

void SettingsWindow::closeButtonPressed()
{
	delete(this);
}

I tried a different way. This code came from some example code many years ago so I wasnt clear on why it was coded this way. So I removed the stream code and now it works.

SettingsWindow::~SettingsWindow()
{
	auto xml = MainContentComponent::getSharedAudioDeviceManager(device).createStateXml();
	if (xml == nullptr) return;
	String xxxx("");
	if (device > 0) xxxx = String(device);
	File outputFile(g_cCurrentPath + "/AudioDevice" + xxxx + ".xml");
	Thread::sleep(250);

	xml->writeTo(outputFile);
}

The problem with this code is there is no check if the file opened or write ok cause writeTo returns null.

So how should I properly check for errors in this code?

Hey, the XmlElement::writeTo that takes a File rather than a OutputStream actually does return a bool depending on the succes. :slight_smile:

https://docs.juce.com/develop/classXmlElement.html#af6065b31b935b297e5084773d31ca833

Thank for pointing that out.

I have one other piece of code that is driving me nuts. Again it is due to the change from ScopedPointer to std::unique_ptr. First I have this line:

std::unique_ptr<PluginDescription> *d (knownPluginList.getTypeForIdentifierString(name));

then

std::unique_ptr<AudioPluginInstance> instance (formatManager.createPluginInstance (*d.get(), g_SampleRate, g_BufferSize, errorMessage);

The instance is always null. The debugger shows that d from the first line is good at the time I call createPlugingInstance. I stepped into the createPluginInstance and the argument is empty. However the debugger still shows it good on return. So clearly something is wrong with the way I am passing it. I have tried several ways but I am at a loss as to what to do.

The argument passed to createPluginInstance is supposed to be of type juce::PluginDescription.


AudioPluginInstance* AudioPluginFormatManager::createPluginInstance (const PluginDescription& description, double rate,
                                                                     int blockSize, String& errorMessage) const

So how do I pass std::unique_ptr *d to a function expecting const PluginDescription& description???

You should store the std::unique_ptr<PluginDescription> itself rather than a pointer to it (which should almost never be needed). You also don’t need to use .get() to dereference a std::unique_ptr. Something like this should work:

std::unique_ptr<PluginDescription> d = knownPluginList.getTypeForIdentifierString (name);
std::unique_ptr<AudioPluginInstance> instance = formatManager.createPluginInstance (*d, g_SampleRate, g_BufferSize, errorMessage);

The solved the problem basically. However, it is going out of scope before I am done with it. This is a VST effect that needs to be available long term. After the code from above is complete, the function ends like this:

return new VstPlugin(instance.get(),...);

VstPlugin is defined:

`VstPlugin::VstPlugin(AudioPlugin(AudioPluginInstance * instance..)
{
`     m_instance = instance;

This appears to work. But then when I later try to open the dialog for the vst plugin, My code does call to a function called doDialog(); Inside doDialog is the following:

PluginWindow* PluginWindow::getWindowFor (AudioPluginInstance* node,
                                          bool useGenericView, int mix, int aux, int idx)
{
    AudioProcessorEditor* ui = nullptr;

    if (! useGenericView)
    {
        ui = node->createEditorIfNeeded();

        if (ui == nullptr)
            useGenericView = true;
    }

This code worked prior to the juce update. But now the object is going out of scope and I loose it and crashes on the createEditorIfNeeded(); because node points to nothing (m_instance was passed in as node).

Originally the createPluginstance returned a pointer. Now it returns a unique_ptr and is deleted when the function goes out of scope.

I think I need to pass ownership of instance to the VstPlugin to be saved in m_instance which would then be deleted when the class VstPlugin is deleted.

How do I do this?

You can use .release() to pass the ownership to VstPlugin:

return new VstPlugin(instance.release(),...);

I assume since it is not passed in as a unique_ptr I need to delete it when the class is destroyed?

That’s correct, the VstPlugin class would then be responsible for deleting the instance whenever it’s appropriate since VstPlugin::m_instance is a raw pointer.

That solved it. Thanks.