Bug 71949 – Add buffering to handle mismatch between hardware buffer size and webaudio render size

• Component: New Bugs
• Status: RESOLVED FIXED

[Raymond Toy]

Add buffering to handle mismatch between hardware buffer size and webaudio render size

[Raymond Toy]

Created attachment 114354 [details]
Patch

[Raymond Toy]

This patch adds buffering to support the case where the hardware buffer size is not a multiple of the webaudio render size.

For some background see http://codereview.chromium.org/8440002/.  In that CL, support for WASAPI on Windows is being added.  The hardware buffer with WASAPI works best when a 10ms buffer is used.  This corresponds to a buffer size of 480 samples (at 48kHz).  But webaudio always audio with a buffer size of 128 and this mismatch needs to be handled.  The fifo handles this by calling webaudio 4 times to generate 512 samples, and delivers the requested 480 samples.  The remaining 32 samples are left for delivery the next time.  It turns out that on the 16 call to webaudio to render data, the fifo will have a full 480 samples available, so the call to webaudio will be skipped.

[Chris Rogers]

Comment on attachment 114354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114354&action=review

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:63
> +    // Get the minimum usable buffer size.

I would change this comment to:

// Use the optimal buffer size recommended by the audio back-end.

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:68
> +

Please add a comment here describing why we're creating m_fifo

> Source/WebKit/chromium/src/AudioDestinationChromium.h:-58
> -    AudioSourceProvider& m_provider;

It would be good to have a comment describing (at least) what FIFO stands for, and why we need this class.

> Source/WebKit/chromium/src/AudioDestinationChromium.h:59
> +    class FIFO {

I'd recommend moving all of the implementation for this class in the .cpp file and just have the declarations of the methods here...

> Source/WebKit/chromium/src/AudioDestinationChromium.h:62
> +                : m_provider(provider)

too much indentation

> Source/WebKit/chromium/src/AudioDestinationChromium.h:65
> +                , m_length(0)

I'm not crazy about the name "m_length" -- how about "m_framesInBuffer" or "m_bufferedFrames"

> Source/WebKit/chromium/src/AudioDestinationChromium.h:68
> +                , m_renderSize(renderSize)

Can we change "m_renderSize"  ->  "m_fillsize"
(and similarly "renderSize" -> "fillSize")

"render" doesn't seem clear enough about whether this is at the filling side or consuming side of the FIFO

> Source/WebKit/chromium/src/AudioDestinationChromium.h:73
> +        ~FIFO()

You can remove empty destructor

> Source/WebKit/chromium/src/AudioDestinationChromium.h:80
> +        void consume(AudioBus* destination, size_t numberOfFrames)

how about making "numberOfFrames" more descriptive, like "framesToConsume"

> Source/WebKit/chromium/src/AudioDestinationChromium.h:81
> +        {

just to be careful, probably should have:

ASSERT(destination);
if (!destination)
    return;

We also need a similar sanity check that (numberOfFrames <= m_fifoLength)

> Source/WebKit/chromium/src/AudioDestinationChromium.h:98
> +                float* destinationChannel = destination->channel(channelIndex)->data();

Consistency: below you simply use "source" as the name, so why not simply "destination" here instead of "destinationChannel"?

> Source/WebKit/chromium/src/AudioDestinationChromium.h:101
> +                memcpy(destinationChannel,

This memcpy() and the one below should all be a single-line

> Source/WebKit/chromium/src/AudioDestinationChromium.h:112
> +            ASSERT(m_length >= 0);

m_length is size_t so can never be negative -- ASSERT can be removed

or you can change this to be a signed type

> Source/WebKit/chromium/src/AudioDestinationChromium.h:118
> +        int updateIndex(int index, int step)

The updateIndex() method is unnecessary - it's pretty simple and more clear to simply update m_readIndex and m_writeIndex directly each on a single-line

> Source/WebKit/chromium/src/AudioDestinationChromium.h:126
> +        {

I would add an ASSERT

ASSERT(index < m_fifoLength && size <= m_fifoLength);

and also do the same check outside of the ASSERT and set part1Length and part2Length to harmless values if so

> Source/WebKit/chromium/src/AudioDestinationChromium.h:140
> +        // data.

Please consolidate into a single-line comment.

> Source/WebKit/chromium/src/AudioDestinationChromium.h:145
> +            // the data into the FIFO.

Comment has typo "at received at least"

> Source/WebKit/chromium/src/AudioDestinationChromium.h:149
> +                size_t numberOfChannels = m_fifoAudioBus.numberOfChannels();

Please move "numberOfChannels" to closest point of first use - in this case just above the for() statement

> Source/WebKit/chromium/src/AudioDestinationChromium.h:152
> +

I'd remove this blank line just before findWrapLengths()

> Source/WebKit/chromium/src/AudioDestinationChromium.h:161
> +                           part1Length * sizeof(*destination));

I'd suggest putting the memcpy() here the next one all on a single line.  It's not very WebKit-style friendly.

> Source/WebKit/chromium/src/AudioDestinationChromium.h:180
> +        // buffer.

one-line comment.

> Source/WebKit/chromium/src/AudioDestinationChromium.h:200
> +

extra blank line

[Raymond Toy]

Comment on attachment 114354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114354&action=review

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:-78
> -    

Oops.  Should probably keep the check that the callback buffer size is less than the maximum buffer size.

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:69
> +    m_fifo = adoptPtr(new FIFO(provider, numberOfChannels, maximumCallbackBufferSize, renderBufferSize));

The fifo size should probably be maximumCallbackBufferSize + renderBufferSize.  Things won't work very well if the callback buffer size is very close to maximumCallbackBufferSize.  We won't have room to add an additional render block.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:68
>> +                , m_renderSize(renderSize)
> 
> Can we change "m_renderSize"  ->  "m_fillsize"
> (and similarly "renderSize" -> "fillSize")
> 
> "render" doesn't seem clear enough about whether this is at the filling side or consuming side of the FIFO

How about m_producerSize, since that's the size of data the producer should produce?

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:98
>> +                float* destinationChannel = destination->channel(channelIndex)->data();
> 
> Consistency: below you simply use "source" as the name, so why not simply "destination" here instead of "destinationChannel"?

Because "destination" is already a variable (input parameter) and that gets confusing.  How about if I change source to sourceChannel instead?

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:112
>> +            ASSERT(m_length >= 0);
> 
> m_length is size_t so can never be negative -- ASSERT can be removed
> 
> or you can change this to be a signed type

Good point.  It should be an int type so that m_length -= numberOfFrames doesn't suddenly become some huge number because it was unsigned.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:118
>> +        int updateIndex(int index, int step)
> 
> The updateIndex() method is unnecessary - it's pretty simple and more clear to simply update m_readIndex and m_writeIndex directly each on a single-line

Perhaps, but I believe in OAOO (once and only once).  If we change the implementation (to say a power-of-two fifo length) we only have to modify just updateIndex, not everywhere.

[Raymond Toy]

Comment on attachment 114354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114354&action=review

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:63

> 
> I would change this comment to:
> 
> // Use the optimal buffer size recommended by the audio back-end.

Done

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:-78
>> -    
> 
> Oops.  Should probably keep the check that the callback buffer size is less than the maximum buffer size.

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:68
>> +
> 
> Please add a comment here describing why we're creating m_fifo

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:69
>> +    m_fifo = adoptPtr(new FIFO(provider, numberOfChannels, maximumCallbackBufferSize, renderBufferSize));
> 
> The fifo size should probably be maximumCallbackBufferSize + renderBufferSize.  Things won't work very well if the callback buffer size is very close to maximumCallbackBufferSize.  We won't have room to add an additional render block.

Done.  But should we change the maximum to something smaller than 16K?  Maybe 4 or 8K would be better?

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:59
>> +    class FIFO {
> 
> I'd recommend moving all of the implementation for this class in the .cpp file and just have the declarations of the methods here...

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:62
>> +                : m_provider(provider)
> 
> too much indentation

Fixed.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:65
>> +                , m_length(0)
> 
> I'm not crazy about the name "m_length" -- how about "m_framesInBuffer" or "m_bufferedFrames"

Using m_framesInFifo.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:73
>> +        ~FIFO()
> 
> You can remove empty destructor

Removed.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:80
>> +        void consume(AudioBus* destination, size_t numberOfFrames)
> 
> how about making "numberOfFrames" more descriptive, like "framesToConsume"

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:81
>> +        {
> 
> just to be careful, probably should have:
> 
> ASSERT(destination);
> if (!destination)
>     return;
> 
> We also need a similar sanity check that (numberOfFrames <= m_fifoLength)

ASSERT added.

How do you want to handle the case of numberOfFrames > m_fifoLength?  Just return?  Fill the destination with zeroes?  Fill the fifo completely, return that data, and zero out the remainder?

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:101
>> +                memcpy(destinationChannel,
> 
> This memcpy() and the one below should all be a single-line

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:126
>> +        {
> 
> I would add an ASSERT
> 
> ASSERT(index < m_fifoLength && size <= m_fifoLength);
> 
> and also do the same check outside of the ASSERT and set part1Length and part2Length to harmless values if so

Done.  part1Length and part2Length are set to 0, so nothing will get copied.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:140
>> +        // data.
> 
> Please consolidate into a single-line comment.

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:145
>> +            // the data into the FIFO.
> 
> Comment has typo "at received at least"

Fixed.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:149
>> +                size_t numberOfChannels = m_fifoAudioBus.numberOfChannels();
> 
> Please move "numberOfChannels" to closest point of first use - in this case just above the for() statement

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:152
>> +
> 
> I'd remove this blank line just before findWrapLengths()

Removed.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:161
>> +                           part1Length * sizeof(*destination));
> 
> I'd suggest putting the memcpy() here the next one all on a single line.  It's not very WebKit-style friendly.

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:180
>> +        // buffer.
> 
> one-line comment.

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:200
>> +
> 
> extra blank line

Removed.

[Chris Rogers]

Comment on attachment 114354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114354&action=review

Hi Ray - it looks like you forgot to upload a new patch along with your comments.  I'll have a closer look when the new patch comes

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:69
>> +    m_fifo = adoptPtr(new FIFO(provider, numberOfChannels, maximumCallbackBufferSize, renderBufferSize));
> 
> The fifo size should probably be maximumCallbackBufferSize + renderBufferSize.  Things won't work very well if the callback buffer size is very close to maximumCallbackBufferSize.  We won't have room to add an additional render block.

OK - or maybe better is to rename "maximumCallbackBufferSize" -> "fifoSize" and then simply ASSERT that   callbackSize + renderBufferSize <= fifoSize

>>> Source/WebKit/chromium/src/AudioDestinationChromium.h:68
>>> +                , m_renderSize(renderSize)
>> 
>> Can we change "m_renderSize"  ->  "m_fillsize"
>> (and similarly "renderSize" -> "fillSize")
>> 
>> "render" doesn't seem clear enough about whether this is at the filling side or consuming side of the FIFO
> 
> How about m_producerSize, since that's the size of data the producer should produce?

That sounds good.

>>> Source/WebKit/chromium/src/AudioDestinationChromium.h:98
>>> +                float* destinationChannel = destination->channel(channelIndex)->data();
>> 
>> Consistency: below you simply use "source" as the name, so why not simply "destination" here instead of "destinationChannel"?
> 
> Because "destination" is already a variable (input parameter) and that gets confusing.  How about if I change source to sourceChannel instead?

I don't like the suffix "Channel" because there's actually a really important Web Audio class called AudioChannel which is actually used on line 98

Looking at how line 98 reads - it makes it even more odd to call it "Channel" -- "Data" seems like an ok suffix here

>>> Source/WebKit/chromium/src/AudioDestinationChromium.h:118
>>> +        int updateIndex(int index, int step)
>> 
>> The updateIndex() method is unnecessary - it's pretty simple and more clear to simply update m_readIndex and m_writeIndex directly each on a single-line
> 
> Perhaps, but I believe in OAOO (once and only once).  If we change the implementation (to say a power-of-two fifo length) we only have to modify just updateIndex, not everywhere.

Seems like we'll never have to change updateIndex() for power-of-two since the MOD operator should work there as well as any other case.

In any case, if you do choose to keep the method, please compact it into a single-line:

return (index + step) % m_fifoLength;

[Chris Rogers]

Hi Ray, I still don't see your new patch??

[Raymond Toy]

(In reply to comment #7)
> Hi Ray, I still don't see your new patch??

I think a midair collision prevented my patch from going in.  (Prevented most of my followup comments from being sent.)

I'll make another patch incorporating your new comments and upload the patch.  Hopefully, no midair collision this time.

[Raymond Toy]

Created attachment 114735 [details]
Patch

[Raymond Toy]

Comment on attachment 114354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114354&action=review

>>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:63
>>> +    // Get the minimum usable buffer size.
>> 
>> I would change this comment to:
>> 
>> // Use the optimal buffer size recommended by the audio back-end.
> 
> 

Done.

>>>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:69
>>>> +    m_fifo = adoptPtr(new FIFO(provider, numberOfChannels, maximumCallbackBufferSize, renderBufferSize));
>>> 
>>> The fifo size should probably be maximumCallbackBufferSize + renderBufferSize.  Things won't work very well if the callback buffer size is very close to maximumCallbackBufferSize.  We won't have room to add an additional render block.
>> 
>> Done.  But should we change the maximum to something smaller than 16K?  Maybe 4 or 8K would be better?
> 
> OK - or maybe better is to rename "maximumCallbackBufferSize" -> "fifoSize" and then simply ASSERT that   callbackSize + renderBufferSize <= fifoSize

Renamed as suggested, along with the ASSERT.

>>>> Source/WebKit/chromium/src/AudioDestinationChromium.h:98
>>>> +                float* destinationChannel = destination->channel(channelIndex)->data();
>>> 
>>> Consistency: below you simply use "source" as the name, so why not simply "destination" here instead of "destinationChannel"?
>> 
>> Because "destination" is already a variable (input parameter) and that gets confusing.  How about if I change source to sourceChannel instead?
> 
> I don't like the suffix "Channel" because there's actually a really important Web Audio class called AudioChannel which is actually used on line 98
> 
> Looking at how line 98 reads - it makes it even more odd to call it "Channel" -- "Data" seems like an ok suffix here

Renamed to Data.

>>>> Source/WebKit/chromium/src/AudioDestinationChromium.h:118
>>>> +        int updateIndex(int index, int step)
>>> 
>>> The updateIndex() method is unnecessary - it's pretty simple and more clear to simply update m_readIndex and m_writeIndex directly each on a single-line
>> 
>> Perhaps, but I believe in OAOO (once and only once).  If we change the implementation (to say a power-of-two fifo length) we only have to modify just updateIndex, not everywhere.
> 
> Seems like we'll never have to change updateIndex() for power-of-two since the MOD operator should work there as well as any other case.
> 
> In any case, if you do choose to keep the method, please compact it into a single-line:
> 
> return (index + step) % m_fifoLength;

Simplified as suggested.

I was thinking of the nano optimization of replacing the % m_fifoLength with & (m_fifoLength - 1).

[Chris Rogers]

Comment on attachment 114735 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114735&action=review

Thanks - looking really good!  Mostly a few style nits, plus some extra paranoid checks near the memcpy() calls...

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:80
> +    ASSERT(m_fifo);

This ASSERT is probably not necessary, since I believe the memory allocator will ASSERT if "new" fails...

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:146
> +

I think we still need a sanity check that (framesToConsume <= m_fifoLength) - ASSERT and early return

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:165
> +

I know that findWrapLength() should be returning sane values, but we tend to be super paranoid when doing memcpy(), so I think we should add an ASSERT and early return here:

bool isCopyGood = part1Length < framesToConsume && part2Length < framesToConsume && part1Length + part2Length < framesToConsume;

(the third check should guard against wrap-around)

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:214
> +

Probably should have ASSERT and early return to sanity check the memcpy() similar to what I recommend above

> Source/WebKit/chromium/src/AudioDestinationChromium.h:60
> +    // audio buffer size and the Web Audio render size.

nit: I would change "audio buffer size" -> "hardware buffer size" (or even better "audio back-end hardware buffer size"

> Source/WebKit/chromium/src/AudioDestinationChromium.h:64
> +        // will be large enough to hold |length| frames of data of

|length| -> |fifoLength|

> Source/WebKit/chromium/src/AudioDestinationChromium.h:66
> +        // provide data in frames of size |providerSize|.

I would reword this slightly:

"The AudioSourceProvider must provide data in frames of size |providerSize|."

--->

"The AudioSourceProvider will be asked to produce |providerSize| frames when the FIFO needs more data."

> Source/WebKit/chromium/src/AudioDestinationChromium.h:69
> +        // Get data from |framestoConsume| frames from the FIFO and

This sentence sounds a little awkward.  How about:

"Read |framesToConsume| from the FIFO into the destination."

> Source/WebKit/chromium/src/AudioDestinationChromium.h:79
> +        {

style nit: single-line inline method should be put on a single-line:

int updateIndex(int index, int step) { return (index + step) % m_fifoLength; }

[Raymond Toy]

Created attachment 114795 [details]
Patch

[Raymond Toy]

Comment on attachment 114735 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114735&action=review

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:80
>> +    ASSERT(m_fifo);
> 
> This ASSERT is probably not necessary, since I believe the memory allocator will ASSERT if "new" fails...

Removed

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:146
>> +
> 
> I think we still need a sanity check that (framesToConsume <= m_fifoLength) - ASSERT and early return

Check added.

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:165
>> +
> 
> I know that findWrapLength() should be returning sane values, but we tend to be super paranoid when doing memcpy(), so I think we should add an ASSERT and early return here:
> 
> bool isCopyGood = part1Length < framesToConsume && part2Length < framesToConsume && part1Length + part2Length < framesToConsume;
> 
> (the third check should guard against wrap-around)

Check added.

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:214
>> +
> 
> Probably should have ASSERT and early return to sanity check the memcpy() similar to what I recommend above

Check added.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:60
>> +    // audio buffer size and the Web Audio render size.
> 
> nit: I would change "audio buffer size" -> "hardware buffer size" (or even better "audio back-end hardware buffer size"

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:64
>> +        // will be large enough to hold |length| frames of data of
> 
> |length| -> |fifoLength|

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:66
>> +        // provide data in frames of size |providerSize|.
> 
> I would reword this slightly:
> 
> "The AudioSourceProvider must provide data in frames of size |providerSize|."
> 
> --->
> 
> "The AudioSourceProvider will be asked to produce |providerSize| frames when the FIFO needs more data."

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:69
>> +        // Get data from |framestoConsume| frames from the FIFO and
> 
> This sentence sounds a little awkward.  How about:
> 
> "Read |framesToConsume| from the FIFO into the destination."

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.h:79
>> +        {
> 
> style nit: single-line inline method should be put on a single-line:
> 
> int updateIndex(int index, int step) { return (index + step) % m_fifoLength; }

Done.

[Raymond Toy]

Applied suggested fixes.

[Chris Rogers]

Comment on attachment 114795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114795&action=review

A few last tweaks

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:140
> +void AudioDestinationChromium::FIFO::consume(AudioBus* destination, int framesToConsume)

Can we make "framesToConsume" size_t instead of int and avoid the several static_cast<int> associated with it?

It seems a bit ugly to have these static_casts -- best to avoid them

you may need to change a few other places "int" -> "size_t" to make it work

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:166
> +        bool isCopyGood = (static_cast<int>(part1Length) <= framesToConsume

This also needs to sanity check "m_readIndex"

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:168
> +                           && static_cast<int>(part1Length + part2Length) <= framesToConsume);

See comment about static_cast<int> above

> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:222
> +            bool isCopyGood = (part1Length <= m_providerSize

Sanity check also needs to factor in m_writeIndex

[Chris Rogers]

Hi Ray, if you make the last few changes then this looks ready to go.

[Raymond Toy]

Created attachment 114977 [details]
Patch

[Raymond Toy]

Comment on attachment 114795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114795&action=review

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:140
>> +void AudioDestinationChromium::FIFO::consume(AudioBus* destination, int framesToConsume)
> 
> Can we make "framesToConsume" size_t instead of int and avoid the several static_cast<int> associated with it?
> 
> It seems a bit ugly to have these static_casts -- best to avoid them
> 
> you may need to change a few other places "int" -> "size_t" to make it work

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:166
>> +        bool isCopyGood = (static_cast<int>(part1Length) <= framesToConsume
> 
> This also needs to sanity check "m_readIndex"

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:168
>> +                           && static_cast<int>(part1Length + part2Length) <= framesToConsume);
> 
> See comment about static_cast<int> above

Done.

>> Source/WebKit/chromium/src/AudioDestinationChromium.cpp:222
>> +            bool isCopyGood = (part1Length <= m_providerSize
> 
> Sanity check also needs to factor in m_writeIndex

Done.

[Raymond Toy]

Comment on attachment 114977 [details]
Patch

Ken,

Can you take a look when you get a chance?

[Raymond Toy]

Created attachment 115015 [details]
Patch

[Raymond Toy]

Created attachment 115023 [details]
Patch

[Raymond Toy]

Oops.  The previous patch had a couple of off-by-one errors in the code that was checking for good limits for memcpy.  This is fixed now in this patch.

[Chris Rogers]

Looks fine to me.

[Kenneth Russell]

Comment on attachment 115023 [details]
Patch

Looks good. rs=me

[WebKit Review Bot]

Comment on attachment 115023 [details]
Patch

Clearing flags on attachment: 115023

Committed r100210: <http://trac.webkit.org/changeset/100210>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.