Attachment 341757 Details for Bug 184588 – Rebased version of the patch taking into account previous comments and fixing little details

[patch] Rebased version of the patch taking into account previous comments and fixing little details

GStreamer-Fix-the-way-GstStreamCollection-is-handl.patch (text/plain), 11.72 KB, created by Thibault Saunier on 2018-06-01 06:50:49 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Thibault Saunier

Created: 2018-06-01 06:50:49 PDT

Size: 11.72 KB

patch

obsolete

>From d09fbfc5790980825fc04c3a801d753283ea8433 Mon Sep 17 00:00:00 2001
>From: Thibault Saunier <tsaunier@igalia.com>
>Date: Thu, 12 Apr 2018 10:24:28 -0300
>Subject: [PATCH xserver] [GStreamer] Fix the way GstStreamCollection is
> handled
>
>The stream collection message replaces the collection of stream previously
>advertised, this means that we should rebuild our set of Track from scratch
>and not update previously exposed tracks.
>
>In the end, this simplifies the code as we do not care about what
>tracks existed previously, we just need to expose what GStreamer tells
>us, deleting any previous state.
>
>Handle the STREAM_COLLECTION message from the sync handler so that tracks
>are updated before we mark the pipeline as READY for the live case (everything
>happen synchronously with the call to the `load()` method in that case),
>still the update happens on the main thread.
>
>https://bugs.webkit.org/show_bug.cgi?id=184588
>---
> Source/WebCore/ChangeLog                      |  31 ++++
> .../gstreamer/MediaPlayerPrivateGStreamer.cpp | 137 ++++++++----------
> .../gstreamer/MediaPlayerPrivateGStreamer.h   |   3 +
> 3 files changed, 98 insertions(+), 73 deletions(-)
>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 5fa0d034260..37a7c3cb789 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,34 @@
>+2018-04-13  Thibault Saunier  <tsaunier@igalia.com>
>+
>+        [GStreamer] Fix the way GstStreamCollection is handled
>+        https://bugs.webkit.org/show_bug.cgi?id=184588
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The stream collection message replaces the collection of stream previously
>+        advertised, this means that we should rebuild our set of Track from scratch
>+        and not update previously exposed tracks.
>+
>+        In the end, this simplifies the code as we do not care about what
>+        tracks existed previously, we just need to expose what GStreamer tells
>+        us, deleting any previous state.
>+
>+        Handle the STREAM_COLLECTION message from the sync handler so that tracks
>+        are updated before we mark the pipeline as READY for the live case (everything
>+        happen synchronously with the call to the `load()` method in that case),
>+        the update still always happens on the main thread.
>+
>+        No new tests is added as this is mostly refactoring, it is already tested and it
>+        will fix MediaStream tests that are currently disabled as the support is being
>+        implemented in #185787.
>+
>+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
>+        (WebCore::MediaPlayerPrivateGStreamer::clearTracks): Removes all tracks.
>+        (WebCore::MediaPlayerPrivateGStreamer::updateTracks): Updates configured tracks from the new GstStreamColection track.
>+        (WebCore::MediaPlayerPrivateGStreamer::handleMessage): Stop handling GST_STREAM_COLLECTION event.
>+        (WebCore::MediaPlayerPrivateGStreamer::handleSyncMessage): Handle stream collection event synchronously.
>+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: Add handleSyncMessage
>+
> 2018-06-01  Carlos Eduardo Ramalho  <cadubentzen@gmail.com>
> 
>         [GTK] Difficult to read combo box text in dark theme
>diff --git a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
>index 32796edf0e0..0eb7ce732b2 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
>@@ -622,75 +622,67 @@ bool MediaPlayerPrivateGStreamer::seeking() const
> }
> 
> #if GST_CHECK_VERSION(1, 10, 0)
>+#define CLEAR_TRACKS(tracks, method) \
>+    for (auto& track : tracks.values())\
>+        method(*track);\
>+    tracks.clear();
>+
>+void MediaPlayerPrivateGStreamer::clearTracks()
>+{
>+#if ENABLE(VIDEO_TRACK)
>+    CLEAR_TRACKS(m_audioTracks, m_player->removeAudioTrack);
>+    CLEAR_TRACKS(m_videoTracks, m_player->removeVideoTrack);
>+    CLEAR_TRACKS(m_textTracks, m_player->removeTextTrack);
>+#endif // ENABLE(VIDEO_TRACK)
>+}
>+#undef CLEAR_TRACKS
>+
>+#if ENABLE(VIDEO_TRACK)
>+#define CREATE_TRACK(type, Type) \
>+    m_has##Type = true; \
>+    if (!useMediaSource) {\
>+        RefPtr<Type##TrackPrivateGStreamer> track = Type##TrackPrivateGStreamer::create(createWeakPtr(), i, stream); \
>+        m_##type##Tracks.add(track->id(), track); \
>+        m_player->add##Type##Track(*track);\
>+        if (gst_stream_get_stream_flags(stream.get()) & GST_STREAM_FLAG_SELECT) {                                    \
>+            m_current##Type##StreamId = String(gst_stream_get_stream_id(stream.get()));                              \
>+        }                                                                                                            \
>+    }
>+#else
>+#define CREATE_TRACK(type, _id, tracks, method, stream) m_has##Type## = true;
>+#endif // ENABLE(VIDEO_TRACK)
>+
> void MediaPlayerPrivateGStreamer::updateTracks()
> {
>     ASSERT(!m_isLegacyPlaybin);
> 
>     bool useMediaSource = isMediaSource();
>     unsigned length = gst_stream_collection_get_size(m_streamCollection.get());
>-    Vector<String> validAudioStreams;
>-    Vector<String> validVideoStreams;
>-    Vector<String> validTextStreams;
>+
>+    bool oldHasAudio = m_hasAudio;
>+    bool oldHasVideo = m_hasVideo;
>+    // New stream collections override previous ones.
>+    clearTracks();
>     for (unsigned i = 0; i < length; i++) {
>         GRefPtr<GstStream> stream = gst_stream_collection_get_stream(m_streamCollection.get(), i);
>         String streamId(gst_stream_get_stream_id(stream.get()));
>         GstStreamType type = gst_stream_get_stream_type(stream.get());
>-        GST_DEBUG("Inspecting %s track with ID %s", gst_stream_type_get_name(type), streamId.utf8().data());
>-        if (type & GST_STREAM_TYPE_AUDIO) {
>-            validAudioStreams.append(streamId);
>-#if ENABLE(VIDEO_TRACK)
>-            if (!useMediaSource) {
>-                unsigned localIndex = i - validVideoStreams.size() - validTextStreams.size();
>-                if (localIndex < m_audioTracks.size()) {
>-                    if (m_audioTracks.contains(streamId))
>-                        continue;
>-                }
> 
>-                RefPtr<AudioTrackPrivateGStreamer> track = AudioTrackPrivateGStreamer::create(createWeakPtr(), i, stream);
>-                m_audioTracks.add(track->id(), track);
>-                m_player->addAudioTrack(*track);
>-            }
>-#endif
>+        GST_DEBUG_OBJECT(pipeline(), "Inspecting %s track with ID %s", gst_stream_type_get_name(type), streamId.utf8().data());
>+        if (type & GST_STREAM_TYPE_AUDIO) {
>+            CREATE_TRACK(audio, Audio)
>         } else if (type & GST_STREAM_TYPE_VIDEO) {
>-            validVideoStreams.append(streamId);
>+            CREATE_TRACK(video, Video)
>+        } else if (type & GST_STREAM_TYPE_TEXT && !useMediaSource) {
> #if ENABLE(VIDEO_TRACK)
>-            if (!useMediaSource) {
>-                unsigned localIndex = i - validAudioStreams.size() - validTextStreams.size();
>-                if (localIndex < m_videoTracks.size()) {
>-                    if (m_videoTracks.contains(streamId))
>-                        continue;
>-                }
>-
>-                RefPtr<VideoTrackPrivateGStreamer> track = VideoTrackPrivateGStreamer::create(createWeakPtr(), i, stream);
>-                m_videoTracks.add(track->id(), track);
>-                m_player->addVideoTrack(*track);
>-            }
>-#endif
>-        } else if (type & GST_STREAM_TYPE_TEXT) {
>-            validTextStreams.append(streamId);
>-#if ENABLE(VIDEO_TRACK)
>-            if (!useMediaSource) {
>-                unsigned localIndex = i - validVideoStreams.size() - validAudioStreams.size();
>-                if (localIndex < m_textTracks.size()) {
>-                    if (m_textTracks.contains(streamId))
>-                        continue;
>-                }
>-
>-                RefPtr<InbandTextTrackPrivateGStreamer> track = InbandTextTrackPrivateGStreamer::create(localIndex, stream);
>-                m_textTracks.add(streamId, track);
>-                m_player->addTextTrack(*track);
>-            }
>+            RefPtr<InbandTextTrackPrivateGStreamer> track = InbandTextTrackPrivateGStreamer::create(i, stream);
>+            m_textTracks.add(streamId, track);
>+            m_player->addTextTrack(*track);
> #endif
>         } else
>             GST_WARNING("Unknown track type found for stream %s", streamId.utf8().data());
>     }
> 
>-    GST_INFO("Media has %zu video tracks, %zu audio tracks and %zu text tracks", validVideoStreams.size(), validAudioStreams.size(), validTextStreams.size());
>-
>-    bool oldHasAudio = m_hasAudio;
>-    bool oldHasVideo = m_hasVideo;
>-    m_hasAudio = !validAudioStreams.isEmpty();
>-    m_hasVideo = !validVideoStreams.isEmpty();
>     if ((oldHasVideo != m_hasVideo) || (oldHasAudio != m_hasAudio))
>         m_player->characteristicChanged();
> 
>@@ -703,15 +695,10 @@ void MediaPlayerPrivateGStreamer::updateTracks()
>         return;
>     }
> 
>-#if ENABLE(VIDEO_TRACK)
>-    purgeInvalidAudioTracks(validAudioStreams);
>-    purgeInvalidVideoTracks(validVideoStreams);
>-    purgeInvalidTextTracks(validTextStreams);
>-#endif
>-
>     m_player->client().mediaPlayerEngineUpdated(m_player);
> }
>-#endif
>+#undef CLEAR_TRACKS
>+#endif // GST_CHECK_VERSION(1, 10, 0)
> 
> void MediaPlayerPrivateGStreamer::enableTrack(TrackPrivateBaseGStreamer::TrackType trackType, unsigned index)
> {
>@@ -1299,21 +1286,6 @@ void MediaPlayerPrivateGStreamer::handleMessage(GstMessage* message)
>         break;
>     }
> #if GST_CHECK_VERSION(1, 10, 0)
>-    case GST_MESSAGE_STREAM_COLLECTION: {
>-        if (m_isLegacyPlaybin)
>-            break;
>-
>-        GRefPtr<GstStreamCollection> collection;
>-        gst_message_parse_stream_collection(message, &collection.outPtr());
>-
>-        if (collection) {
>-            m_streamCollection.swap(collection);
>-            m_notifier->notify(MainThreadNotification::StreamCollectionChanged, [this] {
>-                this->updateTracks();
>-            });
>-        }
>-        break;
>-    }
>     case GST_MESSAGE_STREAMS_SELECTED: {
>         GRefPtr<GstStreamCollection> collection;
>         gst_message_parse_streams_selected(message, &collection.outPtr());
>@@ -1983,6 +1955,25 @@ void MediaPlayerPrivateGStreamer::updateStates()
>     }
> }
> 
>+bool MediaPlayerPrivateGStreamer::handleSyncMessage(GstMessage* message)
>+{
>+#if GST_CHECK_VERSION(1, 10, 0)
>+    if (GST_MESSAGE_TYPE(message) == GST_MESSAGE_STREAM_COLLECTION && !m_isLegacyPlaybin) {
>+        GRefPtr<GstStreamCollection> collection;
>+        gst_message_parse_stream_collection(message, &collection.outPtr());
>+
>+        if (collection) {
>+            m_streamCollection.swap(collection);
>+            m_notifier->notify(MainThreadNotification::StreamCollectionChanged, [this] {
>+                this->updateTracks();
>+            });
>+        }
>+    }
>+#endif
>+
>+    return MediaPlayerPrivateGStreamerBase::handleSyncMessage(message);
>+}
>+
> void MediaPlayerPrivateGStreamer::mediaLocationChanged(GstMessage* message)
> {
>     if (m_mediaLocations)
>diff --git a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h
>index b78046148d9..1ae4a54bae0 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h
>+++ b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h
>@@ -133,6 +133,8 @@ public:
> 
>     void enableTrack(TrackPrivateBaseGStreamer::TrackType, unsigned index);
> 
>+    bool handleSyncMessage(GstMessage*) override;
>+
> private:
>     static void getSupportedTypes(HashSet<String, ASCIICaseInsensitiveHash>&);
>     static MediaPlayer::SupportsType supportsType(const MediaEngineSupportParameters&);
>@@ -180,6 +182,7 @@ private:
> 
> #if GST_CHECK_VERSION(1, 10, 0)
>     void updateTracks();
>+    void clearTracks();
> #endif
> 
> protected:
>-- 
>2.17.1

Flags:

pnormand: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 184588: 337886 | 337887 | 338581 | 339991 | 341757 | 342084