Bug 84639 - Re-factor scheduling logic from AudioBufferSourceNode into AudioScheduledSourceNode
Summary: Re-factor scheduling logic from AudioBufferSourceNode into AudioScheduledSour...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Rogers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-23 15:07 PDT by Chris Rogers
Modified: 2012-04-27 13:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (27.58 KB, patch)
2012-04-23 15:17 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (27.55 KB, patch)
2012-04-23 16:24 PDT, Chris Rogers
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2012-04-23 15:07:17 PDT
Re-factor scheduling logic from AudioBufferSourceNode into AudioScheduledSourceNode
Comment 1 Chris Rogers 2012-04-23 15:17:59 PDT
Created attachment 138434 [details]
Patch
Comment 2 Build Bot 2012-04-23 15:46:40 PDT
Comment on attachment 138434 [details]
Patch

Attachment 138434 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12530041
Comment 3 Chris Rogers 2012-04-23 16:24:37 PDT
Created attachment 138453 [details]
Patch
Comment 4 Chris Rogers 2012-04-23 16:25:34 PDT
Fixed simple compile error from first patch caused by last minute cleanup.
Comment 5 Chris Rogers 2012-04-25 15:21:59 PDT
Hi Eric and Jer, would one of you be able to look over this patch?  It's not as bad as it looks since it's essentially just cutting some code from AudioBufferSourceNode and putting it into a new base-class.  Thanks!
Comment 6 Jer Noble 2012-04-26 13:25:57 PDT
Comment on attachment 138453 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.h:-85
> -    void noteOn(double when);
>      void noteGrainOn(double when, double grainOffset, double grainDuration);
> -    void noteOff(double when);

Why wasn't noteGrainOn() included in this patch?  It appears that updateSchedulingInfo() does much of the same calculations as noteGrainOn().  Is there additional refactoring which could be done here?  It seems that it's concepts (i.e. mucking around with the m_playbackState variable) belong in AudioScheduledSourceNode and not in AudioBufferSourceNode.

I would suggest adding a pure virtual "double maxGrainDuration() const = 0;" function in AudioScheduledSourceNode.h, overridden by AudioBufferSourceNode as "buffer()->duration()", and use the result of that function to do the necessary calculations in noteGrainOn().
Comment 7 Chris Rogers 2012-04-26 14:47:57 PDT
(In reply to comment #6)
> (From update of attachment 138453 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138453&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.h:-85
> > -    void noteOn(double when);
> >      void noteGrainOn(double when, double grainOffset, double grainDuration);
> > -    void noteOff(double when);
> 
> Why wasn't noteGrainOn() included in this patch?  It appears that updateSchedulingInfo() does much of the same calculations as noteGrainOn().  Is there additional refactoring which could be done here?  It seems that it's concepts (i.e. mucking around with the m_playbackState variable) belong in AudioScheduledSourceNode and not in AudioBufferSourceNode.
> 
> I would suggest adding a pure virtual "double maxGrainDuration() const = 0;" function in AudioScheduledSourceNode.h, overridden by AudioBufferSourceNode as "buffer()->duration()", and use the result of that function to do the necessary calculations in noteGrainOn().

Hi Jer, the reason is because noteGrainOn() is a concept specific to AudioBufferSourceNodes, since it accesses parts of an AudioBuffer.  The Oscillator AudioSourceNode does not have this concept of noteGrainOn(), but only noteOn() and noteOff() for basic starting/stopping playback, and going through specific playback states.
Comment 8 Chris Rogers 2012-04-27 13:27:00 PDT
Committed r115485: <http://trac.webkit.org/changeset/115485>
Comment 9 Chris Rogers 2012-04-27 13:27:37 PDT
Thanks Eric!