dsp::ProcessContextReplacing takes non-const dsp::AudioBlock& – design flaw or intentional?

General JUCE discussion
1

If I get it right, the const-ness of the samples hold by a dsp::AudioBlock is denoted by declaring the sample type const, e.g. dsp::AudioBlock<const float> but not by declaring the block itself const.

This makes it easy to write interfaces like void myDspFunction (const dsp::AudioBlock<float>& b) and directly pass an AudioBuffer<float> to it, making use of the implicit conversion constructor of the dsp::AudioBlock.

Now the dsp::ProcessContextReplacing constructor takes a non-const reference to an audio block, which makes it impossible to pass an AudioBuffer to it directly, instead you have to create a temporary audio block on the stack and pass that to the context, which makes some short processing routines unnecessarily convoluted.

I wonder if this is an implementation flaw that could be changed (and if so would kindly ask the team to do so) or if I’m overlooking something here?

2

Ran into this today. Not a huge deal, but I am also wondering if this is intentional, and if so, what is the reason?

3

It’s a process contact REPLACING, it can’t really be const, can it?

4

I believe this is intentional - since the docs for dsp::ProcessContextReplacing read:

This context is intended for use in situations where a single block is being used for both the input and output

Hence that block cannot be const, since it is intended to be modified.

PS: See also dsp::ProcessContextNonReplacing if you want to pass a const AudioBlock.

5

As I said above, the convention used in other places of the dsp classes to mark a block as constant is to declare the templated sample type const – not the block as a wrapper class itself. A const block will always point to the same sample buffer. Therefore, functions like dsp::AudioBuffer::swap are non-const. However, if samples can be written or just read is marked by the const-ness of the template type. This means, a const dsp::AudioBlock<float> wraps a buffer of writable samples, a const dsp::AudioBlock<const float> wraps a buffer of read only samples. All the member functions that can alter the sample values are actually declared const, just have a look at the documentation:

image
image1638×548 96 KB

In contrast to that, a non const dsp::AudioBlock<float> allows you to swap the underlying buffer inside a replacing context – this cannot be correct. I have to admit that it felt unintuitive to me at first, but after thinking it through, this is a great and clever design choice from the JUCE team.

As I said, changing the argument to become a reference to a const block would allow to use the implicit conversion operator taking an AudioBuffer – which would make totally sense and would make the code clean and straightforward.

So, I still think that this is more a flaw or even bug in the API than a design choice by purpose.

2 Likes
6

Ok, I generally get what you mean, though I’d have to sit down a minute with the JUCE code before I fully see what the effect would be of the change you are proposing. It feels easily confusable, with the layers of templating/abstraction involved.

This is what I wonder about. Maybe that was correct and intentional. Since an AudioBlock is intentionally an abstraction of audio data (from many sources, not just from AudioBuffers), perhaps the intent was to allow swapping around the underlying data. After all, if they wanted the dsp replacing context to be bound to a single AudioBuffer, then dsp could have be written based around AudioBuffers instead of AudioBlocks. But they didn’t, they used the more abstracted AudioBlocks, and forcing those to be const would in theory restrict some of their possible uses.

Sorry if I’m not being terribly clear here - again, I’ll have to sit down with the actual code a bit more to fully see what you’re talking about.

7

Friendly bump. Was hit by this again today while implementing some new dsp classes. This time the piece of code I was about to implement looked somewhat like this:

void process (const juce::dsp::ProcessContextReplacing<float>& context)
{
    // Limit processing block size 
    if (context.getOutputBlock().getNumSamples() > someLimit)
    {
        process (context.getOutputBlock().getSubBlock (0, someLimit));
        process (context.getOutputBlock().getSubBlock (someLimit));

        return;
    }
    
    // all further processing...
}

To make this compile, I needed to store those temporarily blocks into an intermediate variable first. And of course, introducing this two variables this is really no big issue in this particular case. But to me it feels a bit inconsistent.

So still, I’d love to get some feedback from the team regarding this AudioBlock constness thing, or do you agree that this could/should be changed?

1 Like
8

what about simply passing it by value, as it is done in DryWetMixer?

void pushDrySamples (const AudioBlock<const SampleType> drySamples);

void mixWetSamples (AudioBlock<SampleType> wetSamples);
9

bump. Any reason ProcessContectNonReplacing doesn’t take a const AudioBlockType& ?
like

ProcessContextNonReplacing (const ConstAudioBlockType& input, const AudioBlockType& output)
{
...
}
10

Using a const& would allow users to pass temporaries to the constructor. ProcessContextNonReplacing holds a reference to the outputBlock, which will dangle if the user passes a temporary.

Passing a const& and also storing the outputBlock by value might be viable - but then, getOutputBlock would have to be non-const, or return a const& too. Any changes here are likely to have a ‘ripple effect’, and it would take some detailed investigation to get it right, while minimising source incompatibilities.

2 Likes
11

I’ve run into this issue as well. As per the c++ core guidelines, in our codebase I’m trying to stick to passing const references to objects by default, in order that interfaces are explicit about when objects should and shouldn’t be modified. e.g.:

// Cannot reassign block, nor modify the audio data it points to
void a(const AudioBlock<const float>& block);

// Cannot reassign block, but may modify the audio data it points to
void b(const AudioBlock<float>& block);

// Can reassign block, but the audio data it points to is immutable
void c(AudioBlock<const float>& block);

// Can reassign block, and the audio data it points to is mutable
void d(AudioBlock<float>& block);

Unfortunately we often see interfaces which look like c or d, when what we actually want to enforce is a or b. This permits the following kind of error which could otherwise be prevented by the compiler:

void pathological(AudioBlock<float>& block)
{
   // I just love creating audio glitches, mwahahaha!
   block = block.getSubBlock(0, block.getNumSamples() / 2);
}

void myProcessFunction(AudioBlock<float>& block)
{
   pathological(block);
   // Was I expecting that function call to have altered the
   // length of block? Was the caller? Probably not!
}

Frustratingly, the fact that ProcessContextReplacing takes a non-const AudioBlock& forces us either to use less safe interfaces of the form c and d, or do some hacky const_cast’ing to work around this.

IMHO I agree with others here in that the better thing would be for ProcessContextReplacing to take and return a const AudioBlock& for the output block. This would explicitly prevent nonsense like this:

void pathological2(ProcessContextReplacing<float> context)
{
   context.getOutputBlock() = someOtherAudioBlock;
}

void myFunction()
{
   AudioBuffer<float> myBuffer{ 2, 64 };
   AudioBlock<float> myBlock( myBuffer };
   
   pathological2( { myBlock } );

   // Oh dear, myBlock may not point at myBuffer anymore!
}

Using a const& would allow users to pass temporaries to the constructor. ProcessContextNonReplacing holds a reference to the outputBlock, which will dangle if the user passes a temporary.

If I’m not mistaken, this could also be solved by declaring a deleted rvalue reference constructor:

ProcessContextReplacing(const AudioBlock<T>&);
ProcessContextReplacing(AudioBlock<T>&&) = delete; // prevent construction from a temporary, which would result in a dangling reference

I do appreciate the point about rippling/breaking changes though.

1 Like