Bug 90398 – Add AudioFIFO class and simplify AudioPullFIFO

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond Toy]

Add AudioPushFIFO class

[Raymond Toy]

Created attachment 150470 [details]
Patch

[Chris Rogers]

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

[Raymond Toy]

Created attachment 150667 [details]
Patch

[Raymond Toy]

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.

[Raymond Toy]

(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

[Chris Rogers]

(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.

[Raymond Toy]

Created attachment 150670 [details]
Patch

[Raymond Toy]

(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.

[Chris Rogers]

Comment on attachment 150670 [details]
Patch

Looks good

[WebKit Review Bot]

Comment on attachment 150670 [details]
Patch

Clearing flags on attachment: 150670

Committed r121810: <http://trac.webkit.org/changeset/121810>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.