Summary: | [Mac] Update CARingBuffer class | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, dino, jer.noble, thiago.lacerda, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Other | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Eric Carlson
2017-01-31 11:47:20 PST
Created attachment 300248 [details]
Proposed patch
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.
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? 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. Created attachment 300359 [details]
Updated patch, with API test.
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.
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. :) Committed r211539 and r211540: https://trac.webkit.org/r211539, https://trac.webkit.org/r211540 Plus https://trac.webkit.org/r211545 to fix the 32-bit build. |