Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 7f475495 authored by Andy Hung's avatar Andy Hung
Browse files

Fix AudioMixer floating interaction with downmixer

Previously _if_ the full floating point mixer is enabled, a downmixer
would force the mixer input for a session submix to integer, breaking
other mixer inputs to the same submix that were in float.

Use another ReformatBufferProvider after the downmixer to solve
this issue.

Update the test-mixer app and the mixer_to_wave_tests shell
script to detect this issue.

Bug: 17363939
Change-Id: I74a56333f9ee75ddde39a75392c021c5eebddbef
parent 0f451e92
Loading
Loading
Loading
Loading
+50 −18
Original line number Diff line number Diff line
@@ -430,6 +430,10 @@ void AudioMixer::setLog(NBLog::Writer *log)
    mState.mLog = log;
}

static inline audio_format_t selectMixerInFormat(audio_format_t inputFormat __unused) {
    return kUseFloat && kUseNewMixer ? AUDIO_FORMAT_PCM_FLOAT : AUDIO_FORMAT_PCM_16_BIT;
}

int AudioMixer::getTrackName(audio_channel_mask_t channelMask,
        audio_format_t format, int sessionId)
{
@@ -492,10 +496,11 @@ int AudioMixer::getTrackName(audio_channel_mask_t channelMask,
        t->mInputBufferProvider = NULL;
        t->mReformatBufferProvider = NULL;
        t->downmixerBufferProvider = NULL;
        t->mPostDownmixReformatBufferProvider = NULL;
        t->mMixerFormat = AUDIO_FORMAT_PCM_16_BIT;
        t->mFormat = format;
        t->mMixerInFormat = kUseFloat && kUseNewMixer
                ? AUDIO_FORMAT_PCM_FLOAT : AUDIO_FORMAT_PCM_16_BIT;
        t->mMixerInFormat = selectMixerInFormat(format);
        t->mDownmixRequiresFormat = AUDIO_FORMAT_INVALID; // no format required
        t->mMixerChannelMask = audio_channel_mask_from_representation_and_bits(
                AUDIO_CHANNEL_REPRESENTATION_POSITION, AUDIO_CHANNEL_OUT_STEREO);
        t->mMixerChannelCount = audio_channel_count_from_out_mask(t->mMixerChannelMask);
@@ -505,9 +510,7 @@ int AudioMixer::getTrackName(audio_channel_mask_t channelMask,
            ALOGE("AudioMixer::getTrackName invalid channelMask (%#x)", channelMask);
            return -1;
        }
        // prepareForDownmix() may change the input format requirement.
        // If you desire floating point input to the mixer, it may change
        // to integer because the downmixer requires integer to process.
        // prepareForDownmix() may change mDownmixRequiresFormat
        ALOGVV("mMixerFormat:%#x  mMixerInFormat:%#x\n", t->mMixerFormat, t->mMixerInFormat);
        t->prepareForReformat();
        mTrackNames |= 1 << n;
@@ -526,7 +529,7 @@ void AudioMixer::invalidateState(uint32_t mask)
 }

// Called when channel masks have changed for a track name
// TODO: Fix Downmixbufferprofider not to (possibly) change mixer input format,
// TODO: Fix DownmixerBufferProvider not to (possibly) change mixer input format,
// which will simplify this logic.
bool AudioMixer::setChannelMasks(int name,
        audio_channel_mask_t trackChannelMask, audio_channel_mask_t mixerChannelMask) {
@@ -551,21 +554,18 @@ bool AudioMixer::setChannelMasks(int name,

    // channel masks have changed, does this track need a downmixer?
    // update to try using our desired format (if we aren't already using it)
    const audio_format_t prevMixerInFormat = track.mMixerInFormat;
    track.mMixerInFormat = kUseFloat && kUseNewMixer
            ? AUDIO_FORMAT_PCM_FLOAT : AUDIO_FORMAT_PCM_16_BIT;
    const audio_format_t prevDownmixerFormat = track.mDownmixRequiresFormat;
    const status_t status = mState.tracks[name].prepareForDownmix();
    ALOGE_IF(status != OK,
            "prepareForDownmix error %d, track channel mask %#x, mixer channel mask %#x",
            status, track.channelMask, track.mMixerChannelMask);

    const bool mixerInFormatChanged = prevMixerInFormat != track.mMixerInFormat;
    if (mixerInFormatChanged) {
    if (prevDownmixerFormat != track.mDownmixRequiresFormat) {
        track.prepareForReformat(); // because of downmixer, track format may change!
    }

    if (track.resampler && (mixerInFormatChanged || mixerChannelCountChanged)) {
        // resampler input format or channels may have changed.
    if (track.resampler && mixerChannelCountChanged) {
        // resampler channels may have changed.
        const uint32_t resetToSampleRate = track.sampleRate;
        delete track.resampler;
        track.resampler = NULL;
@@ -579,6 +579,7 @@ bool AudioMixer::setChannelMasks(int name,
void AudioMixer::track_t::unprepareForDownmix() {
    ALOGV("AudioMixer::unprepareForDownmix(%p)", this);

    mDownmixRequiresFormat = AUDIO_FORMAT_INVALID;
    if (downmixerBufferProvider != NULL) {
        // this track had previously been configured with a downmixer, delete it
        ALOGV(" deleting old downmixer");
@@ -611,7 +612,7 @@ status_t AudioMixer::track_t::prepareForDownmix()
                sampleRate, sessionId, kCopyBufferFrameCount);

        if (pDbp->isValid()) { // if constructor completed properly
            mMixerInFormat = AUDIO_FORMAT_PCM_16_BIT; // PCM 16 bit required for downmix
            mDownmixRequiresFormat = AUDIO_FORMAT_PCM_16_BIT; // PCM 16 bit required for downmix
            downmixerBufferProvider = pDbp;
            reconfigureBufferProviders();
            return NO_ERROR;
@@ -630,9 +631,18 @@ status_t AudioMixer::track_t::prepareForDownmix()

void AudioMixer::track_t::unprepareForReformat() {
    ALOGV("AudioMixer::unprepareForReformat(%p)", this);
    bool requiresReconfigure = false;
    if (mReformatBufferProvider != NULL) {
        delete mReformatBufferProvider;
        mReformatBufferProvider = NULL;
        requiresReconfigure = true;
    }
    if (mPostDownmixReformatBufferProvider != NULL) {
        delete mPostDownmixReformatBufferProvider;
        mPostDownmixReformatBufferProvider = NULL;
        requiresReconfigure = true;
    }
    if (requiresReconfigure) {
        reconfigureBufferProviders();
    }
}
@@ -640,14 +650,29 @@ void AudioMixer::track_t::unprepareForReformat() {
status_t AudioMixer::track_t::prepareForReformat()
{
    ALOGV("AudioMixer::prepareForReformat(%p) with format %#x", this, mFormat);
    // discard the previous reformatter if there was one
    // discard previous reformatters
    unprepareForReformat();
    // only configure reformatter if needed
    if (mFormat != mMixerInFormat) {
    // only configure reformatters as needed
    const audio_format_t targetFormat = mDownmixRequiresFormat != AUDIO_FORMAT_INVALID
            ? mDownmixRequiresFormat : mMixerInFormat;
    bool requiresReconfigure = false;
    if (mFormat != targetFormat) {
        mReformatBufferProvider = new ReformatBufferProvider(
                audio_channel_count_from_out_mask(channelMask),
                mFormat, mMixerInFormat,
                mFormat,
                targetFormat,
                kCopyBufferFrameCount);
        requiresReconfigure = true;
    }
    if (targetFormat != mMixerInFormat) {
        mPostDownmixReformatBufferProvider = new ReformatBufferProvider(
                audio_channel_count_from_out_mask(mMixerChannelMask),
                targetFormat,
                mMixerInFormat,
                kCopyBufferFrameCount);
        requiresReconfigure = true;
    }
    if (requiresReconfigure) {
        reconfigureBufferProviders();
    }
    return NO_ERROR;
@@ -664,6 +689,10 @@ void AudioMixer::track_t::reconfigureBufferProviders()
        downmixerBufferProvider->setBufferProvider(bufferProvider);
        bufferProvider = downmixerBufferProvider;
    }
    if (mPostDownmixReformatBufferProvider) {
        mPostDownmixReformatBufferProvider->setBufferProvider(bufferProvider);
        bufferProvider = mPostDownmixReformatBufferProvider;
    }
}

void AudioMixer::deleteTrackName(int name)
@@ -1026,6 +1055,9 @@ void AudioMixer::setBufferProvider(int name, AudioBufferProvider* bufferProvider
    if (mState.tracks[name].mReformatBufferProvider != NULL) {
        mState.tracks[name].mReformatBufferProvider->reset();
    } else if (mState.tracks[name].downmixerBufferProvider != NULL) {
        mState.tracks[name].downmixerBufferProvider->reset();
    } else if (mState.tracks[name].mPostDownmixReformatBufferProvider != NULL) {
        mState.tracks[name].mPostDownmixReformatBufferProvider->reset();
    }

    mState.tracks[name].mInputBufferProvider = bufferProvider;
+18 −2
Original line number Diff line number Diff line
@@ -205,17 +205,34 @@ private:
        int32_t*           auxBuffer;

        // 16-byte boundary

        /* Buffer providers are constructed to translate the track input data as needed.
         *
         * 1) mInputBufferProvider: The AudioTrack buffer provider.
         * 2) mReformatBufferProvider: If not NULL, performs the audio reformat to
         *    match either mMixerInFormat or mDownmixRequiresFormat, if the downmixer
         *    requires reformat. For example, it may convert floating point input to
         *    PCM_16_bit if that's required by the downmixer.
         * 3) downmixerBufferProvider: If not NULL, performs the channel remixing to match
         *    the number of channels required by the mixer sink.
         * 4) mPostDownmixReformatBufferProvider: If not NULL, performs reformatting from
         *    the downmixer requirements to the mixer engine input requirements.
         */
        AudioBufferProvider*     mInputBufferProvider;    // externally provided buffer provider.
        CopyBufferProvider*      mReformatBufferProvider; // provider wrapper for reformatting.
        CopyBufferProvider*      downmixerBufferProvider; // wrapper for channel conversion.
        CopyBufferProvider*      mPostDownmixReformatBufferProvider;

        // 16-byte boundary
        int32_t     sessionId;

        // 16-byte boundary
        audio_format_t mMixerFormat;     // output mix format: AUDIO_FORMAT_PCM_(FLOAT|16_BIT)
        audio_format_t mFormat;          // input track format
        audio_format_t mMixerInFormat;   // mix internal format AUDIO_FORMAT_PCM_(FLOAT|16_BIT)
                                         // each track must be converted to this format.
        audio_format_t mDownmixRequiresFormat;  // required downmixer format
                                                // AUDIO_FORMAT_PCM_16_BIT if 16 bit necessary
                                                // AUDIO_FORMAT_INVALID if no required format

        float          mVolume[MAX_NUM_VOLUMES];     // floating point set volume
        float          mPrevVolume[MAX_NUM_VOLUMES]; // floating point previous volume
@@ -225,7 +242,6 @@ private:
        float          mPrevAuxLevel;                 // floating point prev aux level
        float          mAuxInc;                       // floating point aux increment

        // 16-byte boundary
        audio_channel_mask_t mMixerChannelMask;
        uint32_t             mMixerChannelCount;

+11 −1
Original line number Diff line number Diff line
@@ -59,12 +59,22 @@ function createwav() {
        mkdir $2
    fi

# Test:
# process__genericResampling
# track__Resample / track__genericResample
    adb shell test-mixer $1 -s 48000 \
        -o /sdcard/tm48000grif.wav \
        sine:2,4000,7520 chirp:2,9200 sine:1,3000,18000 \
        sine:f,6,6000,19000  chirp:i,4,30000
    adb pull /sdcard/tm48000grif.wav $2

# Test:
# process__genericResampling
# track__Resample / track__genericResample
    adb shell test-mixer $1 -s 48000 \
        -o /sdcard/tm48000gr.wav \
        sine:2,4000,7520 chirp:2,9200 sine:1,3000,18000
        sine:2,4000,7520 chirp:2,9200 sine:1,3000,18000 \
        sine:6,6000,19000
    adb pull /sdcard/tm48000gr.wav $2

# Test:
+57 −35
Original line number Diff line number Diff line
@@ -39,7 +39,7 @@ static void usage(const char* name) {
    fprintf(stderr, "Usage: %s [-f] [-m] [-c channels]"
                    " [-s sample-rate] [-o <output-file>] [-a <aux-buffer-file>] [-P csv]"
                    " (<input-file> | <command>)+\n", name);
    fprintf(stderr, "    -f    enable floating point input track\n");
    fprintf(stderr, "    -f    enable floating point input track by default\n");
    fprintf(stderr, "    -m    enable floating point mixer output\n");
    fprintf(stderr, "    -c    number of mixer output channels\n");
    fprintf(stderr, "    -s    mixer sample-rate\n");
@@ -47,8 +47,8 @@ static void usage(const char* name) {
    fprintf(stderr, "    -a    <aux-buffer-file>\n");
    fprintf(stderr, "    -P    # frames provided per call to resample() in CSV format\n");
    fprintf(stderr, "    <input-file> is a WAV file\n");
    fprintf(stderr, "    <command> can be 'sine:<channels>,<frequency>,<samplerate>'\n");
    fprintf(stderr, "                     'chirp:<channels>,<samplerate>'\n");
    fprintf(stderr, "    <command> can be 'sine:[(i|f),]<channels>,<frequency>,<samplerate>'\n");
    fprintf(stderr, "                     'chirp:[(i|f),]<channels>,<samplerate>'\n");
}

static int writeFile(const char *filename, const void *buffer,
@@ -78,6 +78,18 @@ static int writeFile(const char *filename, const void *buffer,
    return EXIT_SUCCESS;
}

const char *parseFormat(const char *s, bool *useFloat) {
    if (!strncmp(s, "f,", 2)) {
        *useFloat = true;
        return s + 2;
    }
    if (!strncmp(s, "i,", 2)) {
        *useFloat = false;
        return s + 2;
    }
    return s;
}

int main(int argc, char* argv[]) {
    const char* const progname = argv[0];
    bool useInputFloat = false;
@@ -88,8 +100,9 @@ int main(int argc, char* argv[]) {
    std::vector<int> Pvalues;
    const char* outputFilename = NULL;
    const char* auxFilename = NULL;
    std::vector<int32_t> Names;
    std::vector<SignalProvider> Providers;
    std::vector<int32_t> names;
    std::vector<SignalProvider> providers;
    std::vector<audio_format_t> formats;

    for (int ch; (ch = getopt(argc, argv, "fmc:s:o:a:P:")) != -1;) {
        switch (ch) {
@@ -138,54 +151,65 @@ int main(int argc, char* argv[]) {
    size_t outputFrames = 0;

    // create providers for each track
    Providers.resize(argc);
    names.resize(argc);
    providers.resize(argc);
    formats.resize(argc);
    for (int i = 0; i < argc; ++i) {
        static const char chirp[] = "chirp:";
        static const char sine[] = "sine:";
        static const double kSeconds = 1;
        bool useFloat = useInputFloat;

        if (!strncmp(argv[i], chirp, strlen(chirp))) {
            std::vector<int> v;
            const char *s = parseFormat(argv[i] + strlen(chirp), &useFloat);

            parseCSV(argv[i] + strlen(chirp), v);
            parseCSV(s, v);
            if (v.size() == 2) {
                printf("creating chirp(%d %d)\n", v[0], v[1]);
                if (useInputFloat) {
                    Providers[i].setChirp<float>(v[0], 0, v[1]/2, v[1], kSeconds);
                if (useFloat) {
                    providers[i].setChirp<float>(v[0], 0, v[1]/2, v[1], kSeconds);
                    formats[i] = AUDIO_FORMAT_PCM_FLOAT;
                } else {
                    Providers[i].setChirp<int16_t>(v[0], 0, v[1]/2, v[1], kSeconds);
                    providers[i].setChirp<int16_t>(v[0], 0, v[1]/2, v[1], kSeconds);
                    formats[i] = AUDIO_FORMAT_PCM_16_BIT;
                }
                Providers[i].setIncr(Pvalues);
                providers[i].setIncr(Pvalues);
            } else {
                fprintf(stderr, "malformed input '%s'\n", argv[i]);
            }
        } else if (!strncmp(argv[i], sine, strlen(sine))) {
            std::vector<int> v;
            const char *s = parseFormat(argv[i] + strlen(sine), &useFloat);

            parseCSV(argv[i] + strlen(sine), v);
            parseCSV(s, v);
            if (v.size() == 3) {
                printf("creating sine(%d %d %d)\n", v[0], v[1], v[2]);
                if (useInputFloat) {
                    Providers[i].setSine<float>(v[0], v[1], v[2], kSeconds);
                if (useFloat) {
                    providers[i].setSine<float>(v[0], v[1], v[2], kSeconds);
                    formats[i] = AUDIO_FORMAT_PCM_FLOAT;
                } else {
                    Providers[i].setSine<int16_t>(v[0], v[1], v[2], kSeconds);
                    providers[i].setSine<int16_t>(v[0], v[1], v[2], kSeconds);
                    formats[i] = AUDIO_FORMAT_PCM_16_BIT;
                }
                Providers[i].setIncr(Pvalues);
                providers[i].setIncr(Pvalues);
            } else {
                fprintf(stderr, "malformed input '%s'\n", argv[i]);
            }
        } else {
            printf("creating filename(%s)\n", argv[i]);
            if (useInputFloat) {
                Providers[i].setFile<float>(argv[i]);
                providers[i].setFile<float>(argv[i]);
                formats[i] = AUDIO_FORMAT_PCM_FLOAT;
            } else {
                Providers[i].setFile<short>(argv[i]);
                providers[i].setFile<short>(argv[i]);
                formats[i] = AUDIO_FORMAT_PCM_16_BIT;
            }
            Providers[i].setIncr(Pvalues);
            providers[i].setIncr(Pvalues);
        }
        // calculate the number of output frames
        size_t nframes = (int64_t) Providers[i].getNumFrames() * outputSampleRate
                / Providers[i].getSampleRate();
        size_t nframes = (int64_t) providers[i].getNumFrames() * outputSampleRate
                / providers[i].getSampleRate();
        if (i == 0 || outputFrames > nframes) { // choose minimum for outputFrames
            outputFrames = nframes;
        }
@@ -213,22 +237,20 @@ int main(int argc, char* argv[]) {
    // create the mixer.
    const size_t mixerFrameCount = 320; // typical numbers may range from 240 or 960
    AudioMixer *mixer = new AudioMixer(mixerFrameCount, outputSampleRate);
    audio_format_t inputFormat = useInputFloat
            ? AUDIO_FORMAT_PCM_FLOAT : AUDIO_FORMAT_PCM_16_BIT;
    audio_format_t mixerFormat = useMixerFloat
            ? AUDIO_FORMAT_PCM_FLOAT : AUDIO_FORMAT_PCM_16_BIT;
    float f = AudioMixer::UNITY_GAIN_FLOAT / Providers.size(); // normalize volume by # tracks
    float f = AudioMixer::UNITY_GAIN_FLOAT / providers.size(); // normalize volume by # tracks
    static float f0; // zero

    // set up the tracks.
    for (size_t i = 0; i < Providers.size(); ++i) {
        //printf("track %d out of %d\n", i, Providers.size());
        uint32_t channelMask = audio_channel_out_mask_from_count(Providers[i].getNumChannels());
    for (size_t i = 0; i < providers.size(); ++i) {
        //printf("track %d out of %d\n", i, providers.size());
        uint32_t channelMask = audio_channel_out_mask_from_count(providers[i].getNumChannels());
        int32_t name = mixer->getTrackName(channelMask,
                inputFormat, AUDIO_SESSION_OUTPUT_MIX);
                formats[i], AUDIO_SESSION_OUTPUT_MIX);
        ALOG_ASSERT(name >= 0);
        Names.push_back(name);
        mixer->setBufferProvider(name, &Providers[i]);
        names[i] = name;
        mixer->setBufferProvider(name, &providers[i]);
        mixer->setParameter(name, AudioMixer::TRACK, AudioMixer::MAIN_BUFFER,
                (void *)outputAddr);
        mixer->setParameter(
@@ -240,7 +262,7 @@ int main(int argc, char* argv[]) {
                name,
                AudioMixer::TRACK,
                AudioMixer::FORMAT,
                (void *)(uintptr_t)inputFormat);
                (void *)(uintptr_t)formats[i]);
        mixer->setParameter(
                name,
                AudioMixer::TRACK,
@@ -255,7 +277,7 @@ int main(int argc, char* argv[]) {
                name,
                AudioMixer::RESAMPLE,
                AudioMixer::SAMPLE_RATE,
                (void *)(uintptr_t)Providers[i].getSampleRate());
                (void *)(uintptr_t)providers[i].getSampleRate());
        if (useRamp) {
            mixer->setParameter(name, AudioMixer::VOLUME, AudioMixer::VOLUME0, &f0);
            mixer->setParameter(name, AudioMixer::VOLUME, AudioMixer::VOLUME1, &f0);
@@ -277,11 +299,11 @@ int main(int argc, char* argv[]) {
    // pump the mixer to process data.
    size_t i;
    for (i = 0; i < outputFrames - mixerFrameCount; i += mixerFrameCount) {
        for (size_t j = 0; j < Names.size(); ++j) {
            mixer->setParameter(Names[j], AudioMixer::TRACK, AudioMixer::MAIN_BUFFER,
        for (size_t j = 0; j < names.size(); ++j) {
            mixer->setParameter(names[j], AudioMixer::TRACK, AudioMixer::MAIN_BUFFER,
                    (char *) outputAddr + i * outputFrameSize);
            if (auxFilename) {
                mixer->setParameter(Names[j], AudioMixer::TRACK, AudioMixer::AUX_BUFFER,
                mixer->setParameter(names[j], AudioMixer::TRACK, AudioMixer::AUX_BUFFER,
                        (char *) auxAddr + i * auxFrameSize);
            }
        }