-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support WebAudio #4577
base: main
Are you sure you want to change the base?
Support WebAudio #4577
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apart from open comments, lgtm
@@ -191,7 +191,13 @@ void CreateRendererTestService( | |||
|
|||
} // namespace | |||
|
|||
ShellContentRendererClient::ShellContentRendererClient() {} | |||
ShellContentRendererClient::ShellContentRendererClient() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm we should avoid modifying //content, and in any case wouldn't this only work for linux-like platforms? (I.e. not for Android TV). In any case, we could add a ContentRendererClient in //cobalt much like the others: https://source.chromium.org/search?q=%22public%20content::ContentRendererClient%22%20-test&ss=chromium%2Fchromium%2Fsrc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, thank you! CobaltContentRendererClient added in #4661
I'll wait for that to get merged, then rebase here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now moved to impl in CobaltContentRendererClient.
in any case wouldn't this only work for linux-like platforms? (I.e. not for Android TV).
why would this not work for Android? I've been testing on Android.
OutputDeviceInfo StarboardAudioDeviceFactory::GetOutputDeviceInfo( | ||
const blink::LocalFrameToken& frame_token, | ||
const std::string& device_id) { | ||
LOG(ERROR) << "GetOutputDeviceInfo - NOT IMPL"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider to add NOTIMPLEMENTED()
if this is intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for all other not implemented places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
namespace media { | ||
|
||
class MEDIA_EXPORT StarboardAudioDeviceFactory final | ||
: public blink::AudioDeviceFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AsAudioDeviceFactory
is the class belong to blink, we may consider to move starboard_audio_device_factory.{cc.h}
to //cobalt
, instead of putting them under //media
.
Examples like CastAudioDeviceFactory
is under //chromecast.
https://source.chromium.org/chromium/chromium/src/+/main:chromecast/media/audio/cast_audio_device_factory.h;l=22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we leave it named StarboardAudioDeviceFactory or should i rename it CobaltAudioDeviceFactory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, moved and renamed.
#include "media/starboard/starboard_audio_renderer_sink.h" | ||
|
||
#include "base/logging.h" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove new line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
#include "starboard/audio_sink.h" | ||
|
||
#include "base/synchronization/lock.h" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove new line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…board to cobalt/media/audio
Implement SbAudioSink based WebAudioDevice.
b/340599896
Tested on a Kirkwood device by running:
https://github.com/youtube/cobalt/tree/25.lts.1%2B/cobalt/demos/content/web-audio-demo
and
https://webaudiodemos.appspot.com/oscilloscope/index.html