Inconsistency/confusion about when setNotifyAudioChange() is needed

[LinusHeu]

Hi,
Is there a reason why VolumeStream is the only stream that automatically sets p_notify in its constructors?

arduino-audio-tools/src/AudioTools/VolumeStream.h

Lines 54 to 59 in af3e2c5

	/// Constructor which assigns Stream input or output
	VolumeStream(AudioStream &io) {
	Stream *p_stream = &io;
	setStream(*p_stream);
	p_notify = (AudioInfoSupport *)&io;
	}

For example, for EncodedAudioStream you have to manually call AudioStream::setNotifyAudioChange(), as p_notify is not set in its constructors:

arduino-audio-tools/src/AudioCodecs/AudioEncoded.h

Lines 421 to 425 in af3e2c5

	EncodedAudioStream(AudioStream *ioStream, AudioDecoder *decoder)
	: EncodedAudioOutput(ioStream, decoder) {
	// the indicated stream can be used as input
	setStream(ioStream);
	}

And VolumeStream does NOT automatically change p_notify when you change its output using setTarget():

arduino-audio-tools/src/AudioTools/VolumeStream.h

Lines 67 to 70 in af3e2c5

	void setTarget(Stream &in){
	p_in = ∈
	p_out = p_in;
	}

Is there a reason for this?
Imho it seems kinda inconsistent to me. And I think it would be convenient if every AudioStream would automatically notify the next AudioStream, like VolumeStream does. Is there a reason why that might not be wanted?

And another question:
In the example in the wiki https://github.com/pschatzmann/arduino-audio-tools/wiki/Volume-Control it looks to me like the VolumeStream will not get notified should the AudioConfig change.
Instead of:

  // setup I2S based on sampling rate provided by decoder
  dec.setNotifyAudioChange(i2s);
  dec.begin();

  // set initial volume
  volume.begin(config); // we need to provide the bits_per_sample and channels
  volume.setVolume(0.3);

I think that this would be better, because then both I2S and VolumeStream would get notified about any changes:

  // setup VolumeStream based on sampling rate provided by decoder
  dec.setNotifyAudioChange(volume);
  dec.begin();
  //VolumeStream will automatically notify I2S about any changes to the AudioConfig

Or is that not needed because the VolumeStream doesn't care about sampling rate and in this example the bits per sample and channels will never change?

[pschatzmann]

Actually as far as I can see the API seems to be quite consistent for both the VolumeStream and EncodedAudioStream. If you give an AudioStream or AudioPrint in the constructor it gets automatically notified and you do not need to call setNotifyAudioChange().

I have corrected the example by removing the setNotifyAudioChange.

ps. I rarely had the case when the sampling rate or the channels or the bits per sample were changing and it is good practice to set things up to the correct expected values.

[LinusHeu]

then why is this line in the tutorial needed?
dec.setNotifyAudioChange(i2s);

[LinusHeu]

Ah, I see...

Code flow, that I totally missed when I wrote the question

The EncodedAudioStream Constructor calls the EncodedAuioOutput-Constructor

EncodedAudioStream(AudioStream *ioStream, AudioDecoder *decoder)
      : EncodedAudioOutput(ioStream, decoder) {
    // the indicated stream can be used as input
    setStream(ioStream);
  }

And that EncodedAudioOutput constructor takes care of the setNotifyAudioChange!

  EncodedAudioOutput(AudioStream *outputStream, AudioDecoder *decoder) {
    TRACED();
    ptr_out = outputStream;
    decoder_ptr = decoder;
    decoder_ptr->setOutput(*outputStream);
    decoder_ptr->setNotifyAudioChange(*outputStream);
    writer_ptr = decoder_ptr;
    active = false;
  }

I totally missed that.

But my question about the dec.setNotifyAudioChange(i2s); in the tutorial still stands.
When the EncodedAudioStream automatically notifies the VolumeStream, and it automatically notifies the I2SStream, then doesn't dec.setNotifyAudioChange(i2s); break this "notification flow"? Because it changes which Stream is notified by EncodedAudioStream.

[pschatzmann]

Maybe when this was written, this functionality was not in place, and it never got updated. I did not have this in place in the beginning and just later found that it would make sense to do this automatically. You might also still find other examples where setNotifyAudioChange() is still called.

[LinusHeu]

Now I understand it. Thank you

[pschatzmann]

You are right. The notification flow got broken in the example. However the most likely thing to change is the sampling rate and the VolumeStream does not care about this...

[LinusHeu]

Thanks for the confirmation. :D