Bug 49557 – Add loadPlatformAudioResource() and decodeAudioFileData() to ChromiumBridge

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

Add loadPlatformAudioResource() and decodeAudioFileData() to ChromiumBridge

[Chris Rogers]

Created attachment 73923 [details]
Patch

[WebKit Review Bot]

Attachment 73923 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5967089

[Chris Rogers]

Created attachment 73930 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 73930 [details]
Patch

ok.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 73930 [details]:

animations/play-state-suspend.html
animations/suspend-resume-animation.html

Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 73930 [details]:

fast/blockflow/vertical-font-fallback.html
animations/combo-transform-translate+scale.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 73930 [details]:

compositing/iframes/overlapped-nested-iframes.html
animations/combo-transform-translate+scale.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 73930 [details]:

compositing/iframes/overlapped-nested-iframes.html
fast/blockflow/vertical-font-fallback.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 73930 [details]:

compositing/iframes/overlapped-nested-iframes.html

Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 73930 [details]
Patch

Clearing flags on attachment: 73930

Committed r72281: <http://trac.webkit.org/changeset/72281>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/72281 might have broken GTK Linux 64-bit Debug

[Darin Fisher (:fishd, Google)]

Comment on attachment 73930 [details]
Patch

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

> WebKit/chromium/public/WebAudioBus.h:46
> +    ~WebAudioBus();

so any public method of a WebKit API class that is not inline needs to be decorated with WEBKIT_API.
otherwise, it will break the Chromium multi-DLL build if Chromium code tries to call one of these
methods.

now, stepping back... i don't think you actually intend for Chromium to call the destructor or the
initialize method.  this suggests that they should be hidden from Chromium, so that Chromium cannot
possibly call them.

since your goal is to pass data to Chromium in the scope of a decodeAudioFileData call, I suspect
that you would be better served defining WebAudioBus as an interface, like WebVideoFrame.  then,
in chromium/src/, just define a WebAudioBusImpl class that implements the interface and provides
methods for initialization and releasing the contained AudioBus object.

> WebKit/chromium/public/WebAudioBus.h:63
> +    WebAudioBus(const WebAudioBus& d) : m_private(0) { }

you also want to implement an assignment operator, and it is not necessary
to actually implement this copy constructor since you never intend for anyone
to call it.  you basically want the equivalent of DISALLOW_COPY_AND_ASSIGN
from base/basictypes.h in the Chromium code base.

> WebKit/chromium/public/WebKitClient.h:226
> +    virtual bool decodeAudioFileData(WebAudioBus* destinationBus, const char* audioFileData, size_t dataSize, double sampleRate) { return false; }

since audioFileData is intended to refer to an in-memory audio file, it would be more
consistent with API terminology to not call it a file.  instead, call it a resource.

then, i would suggest naming this method loadAudioResource, which matches up with
loadResource.  (i don't think it is helpful to include the name "decode" in the
function since there is no way to fetch the undecoded form.)

[Chris Rogers]

Comment on attachment 73930 [details]
Patch

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

>> WebKit/chromium/public/WebAudioBus.h:46
>> +    ~WebAudioBus();
> 
> so any public method of a WebKit API class that is not inline needs to be decorated with WEBKIT_API.
> otherwise, it will break the Chromium multi-DLL build if Chromium code tries to call one of these
> methods.
> 
> now, stepping back... i don't think you actually intend for Chromium to call the destructor or the
> initialize method.  this suggests that they should be hidden from Chromium, so that Chromium cannot
> possibly call them.
> 
> since your goal is to pass data to Chromium in the scope of a decodeAudioFileData call, I suspect
> that you would be better served defining WebAudioBus as an interface, like WebVideoFrame.  then,
> in chromium/src/, just define a WebAudioBusImpl class that implements the interface and provides
> methods for initialization and releasing the contained AudioBus object.

Actually, the initialize() method is called from within Chromium.  This is how Chromium passes the decoded PCM audio data back to WebKit.
But, the destructor is not called in Chromium.

If you believe that adding the WEBKIT_API prefix to the destructor and initialize() method is not a good solution, then I'll have a look at the WebAudioBusImpl as you suggest.