Summary: | [GStreamer] Pipeline lifetime tracking | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||
Component: | Platform | Assignee: | Philippe Normand <pnormand> | ||||||||||
Status: | RESOLVED DUPLICATE | ||||||||||||
Severity: | Normal | CC: | calvaris, cdumez, cgarcia, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gustavo, gyuyoung.kim, jer.noble, menard, philipj, sergio, vjaquez | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Philippe Normand
2019-11-16 07:42:27 PST
Created attachment 383697 [details]
Patch
Created attachment 383698 [details]
Patch
Created attachment 383700 [details]
Patch
Comment on attachment 383700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383700&action=review > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:389 > + String name = gst_object_get_name(GST_OBJECT_CAST(pipeline)); You're leaking the name here. gst_object_get_name returns a transfer full and the String constructor is going to copy it. You can either use GUniquePtr or something like: String name(StringImpl::createWithoutCopying(gst_object_get_name...)); Besides, I would recommend first return on !GST_IS_ELEMENT (maybe G_RETURN_IF_FAIL) just in case we get funny things. With nulls the best we can get is a critical on gst_object_get_name and the worst a crash if checks are disabled (rare but possible). By the code we're using it looks like we are not getting nulls but for the future, better safe than sorry. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:401 > + String name = gst_object_get_name(GST_OBJECT_CAST(pipeline)); Ditto. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:415 > + Vector<String> names; > + for (auto name : s_webKitGStreamerPipelines.keys()) > + names.append(name); We can try to save some allocations if we check the size first, allocate the vector and then appendUnchecked. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:424 > + return ""; emptyString()? Or even better nullString(). > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:429 > + GRefPtr<GstElement> child = gst_bin_get_by_name(pipeline, subBinName.utf8().data()); gst_bin_get_by_name is transfer full, you need to adopt here. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:433 > + return ""; Ditto Created attachment 384038 [details]
Patch
Comment on attachment 384038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384038&action=review I think it's easier to understand this patch if it also includes the inspector part, so I would merge both patches into one. > Source/WebCore/html/HTMLMediaElement.cpp:7230 > +void HTMLMediaElement::pipelineCreated(GstElement* pipeline) > +{ > + if (!document().page()) > + return; > + > + registerGStreamerPipeline(pipeline, document().page()); > +} > + > +void HTMLMediaElement::pipelineWillBeDestroyed(GstElement* pipeline) > +{ > + if (!document().page()) > + return; > + > + unregisterGStreamerPipeline(pipeline, document().page()); We only need to access the page here to notify the inspector about changes in the pipelines. I think it would be simpler if the register/unregister happens in the platform code, without using the page, and then use the media player to otify the inspector. So, this would be reduced to a single method HTMLMediaElement::gstreamerPipelinesChanged() (for example) that could notify the inspector directly. That way we don't need any GST code here. > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:28 > +#include "Document.h" This is a layering violation > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:32 > +#include "PlatformMediaSessionManager.h" Ditto. > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:98 > + m_pipeline = gst_pipeline_new("webaudio-playback"); > + > + auto& manager = PlatformMediaSessionManager::sharedManager(); > + auto* session = manager.currentSession(); > + if (session) { > + auto& client = session->client(); > + auto* document = client.hostingDocument(); > + if (document) > + registerGStreamerPipeline(m_pipeline, document->page()); > + } Unfortunately we can't use PlatformMediaSessionManager from platform code, we need to find another way to notify the inspector when this pipeline is created. > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:155 > + auto& manager = PlatformMediaSessionManager::sharedManager(); > + auto* session = manager.currentSession(); > + if (session) { > + auto& client = session->client(); > + auto* document = client.hostingDocument(); > + if (document) > + unregisterGStreamerPipeline(m_pipeline, document->page()); > + } Ditto. > Source/WebCore/platform/graphics/MediaPlayer.h:61 > +#if USE(GSTREAMER) > +typedef struct _GstElement GstElement; > +#endif This shouldn't be needed if we split the register/unregister and the inspector notification. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:28 > +#include "Page.h" This is a layering violation too. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:385 > +HashMap<String, GstBin*> s_webKitGStreamerPipelines; We normally use NeverDestroy for this kind of global maps, with a functions to get a reference to it instead of suing a global variable. Use CString instead of String, since we are storing utf8 strings. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:390 > + if (!GST_IS_ELEMENT(pipeline)) > + return; I would assert instead, this is not expected to happen I guess. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:396 > + if (s_webKitGStreamerPipelines.contains(name.get())) > + return; > + > + s_webKitGStreamerPipelines.add(name.get(), GST_BIN_CAST(pipeline)); You can do this with a single call to add: auto addResult = pipelinesMap().add(name.get(), nullptr); if (!addResult.isNewEntry) return; addResult.iterator->value = GST_BIN_CAST(pipeline); > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:398 > + // TODO: Hook to InspectorInstrumentation here. Use FIXME: instead of TODO > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:411 > + if (!s_webKitGStreamerPipelines.contains(name.get())) > + return; > + > + s_webKitGStreamerPipelines.remove(name.get()); Use take(). > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:417 > +Vector<String> gstPipelinesNames() gstreamerPipelineNames() and Use CString > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:427 > +String dumpGStreamerPipeline(String name, String subBinName) const String& name, const String& subBinName. Return a CString > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:434 > + return gst_debug_bin_to_dot_data(pipeline, GST_DEBUG_GRAPH_SHOW_ALL); You are leaking the result of gst_debug_bin_to_dot_data(). > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:437 > + if (child && GST_IS_BIN(child.get())) GST_IS_BIN() already does the null check > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:438 > + return gst_debug_bin_to_dot_data(GST_BIN_CAST(child.get()), GST_DEBUG_GRAPH_SHOW_ALL); Ditto. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:35 > +class Page; Layering violation. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2413 > + if (m_player) > + m_player->client().pipelineWillBeDestroyed(m_pipeline.get()); So here you would call the unregister directly and then m_player->client().gstreamerPipelinesChanaged(). > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:349 > + if (m_pipeline) { > + if (m_player) > + m_player->client().pipelineWillBeDestroyed(m_pipeline.get()); > gst_element_set_state(m_pipeline.get(), GST_STATE_NULL); > + } Ditto. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:358 > + if (m_player) > + m_player->client().pipelineCreated(m_pipeline.get()); Adn same for the register > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:121 > + m_playerPrivate->mediaPlayer()->client().pipelineCreated(m_pipeline.get()); Ditto. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:247 > + if (m_playerPrivate) > + m_playerPrivate->mediaPlayer()->client().pipelineWillBeDestroyed(m_pipeline.get()); Ditto. *** This bug has been marked as a duplicate of bug 204619 *** |