"PlaybackTime" parameter is not present in AudioProcessingEvent Interface as per W3C spec

[kdj]

As per W3C WebAudio Spec AudioProcessingEvent Interface should have parameters like below:

interface AudioProcessingEvent : Event {

        JavaScriptAudioNode node;
        readonly attribute float playbackTime;
        readonly attribute AudioBuffer inputBuffer;
        readonly attribute AudioBuffer outputBuffer; 

    }

W3C Spec link: http://www.w3.org/TR/2012/WD-webaudio-20120802/

The parameter "playbackTime" is not present in latest WebKit-137862. Also if required, necessary calculations to update the playbackTime parameter need to be implemented in AudioProcessingEvent.

Current Implementation AudioProcessingEvent.idl:
[
    Conditional=WEB_AUDIO,
    JSGenerateToJSObject
] interface AudioProcessingEvent : Event {
    readonly attribute AudioBuffer inputBuffer;
    readonly attribute AudioBuffer outputBuffer; 
};

[Praveen Jadhav]

Hi Chris Rogers,

I am interesting in proposing a patch for this issue. Shall I update?

[Chris Rogers]

(In reply to comment #1)
> Hi Chris Rogers,
> 
> I am interesting in proposing a patch for this issue. Shall I update?

You can give it a try, but it's a bit tricky since you have to account for the latency caused by the buffer size.  It's possible with careful work.

[Praveen Jadhav]

(In reply to comment #2)
> (In reply to comment #1)
> > Hi Chris Rogers,
> > 
> > I am interesting in proposing a patch for this issue. Shall I update?
> 
> You can give it a try, but it's a bit tricky since you have to account for the latency caused by the buffer size.  It's possible with careful work.

Sorry for the typo. It should have been "interested" and not "interesting" in my previous comment.

About the playbackTime, shouldn't the value be just the context's currentTime plus inputBuffer's duration?
duration for a given ScriptProcessorNode object will remain a constant and accounts for the latency(time taken to play 1 buffer) and currentTime of the context provides the time offset.

[Praveen Jadhav]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Hi Chris Rogers,
> > > 
> > > I am interesting in proposing a patch for this issue. Shall I update?
> > 
> > You can give it a try, but it's a bit tricky since you have to account for the latency caused by the buffer size.  It's possible with careful work.
> 
> Sorry for the typo. It should have been "interested" and not "interesting" in my previous comment.
> 
> About the playbackTime, shouldn't the value be just the context's currentTime plus inputBuffer's duration?
> duration for a given ScriptProcessorNode object will remain a constant and accounts for the latency(time taken to play 1 buffer) and currentTime of the context provides the time offset.

I tested with the above mentioned changes which takes care of the input time. But still the latency part is not handled with this as it depends on the nodes that are connected while processing the audio. Its indeed a tricky implementation.:)

[Praveen Jadhav]

Created attachment 191405 [details]
Patch

Patch includes the basic implementation for 'playbackTime' attribute in AudioProcessingEvent. Latency because of buffer size is handled by adding buffer duration to playback time(seems like a crude way though). Tested with basic cases and seems like it is working fine(needs more realtime cases to understand how exactly this will affect).
Delay caused by delayNode is not considered.

[WebKit Review Bot]

Comment on attachment 191405 [details]
Patch

Attachment 191405 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17000012

New failing tests:
webaudio/audioprocessingevent-basic.html
webaudio/javascriptaudionode-zero-input-channels.html

[Praveen Jadhav]

Created attachment 191443 [details]
Patch

Test case updated.

[WebKit Review Bot]

Comment on attachment 191443 [details]
Patch

Attachment 191443 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17009065

New failing tests:
webaudio/audioprocessingevent-basic.html
webaudio/javascriptaudionode-zero-input-channels.html

[Praveen Jadhav]

Created attachment 191618 [details]
Patch

callback function 'process' is changed to 'processAudioData' in audioprocessingevent-basic.html. It seems strange that only chromium-ews fails.

Latency is calculated based on outputBuffer duration rather than inputBuffer's. As a null check is already inplace for outputBuffer for 0 numberOfOutputChannels.

[WebKit Review Bot]

Comment on attachment 191618 [details]
Patch

Attachment 191618 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17081050

New failing tests:
webaudio/audioprocessingevent-basic.html

[Praveen Jadhav]

Hi Kentaro Hara,

Can you please review the patch I have included here?
It seems I am missing something in the new layout test case added here. Maybe "context.oncomplete = finishJSTest"(not sure). But the current approach is working for me. Only chromium-ews fails.

[Kentaro Hara]

Comment on attachment 191618 [details]
Patch

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

> Maybe "context.oncomplete = finishJSTest"(not sure). But the current approach is working for me. Only chromium-ews fails.

What is the failure?

> Source/WebCore/ChangeLog:9
> +        Attribute "playbackTime" in AudioProcessingEvent implemented to pass current audio time
> +        of the context.

Please add the spec link here.

> Source/WebCore/Modules/webaudio/AudioProcessingEvent.idl:29
> +    readonly attribute double playbackTime;

The spec specifies 'float' not 'double'. You need to align the implementation with the spec or propose to change the spec:)

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:252
> -        dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer));
> +        // FIXME: Delay node is not considered while calculating playbackTime. Latency should be retested with more scenarios.
> +        dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer, context()->currentTime() + outputBuffer->duration()));

Why is this change needed?

[Praveen Jadhav]

(In reply to comment #12)
> (From update of attachment 191618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191618&action=review
> 
> > Maybe "context.oncomplete = finishJSTest"(not sure). But the current approach is working for me. Only chromium-ews fails.
> 
> What is the failure?

The test case goes for timeout. The error log link is in the path http://webkit-commit-queue.appspot.com/results/17081050. I couldn't makeout the reason behind this failure. Any comments/suggestions will be appreciated.

> 
> > Source/WebCore/ChangeLog:9
> > +        Attribute "playbackTime" in AudioProcessingEvent implemented to pass current audio time
> > +        of the context.
> 
> Please add the spec link here.

I will take care of this in the next patch.

> 
> > Source/WebCore/Modules/webaudio/AudioProcessingEvent.idl:29
> > +    readonly attribute double playbackTime;
> 
> The spec specifies 'float' not 'double'. You need to align the implementation with the spec or propose to change the spec:)

WebAudio specifications https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#AudioProcessingEvent actually says it is of 'double' type:)

> 
> > Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:252
> > -        dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer));
> > +        // FIXME: Delay node is not considered while calculating playbackTime. Latency should be retested with more scenarios.
> > +        dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer, context()->currentTime() + outputBuffer->duration()));
> 
> Why is this change needed?

The value "context()->currentTime() + outputBuffer->duration()" informs the time at which the inputBuffer will be processed by ScriptProcessorNode.

2 inputBuffers and 2 outputBuffers of equal size based on 'bufferSize' param are alloted when ScriptProcessorNode object is created and are indexed 0 and 1. The size of these inputBuffers/outputBuffers remains a constant throughout. Also, inputBuffer[0] and inputBuffer[1] are processed alternatively(same with outputBuffers). dispatchEvent() to create AudioProcessingEvent object is called periodically and immediately after one buffer is completely processed.

Hence the playbackTime of the next inputBuffer is context's currentTime() plus the total duration of outputBuffer(or inputBuffer) which provides the required offset to handle latency.

[Kentaro Hara]

(In reply to comment #13)
> > What is the failure?
> 
> The test case goes for timeout. The error log link is in the path http://webkit-commit-queue.appspot.com/results/17081050. I couldn't makeout the reason behind this failure. Any comments/suggestions will be appreciated.
> 

hmm, I have no idea...

> > > Source/WebCore/Modules/webaudio/AudioProcessingEvent.idl:29
> > > +    readonly attribute double playbackTime;
> > 
> > The spec specifies 'float' not 'double'. You need to align the implementation with the spec or propose to change the spec:)
> 
> WebAudio specifications https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#AudioProcessingEvent actually says it is of 'double' type:)

Makes sense!

[Praveen Jadhav]

Created attachment 191697 [details]
Patch

context.oncomplete = finishJSTest; included in the newly added test case.

[Kentaro Hara]

Comment on attachment 191697 [details]
Patch

LGTM if it passes the chromium-bot. Please cq+ it after confirming the bot result.

[WebKit Review Bot]

Comment on attachment 191697 [details]
Patch

Attachment 191697 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17003151

New failing tests:
webaudio/audioprocessingevent-basic.html

[Kentaro Hara]

Comment on attachment 191697 [details]
Patch

:(

[Praveen Jadhav]

(In reply to comment #18)
> (From update of attachment 191697 [details])
> :(

I am running out of ideas.. :(

[Kentaro Hara]

Comment on attachment 191697 [details]
Patch

I tested locally and confirmed that the test times out.

I think it's easy to build chromium (especially if you have Linux or Mac).

[Praveen Jadhav]

I tried building chromium on my Linux PC, but for security reasons downloading from http://src.chromium.org is blocked.

I went through all the other working test cases and observed that

if (window.testRunner) {
    testRunner.dumpAsText();
    testRunner.waitUntilDone();
}

window.jsTestIsAsync = true;

is missing in the patch I uploaded. Explaination about this doesn't really relate to the timeout issue I face in chromium-bot.

Is this a mandatory requirement for chromium(especially in async test cases)?

[Kentaro Hara]

(In reply to comment #21)
> I tried building chromium on my Linux PC, but for security reasons downloading from http://src.chromium.org is blocked.
> 
> I went through all the other working test cases and observed that
> 
> if (window.testRunner) {
>     testRunner.dumpAsText();
>     testRunner.waitUntilDone();
> }
> 
> window.jsTestIsAsync = true;
> 
> is missing in the patch I uploaded. Explaination about this doesn't really relate to the timeout issue I face in chromium-bot.
> 
> Is this a mandatory requirement for chromium(especially in async test cases)?

I don't think the behavior is platform-dependent. jsTestIsAsync=true should be coupled with finishJSTest(). waitUntilDone() should be coupled with notifyDone(). Either of the two is enough.

Maybe in chromium the oncomplete handler is not triggered, is it? How about calling finishJSTest() at the end of processAudioData() ?

[Chris Rogers]

Praveen, if we can't get a layout test confirming that the playbackTime is correct, can we create a manual test where the playbackTime is used to play a sin() tone with the phase controlled by the playbackTime.  Then we can use an AudioBufferSourceNode (or an OscillatorNode) playing a sin() tone with the inverse phase to verify that the tones overlay perfectly and cancel each other out (silence).  Does this make sense?  If the playbackTime is not correct, then there will be an audible tone.

I would like to make sure that if we implement the attribute that the value reported is correct.

[Kentaro Hara]

Ah, got it.

Layout tests are running on cr-linux and mac only. And all Web Audio tests are disabled on mac. Thus, this would not be a chromium-specific problem. I guess that your test will fail on all platforms.

[Praveen Jadhav]

Chris, your suggestion makes sense as manual testing can make sure even the implementation is validated.
And as Kentaro Hara mentioned, the layout test I prepared might have an error.

I will retest the implementation and get back to you guys.

[Kentaro Hara]

(In reply to comment #25)
> I will retest the implementation and get back to you guys.

Thanks. Timeout means that finishJSTest() is not called. I'd guess that the oncomplete handler is not called.

[Praveen Jadhav]

Created attachment 192417 [details]
Patch

As discussed, patch has been updated with a manual test case.

Here, SINE wave from  Oscillator node is taken as an input and using playbackTime and buffer duration, inverse sine value is overlapped on the original source. The resultant output is amplified using gain node and then passed to destination. No sound(noise) heard at the output.

[Kentaro Hara]

(In reply to comment #27)
> Created an attachment (id=192417) [details]
> Patch
> 
> As discussed, patch has been updated with a manual test case.

Did you find why you cannot run the test on layout tests? Basically it's not a good idea to put a test into manual tests just because it fails on layout tests for an unknown reason. (I'd like to leave the decision up to crogers@.)

[Praveen Jadhav]

(In reply to comment #28)
> (In reply to comment #27)
> > Created an attachment (id=192417) [details] [details]
> > Patch
> > 
> > As discussed, patch has been updated with a manual test case.
> 
> Did you find why you cannot run the test on layout tests? Basically it's not a good idea to put a test into manual tests just because it fails on layout tests for an unknown reason. (I'd like to leave the decision up to crogers@.)

After making so many trials, I am suspecting shouldBeDefined() which I am usin(In reply to comment #28)
> (In reply to comment #27)
> > Created an attachment (id=192417) [details] [details]
> > Patch
> > 
> > As discussed, patch has been updated with a manual test case.
> 
> Did you find why you cannot run the test on layout tests? Basically it's not a good idea to put a test into manual tests just because it fails on layout tests for an unknown reason. (I'd like to leave the decision up to crogers@.)

I couldn't makeout the reason why LayoutTest failed. I would love to include both manual testcase as well as the layout testcase.

As you suggested in comment 22, I had included finishJSTest() in processAudioData() in my first and second patch. But that failed too. Then I suspected shouldBeDefined(). But that too is working fine in my local test using efl port.

One more possible pitfall was is in using
scriptAudioNode = context.createJavaScriptNode(4096, 1, 1); (learnt while creating manual test case. It went for an infinite loop unexpectedly). Instead of this, if I use scriptAudioNode = context.createJavaScriptNode(4096); with default input and output number of channels, It works fine. However, I can't verify if this is the actual reason for failing in chromium.

[Praveen Jadhav]

(In reply to comment #28)
> (In reply to comment #27)
> > Created an attachment (id=192417) [details] [details]
> > Patch
> > 
> > As discussed, patch has been updated with a manual test case.
> 
> Did you find why you cannot run the test on layout tests? Basically it's not a good idea to put a test into manual tests just because it fails on layout tests for an unknown reason. (I'd like to leave the decision up to crogers@.)

Apparently, I had made a mistake in LayoutTest case uploaded earlier by calling createJavaScriptNode() which is an older implementation. The new JS call, createScriptProcessor() seems to work fine.

Let me know if you want me to add the Layout test case as well. Layout test will check for the existance of all the attributes(basic test) and Manual test will make sure that value playbackTime passed in playback time is proper.

[Kentaro Hara]

(In reply to comment #30)
> Let me know if you want me to add the Layout test case as well. Layout test will check for the existance of all the attributes(basic test) and Manual test will make sure that value playbackTime passed in playback time is proper.

If there is a layout test, it's best:)

[Chris Rogers]

Comment on attachment 192417 [details]
Patch

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

Looks like good progress is being made on the test, but it needs to be refined.  I agree with Kentaro that once we have the manual test properly working (which it isn't yet), then we will want to convert it to a proper layout test.  Please see resources/javascriptaudionode-testing.js and the several layout tests that use it to see how to make a working test for the ScriptProcessorNode.  Please note for this type of test the comment:

// For the current implementation of JavaScriptAudioNode, when it works with OfflineAudioContext (which runs much faster
// than real-time) the event.inputBuffer might be overwrite again before onaudioprocess ever get chance to be called.
// We carefully arrange the renderLengthInFrames and bufferSize to have exactly the same value to avoid this issue.

This means that the offline context must render for a duration which is only one or two times the buffer-size used in the ScriptProcessorNode (we only want the processAudioData() function to be called one time)

> ManualTests/webaudio/audioprocessingevent-playback-manual.html:7
> +        var scriptAudioNode = context.createScriptProcessor(4096);

In this case, we might as well create this node with 1 input channel and 1 output channel, since then we don't have to worry about left/right in processAudioData()

> ManualTests/webaudio/audioprocessingevent-playback-manual.html:13
> +        var frequency = 25;

This frequency is a bit low -- I suggest 100

> ManualTests/webaudio/audioprocessingevent-playback-manual.html:14
> +        var duration = 5000;

duration should be a value in seconds (5) and not use setTimeout() as I mention below

> ManualTests/webaudio/audioprocessingevent-playback-manual.html:18
> +            var inputArrayR = event.inputBuffer.getChannelData(1);

If the ScriptProcessorNode has 1 input channel and 1 output channel then we don't need to worry about left/right

> ManualTests/webaudio/audioprocessingevent-playback-manual.html:23
> +            var phase = (2 * Math.PI) * inputArrayL.duration * frequency;

inputArrayL.duration is not defined!  So phase ends up being NaN.  So this test is not really correctly testing silence, but is just rendering NaN
(which happens to kind of sound like silence :)  Once the comments below are fixed, be SUSPICIOUS and run the test with the "offset" purposefully moved to 
incorrect values to validate that a correct and pure sine wave is heard if not completely cancelled by inverse phase.

  The "phase" variable is not really correctly named and it depends on the input buffer length.
We'd like this test to work correctly and render a sine wave for multiple buffers, so we need to keep the "playbackIndex" (what you're currently tracking as "i") as state between calls to processAudioData(). 
Otherwise, as the code currently is, it will not render a continuous sine wave that has continuity between calls to processAudioData().   Instead it currently will render glitches at between buffers -- it's not seamless.

You'll want something like:

for (var i = 0; i < n; ++i, ++renderIndex) {
    outputArray[i] = inputArray[i] - Math.sin(offset + (playbackIndex * phaseIncrement));
}

where the "playbackIndex" variable is initialized to 0 outside the scope of processAudioData()
and "phaseIncrement" is calculated based on sample-rate and the frequency

> ManualTests/webaudio/audioprocessingevent-playback-manual.html:37
> +            oscillator.noteOn(0);

please don't use noteOn() - the newer name is start()

> ManualTests/webaudio/audioprocessingevent-playback-manual.html:38
> +            setTimeout(function(){ oscillator.disconnect(); }, duration);

please don't use setTimeout()!  Instead use stop().  Overall, you can have something like:

            var startTime = context.currentTime;
            oscillator.start(startTime);
            oscillator.stop(startTime + duration);

> ManualTests/webaudio/audioprocessingevent-playback-manual.html:51
> +            setTimeout(function(){ scriptAudioNode.onaudioprocess = null; scriptAudioNode.disconnect(); oscillator.disconnect(); }, duration);

ditto, no setTimeout()

[Praveen Jadhav]

Created attachment 193241 [details]
WIP

Good thing to have pointed out "inputArrayL.duration" not defined. I did some more trials with the test case and have updated the comments.

I couldn't verify the test with OfflineAudioContext because of audio rendering and hence continued with normal AudioContext. However, I have made provision to cyclically increase bufferSize of scriptProcessorNode with each test. i.e., scriptProcessorNode will be created with bufferSize of 256, 512, 1024,..., 16384, 256, 512..etc so that we can verify for all bufferSize values.

Glitch happens with this test at lower bufferSize(256 and 512) values and as the size increases, there is not noticable glitch problem. Also, In the current testcase, audio glitch is noticable when switching from first callback to second in all cases since outputBuffer will be filled with 0s before that.

I have replaced noteOn() with start() but not using stop() as I need a callback function to reset onaudioprocess to null at the end of 'duration'(else, audio continues to play data generated by Math.sin() function).

I have included a layout test case to validate attributes of AudioProcessingEvent as well. In this case, I am using createScriptProcessor() instead of createJavaScriptNode() to correct Timeout error observed in earlier patches. Not sure if we can include the changes implemented in manual test case to layout test as this shouldn't be automated.


Finally, about the logic, m_playbackTime variable is introduced and its value is set before calling callOnMainThread()(to avoid glitch issue between buffers). The output of the overlapping SINE waves is not exactly silent always, but varies with each test from 0 to 2 radians(~116 degrees) for 100Hz frequency and outputBuffer always lagged behind inputBuffer. This works out be to a delay of upto 3ms than the calculated value in m_playbackTime. I tried with below logic to account for the delay:

           m_playbackTime = context()->currentTime() + outputBuffer->duration() - (framesToProcess / context()->sampleRate());

but the phase variation changed for each test. Any inputs for this?

[WebKit Review Bot]

Comment on attachment 193241 [details]
WIP

Attachment 193241 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17218097

New failing tests:
webaudio/audioprocessingevent-basic.html

[Praveen Jadhav]

Created attachment 193280 [details]
Patch

Patch updated with layout test.

[WebKit Review Bot]

Comment on attachment 193280 [details]
Patch

Attachment 193280 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17074653

New failing tests:
webaudio/audioprocessingevent-basic.html
fast/js/dfg-inline-resolve.html

[Soo-Hyun Choi]

FYI - changed the assignee to Praveen Jadhav <[e-mail]>.

[Praveen Jadhav]

Created attachment 193436 [details]
Patch

[Praveen Jadhav]

Created attachment 194741 [details]
Patch

Manual test case removed and layout test case updated.

[Chris Dumez]

Comment on attachment 194741 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Attribute "playbackTime" in AudioProcessingEvent implemented to pass current audio time

According to the spec, playbackTime is not necessarily the current audio time of the context. It is the time when the audio will be played, in the same time coordinate system as AudioContext.currentTime.

> Source/WebCore/Modules/webaudio/AudioProcessingEvent.cpp:41
> +PassRefPtr<AudioProcessingEvent> AudioProcessingEvent::create(PassRefPtr<AudioBuffer> inputBuffer, PassRefPtr<AudioBuffer> outputBuffer, double time)

"playbackTime"?

> Source/WebCore/Modules/webaudio/AudioProcessingEvent.cpp:46
>  AudioProcessingEvent::AudioProcessingEvent()

We should probably initialize m_playbackTime to 0 here to be safe.

> Source/WebCore/Modules/webaudio/AudioProcessingEvent.cpp:50
> +AudioProcessingEvent::AudioProcessingEvent(PassRefPtr<AudioBuffer> inputBuffer, PassRefPtr<AudioBuffer> outputBuffer, double time)

"playbackTime"?

> Source/WebCore/Modules/webaudio/AudioProcessingEvent.h:40
> +    static PassRefPtr<AudioProcessingEvent> create(PassRefPtr<AudioBuffer> inputBuffer, PassRefPtr<AudioBuffer> outputBuffer, double time);

"playbackTime"?

> Source/WebCore/Modules/webaudio/AudioProcessingEvent.h:46
> +    double playbackTime() { return m_playbackTime; }

Getter could be const.

> Source/WebCore/Modules/webaudio/AudioProcessingEvent.h:52
> +    AudioProcessingEvent(PassRefPtr<AudioBuffer> inputBuffer, PassRefPtr<AudioBuffer> outputBuffer, double time);

"playbackTime"?

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:200
> +        m_playbackTime = (m_playbackTime > 0) ? m_playbackTime : 0;

m_playbackTime = std::max(0., context()->currentTime() - outputBuffer->duration()); // might be clearer.

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.h:111
> +    // For AudioProcessingEvent

Not really needed. In any case, should end with a period.

> LayoutTests/webaudio/audioprocessingevent.html:38
> +    for (var i = 0; i < n; ++i) {

Curly brackets can be omitted.

> LayoutTests/webaudio/audioprocessingevent.html:43
> +        if (outputArray[i].toFixed(3) != 0.000)

shouldBeCloseTo() from js-test-pre.js may do what you want and will print out something if the the check passes, not only if it fails.

[Praveen Jadhav]

Created attachment 196446 [details]
Patch

Updated patch attached.

[Praveen Jadhav]

(In reply to comment #40)
> (From update of attachment 194741 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194741&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Attribute "playbackTime" in AudioProcessingEvent implemented to pass current audio time
> 
> According to the spec, playbackTime is not necessarily the current audio time of the context. It is the time when the audio will be played, in the same time coordinate system as AudioContext.currentTime.
> 
> > Source/WebCore/Modules/webaudio/AudioProcessingEvent.cpp:41
> > +PassRefPtr<AudioProcessingEvent> AudioProcessingEvent::create(PassRefPtr<AudioBuffer> inputBuffer, PassRefPtr<AudioBuffer> outputBuffer, double time)
> 
> "playbackTime"?
> 
> > Source/WebCore/Modules/webaudio/AudioProcessingEvent.cpp:46
> >  AudioProcessingEvent::AudioProcessingEvent()
> 
> We should probably initialize m_playbackTime to 0 here to be safe.
> 
> > Source/WebCore/Modules/webaudio/AudioProcessingEvent.cpp:50
> > +AudioProcessingEvent::AudioProcessingEvent(PassRefPtr<AudioBuffer> inputBuffer, PassRefPtr<AudioBuffer> outputBuffer, double time)
> 
> "playbackTime"?
> 
> > Source/WebCore/Modules/webaudio/AudioProcessingEvent.h:40
> > +    static PassRefPtr<AudioProcessingEvent> create(PassRefPtr<AudioBuffer> inputBuffer, PassRefPtr<AudioBuffer> outputBuffer, double time);
> 
> "playbackTime"?
> 
> > Source/WebCore/Modules/webaudio/AudioProcessingEvent.h:46
> > +    double playbackTime() { return m_playbackTime; }
> 
> Getter could be const.
> 
> > Source/WebCore/Modules/webaudio/AudioProcessingEvent.h:52
> > +    AudioProcessingEvent(PassRefPtr<AudioBuffer> inputBuffer, PassRefPtr<AudioBuffer> outputBuffer, double time);
> 
> "playbackTime"?
> 
> > Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:200
> > +        m_playbackTime = (m_playbackTime > 0) ? m_playbackTime : 0;
> 
> m_playbackTime = std::max(0., context()->currentTime() - outputBuffer->duration()); // might be clearer.
> 
> > Source/WebCore/Modules/webaudio/ScriptProcessorNode.h:111
> > +    // For AudioProcessingEvent
> 
> Not really needed. In any case, should end with a period.
> 
> > LayoutTests/webaudio/audioprocessingevent.html:38
> > +    for (var i = 0; i < n; ++i) {
> 
> Curly brackets can be omitted.
> 
> > LayoutTests/webaudio/audioprocessingevent.html:43
> > +        if (outputArray[i].toFixed(3) != 0.000)
> 
> shouldBeCloseTo() from js-test-pre.js may do what you want and will print out something if the the check passes, not only if it fails.

All your comments are included in the latest patch.

[Build Bot]

Comment on attachment 196446 [details]
Patch

Attachment 196446 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17453269

[Praveen Jadhav]

Created attachment 197229 [details]
Patch

Fourth param of ShouldBeCloseTo() is set to true since a huge test expected file is required if it is not done so.

[Andreas Kling]

Comment on attachment 197229 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Praveen Jadhav]

Created attachment 229040 [details]
Patch

Patch based on Blink source change https://codereview.chromium.org/210973002

[Eric Carlson]

Comment on attachment 229040 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Attribute "playbackTime" in AudioProcessingEvent implemented to pass playback time of
> +        audiobuffer associated with ScriptProcessorNode of the context.

Please reference the Blink bug this is based on.

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:261
> +        double playbackTime = (context()->currentSampleFrame() + m_bufferSize) /static_cast<double>(context()->sampleRate());

Nit: missing space after "/"

[Praveen Jadhav]

Created attachment 229099 [details]
Patch

Patch updated.

[Jer Noble]

Comment on attachment 229099 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 229099 [details]
Patch

Clearing flags on attachment: 229099

Committed r167131: <http://trac.webkit.org/changeset/167131>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Unfortunately, script processor nodes are somewhat broken currently (see bug 112521), so I had to add a TestExpectation for the test added here, effectively disabling it.