Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GStreamer][MSE] Append pipeline is counting streams incorrectly with new init segments #12114

Merged
merged 1 commit into from
May 30, 2023

Conversation

calvaris
Copy link
Contributor

@calvaris calvaris commented Mar 29, 2023

847e28d

[GStreamer][MSE] Append pipeline is counting streams incorrectly with new init segments
https://bugs.webkit.org/show_bug.cgi?id=254664

Reviewed by Alicia Boya Garcia.

When you append another init segment we match GStreamer pads against tracks but we are not counting properly as we are
skipping the pads that are already linked we can end up with less pads than tracks. Besides it is against the spec. We
need to match the number of tracks and track IDs if there are more than one for a kind.

* LayoutTests/media/media-source/media-source-seek-detach-crash.html: Fail the test earlier instead of waiting for a
timeout.
* LayoutTests/platform/glib/TestExpectations: Unmark
media/encrypted-media/encrypted-media-append-encrypted-unencrypted.html as failure. It works now.
* Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:
(WebCore::AppendPipeline::didReceiveInitializationSegment):
(WebCore::AppendPipeline::recycleTrackForPad):

Canonical link: https://commits.webkit.org/264676@main

b483c02

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl   πŸ§ͺ ios-wk2   πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1   πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios   πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@calvaris calvaris requested a review from philn as a code owner March 29, 2023 15:26
@calvaris calvaris self-assigned this Mar 29, 2023
@calvaris calvaris added the Media Bugs related to the HTML 5 Media elements. label Mar 29, 2023
@philn philn requested review from ntrrgc and removed request for philn March 29, 2023 15:31
@calvaris calvaris marked this pull request as draft March 29, 2023 15:36
@calvaris
Copy link
Contributor Author

Some tests fail. I need to investigate.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 29, 2023
@calvaris calvaris removed the merging-blocked Applied to prevent a change from being merged label May 29, 2023
@calvaris calvaris marked this pull request as ready for review May 29, 2023 14:21
@calvaris calvaris requested a review from philn May 29, 2023 14:21
Copy link
Contributor

@ntrrgc ntrrgc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this patch.

gst_pad_get_stream_id() is a great find. I have been able to confirm through looking at the source code that both qtdemux and matroskademux emit the track IDs found in the containers metadata (namely the tkhd box for MP4 and the TrackNumber:TrackUID pair for Matroska/WebM), and this is not a new feature, already being present in GStreamer 1.10 and earlier.

So far, for lack of a way to get these bits of information, AppendPipeline has been using pad indices as track IDs (see AppendPipeline::generateTrackId()), so the first video pad gets V1, the second V2 and so on...

Now, taking this into consideration, it makes sense to update the code and replace the autogenerated track IDs with actual track IDs from GStreamer. I also noticed that the MediaSource spec requires track IDs exposed to the user to be distinct but doesn't specify they need to match the exact values in tkhd (although the engine internally must match those).

Something I've been wondering about this patch is, how are we handling recycling pads? This is the current code for matching tracks and pads:

// Try to find a matching pre-existing track. Ideally, tracks should be matched by track ID, but matching by type
// is provided as a fallback -- which will be used, since we don't have a way to fetch those from GStreamer at the moment.
Track* matchingTrack = nullptr;
for (std::unique_ptr<Track>& track : m_tracks) {
    if (track->streamType != streamType)
        continue;
    matchingTrack = &*track;
    if (track->trackId == trackId)
        break;
}

Assuming pad indices are consistent, our code should still work. For that reason, this patch LGTM as it is. Feel free to rework the track IDs to use the data from GStreamer stream IDs if you want, but that can be a future patch as well, leaving this fix contained.

@calvaris calvaris added the merge-queue Applied to send a pull request to merge-queue label May 30, 2023
… new init segments

https://bugs.webkit.org/show_bug.cgi?id=254664

Reviewed by Alicia Boya Garcia.

When you append another init segment we match GStreamer pads against tracks but we are not counting properly as we are
skipping the pads that are already linked we can end up with less pads than tracks. Besides it is against the spec. We
need to match the number of tracks and track IDs if there are more than one for a kind.

* LayoutTests/media/media-source/media-source-seek-detach-crash.html: Fail the test earlier instead of waiting for a
timeout.
* LayoutTests/platform/glib/TestExpectations: Unmark
media/encrypted-media/encrypted-media-append-encrypted-unencrypted.html as failure. It works now.
* Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:
(WebCore::AppendPipeline::didReceiveInitializationSegment):
(WebCore::AppendPipeline::recycleTrackForPad):

Canonical link: https://commits.webkit.org/264676@main
@webkit-commit-queue
Copy link
Collaborator

Committed 264676@main (847e28d): https://commits.webkit.org/264676@main

Reviewed commits have been landed. Closing PR #12114 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 30, 2023
@webkit-commit-queue webkit-commit-queue merged commit 847e28d into WebKit:main May 30, 2023
@calvaris calvaris deleted the eng/254664 branch May 30, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Media Bugs related to the HTML 5 Media elements.
Projects
None yet
5 participants