Bug 70650 – Add chromium bridging for MediaPlayer::setAudioSource() method so we can know audio stream format

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add chromium bridging for MediaPlayer::setAudioSource() method so we can know audio stream format

[Chris Rogers]

Created attachment 112022 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 112022 [details]
Patch

Attachment 112022 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10178982

[Chris Rogers]

Please see details of audio-related WebCore changes in:
https://bugs.webkit.org/show_bug.cgi?id=70155

which describes the WebCore-side changes necessary to get the audio stream format.  That patch must land before this one.

This patch provides the appropriate bridging/wrappers to plumb this through into the chromium implementation.

[Chris Rogers]

Darin, could you please have a look?  The builders are currently *red* because the associated WebCore patch has not yet landed.  But, hopefully it should still be possible to make sense of this patch in relation to the other one.  Thanks!

[Darin Fisher (:fishd, Google)]

Comment on attachment 112022 [details]
Patch

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

> Source/WebKit/chromium/public/WebAudioSourceProvider.h:35
> +    class Client {

nit: we usually go with top-level classes for clients.  that helps so that
you can just include WebAudioSourceProviderClient.h in header files where
this interface is implemented instead of having to import WebAudioSourceProvider
as well.  it also enables you to forward declare the type.

> Source/WebKit/chromium/public/WebMediaPlayer.h:34
> +#include "WebAudioSourceProvider.h"

this is a good example where it would be nice to forward declare the Client interface instead.

> Source/WebKit/chromium/public/WebMediaPlayer.h:164
> +    virtual void setAudioSource(WebKit::WebAudioSourceProvider::Client*) { }

given the name of the function, it looks like WebKit::WebAudioSourceProvider::Client could be referred
to as an audio source.  perhaps instead of WebAudioSourceProviderClient, we should just have
WebAudioSource?  otherwise, setAudioSource should probably be setAudioSourceProviderClient.  i'd
also consider making this be a setClient call on WebAudioSourceProvider.

[Chris Rogers]

Created attachment 112286 [details]
Patch

[Chris Rogers]

Comment on attachment 112022 [details]
Patch

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

>> Source/WebKit/chromium/public/WebAudioSourceProvider.h:35
>> +    class Client {
> 
> nit: we usually go with top-level classes for clients.  that helps so that
> you can just include WebAudioSourceProviderClient.h in header files where
> this interface is implemented instead of having to import WebAudioSourceProvider
> as well.  it also enables you to forward declare the type.

FIXED: I've split out WebAudioSourceProvider and WebAudioSourceProviderClient into separate header files.

>> Source/WebKit/chromium/public/WebMediaPlayer.h:34
>> +#include "WebAudioSourceProvider.h"
> 
> this is a good example where it would be nice to forward declare the Client interface instead.

FIXED: there are now no changes to this file

>> Source/WebKit/chromium/public/WebMediaPlayer.h:164

> 
> given the name of the function, it looks like WebKit::WebAudioSourceProvider::Client could be referred
> to as an audio source.  perhaps instead of WebAudioSourceProviderClient, we should just have
> WebAudioSource?  otherwise, setAudioSource should probably be setAudioSourceProviderClient.  i'd
> also consider making this be a setClient call on WebAudioSourceProvider.

Darin, thanks for taking the time to talk me through this.  As you've suggested, I've gotten rid of a direct setAudioSource() method on MediaPlayer / MediaPlayerPrivate / WebMediaPlayer, etc.
and now have instead added a AudioSourceProvider::setClient() method.

Hopefully, this will now be slightly easier to understand.

I've also uploaded a new patch for the corresponding WebCore files:
https://bugs.webkit.org/show_bug.cgi?id=70155

[WebKit Review Bot]

Comment on attachment 112286 [details]
Patch

Attachment 112286 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10212031

[Darin Fisher (:fishd, Google)]

Comment on attachment 112286 [details]
Patch

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

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:673
> +    m_webAudioSourceProvider = provider;

before replacing m_webAudioSourceProvider, should we perhaps null out the client
of the existing provider?

  if (m_webAudioSourceProvider)
      m_webAudioSourceProvider->setClient(0);
  m_webAudioSourceProvider = provider;
  if (m_webAudioSourceProvider)
      m_webAudioSourceProvider->setClient(&m_client);

This way we don't have to worry about the old provider holding a pointer back to m_client.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:678
> +void WebMediaPlayerClientImpl::AudioSourceProviderImpl::setClient(WebCore::AudioSourceProviderClient* client)

nit: it looks like you can drop the WebCore:: prefixes in this file.

R=me otherwise.

[Chris Rogers]

Committed r98514: <http://trac.webkit.org/changeset/98514>