I have a WIP patch ready for discussion and bikeshedding.
Created attachment 390523 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
With this patch several things would be possible: - internal audio mixing in the WebProcess, meaning that the sound server would have a single connection with the WebProcess, thus avoiding UI inconsistencies in the GNOME settings - audio off-loading to the UIProcess. For this some changes would be needed in WPEBackend-FDO, WIP branch there: https://github.com/Igalia/WPEBackend-fdo/tree/audio and this would allow GstWPE to receive audio and inject it back into the pipeline. WIP branch there: https://gitlab.freedesktop.org/philn/gst-plugins-bad/commits/wpe-audio
Another idea would be to mix in the WebProcess and off-load to the UIProcess. This isn't supported but could perhaps be useful for WebKitGTK-based browsers. Showing the app name in the GNOME settings then.
Comment on attachment 390523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390523&action=review I know you didn't mark r? but considering a so big patch I thought it could be a good idea to pre-review this. > Source/WebCore/platform/graphics/MediaPlayerEnums.h:119 > +WTF::String convertEnumerationToString(MediaPlayerEnums::AudioRenderingPolicy); Do you need the WTF::? > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:36 > + const char* environmentVariable = getenv("WEBKIT_GST_USE_AUDIOMIXER"); g_getenv? > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:52 > + m_pipeline = adoptGRef(GST_ELEMENT_CAST(g_object_ref_sink(gst_element_factory_make("pipeline", "webkitaudiomixer")))); I think you can avoid the ref_sink, the ELEMENT_CAST and the adopt and let the the operator= do the ref_sink for you, as you do below for the audiomixer. > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:55 > + GRefPtr<GstClock> clock = gst_system_clock_obtain(); This is transfer full, you need to adopt. > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:103 > + GstElement* src = gst_element_factory_make("proxysrc", nullptr); I think we should ASSERT or RELEASE_ASSERT on src being GST_IS_BIN or we'll have trouble later. > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:111 > + if (factory && !strcmp(GST_OBJECT_NAME(factory), "queue")) g_strcmp0 > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:131 > + return WTFMove(mixerPad); No need to move here, there is copy elision. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1573 > + if (!audioSink) > + return nullptr; Isn't this a disaster? Shouldn't we GST_ERROR and ASSERT or RELEASE_ASSERT on this? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3002 > + GRefPtr<GstClock> clock = gst_system_clock_obtain(); adopt > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:47 > + gboolean enableInternalMixing; I think this should be bool > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:50 > + int m_streamId; I think you can remove the m_, current trend. Personal thought, considering these are attributes of a GObject, if we combine this with our coding style I think we should add m_ to all attributes. But obviously not now and not without a discussion. > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:83 > + GST_ERROR_OBJECT(sink, "Failed to create proxysink"); We can ASSERT_NOT_REACHED as well. > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:99 > + return GST_PAD_PROBE_OK; I guess the idea is letting the events continue downstream the pipeline, right? > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:154 > + uint32_t id = sink->priv->m_streamId; > + auto* buffer = gst_sample_get_buffer(sample.get()); > + auto mappedBuffer = GstMappedBuffer::create(buffer, GST_MAP_READ); > + int32_t fd = memfd_create("audio-buffer", MFD_CLOEXEC); > + ftruncate(fd, mappedBuffer->size()); > + write(fd, mappedBuffer->data(), mappedBuffer->size()); > + lseek(fd, 0, SEEK_SET); > + > + wpe_audio_source_packet(wpeAudioSource, id, fd, mappedBuffer->size(), [](void* data) { > + gst_buffer_unref(GST_BUFFER_CAST(data)); > + }, gst_buffer_ref(buffer)); > + close(fd); I don't manage to understand this piece of code. You map the buffer, create the fd, write to it (copy the data), then call wpe_audio_source_packet by passing the reffed buffer (whose contents should be on the fd and then close the fd and unref the buffer in the callback. Can you please explain? I think a comment in the code would be interesting. > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:203 > +static GstObject* getInternalVolumeProxy(WebKitAudioSink* sink) I think the name is not very appropriate since you are not returning a volume proxy element, am I missing anything? > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:274 > + g_value_set_boolean(value, sink->priv->enableInternalMixing); Now I see where the other gboolean is coming from. I think this is still borderline with the agreement of using gtypes for variables, since this is not a "variable", more like an object attribute. Anyway, this is a personal preference. > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:290 > + // || GST_QUERY_TYPE(query) == GST_QUERY_DURATION I guess you need to remove this > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:294 > + // if (gst_pad_query(sink->priv->mixerPad.get(), query)) > + // return TRUE; And this > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:311 > + if (sink->priv->enableInternalMixing && (stateChange == GST_STATE_CHANGE_NULL_TO_READY)) { You don't need the inner () enclosing stateChange comparison. > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:321 > + if (!sink->priv->enableInternalMixing && (stateChange == GST_STATE_CHANGE_PLAYING_TO_PAUSED)) { ditto > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:332 > + if (sink->priv->enableInternalMixing && (stateChange == GST_STATE_CHANGE_READY_TO_NULL) && (result > GST_STATE_CHANGE_FAILURE)) ditto and result as well > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:333 > + mixer.unregisterProducer(WTFMove(sink->priv->mixerPad)); It is a good idea that you only move local variables or parameters (mainly && ones), not attributes from objects cause this pattern behavior can be undefined with respect of the original variable (depending on if there is a move constructor and how it behaves, etc.) and also depends on what the callee is doing with the variable. In this case, as you are not assigning the parameter anywhere, nothing will happen and the attribute will remain as it is. I think what you want to do is probably passing the bare pointer (or a const GRefPtr&) and after the call, assigning the smart pointer to nullptr to release it. > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:336 > + if (!sink->priv->enableInternalMixing && sink->priv->sourceStarted && (stateChange == GST_STATE_CHANGE_PAUSED_TO_PLAYING) && (result > GST_STATE_CHANGE_FAILURE)) { re-ditto > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:354 > + g_param_spec_double("volume", "Volume", "The audio volume, 1.0=100%", 0.0, 10.0, 1.0, Unless required in order to force floating point math, do not append .0, .f and .0f to floating point literals. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:811 > + case PROP_AUDIO_RENDERING_POLICY: { > + webView->priv->audioRenderingPolicy = static_cast<WebKitAudioRenderingPolicy>(g_value_get_enum(value)); > + break; > + } You don't need the { } > Source/WebKit/UIProcess/WebPageProxy.cpp:475 > + , m_audioRenderingPolicy(AudioRenderingPolicy::HostDefault) I'm sure Alexander would be extremely happy if we just used InternalMixing as default :) > Source/cmake/GStreamerChecks.cmake:40 > + message(FATAL_ERROR "WPEBackend-fdo is required for audio passthrough support") Isn't it that if we just used host default (pulseaudio) we are going to have passthrough? Am I missing anything?
Comment on attachment 390523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390523&action=review >> Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:154 >> + close(fd); > > I don't manage to understand this piece of code. You map the buffer, create the fd, write to it (copy the data), then call wpe_audio_source_packet by passing the reffed buffer (whose contents should be on the fd and then close the fd and unref the buffer in the callback. Can you please explain? I think a comment in the code would be interesting. I wrote this wpe_audio_source_packet() API to not be tied to GStreamer APIs, the last arg is passed as an opaque pointer.
(In reply to Philippe Normand from comment #6) > Comment on attachment 390523 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=390523&action=review > > >> Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:154 > >> + close(fd); > > > > I don't manage to understand this piece of code. You map the buffer, create the fd, write to it (copy the data), then call wpe_audio_source_packet by passing the reffed buffer (whose contents should be on the fd and then close the fd and unref the buffer in the callback. Can you please explain? I think a comment in the code would be interesting. > > I wrote this wpe_audio_source_packet() API to not be tied to GStreamer APIs, > the last arg is passed as an opaque pointer. I guess what I miss is the implementation of that function. Anyway, my question still stands. If you write to the fd and pass the fd to the function (where the fd contains the copied data), why do you need to keep the alive buffer around?
You're right, as there is a copy there's no need to keep the buffer around.
Comment on attachment 390523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390523&action=review >> Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:99 >> + return GST_PAD_PROBE_OK; > > I guess the idea is letting the events continue downstream the pipeline, right? There's no downstream, as this is a sink. >> Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:203 >> +static GstObject* getInternalVolumeProxy(WebKitAudioSink* sink) > > I think the name is not very appropriate since you are not returning a volume proxy element, am I missing anything? The returned object is expected to have volume and mute properties. To me it's like a black box with those 2 properties. >> Source/cmake/GStreamerChecks.cmake:40 >> + message(FATAL_ERROR "WPEBackend-fdo is required for audio passthrough support") > > Isn't it that if we just used host default (pulseaudio) we are going to have passthrough? Am I missing anything? No, passthrough is not a good term explaining what this will do. I'll rephrase this if this feature ever lands again in Bugzilla.
Created attachment 399592 [details] Patch
Comment on attachment 399592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399592&action=review > Source/WebKit/ChangeLog:9 > + 2 values, HostDefault (default) and InternalMixing. A third value might be added for Given my former opinions you could think I would support host default as default but I really think once we have the oportunity of having this option, the default should be to internal mixing. > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:100 > - GRefPtr<GstElement> audioSink = gst_element_factory_make("autoaudiosink", nullptr); > + MediaPlayerEnums::AudioRenderingPolicy audioRenderingPolicy = callback.audioRenderingPolicy(); > + GstElement* audioSink = createAudioSinkForPolicy(audioRenderingPolicy); Why did you remove the smart pointer? You could still be leaking this on line 118. > Source/WebCore/platform/graphics/MediaPlayer.h:285 > + > + Maybe remove these. > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:26 > + Remove this? > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:48 > + m_pipeline = adoptGRef(GST_ELEMENT_CAST(gst_element_factory_make("pipeline", "webkitaudiomixer"))); You can't adopt there, make returns floating. > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:110 > + }), nullptr) == GST_ITERATOR_RESYNC) This line needs proper intentation. > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:249 > + mixer.unregisterProducer(WTFMove(sink->priv->mixerPad)); I see what you try to do here but this very call does not have the effect you're trying to achieve. The unregisterProducer method takes an r-value reference but does nothing with it so the sink->priv->mixerPad field is going to be very alive after this call. I think the proper signature for that method would be a const GRefPtr& and then asigning it to null here. These kind of calls can be undefined behavior as well depending on how you define the r-value operators and constructors.
Comment on attachment 399592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399592&action=review I'm generally confused why we're adding a separate pipeline and various special cases for clocks. Internal mixing sounded in theory like a really easy thing, these proxies and separate pipelines at least deserve some explanation in the ChangeLog. > Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:94 > + switch (context().document()->page()->chrome().client().audioRenderingPolicy()) { page() can be null, potentially the calls as well. I'd break this conditional down and carefully check for nulls. > Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.h:68 > + MediaPlayerEnums::AudioRenderingPolicy m_audioRenderingPolicy; Initialize to HostDefault? > Source/WebCore/html/HTMLMediaElement.cpp:7075 > + switch (document().page()->chrome().client().audioRenderingPolicy()) { Refactor as above > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:99 > + MediaPlayerEnums::AudioRenderingPolicy audioRenderingPolicy = callback.audioRenderingPolicy(); let context provide some brevity: auto policy = callback.audioRenderingPolicy() > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:107 > + if (GST_IS_BIN(audioSink)) { The ChangeLog should describe why you're doing what you do. > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:51 > + auto clock = adoptGRef(gst_system_clock_obtain()); Why? > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:103 > + // Workaround to reduce the latency introduced by the queue in the proxysrc element. Is there an upstream bug for this? > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:152 > + GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS(GST_BIN_CAST(m_pipeline.get()), GST_DEBUG_GRAPH_SHOW_ALL, "audio-mixer-after-producer-unregistration"); Nit, I'd rename this as addProducer / removeProducer. > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.h:29 > +static GstClockTime mixerBaseTime() Please explain the clock stuff in the ChangeLog, it's weird GStreamer requires user-code to do all this. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2757 > + if (GStreamerAudioMixer::isAllowed()) { ChangeLog should explain this > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:84 > + // basesink, we have to take care of it ourselves. Why doesn't it inherit from basesink? This seems like a design bug forcing clients to handle such arcana. > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:99 > + g_object_notify(G_OBJECT(sink), "mute"); Why? > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:148 > + if (!internalObject) { if (auto* internalObject = getInternalVolumeProxy(sink)) { g_object_set(...) } else { GST_WARNING; } break > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:156 > + GstObject* internalObject = getInternalVolumeProxy(sink); Ditto > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:180 > + if (!internalObject) { Ditto > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:191 > + if (!internalObject) { Ditto > Source/WebKit/UIProcess/WebPageProxy.cpp:496 > + , m_audioRenderingPolicy(AudioRenderingPolicy::HostDefault) Better to use member initialization
Comment on attachment 399592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399592&action=review >> Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:51 >> + auto clock = adoptGRef(gst_system_clock_obtain()); > > Why? I'm just following the docs: https://gstreamer.freedesktop.org/documentation/proxy/proxysrc.html?gi-language=c#proxysrc The audio mixer pipeline is live... Anyway, I'll explain all the gritty details in the ChangeLog!!! >> Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:84 >> + // basesink, we have to take care of it ourselves. > > Why doesn't it inherit from basesink? This seems like a design bug forcing clients to handle such arcana. This is a bin, it can't inherit from basesink. >> Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:148 >> + if (!internalObject) { > > if (auto* internalObject = getInternalVolumeProxy(sink)) { > g_object_set(...) > } else { > GST_WARNING; > } > break We do early returns, usually. Why shouldn't it be the case here?
Comment on attachment 399592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399592&action=review >>> Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:51 >>> + auto clock = adoptGRef(gst_system_clock_obtain()); >> >> Why? > > I'm just following the docs: https://gstreamer.freedesktop.org/documentation/proxy/proxysrc.html?gi-language=c#proxysrc > The audio mixer pipeline is live... Anyway, I'll explain all the gritty details in the ChangeLog!!! Please do, it will help other maintainers and potentially yourself in the future, docs can easily drift out of sync with code. >>> Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:84 >>> + // basesink, we have to take care of it ourselves. >> >> Why doesn't it inherit from basesink? This seems like a design bug forcing clients to handle such arcana. > > This is a bin, it can't inherit from basesink. Of course, but I'm not talking about WebKitAudioSink, I was referring to your comment: why proxysink and basesink are not sharing such arcana. It seems pretty wrong to have to do this in WebKit. >>> Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:148 >>> + if (!internalObject) { >> >> if (auto* internalObject = getInternalVolumeProxy(sink)) { >> g_object_set(...) >> } else { >> GST_WARNING; >> } >> break > > We do early returns, usually. Why shouldn't it be the case here? We're inconsistent about it :) I get nits often enough about adopting `if (auto* foo = fooer()) { success; }` pattern than it seems like a WebKit guideline. As you wish.
Comment on attachment 399592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399592&action=review >> Source/WebKit/UIProcess/WebPageProxy.cpp:496 >> + , m_audioRenderingPolicy(AudioRenderingPolicy::HostDefault) > > Better to use member initialization Usually yes, but this is a big header file and the enum is forward-declared. I would avoid including the header declaring it if possible.
Comment on attachment 390523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390523&action=review >>>> Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:154 >>>> + close(fd); >>> >>> I don't manage to understand this piece of code. You map the buffer, create the fd, write to it (copy the data), then call wpe_audio_source_packet by passing the reffed buffer (whose contents should be on the fd and then close the fd and unref the buffer in the callback. Can you please explain? I think a comment in the code would be interesting. >> >> I wrote this wpe_audio_source_packet() API to not be tied to GStreamer APIs, the last arg is passed as an opaque pointer. > > I guess what I miss is the implementation of that function. Anyway, my question still stands. If you write to the fd and pass the fd to the function (where the fd contains the copied data), why do you need to keep the alive buffer around? Actually, getting back to this and reading the memfd and mmap man pages... IIUC there's no copy here, the buffer contents are reffed into a memory region, not copied. So in the end I think we need a destroy-notify mechanism here.
Created attachment 404938 [details] Patch
I'll check the GTK API failures.
Created attachment 405026 [details] Patch
Comment on attachment 405026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405026&action=review > Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:97 > + case ChromeClient::AudioRenderingPolicy::HostDefault: I think a better name is needed here, instead of HostDefault. Because if we ever make the default to internal-mixing, host-default will no longer be the default. :)
Created attachment 405036 [details] Patch
Comment on attachment 405036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405036&action=review Other than minor changes, the GStreamer part LGTM after the required tweaks. I am though deferring the rest of the review who someone with more knownledge. > Source/WebCore/platform/graphics/MediaPlayer.cpp:1708 > + static_assert(!static_cast<size_t>(MediaPlayerEnums::AudioRenderingPolicy::HostDefault), "MediaPlayerEnums::AudioRenderingPolicy::HostDefault is not 0 as expected"); > + static_assert(static_cast<size_t>(MediaPlayerEnums::AudioRenderingPolicy::InternalMixing) == 1, "MediaPlayerEnums::AudioRenderingPolicy::InternalMixing is not 1 as expected"); > + static_assert(static_cast<size_t>(MediaPlayerEnums::AudioRenderingPolicy::External) == 2, "MediaPlayerEnums::AudioRenderingPolicy::External is not 2 as expected"); I am not saying you need to change this but IMHO this is one of the cases to break the style rules regarding the == 0 :) > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:427 > +GstElement* createAudioSinkForPolicy(MediaPlayerEnums::AudioRenderingPolicy audioRenderingPolicy) Not to do now but a reflection for possible improvements. I think we should tend to return smart pointers here. Then if we return something and there's an error in the caller, we'll have less possibilities of having a memory leak. This said, once we do this for things like elements, we shouldn't do any leakRef() on them when assigning them to something that takes ownership because then we'll have a leak! (the smart pointer will sink the ref, then GStreamer will try to sink_ref again but as it is already sinked, it will just bump it) > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:44 > + gulong padEventProbeId; > + gulong padBufferProbeId; I see these checked during disposal, but never set to anything anywhere. Should they be removed? > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:90 > + if (isMapped) > + gst_buffer_unmap(buffer, &info); > + gst_buffer_unref(buffer); This looks like a job for the new and shiny GstMappedOwnedBuffer! > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:148 > + wpe_audio_source_packet(wpeAudioSource, sink->priv->m_streamId, std::get<0>(*mapData), std::get<1>(*mapData), [](void* data) { Can't you just do mapData->first and second? > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:150 > + auto* holder = reinterpret_cast<AudioPacketHolder*>(data); > + delete holder; I guess wpe_audio_source_packet is a C callback, right? Otherwise you could capture AudioPackedHolder in a UniqueRef in the lambda. > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:208 > + GST_ERROR_OBJECT(sink, "WPEBackend-FDO Audio extension is missing, update to WPEBackend-FDO 1.8.x."); Let's ASSERT_NOT_REACHED after this > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:211 > + GST_ERROR_OBJECT(sink, "External audio rendering is supported only in WPEWebKit"); Ditto > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:230 > + if (priv->padEventProbeId) { > + GRefPtr<GstPad> sinkPad = adoptGRef(gst_element_get_static_pad(priv->interaudioSink.get(), "sink")); > + gst_pad_remove_probe(sinkPad.get(), priv->padEventProbeId); > + priv->padEventProbeId = 0; > + } > + > + if (priv->padBufferProbeId) { > + GRefPtr<GstPad> sinkPad = adoptGRef(gst_element_get_static_pad(priv->interaudioSink.get(), "sink")); > + gst_pad_remove_probe(sinkPad.get(), priv->padBufferProbeId); > + priv->padBufferProbeId = 0; > + } As I said above, I can't find the code where these are set/initialized, so I think we can remove this. > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:235 > +#if PLATFORM(WPE) && USE(WPEBACKEND_FDO_AUDIO_EXTENSION) > + if (priv->wpeAudioSource) > + wpe_audio_source_destroy(priv->wpeAudioSource); > +#endif Can't we make priv->wpeAudioSource a unique_ptr with a custom deleter and avoid this? > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:259 > + GST_WARNING_OBJECT(sink, "No associated mixer pad, can't set %s property", g_param_spec_get_name(pspec)); ASSERT_NOT_REACHED? > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:268 > + GST_WARNING_OBJECT(sink, "No associated mixer pad, can't set %s property", g_param_spec_get_name(pspec)); Ditto > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:290 > + if (!internalObject) { ditto > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:300 > + GST_WARNING_OBJECT(sink, "No associated mixer pad, can't get %s property", g_param_spec_get_name(pspec)); ditto > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:328 > + GST_ELEMENT_ERROR(sink, CORE, MISSING_PLUGIN, (nullptr), ("no interaudiosink")); ditto > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:351 > + if (priv->enableInternalMixing && (stateChange == GST_STATE_CHANGE_READY_TO_NULL) && (result > GST_STATE_CHANGE_FAILURE)) { you can remove the internal () () > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:356 > + if (!priv->enableInternalMixing && (priv->sourceState == GST_STATE_PAUSED) && (stateChange == GST_STATE_CHANGE_PAUSED_TO_PLAYING) && (result > GST_STATE_CHANGE_FAILURE)) { ditto
Comment on attachment 405036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405036&action=review >> Source/WebCore/platform/graphics/MediaPlayer.cpp:1708 >> + static_assert(static_cast<size_t>(MediaPlayerEnums::AudioRenderingPolicy::External) == 2, "MediaPlayerEnums::AudioRenderingPolicy::External is not 2 as expected"); > > I am not saying you need to change this but IMHO this is one of the cases to break the style rules regarding the == 0 :) I followed the same approach as the other enums used in this file. >> Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:44 >> + gulong padBufferProbeId; > > I see these checked during disposal, but never set to anything anywhere. Should they be removed? Right, I forgot to remove this when I switched from the proxy to inter plugin. >> Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:150 >> + delete holder; > > I guess wpe_audio_source_packet is a C callback, right? Otherwise you could capture AudioPackedHolder in a UniqueRef in the lambda. Yes, this is a C callback.
Comment on attachment 405036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405036&action=review > Source/WebCore/ChangeLog:19 > + the WebProcess wil forward audio samples to the UIProcess, using a Wayland protocol defined wil -> will > Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:95 > + auto* page = context().document()->page(); > + if (page) { if (auto* page = context().document()->page()) { > Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:107 > + default: > + ASSERT_NOT_REACHED(); Better not to add default when we are handling all possible options. > Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.h:44 > + MediaPlayerEnums::AudioRenderingPolicy audioRenderingPolicy() const final { return m_audioRenderingPolicy; } Why do we cache the value in this case? The media player is always calling the chrome client to ask for it. Can the value change? > Source/WebCore/html/HTMLMediaElement.cpp:7026 > + default: > + ASSERT_NOT_REACHED(); Ditto. > Source/WebCore/html/HTMLMediaElement.cpp:7028 > + return MediaPlayerEnums::AudioRenderingPolicy::HostDefault; We can add RELEASE_ASSERT_NOT_REACHED() here to avoid this return and make the gcc happy. > Source/WebCore/platform/graphics/MediaPlayerEnums.h:114 > + enum class AudioRenderingPolicy : uint8_t { > + HostDefault, > + InternalMixing, > + External > + }; Why is this enum defined twice? > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:33 > +bool GStreamerAudioMixer::isAllowed() isAvailable? > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.cpp:60 > + gst_element_set_state(m_pipeline.get(), GST_STATE_NULL); This is never destroyed, if we really need to change pipeline state it should be done somewhere else. > Source/WebCore/platform/graphics/gstreamer/GStreamerAudioMixer.h:41 > +protected: > + GStreamerAudioMixer(); > + ~GStreamerAudioMixer(); Why is this protected? >> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:427 >> +GstElement* createAudioSinkForPolicy(MediaPlayerEnums::AudioRenderingPolicy audioRenderingPolicy) > > Not to do now but a reflection for possible improvements. I think we should tend to return smart pointers here. Then if we return something and there's an error in the caller, we'll have less possibilities of having a memory leak. This said, once we do this for things like elements, we shouldn't do any leakRef() on them when assigning them to something that takes ownership because then we'll have a leak! (the smart pointer will sink the ref, then GStreamer will try to sink_ref again but as it is already sinked, it will just bump it) If this is always expected to be added to a "container" that sinks the ref, it's ok to use raw pointer. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:432 > + audioSink = webkitAudioSinkNew(false); Better use an enum instead of a boolean or different New methods webkitAudioSinkNew() and webkitAudioSinkNewInternal(). > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:81 > + AudioPacketHolder(GstBuffer* buffer) explicit > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:84 > + { } > + ~AudioPacketHolder() Add an empty line there > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:234 > + if (priv->wpeAudioSource) > + wpe_audio_source_destroy(priv->wpeAudioSource); Dispose is called multiple times, you need to set the value to nullptr, or even better use g_clear_pointer(&priv->wpeAudioSource, wpe_audio_source_destroy); > Source/WebKit/Shared/AudioRenderingPolicy.h:28 > +enum class AudioRenderingPolicy : uint8_t { > + HostDefault, > + InternalMixing, > + External > +}; And here again. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1335 > + * WebKitWebView::audio-rendering-policy: WebKitWebView::audio-rendering-policy: -> WebKitWebView:audio-rendering-policy: :: means it's a signal. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1337 > + * Whether the #WebKitWebView should manage audio rendering through the host This sound like the property is boolean rather than an enum. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1345 > + * Maybe it's just me, but this sound more like a Mode, not a Policy, so maybe audio-rendering-mode? > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2341 > + case WEBKIT_AUDIO_RENDERING_POLICY_HOST_DEFAULT: > + policy = WebKit::AudioRenderingPolicy::HostDefault; > + break; > + case WEBKIT_AUDIO_RENDERING_POLICY_INTERNAL_MIXING: > + policy = WebKit::AudioRenderingPolicy::InternalMixing; > + break; > +#if PLATFORM(WPE) > + case WEBKIT_AUDIO_RENDERING_POLICY_EXTERNAL: I think I would use Default, Internal, External or Host/System, Internal, External > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2346 > + getPage(webView).setAudioRenderingPolicy(policy); Why is this called here and not when the property is set? If it's construct only, then a page configuration option should be used instead. > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:210 > + * @WEBKIT_AUDIO_RENDERING_POLICY_INTERNAL_MIXING: perform internal mixing and render the resulting stream to host audio daemon. > + * Audio rendering policy. Add an empty line betwen these > Source/WebKit/UIProcess/gstreamer/WebPageProxyGStreamer.cpp:48 > + m_audioRenderingPolicy = policy; > + m_process->send(Messages::WebPage::SetAudioRenderingPolicy(policy), m_webPageID); Since this is set on construction and can't be changed better to use page configuration and send it to the web process as intialization parameter (this should be done in any case, to ensure it's initialized if set before the process is launched). > Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1323 > + default: > + ASSERT_NOT_REACHED(); Remove the default. > Tools/MiniBrowser/gtk/main.c:91 > + "audio-rendering-policy", audioRenderingPolicy, This should be initialized in all other web view creates. I think we need a public getter for the property.
Comment on attachment 405036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405036&action=review >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1337 >> + * Whether the #WebKitWebView should manage audio rendering through the host > > This sound like the property is boolean rather than an enum. In GTK it could be a boolean yes because the enum has 2 values only. But in WPE there's 3 values, so I'd prefer to keep the API consistent and keep the enum prop in both ports. >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1345 >> + * > > Maybe it's just me, but this sound more like a Mode, not a Policy, so maybe audio-rendering-mode? SGTM! >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2341 >> + case WEBKIT_AUDIO_RENDERING_POLICY_EXTERNAL: > > I think I would use Default, Internal, External or Host/System, Internal, External Host, Internal, External ... yes, why not
Comment on attachment 405036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405036&action=review >>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1337 >>> + * Whether the #WebKitWebView should manage audio rendering through the host >> >> This sound like the property is boolean rather than an enum. > > In GTK it could be a boolean yes because the enum has 2 values only. But in WPE there's 3 values, so I'd prefer to keep the API consistent and keep the enum prop in both ports. Exactly, I meant the description starting with whether sounds like it's a boolean property, but it isn't, so I would rephrase it. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1354 > + _("Whether the view is internally rendering audio"), And here too.
Comment on attachment 405036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405036&action=review >> Source/WebCore/platform/graphics/MediaPlayerEnums.h:114 >> + }; > > Why is this enum defined twice? I haven't managed to keep a single one, WebKit IPC seems to require another one. I'll try to debug this... >>> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:427 >>> +GstElement* createAudioSinkForPolicy(MediaPlayerEnums::AudioRenderingPolicy audioRenderingPolicy) >> >> Not to do now but a reflection for possible improvements. I think we should tend to return smart pointers here. Then if we return something and there's an error in the caller, we'll have less possibilities of having a memory leak. This said, once we do this for things like elements, we shouldn't do any leakRef() on them when assigning them to something that takes ownership because then we'll have a leak! (the smart pointer will sink the ref, then GStreamer will try to sink_ref again but as it is already sinked, it will just bump it) > > If this is always expected to be added to a "container" that sinks the ref, it's ok to use raw pointer. Right, the element will always be added to a GstBin. >> Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:234 >> + wpe_audio_source_destroy(priv->wpeAudioSource); > > Dispose is called multiple times, you need to set the value to nullptr, or even better use g_clear_pointer(&priv->wpeAudioSource, wpe_audio_source_destroy); I'm switching to a GUniquePtr<struct wpe_audio_source>..
Created attachment 405225 [details] Patch
Created attachment 405226 [details] Patch
Some discussed-elsewhere TODO: - add env var to enable/disable internal mixing. - remove WebView API controlling rendering policy - enable WPE external rendering only if a client is connected through wayland IPC. (Is this even doable? IDK)
Created attachment 409843 [details] Patch
WPE EWS likely to fail, needs https://github.com/Igalia/WPEBackend-fdo/pull/122
(In reply to Philippe Normand from comment #32) > WPE EWS likely to fail, needs > https://github.com/Igalia/WPEBackend-fdo/pull/122 Hah. Forgot to update the SDK, how fortunate :) Could NOT find WPEBackend_fdo: Found unsuitable version "1.7.0", but required is at least "1.8.0" (found /usr/include/wpe-fdo-1.0)
Comment on attachment 409843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409843&action=review > Source/WebCore/platform/graphics/gstreamer/WebKitAudioSinkGStreamer.cpp:186 > + if (value && equalIgnoringASCIICase(value, "1")) { Nit: You probably don't need to ignore case here.
Created attachment 410113 [details] Patch for landing
Committed r267787: <https://trac.webkit.org/changeset/267787>
<rdar://problem/69790013>