996224 - Possible typo in Reverb::process()

Status: RESOLVED WONTFIX

(Core :: Web Audio, defect, P3)

Description

[Maks Naumov]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

content/media/webaudio/blink/Reverb.cpp 177

bool isCopySafe = destinationChannelL && destinationChannelR &&
size_t(destinationBus->mDuration) >= framesToProcess &&
size_t(destinationBus->mDuration) >= framesToProcess;

I think should be:
bool isCopySafe = destinationChannelL && destinationChannelR &&
size_t(sourceBus->mDuration) >= framesToProcess &&
size_t(destinationBus->mDuration) >= framesToProcess;

Comment 1

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/annotate/265d82091bce/content/media/webaudio/blink/Reverb.cpp#l177

The source and destination of the copy below are both in destinationBus, so the check is correct, but with unnecessary duplication.

"framesToProcess <= size_t(sourceBus->mDuration)" is already above checked as part of isSafeToProcess.

There's nothing to gain from stereo output with 1 input channel and 1 impulse response, so we should probably change the caller to use 1 output channel and remove this block.

Comment 2

[u538282]

As far as I understand, the fix consists in removing
size_t(sourceBus->mDuration) >= framesToProcess &&
size_t(destinationBus->mDuration) >= framesToProcess;
from http://hg.mozilla.org/mozilla-central/annotate/265d82091bce/content/media/webaudio/blink/Reverb.cpp#l177 since this test is already done in http://hg.mozilla.org/mozilla-central/annotate/265d82091bce/content/media/webaudio/blink/Reverb.cpp#l141
Is that correct?

Comment 3

[Paul Adenot (:padenot)]

Yep I think that's it.

Comment 4

[u538282]

Attachment: isCopySafe.patch

Comment 5

[Paul Adenot (:padenot)]

Ah yeah no this has already been done by another commit apparently.