AudioFormatWriter::createWriterFor: why the optional deletion?

General JUCE discussion
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

3 Likes
2

Bump!
This bugs me too, I have to take the return of file.createOutputStream() and release it! Then, create a condition for its (manual) deletion.
It makes me squirm that I have to treat a unique_ptr this way, why even bother returning a unique_ptr if you’re going to have to do these gymnastics around it? ¯\_(ツ)_/¯

3

This is in analogy to createReaderFor. This is used to probe a file and move to the next format, if opening fails.

However, when creating a writer, I wonder what would be a situation where creating a writer fails?
And what would the user expect to happen in that case?

Changing the behaviour is not an option though, because all programs would fail at runtime.
It would require a different method that cannot accidently be used by the compiler, so the user can adapt to that new behaviour.

I think it would be worthwhile and deprecating this method in turn.

EDIT: And while we are on it, returning an unique_ptr would be a welcome improvement :wink:
Maybe an overload that takes an std::unique_ptr<OutputStream> and returns a std::unique_ptr<AudioFormatWriter> would be an option

1 Like
4

If you’re passing around input streams, or using memoryInputStream there’s always a chance that can fail, and to ensure that if one of those streams has been consumed by something else it will fail if the input stream was deleted.

5

However, the client would check for a valid stream anyway. That is a prerequisite to call that function, nothing that would fail inside the creation of a writer.

6

Just looking at the wav::CreateWriterFor you can see plenty of possible reasons, incorrect bit depths, unsupported number of channels, input stream not being available (ie a file input stream not being created / out of disk space). There’s good reason to keep it as it is.

Although I do agree it should return a unique_ptr ;p

7

Yeah, it was a two part rhetorical question: what should happen if it fails? The workflow of the reader to just probe another format doesn’t really apply. That’s why I said (or implied) it should just return a nullptr and still consume the input stream.