| Summary: | [GStreamer] Some media stream tests are crashing | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||||
| Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | bugs-noreply, commit-queue, ews-bot+gtk-4, ews-watchlist, mcatanzaro, pnormand, tsaunier, webkit-bug-importer | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Linux | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Michael Catanzaro
2018-06-15 10:17:19 PDT
Also fast/mediastream/MediaStream-video-element-track-stop.html Created attachment 342940 [details]
Patch.
Note that I was not able to reproduce those crashes running the tests locally
or on our buildbox but I had a very similare segfault running MiniBrowser in
some rare cases and this patch seems/should to fix it. I think it is conceptually
correct/better anyway.
I am investigating the root cause of that issue, but the decodebin3 code is still
moving quite fast and not being able to reproduce properly myself is annoying.
The approach taken here is to make sure to not do "stupid things" on our end so
that we avoid hitting the race in decodebin3, still the bug is still there.
Attachment 342940 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:17: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WebCore/ChangeLog:18: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 342941 [details]
Patch.
Fix style in ChangeLog
Comment on attachment 342941 [details]
Patch.
I am digging further and might have found something else, do not review yet.
Created attachment 342970 [details]
Patch.
Also avoid spurious SELECT_STREAM for GstStream marked with GST_STREAM_FLAG_SELECT
as this flags makes it so decodebin3 selects them by default.
Comment on attachment 342970 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=342970&action=review > Source/WebCore/ChangeLog:16 > + Should fix following flaks: flakes Comment on attachment 342970 [details] Patch. Attachment 342970 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8245539 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html http/tests/security/contentSecurityPolicy/video-redirect-allowed.html Created attachment 343044 [details]
Archive of layout-test-results from ews204 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Build Bot from comment #9) > Created attachment 343044 [details] > Archive of layout-test-results from ews204 for win-future > > The attached test failures were seen while running run-webkit-tests on the > win-ews. > Bot: ews204 Port: win-future Platform: > CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit Seems totally unrelated to me. Comment on attachment 342970 [details]
Patch.
Looks good! Would be nice to avoid the goto though, probably by using a Vector for the streams list and only converting it to a GList when needed. Would it be possible?
Created attachment 343328 [details]
Patch.
Add a missing check to avoid calling gst_stream_collection_get () on a NULL collection
when using playbin2. Removed the goto by using a Vector<String> and convert that to a
GList only when required.
Comment on attachment 343328 [details]
Patch.
Thanks :)
Comment on attachment 343328 [details] Patch. Clearing flags on attachment: 343328 Committed r233081: <https://trac.webkit.org/changeset/233081> All reviewed patches have been landed. Closing bug. Comment on attachment 343328 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=343328&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:755 > + GstStream* stream = nullptr; > + > + if (!m_isLegacyPlaybin) { > + gst_stream_collection_get_stream(m_streamCollection.get(), index); > + if (!stream) { > + GST_WARNING_OBJECT(pipeline(), "No stream to select at index %u", index); > + return; > + } Was this actually tested? :) I've just noticed now that stream is never set :D Comment on attachment 343328 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=343328&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:755 >> + } > > Was this actually tested? :) > I've just noticed now that stream is never set :D Rooops, sorry about that, stupid refactoring before proposing again. |