[GStreamer][Playbin3] Stream collection handling fixes

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

Our internal list of tracks mapping the stream collection contents can currently easily become inconsistent with the actual collection contents. We tried to orphan obsolete tracks but even so, this is messing up track indexes. It would be simpler to re-create our internal list whenever a new collection is received.

[Philippe Normand]

Created attachment 421326 [details]
Patch

[Philippe Normand]

Comment on attachment 421326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421326&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1567
> +    GST_DEBUG_OBJECT(pipeline(), "phil Collection %p len: %u", collection.get(), numStreams);

I'll reword this one.

[Alicia Boya García]

Comment on attachment 421326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421326&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1465
> +void MediaPlayerPrivateGStreamer::updateTracks(const GRefPtr<GstStreamCollection> streamCollection)

Did you mean `const GRefPtr<GstStreamCollection>?`

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1477
> +    // consistent with the collection contents, we can't reuse our old tracks.

Can you elaborate?

[Xabier Rodríguez Calvar]

Comment on attachment 421326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421326&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:448
> +    void updateTracks(const GRefPtr<GstStreamCollection>);

This should be a const GRefPtr&

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:513
> +    const char* sourceType = "";

We could better initialize to "unknown". DEBUG below could be more informative in case we don't hit the assert in a release build.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:556
> -static void webkitMediaStreamSrcPostStreamCollection(WebKitMediaStreamSrc* self)
> +void webkitMediaStreamSrcPostStreamCollection(WebKitMediaStreamSrc* self)

Do you need to drop the static? I does not look to me at a first glance.

> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:648
> +            if (!g_strcmp0(streamId, track.id().utf8().data())) {

There are String operators for this:

inline bool operator==(const String& a, const char* b) { return equal(a.impl(), reinterpret_cast<const LChar*>(b)); }
inline bool operator==(const char* a, const String& b) { return equal(reinterpret_cast<const LChar*>(a), b.impl()); }

[Philippe Normand]

Comment on attachment 421326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421326&action=review

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1477
>> +    // consistent with the collection contents, we can't reuse our old tracks.
> 
> Can you elaborate?

Sure! Taking the example of http/tests/media/hls/hls-audio-tracks.html, the player would receive various stream-collection messages and actually handle them from different threads, as this log shows:

0:00:00.943368778   340      0x210cde0 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1597:handleSyncMessage:<media-player-0> Received STREAM_COLLECTION message with upstream id "(null)" defining the following streams:
0:00:00.943407182   340 0x7fe1e8008cc0 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1597:handleSyncMessage:<media-player-0> Received STREAM_COLLECTION message with upstream id "decodebin3" defining the following streams:
0:00:00.943432352   340      0x210cde0 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1601:handleSyncMessage:<media-player-0> #0 audio f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_2
0:00:00.943467669   340 0x7fe1e8008cc0 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1601:handleSyncMessage:<media-player-0> #0 audio f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_1
0:00:00.943412535   340      0x1c85610 LOG        webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1800:handleMessage:<media-player-0> Message state-changed received from element audio-concat
0:00:00.943504040   340 0x7fe1e8008cc0 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1601:handleSyncMessage:<media-player-0> #1 audio f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_2
...
0:00:00.943771346   340      0x1c85610 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1500:updateTracks:<media-player-0> Processing a stream collection with 1 streams
0:00:00.943858510   340      0x1c85610 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1521:updateTracks:<media-player-0> Inspecting audio track with ID f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_2
0:00:00.943910641   340      0x1c85610 DEBUG      webkitmediaplayer TrackPrivateBaseGStreamer.cpp:113:tagsChanged: Inspecting track at index 0 with tags: (NULL)
...
0:00:00.955157540   340      0x21094c0 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1597:handleSyncMessage:<media-player-0> Received STREAM_COLLECTION message with upstream id "decodebin3" defining the following streams:
0:00:00.955224337   340      0x1c85610 LOG        webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1800:handleMessage:<media-player-0> Message state-changed received from element aconv
0:00:00.955236157   340      0x21094c0 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1601:handleSyncMessage:<media-player-0> #0 video f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_0:1/00000101
0:00:00.955290618   340      0x1c85610 LOG        webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1800:handleMessage:<media-player-0> Message state-changed received from element aqueue
0:00:00.955316558   340      0x21094c0 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1601:handleSyncMessage:<media-player-0> #1 audio f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_0:1/00000102
0:00:00.955372572   340      0x1c85610 LOG        webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1800:handleMessage:<media-player-0> Message state-changed received from element scaletempo0
0:00:00.955377016   340      0x21094c0 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1601:handleSyncMessage:<media-player-0> #2 audio f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_1
0:00:00.955426783   340      0x1c85610 LOG        webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1800:handleMessage:<media-player-0> Message state-changed received from element filter-convert
0:00:00.955457383   340      0x21094c0 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1601:handleSyncMessage:<media-player-0> #3 audio f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_2
0:00:00.955776957   340      0x1c85610 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1521:updateTracks:<media-player-0> Inspecting video track with ID f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_0:1/00000101
0:00:00.955839477   340      0x1c85610 DEBUG      webkitmediaplayer TrackPrivateBaseGStreamer.cpp:113:tagsChanged: Inspecting track at index 0 with tags: taglist, video-codec=(string)H.264;
0:00:00.956053700   340      0x1c85610 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1521:updateTracks:<media-player-0> Inspecting audio track with ID f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_0:1/00000102
0:00:00.956105627   340      0x1c85610 DEBUG      webkitmediaplayer TrackPrivateBaseGStreamer.cpp:113:tagsChanged: Inspecting track at index 0 with tags: taglist, audio-codec=(string)"MPEG-4\ AAC";
0:00:00.956134517   340      0x1c85610 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1463:playbin3SendSelectStreamsIfAppropriate:<media-player-0> Checking if to send SELECT_STREAMS, m_waitingForStreamsSelectedEvent = false, haveDifferentStreamIds = true, m_currentState = READY... shouldSendSelectStreams = false
0:00:00.956173911   340      0x1c85610 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1521:updateTracks:<media-player-0> Inspecting audio track with ID f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_1
0:00:00.956333746   340      0x1c85610 DEBUG      webkitmediaplayer TrackPrivateBaseGStreamer.cpp:113:tagsChanged: Inspecting track at index 1 with tags: taglist, private-data=(sample)00000000000daaca:None:c2VnbWVudCwgZmxhZ3M9KEdzdFNlZ21lbnRGbGFncylHU1RfU0VHTUVOVF9GTEFHX05PTkUsIHJhdGU9KGRvdWJsZSkxLCBhcHBsaWVkLXJhdGU9KGRvdWJsZSkxLCBmb3JtYXQ9KEdzdEZvcm1hdCl0aW1lLCBiYXNlPShndWludDY0KTAsIG9mZnNldD0oZ3VpbnQ2NCkwLCBzdGFydD0oZ3VpbnQ2NCkwLCBzdG9wPShndWludDY0KTE4NDQ2NzQ0MDczNzA5NTUxNjE1LCB0aW1lPShndWludDY0KTAsIHBvc2l0aW9uPShndWludDY0KTAsIGR1cmF0aW9uPShndWludDY0KTE4NDQ2NzQ0MDczNzA5NTUxNjE1OwA_:SUQzUHJpdmF0ZUZyYW1lLCBvd25lcj0oc3RyaW5nKWNvbS5hcHBsZS5zdHJlYW1pbmcudHJhbnNwb3J0U3RyZWFtVGltZXN0YW1wOwA_, nominal-bitrate=(uint)125659, audio-codec=(string)"MPEG-4\ AAC", minimum-bitrate=(uint)85788, maximum-bitrate=(uint)103703, bitrate=(uint)92303;
0:00:00.956402430   340      0x1c85610 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1521:updateTracks:<media-player-0> Inspecting audio track with ID f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_2
0:00:00.956425207   340      0x1c85610 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1525:updateTracks:<media-player-0> audio track with ID f0963b9f548a9f7bb591f70ba2b10f0c444b9d5f65181c654403b2a206edaf76/src_2 already exists, skipping

The track orphaning code would then kick-in and try to remove the tracks no longer present in the collection, but leaving indexes unchanged, so for instance if we received an initial collection with 2 audio streams (A1, A2) and then later on A2 is removed, A3 added we would end up with a final list of (A1, A3) where A1 has an index of 0 as expected, but A3 would have an index of 2 instead of 1.

The current MediaPlayer interface for track management allows only to append a new track, you can't insert at a given index, which actually is an issue for another test I started debugging.

[Philippe Normand]

Comment on attachment 421326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421326&action=review

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1465
>> +void MediaPlayerPrivateGStreamer::updateTracks(const GRefPtr<GstStreamCollection> streamCollection)
> 
> Did you mean `const GRefPtr<GstStreamCollection>?`

const GRefPtr<T>& indeed, that slipped through the crack :)

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:513
>> +    const char* sourceType = "";
> 
> We could better initialize to "unknown". DEBUG below could be more informative in case we don't hit the assert in a release build.

Yes good point!

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:556
>> +void webkitMediaStreamSrcPostStreamCollection(WebKitMediaStreamSrc* self)
> 
> Do you need to drop the static? I does not look to me at a first glance.

No need, good spot.

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:648
>> +            if (!g_strcmp0(streamId, track.id().utf8().data())) {
> 
> There are String operators for this:
> 
> inline bool operator==(const String& a, const char* b) { return equal(a.impl(), reinterpret_cast<const LChar*>(b)); }
> inline bool operator==(const char* a, const String& b) { return equal(reinterpret_cast<const LChar*>(a), b.impl()); }

Indeed! Good cleanup.

[Philippe Normand]

Created attachment 421522 [details]
Patch

[Alicia Boya García]

> for instance if we received an initial collection with 2 audio streams (A1, A2) and then later on A2 is removed, A3 added we would end up with a final list of (A1, A3) where A1 has an index of 0 as expected, but A3 would have an index of 2 instead of 1.

Is this something happening in actual streams or a consequence of bugs in GStreamer's handling of stream collections?

[Philippe Normand]

afaik any demuxer can send a variable amount of stream-collection messages and decodebin3 is expected to merge them together and the "app" is expected to clean-up internal mappings of the collection whenever a new one is received.

[EWS]

Committed r273644: <https://commits.webkit.org/r273644>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421522 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74861022>