Bug 90398

Summary: Add AudioFIFO class and simplify AudioPullFIFO
Product: WebKit Reporter: Raymond Toy <rtoy>
Component: Web AudioAssignee: Raymond Toy <rtoy>
Status: RESOLVED FIXED    
Severity: Normal CC: crogers, eric.carlson, feature-media-reviews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Raymond Toy 2012-07-02 13:41:17 PDT
Add AudioPushFIFO class
Comment 1 Raymond Toy 2012-07-02 13:52:28 PDT
Created attachment 150470 [details]
Patch
Comment 2 Chris Rogers 2012-07-03 13:23:32 PDT
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
Comment 3 Raymond Toy 2012-07-03 14:05:40 PDT
Created attachment 150667 [details]
Patch
Comment 4 Raymond Toy 2012-07-03 14:08:13 PDT
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.
Comment 5 Raymond Toy 2012-07-03 14:09:11 PDT
(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
Comment 6 Chris Rogers 2012-07-03 14:18:27 PDT
(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.
Comment 7 Raymond Toy 2012-07-03 14:33:04 PDT
Created attachment 150670 [details]
Patch
Comment 8 Raymond Toy 2012-07-03 14:34:34 PDT
(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 9 Chris Rogers 2012-07-03 15:52:28 PDT
Comment on attachment 150670 [details]
Patch

Looks good
Comment 10 WebKit Review Bot 2012-07-03 16:41:05 PDT
Comment on attachment 150670 [details]
Patch

Clearing flags on attachment: 150670

Committed r121810: <http://trac.webkit.org/changeset/121810>
Comment 11 WebKit Review Bot 2012-07-03 16:41:10 PDT
All reviewed patches have been landed.  Closing bug.