Skip to content

Commit

Permalink
Hold AudioConverterRef via ScopedTypeRef
Browse files Browse the repository at this point in the history
There were two issues with the way AudioConverterRef was used by
AudioToolboxAudioDecoder:

  - `decoder_` was a plain pointer that was used uninitialized if
    AudioConverterNew() was never called

  - CreateAACDecoder() leaked an AudioConverter object upon
    re-initialization.

Both issues are fixed automatically by using base::ScopedTypeRef, and
potential future similar issues are easier to avoid this way too.

Fixed: 1394683
Change-Id: I9f4c4695c931ee840dd2c40ce0db31d144ec0b1c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4061479
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Auto-Submit: Wojciech Dzierżanowski <wdzierzanowski@opera.com>
Cr-Commit-Position: refs/heads/main@{#1077615}
  • Loading branch information
wdzierzanowski authored and Chromium LUCI CQ committed Nov 30, 2022
1 parent 00e022d commit 8a5526d
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 8 deletions.
27 changes: 20 additions & 7 deletions media/filters/mac/audio_toolbox_audio_decoder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -128,17 +128,26 @@ OSStatus ProvideInputCallback(AudioConverterRef decoder,

} // namespace

AudioToolboxAudioDecoder::AudioToolboxAudioDecoder() = default;

AudioToolboxAudioDecoder::~AudioToolboxAudioDecoder() {
if (!decoder_)
return;
// static
AudioConverterRef
AudioToolboxAudioDecoder::ScopedAudioConverterRefTraits::Retain(
AudioConverterRef converter) {
NOTREACHED() << "Only compatible with ASSUME policy";
return converter;
}

const auto result = AudioConverterDispose(decoder_);
// static
void AudioToolboxAudioDecoder::ScopedAudioConverterRefTraits::Release(
AudioConverterRef converter) {
const auto result = AudioConverterDispose(converter);
OSSTATUS_DLOG_IF(WARNING, result != noErr, result)
<< "AudioConverterDispose() failed";
}

AudioToolboxAudioDecoder::AudioToolboxAudioDecoder() = default;

AudioToolboxAudioDecoder::~AudioToolboxAudioDecoder() = default;

AudioDecoderType AudioToolboxAudioDecoder::GetDecoderType() const {
return AudioDecoderType::kAudioToolbox;
}
Expand All @@ -160,6 +169,9 @@ void AudioToolboxAudioDecoder::Initialize(const AudioDecoderConfig& config,
return;
}

// This decoder supports re-initialization.
decoder_.reset();

output_cb_ = output_cb;
BindToCurrentLoop(std::move(init_cb))
.Run(CreateAACDecoder(config)
Expand Down Expand Up @@ -280,7 +292,8 @@ bool AudioToolboxAudioDecoder::CreateAACDecoder(
output_format.mBitsPerChannel / 8;

// Create the decoder.
auto result = AudioConverterNew(&input_format, &output_format, &decoder_);
auto result = AudioConverterNew(&input_format, &output_format,
decoder_.InitializeInto());
if (result != noErr) {
OSSTATUS_DLOG(ERROR, result) << "AudioConverterNew() failed";
return false;
Expand Down
10 changes: 9 additions & 1 deletion media/filters/mac/audio_toolbox_audio_decoder.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,18 @@ class MEDIA_EXPORT AudioToolboxAudioDecoder : public AudioDecoder {
bool NeedsBitstreamConversion() const override;

private:
struct ScopedAudioConverterRefTraits {
static AudioConverterRef InvalidValue() { return nullptr; }
static AudioConverterRef Retain(AudioConverterRef converter);
static void Release(AudioConverterRef converter);
};
using ScopedAudioConverterRef =
base::ScopedTypeRef<AudioConverterRef, ScopedAudioConverterRefTraits>;

bool CreateAACDecoder(const AudioDecoderConfig& config);

// "Converter" for turning encoded samples into raw audio.
AudioConverterRef decoder_;
ScopedAudioConverterRef decoder_;

// Actual channel count and layout from decoder, may be different than config.
uint32_t channel_count_ = 0u;
Expand Down

0 comments on commit 8a5526d

Please sign in to comment.