Bug 184588 – [GStreamer] Fix the way GstStreamCollection is handled

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Thibault Saunier]

See commit message.

[Thibault Saunier]

Created attachment 337886 [details]
[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.

[Thibault Saunier]

Created attachment 337887 [details]
[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.

[Thibault Saunier]

I found an issue with that patch, I will update as soon as I get it fixed.

[Philippe Normand]

Comment on attachment 337887 [details]
[GStreamer] Fix the way GstStreamCollection is handled

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-693
> -    m_hasAudio = !validAudioStreams.isEmpty();
> -    m_hasVideo = !validVideoStreams.isEmpty();

These are not set anymore, it seems.

[Thibault Saunier]

(In reply to Philippe Normand from comment #4)
> Comment on attachment 337887 [details]
> [GStreamer] Fix the way GstStreamCollection is handled
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337887&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-693
> > -    m_hasAudio = !validAudioStreams.isEmpty();
> > -    m_hasVideo = !validVideoStreams.isEmpty();
> 
> These are not set anymore, it seems.

Oops, that was supposed to be done in the macro.

[Thibault Saunier]

Created attachment 338581 [details]
[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.

[Philippe Normand]

Comment on attachment 338581 [details]
[GStreamer] Fix the way GstStreamCollection is handled

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-646
> -                unsigned localIndex = i - validVideoStreams.size() - validTextStreams.size();
> -                if (localIndex < m_audioTracks.size()) {
> -                    if (m_audioTracks.contains(streamId))
> -                        continue;

Hum, the reason for this was to avoid adding a track that exists already... Removing this code might lead to unintended track-added (or something) <video> events. Are you sure this patch doesn't break any test?

[Thibault Saunier]

(In reply to Philippe Normand from comment #7)
> Comment on attachment 338581 [details]
> [GStreamer] Fix the way GstStreamCollection is handled
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338581&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-646
> > -                unsigned localIndex = i - validVideoStreams.size() - validTextStreams.size();
> > -                if (localIndex < m_audioTracks.size()) {
> > -                    if (m_audioTracks.contains(streamId))
> > -                        continue;
> 
> Hum, the reason for this was to avoid adding a track that exists already...
> Removing this code might lead to unintended track-added (or something)
> <video> events. Are you sure this patch doesn't break any test?

Right, thing is that now my first step is to remove all tracks previously configure tracks, those track are being obseleted by the new StreamCollection, so they should not be taken in consideration.

I will re run the tests again, do not merge yet.

[Thibault Saunier]

Created attachment 339991 [details]
[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.

[Philippe Normand]

What changed since the previous iteration?

[Thibault Saunier]

(In reply to Philippe Normand from comment #10)
> What changed since the previous iteration?

Supposedly a rebase after the patch from #185479 - I am still working on that one though I think it is correct already. I will let you know when I am fully happy with it. (Sorry for the noise.)

[Thibault Saunier]

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

while using it (especially in the MediaStream use case).

This new patch should be good to review.

[Philippe Normand]

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

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:700
> +#undef CLEAR_TRACKS

This looks like a duplicate.

[Thibault Saunier]

Created attachment 342084 [details]
Patch.

(In reply to Philippe Normand from comment #13)
> Comment on attachment 341757 [details]
> Rebased version of the patch taking into account previous comments and
> fixing little details
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341757&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:700
> > +#undef CLEAR_TRACKS
> 
> This looks like a duplicate.

Fixed.

[WebKit Commit Bot]

Comment on attachment 342084 [details]
Patch.

Clearing flags on attachment: 342084

Committed r232579: <https://trac.webkit.org/changeset/232579>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.