Changeset 244584 in webkit


Ignore:
Timestamp:
Apr 24, 2019 5:11:50 AM (5 years ago)
Author:
Philippe Normand
Message:

[GStreamer] Crash in AudioTrackPrivate with playbin3 enabled
https://bugs.webkit.org/show_bug.cgi?id=196913

Reviewed by Xabier Rodriguez-Calvar.

The crash was due to a playbin3 code path being triggered during
MSE playback, which is not supposed to work in playbin3 anyway.
The problem is that setting the USE_PLAYBIN3 environment variable
to "1" makes the GStreamer playback plugin register the playbin3
element under the playbin name. So that leads to playbin3 being
used everywhere in WebKit where we assume the playbin element is
used. So the proposed solution is to:

  • use a WebKit-specific environment variable instead of the

GStreamer USE_PLAYBIN3 variable.

  • emit a warning if the USE_PLAYBIN3 environment variable is

detected. We can't unset it ourselves for security reasons.

The patch also includes a code cleanup of the player method
handling the pipeline creation. The previous code had a bug
leading to playbin3 being used for MSE.

  • platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:

(WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin):

Location:
trunk/Source/WebCore
Files:
4 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r244582 r244584  
     12019-04-24  Philippe Normand  <pnormand@igalia.com>
     2
     3        [GStreamer] Crash in AudioTrackPrivate with playbin3 enabled
     4        https://bugs.webkit.org/show_bug.cgi?id=196913
     5
     6        Reviewed by Xabier Rodriguez-Calvar.
     7
     8        The crash was due to a playbin3 code path being triggered during
     9        MSE playback, which is not supposed to work in playbin3 anyway.
     10        The problem is that setting the USE_PLAYBIN3 environment variable
     11        to "1" makes the GStreamer playback plugin register the playbin3
     12        element under the playbin name. So that leads to playbin3 being
     13        used everywhere in WebKit where we assume the playbin element is
     14        used. So the proposed solution is to:
     15
     16        - use a WebKit-specific environment variable instead of the
     17        GStreamer USE_PLAYBIN3 variable.
     18        - emit a warning if the USE_PLAYBIN3 environment variable is
     19        detected. We can't unset it ourselves for security reasons.
     20
     21        The patch also includes a code cleanup of the player method
     22        handling the pipeline creation. The previous code had a bug
     23        leading to playbin3 being used for MSE.
     24
     25        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
     26        (WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin):
     27
    1282019-04-24  chris fleizach  <cfleizach@apple.com>
    229
  • trunk/Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp

    r242789 r244584  
    224224        isGStreamerInitialized = false;
    225225
     226        // USE_PLAYBIN3 is dangerous for us because its potential sneaky effect
     227        // is to register the playbin3 element under the playbin namespace. We
     228        // can't allow this, when we create playbin, we want playbin2, not
     229        // playbin3.
     230        if (g_getenv("USE_PLAYBIN3"))
     231            WTFLogAlways("The USE_PLAYBIN3 variable was detected in the environment. Expect playback issues or please unset it.");
     232
    226233#if ENABLE(VIDEO) || ENABLE(WEB_AUDIO)
    227234        Vector<String> parameters = options.valueOr(extractGStreamerOptionsFromCommandLine());
  • trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp

    r244109 r244584  
    249249void MediaPlayerPrivateGStreamer::load(const String& urlString)
    250250{
    251     loadFull(urlString, nullptr, String());
     251    loadFull(urlString, String());
    252252}
    253253
     
    274274}
    275275
    276 void MediaPlayerPrivateGStreamer::loadFull(const String& urlString, const gchar* playbinName,
    277     const String& pipelineName)
     276void MediaPlayerPrivateGStreamer::loadFull(const String& urlString, const String& pipelineName)
    278277{
    279278    // FIXME: This method is still called even if supportsType() returned
     
    290289
    291290    if (!m_pipeline)
    292         createGSTPlayBin(isMediaSource() ? "playbin" : playbinName, pipelineName);
     291        createGSTPlayBin(url, pipelineName);
    293292    syncOnClock(true);
    294293    if (m_fillTimer.isActive())
     
    336335        "_0x", hex(reinterpret_cast<uintptr_t>(this), Lowercase));
    337336
    338     loadFull(String("mediastream://") + stream.id(), "playbin3", pipelineName);
     337    loadFull(String("mediastream://") + stream.id(), pipelineName);
    339338    syncOnClock(false);
    340339
     
    23632362#endif
    23642363
    2365 void MediaPlayerPrivateGStreamer::createGSTPlayBin(const gchar* playbinName, const String& pipelineName)
    2366 {
     2364void MediaPlayerPrivateGStreamer::createGSTPlayBin(const URL& url, const String& pipelineName)
     2365{
     2366    const gchar* playbinName = "playbin";
     2367
     2368    // MSE doesn't support playbin3. Mediastream requires playbin3. Regular
     2369    // playback can use playbin3 on-demand with the WEBKIT_GST_USE_PLAYBIN3
     2370    // environment variable.
     2371#if GST_CHECK_VERSION(1, 10, 0)
     2372    if ((!isMediaSource() && g_getenv("WEBKIT_GST_USE_PLAYBIN3")) || url.protocolIs("mediastream"))
     2373        playbinName = "playbin3";
     2374#endif
     2375
    23672376    if (m_pipeline) {
    2368         if (!playbinName) {
    2369             GST_INFO_OBJECT(pipeline(), "Keeping same playbin as nothing forced");
    2370             return;
    2371         }
    2372 
    23732377        if (!g_strcmp0(GST_OBJECT_NAME(gst_element_get_factory(m_pipeline.get())), playbinName)) {
    23742378            GST_INFO_OBJECT(pipeline(), "Already using %s", playbinName);
     
    23762380        }
    23772381
    2378         GST_INFO_OBJECT(pipeline(), "Tearing down as we need to use %s now.",
    2379             playbinName);
     2382        GST_INFO_OBJECT(pipeline(), "Tearing down as we need to use %s now.", playbinName);
    23802383        changePipelineState(GST_STATE_NULL);
    23812384        m_pipeline = nullptr;
     
    23832386
    23842387    ASSERT(!m_pipeline);
    2385 
    2386 #if GST_CHECK_VERSION(1, 10, 0)
    2387     if (g_getenv("USE_PLAYBIN3"))
    2388         playbinName = "playbin3";
    2389 #else
    2390     playbinName = "playbin";
    2391 #endif
    2392 
    2393     if (!playbinName)
    2394         playbinName = "playbin";
    23952388
    23962389    m_isLegacyPlaybin = !g_strcmp0(playbinName, "playbin");
  • trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h

    r244109 r244584  
    150150    virtual void asyncStateChangeDone();
    151151
    152     void createGSTPlayBin(const gchar* playbinName, const String& pipelineName);
     152    void createGSTPlayBin(const URL&, const String& pipelineName);
    153153
    154154    bool loadNextLocation();
     
    181181
    182182    void setPlaybinURL(const URL& urlString);
    183     void loadFull(const String& url, const gchar* playbinName, const String& pipelineName);
     183    void loadFull(const String& url, const String& pipelineName);
    184184
    185185#if GST_CHECK_VERSION(1, 10, 0)
Note: See TracChangeset for help on using the changeset viewer.