PSA: AudioEncoder::Encode interface is changing

[Oskar Sundbom]

Just wanted to let you all know that the AudioEncoder interface is changing - specifically AudioEncoder::Encode and AudioEncoder::MaxEncodedBytes.

This change hands the responsibility of ensuring the buffer is large enough to the encoder implementation, rather than the caller. Specifically, this means:

• Encode() and EncodeInternal() now take an rtc::Buffer*, rather than a size_t and a uint8_t*.
• Encoding calls are now expected to append data to the buffer, rather than overwrite it. New implementations of EncodeInternal() should append the encoded data to the buffer!
• MaxEncodedBytes() will disappear, since the caller no longer needs to handle the buffer size manually.
• The new version of EncodeInternal() is now protected rather than public, since EncodeInternal() should not be called directly by user code.

The internal codecs, as well as calls to Encode(), have been changed to match this. There are currently backwards compatible versions of Encode() and EncodeInternal() that will ensure old encoder implementations work with callers using the new interface and vice-versa.

We plan to remove this backwards compatibility by Apr 8th, 2016.

What should I do?

If you've written code that calls Encode() on an encoder, you should update the call to use the new signature. This means you should no longer have to manage the output buffer manually, as rtc::Buffer is growable.

Remember: Encode() now appends data to the buffer. Call Clear() on it first, if you don't want to continuously accumulate encoded audio. Also, make sure you don't call MaxEncodedBytes() anywhere.

If you've written an AudioEncoder implementation, you should change it to implement the new variant of EncodeInternal(). This means ensuring the buffer is large enough, expanding it if necessary, and appending the encoded data to it. Fortunately, there is handy functionality in rtc::Buffer to do just that.

For a description of that functionality, see: https://codereview.webrtc.org/1717273002/ 

For an example of how it was used to convert an existing encoder, see: https://codereview.webrtc.org/1725143003/diff/120001/webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc.

[Oskar Sundbom]

Just a quick update!

For reasons of backwards compatibility, the new version of EncodeInternal() has been renamed EncodeImpl(). Keeping the same name for both versions caused compilation errors when compiling with GCC and -Woverloaded-virtual enabled. It could be (and was) worked around by explicitly importing both EncodeInternal variants into each AudioEncoder subclass by adding 'using AudioEncoder::EncodeInternal;'. Requiring this, however, breaks backwards compatibility, since existing implementations of AudioEncoder would need to be modified, albeit slightly.

As such, everything in the first post still applies, except encoders should be updated to implement EncodeImpl() instead.

For details on this latest change, see: https://codereview.webrtc.org/1764583003/.

[Oskar Sundbom]

A change just landed that removes the old EncodeInternal interface, as well as Encode taking raw pointer and size.

With this change, MaxEncodedBytes has been buried as a private member with a default implementation. It is no longer to be used but is retained to allow any subclasses to remove their implementations of it.

Please: remove MaxEncodedBytes from your implementations as soon as possible. It will be going away from the base class in a week or two and chances are your code will stop compiling at that point.

For more information about this last change, see the associated issue: https://bugs.chromium.org/p/webrtc/issues/detail?id=5591

[Oskar Sundbom]

Yet another quick update!

The aforementioned CL has had to be reverted while some users are being updated. The plan is still the same, but will have to be postponed for a few days.

More updates to follow!

[Oskar Sundbom]

The changes are now back on track. EncodeInternal was removed little over a week ago, together with the deprecation of MaxEncodedBytes.

MaxEncodedBytes will be removed from AudioEncoder in two weeks. If your encoder implementation is not using it, it should be removed.

If your encoder implementation is using it internally, for example to estimate the number of bytes for the Buffer::AppendData call, that internal function should be renamed (and no longer marked as an override, of course).

Chances are just removing the override from your internal MaxEncodedBytes will cause warnings, possibly treated as errors, when building now and keeping the override will cause errors once MaxEncodedBytes is removed.

[Oskar Sundbom]

A CL that removes MaxEncodedBytes from AudioEncoder just landed.

This should bring to a close this swathe of changes to AudioEncoder, and bug 5591 (see below) in particular.

Have a great day!

Oskar

[1] https://bugs.chromium.org/p/webrtc/issues/detail?id=5591