| Summary: | [GStreamer] Fix the way GstStreamCollection is handled | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Thibault Saunier <tsaunier> |
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | commit-queue, pnormand, tsaunier |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Nightly Build | ||
| Hardware: | All | ||
| OS: | All | ||
| Bug Depends on: | |||
| Bug Blocks: | 185787 | ||
| Attachments: | |||
|
Description
Thibault Saunier
2018-04-13 07:38:17 PDT
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.
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.
I found an issue with that patch, I will update as soon as I get it fixed. 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. (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. 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.
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? (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. 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.
What changed since the previous iteration? (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.) 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.
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. 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. Comment on attachment 342084 [details] Patch. Clearing flags on attachment: 342084 Committed r232579: <https://trac.webkit.org/changeset/232579> All reviewed patches have been landed. Closing bug. |