Bug 49557 - Add loadPlatformAudioResource() and decodeAudioFileData() to ChromiumBridge
Summary: Add loadPlatformAudioResource() and decodeAudioFileData() to ChromiumBridge
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-15 12:25 PST by Chris Rogers
Modified: 2010-11-23 10:32 PST (History)
6 users (show)

See Also:


Attachments
Patch (12.21 KB, patch)
2010-11-15 12:48 PST, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (12.27 KB, patch)
2010-11-15 14:25 PST, Chris Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2010-11-15 12:25:15 PST
Add loadPlatformAudioResource() and decodeAudioFileData() to ChromiumBridge
Comment 1 Chris Rogers 2010-11-15 12:48:25 PST
Created attachment 73923 [details]
Patch
Comment 2 WebKit Review Bot 2010-11-15 13:11:33 PST
Attachment 73923 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5967089
Comment 3 Chris Rogers 2010-11-15 14:25:28 PST
Created attachment 73930 [details]
Patch
Comment 4 Dimitri Glazkov (Google) 2010-11-17 12:37:35 PST
Comment on attachment 73930 [details]
Patch

ok.
Comment 5 WebKit Commit Bot 2010-11-17 20:57:37 PST
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 cmarrin@apple.com.  The commit-queue is continuing to process your patch.
Comment 6 WebKit Commit Bot 2010-11-17 21:37:59 PST
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 cmarrin@apple.com, darin@apple.com, hyatt@apple.com, ojan@chromium.org, and pol@apple.com.  The commit-queue is continuing to process your patch.
Comment 7 WebKit Commit Bot 2010-11-17 23:11:29 PST
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 cmarrin@apple.com, darin@apple.com, ojan@chromium.org, pol@apple.com, and simon.fraser@apple.com.  The commit-queue is continuing to process your patch.
Comment 8 WebKit Commit Bot 2010-11-17 23:29:41 PST
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 hyatt@apple.com and simon.fraser@apple.com.  The commit-queue is continuing to process your patch.
Comment 9 WebKit Commit Bot 2010-11-18 03:42:49 PST
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 simon.fraser@apple.com.  The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2010-11-18 03:55:40 PST
Comment on attachment 73930 [details]
Patch

Clearing flags on attachment: 73930

Committed r72281: <http://trac.webkit.org/changeset/72281>
Comment 11 WebKit Commit Bot 2010-11-18 03:55:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Review Bot 2010-11-18 04:47:54 PST
http://trac.webkit.org/changeset/72281 might have broken GTK Linux 64-bit Debug
Comment 13 Darin Fisher (:fishd, Google) 2010-11-23 09:04:24 PST
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.)
Comment 14 Chris Rogers 2010-11-23 10:32:19 PST
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.