Bug 167656 – [Mac] Update CARingBuffer class

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Update class to be able to copy or mix input, and to allocate storage from the information in a stream description.

[Eric Carlson]

Created attachment 300248 [details]
Proposed patch

[WebKit Commit Bot]

Attachment 300248 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/audio/mac/CARingBuffer.h:58:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Comment on attachment 300248 [details]
Proposed patch

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

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:140
> +        case CARingBuffer::Mix:
> +            float* destination = static_cast<float*>(dest->mData);
> +            vadd(destination, 1, reinterpret_cast<float*>(*buffers + srcOffset), 1, destination, 1, std::min<size_t>(nbytes, dest->mDataByteSize - destOffset));
> +            break;

This doesn't feel right. A float is not the same size as a byte, so I feel like this is going to overrun outside the bounds of the passed in list. (Well, the std::min will keep that from happening, but it'll still not behave correctly.)

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:163
> +    ZeroRange(static_cast<Byte**>(m_buffers->data()), m_channelCount, 0, m_capacityBytes);

You shouldn't have to zero anything here. Rather, just update the bounds below.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:184
> +    uint64_t start, end;
> +    getCurrentFrameBounds(start, end);
> +    if (start == end)
> +        setCurrentFrameBounds(startFrame, startFrame);
> +

What's this for?

[Eric Carlson]

Comment on attachment 300248 [details]
Proposed patch

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

>> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:140
>> +            break;
> 
> This doesn't feel right. A float is not the same size as a byte, so I feel like this is going to overrun outside the bounds of the passed in list. (Well, the std::min will keep that from happening, but it'll still not behave correctly.)

Good point. While we the upcoming patches only use it with floats, we might as well support other the formats.

>> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:163
>> +    ZeroRange(static_cast<Byte**>(m_buffers->data()), m_channelCount, 0, m_capacityBytes);
> 
> You shouldn't have to zero anything here. Rather, just update the bounds below.

Fixed.

>> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:184
>> +
> 
> What's this for?

Oops, leftover testing.

[Eric Carlson]

Created attachment 300359 [details]
Updated patch, with API test.

[WebKit Commit Bot]

Attachment 300359 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/audio/mac/CARingBuffer.h:59:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Comment on attachment 300359 [details]
Updated patch, with API test.

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

r=me, with nit

> LayoutTests/webaudio/mediastreamaudiosourcenode.html:32
> -                stream.getAudioTracks()[0].stop();
> +//                stream.getAudioTracks()[0].stop();

We probably still need this. :)

[Eric Carlson]

Committed r211539 and r211540: https://trac.webkit.org/r211539, https://trac.webkit.org/r211540

[Radar WebKit Bug Importer]

<rdar://problem/30318329>

[Eric Carlson]

Plus https://trac.webkit.org/r211545 to fix the 32-bit build.