AudioFormatWriter::createWriterFor: why the optional deletion?


#1
        @param streamToWriteTo      the stream that the data will go to - this will be
                                    deleted by the AudioFormatWriter object when it's no longer
                                    needed. If no AudioFormatWriter can be created by this method,
                                    the stream will NOT be deleted, so that the caller can re-use it

Why was this design chosen? Why not just specify that the caller should delete the passed-in stream?

I might be wrong, but I believe that would allow us to use the FileOutputStream(const File& f, size_t buffer) constructor as a stack variable:

        audioFile.deleteFile();
        audioFile.create();
        if( audioFile.existsAsFile() )
        {
            FileOutputStream fos(audioFile);
            if( fos.openedOk() )
            {
                AiffAudioFormat aiff;
                ScopedPointer<AudioFormatWriter> writer(aiff.createWriterFor(&fos,
                                                                             44100,
                                                                             1,
                                                                             16,
                                                                             {},
                                                                             0));

that would give us very obvious variable construction/destruction safety with no dangling pointers. This example from AudioRecordingDemo.h is just confusing without reading the docs for AudioFormatWriter:

    void startRecording (const File& file)
    {
        stop();

        if (sampleRate > 0)
        {
            // Create an OutputStream to write to our destination file...
            file.deleteFile();
            ScopedPointer<FileOutputStream> fileStream (file.createOutputStream());

            if (fileStream != nullptr)
            {
                // Now create a WAV writer object that writes to our output stream...
                WavAudioFormat wavFormat;
                AudioFormatWriter* writer = wavFormat.createWriterFor (fileStream, sampleRate, 1, 16, StringPairArray(), 0);

                if (writer != nullptr)
                {
                    fileStream.release(); // (passes responsibility for deleting the stream to the writer object that is now using it)

That last line just seems to be at odds with the JUCE style of programming safe object management techniques…

a simple change here would clear up this confusion

AudioFormatWriter::~AudioFormatWriter()
{
    //delete output;  
    //ahhh AudioFormatWriter is no longer responsible for the OuputStream :-)
}

Just digging a bit further:

    //==============================================================================
    /** Creates an AudioFormatWriter object.

        @param destStream       the stream to write to - this will be deleted
                                by this object when it is no longer needed
    */
    AudioFormatWriter (OutputStream* destStream,

Here lies the rub. It’s another one of those spots in the framework where ownership of the passed-in value is not defined by the constructor argument. I guess this is a spot where you guys are desiring to move to std::unique_ptr<OutputStream> destStream as the function argument, so the indication of ownership is very clear? Similar to the OwnedArray class’s add() function?

Not only that, but the AudioFormatWriter constructor explicitly states it will take ownership, but the AudioFormat::createWriterFor() member function (which makes AudioFormatWriters) says ownership might not happen if it can’t allocate one.

so, this function could be changed to indicate that ownership will be passed to the AudioFormat class:

 AudioFormatWriter* AiffAudioFormat::createWriterFor (std::unique_ptr<OutputStream> out, //OutputStream* out,
                                                     double sampleRate,
                                                     unsigned int numberOfChannels,
                                                     int bitsPerSample,
                                                     const StringPairArray& metadataValues,
                                                     int /*qualityOptionIndex*/)
{
    if (out != nullptr && getPossibleBitDepths().contains (bitsPerSample))
        return new AiffAudioFormatWriter (out.release(), sampleRate, numberOfChannels,
                                          (unsigned int) bitsPerSample, metadataValues);

    return nullptr;
} //out is safely deleted here if it holds anything still.

@jules @fabian @ed95