Add AudioPushFIFO class
Created attachment 150470 [details] Patch
Comment on attachment 150470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150470&action=review Looks pretty good - I assume you've tested this in the chromium port (which is the only port using it right now)? > Source/WebCore/ChangeLog:8 > + No new tests; existing tests cover changes. Just remove this line, because this statement isn't true. Or say: No new tests. This code will be used in audio back-end implementation. > Source/WebCore/WebCore.gypi:3265 > + 'platform/audio/AudioPushFIFO.h', This file is not needed > Source/WebCore/platform/audio/AudioFIFO.h:38 > + // Create a FIFO large enough to hold |fifoLength| frames of data of |nubmerOfChannels| channels. typo: nubmerOfChannels > Source/WebCore/platform/audio/AudioFIFO.h:42 > + void push(const AudioBus* source); nit: WebKit-style - can remove "source" > Source/WebCore/platform/audio/AudioFIFO.h:46 > + void pull(AudioBus* destination, size_t framesToConsume); This method is exactly the same as AudioPullFIFO::consume() We should have consistent naming, so either call this consume() or rename the other one pull() > Source/WebCore/platform/audio/AudioPushFIFO.h:37 > + // Just inherit everything from AudioFIFO. It looks like it's not worth creating the class AudioPushFIFO if it's exactly the same as AudioFIFO :) We can just use an AudioFIFO object directly, so no need for this file
Created attachment 150667 [details] Patch
Comment on attachment 150470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150470&action=review >> Source/WebCore/ChangeLog:8 >> + No new tests; existing tests cover changes. > > Just remove this line, because this statement isn't true. Or say: > > No new tests. This code will be used in audio back-end implementation. Actually since AudioPullFIFO uses AudioFIFO, AudioFIFO will get tested. >> Source/WebCore/WebCore.gypi:3265 >> + 'platform/audio/AudioPushFIFO.h', > > This file is not needed Removed. >> Source/WebCore/platform/audio/AudioFIFO.h:38 >> + // Create a FIFO large enough to hold |fifoLength| frames of data of |nubmerOfChannels| channels. > > typo: nubmerOfChannels Fixed. >> Source/WebCore/platform/audio/AudioFIFO.h:42 >> + void push(const AudioBus* source); > > nit: WebKit-style - can remove "source" Fixed. >> Source/WebCore/platform/audio/AudioFIFO.h:46 >> + void pull(AudioBus* destination, size_t framesToConsume); > > This method is exactly the same as AudioPullFIFO::consume() > > We should have consistent naming, so either call this consume() or rename the other one pull() Renamed to consume(). >> Source/WebCore/platform/audio/AudioPushFIFO.h:37 >> + // Just inherit everything from AudioFIFO. > > It looks like it's not worth creating the class AudioPushFIFO if it's exactly the same as AudioFIFO :) > We can just use an AudioFIFO object directly, so no need for this file Ok. I wasn't sure if you still wanted an AudioPushFIFO class or not. File removed.
(In reply to comment #2) > (From update of attachment 150470 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150470&action=review > > Looks pretty good - I assume you've tested this in the chromium port (which is the only port using it right now)? Yes, I ran the layout tests and a bunch of demos with chromium. > > > Source/WebCore/ChangeLog:8 > > + No new tests; existing tests cover changes. > > Just remove this line, because this statement isn't true. Or say: > > No new tests. This code will be used in audio back-end implementation. > > > Source/WebCore/WebCore.gypi:3265 > > + 'platform/audio/AudioPushFIFO.h', > > This file is not needed > > > Source/WebCore/platform/audio/AudioFIFO.h:38 > > + // Create a FIFO large enough to hold |fifoLength| frames of data of |nubmerOfChannels| channels. > > typo: nubmerOfChannels > > > Source/WebCore/platform/audio/AudioFIFO.h:42 > > + void push(const AudioBus* source); > > nit: WebKit-style - can remove "source" > > > Source/WebCore/platform/audio/AudioFIFO.h:46 > > + void pull(AudioBus* destination, size_t framesToConsume); > > This method is exactly the same as AudioPullFIFO::consume() > > We should have consistent naming, so either call this consume() or rename the other one pull() > > > Source/WebCore/platform/audio/AudioPushFIFO.h:37 > > + // Just inherit everything from AudioFIFO. > > It looks like it's not worth creating the class AudioPushFIFO if it's exactly the same as AudioFIFO :) > We can just use an AudioFIFO object directly, so no need for this file
(In reply to comment #4) > (From update of attachment 150470 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150470&action=review > > >> Source/WebCore/ChangeLog:8 > >> + No new tests; existing tests cover changes. > > > > Just remove this line, because this statement isn't true. Or say: > > > > No new tests. This code will be used in audio back-end implementation. > > Actually since AudioPullFIFO uses AudioFIFO, AudioFIFO will get tested. Are you sure - which of our tests calls into AudioFIFO code? Looks good otherwise.
Created attachment 150670 [details] Patch
(In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 150470 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=150470&action=review > > > > >> Source/WebCore/ChangeLog:8 > > >> + No new tests; existing tests cover changes. > > > > > > Just remove this line, because this statement isn't true. Or say: > > > > > > No new tests. This code will be used in audio back-end implementation. > > > > Actually since AudioPullFIFO uses AudioFIFO, AudioFIFO will get tested. > > Are you sure - which of our tests calls into AudioFIFO code? I was thinking of the demos. The layout tests don't use the fifo. New patch uploaded, correcting the ChangeLog comment.
Comment on attachment 150670 [details] Patch Looks good
Comment on attachment 150670 [details] Patch Clearing flags on attachment: 150670 Committed r121810: <http://trac.webkit.org/changeset/121810>
All reviewed patches have been landed. Closing bug.