| Summary: | REGRESSION(r207879-207891): [GStreamer] Introduced many layout test failures and crashes, bots exiting early | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||||||
| Component: | Media | Assignee: | Xabier Rodríguez Calvar <calvaris> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | bugs-noreply, calvaris, cgarcia, commit-queue, eocanha, mcatanzaro | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | Other | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Linux | ||||||||||||||||
| Bug Depends on: | 164116 | ||||||||||||||||
| Bug Blocks: | 143233, 143704, 143706, 158925, 160019, 162719, 163514, 163515, 163516, 163824 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Michael Catanzaro
2016-10-26 10:43:00 PDT
The release bot has been exiting early since yesterday. If the problem is encrypted media, then we can simply disable it until it's ready to avoid roll outs, or more gardening work. Otherwise I'm sorry but we should probably roll out the patches. Next time, before introducing a large change like this one, run the tests before landing. I'm terribly sorry for the inconvenience. I'm having a look into this issue a soon as I get home (~2 hours). Anyway, probably the first step will be disabling MEDIA_SOURCE and LEGACY_ENCRYPTED_MEDIA. (In reply to comment #1) > The release bot has been exiting early since yesterday. If the problem is > encrypted media, then we can simply disable it until it's ready to avoid > roll outs, or more gardening work. Otherwise I'm sorry but we should > probably roll out the patches. Next time, before introducing a large change > like this one, run the tests before landing. I think rolling out the patches is overkill. It should be enough with skipping MSE and EME tests for the time being. I will submit a patch soon. I am removing these lines from the gtk expectations, please bear them in mind when reactiving them again. webkit.org/b/99065 media/media-source/media-source-append-failed.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-multiple-initialization-segments.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-append-nonsync-sample-after-abort.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-duplicate-seeked.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-canplaythrough.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-duration-after-append.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-end-of-stream-buffered.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-end-of-stream.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-fudge-factor.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-monitor-source-buffers.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-overlapping-append.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-overlapping-decodetime.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-play.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-remove.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-seek-complete.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-track-enabled.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-tracks.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-video-playback-quality.html [ Skip ] webkit.org/b/99065 media/media-source/media-source-seek-detach-crash.html [ Skip ] webkit.org/b/143233 media/media-source/media-source-append-buffer-with-append-window.html [ Pass Failure ] webkit.org/b/143704 media/media-source/media-source-append-media-segment-without-init.html [ Failure Pass ] webkit.org/b/142491 media/media-source/media-source-stalled-holds-sleep-assertion.html [ Timeout ] webkit.org/b/143706 media/media-source/media-source-end-of-stream-readyState.html [ Failure ] webkit.org/b/158925 media/media-source/media-source-loadedmetada-with-two-sourcebuffers.html [ Timeout ] webkit.org/b/160019 media/media-source/media-source-small-gap.html [ Timeout ] webkit.org/b/163514 media/media-source/media-source-sequence-timestamps.html [ Failure ] webkit.org/b/163515 media/media-source/media-source-stpp-crash.html [ Failure ] webkit.org/b/163516 media/media-source/media-source-timeoffset.html [ Failure ] webkit.org/b/163824 media/media-source/media-source-init-segment-duration.html [ Failure ] Created attachment 293012 [details]
Patch for landing
Comment on attachment 293012 [details] Patch for landing Clearing flags on attachment: 293012 Committed r207989: <http://trac.webkit.org/changeset/207989> All reviewed patches have been landed. Closing bug. (In reply to comment #2) > I'm terribly sorry for the inconvenience. I'm having a look into this issue > a soon as I get home (~2 hours). Anyway, probably the first step will be > disabling MEDIA_SOURCE and LEGACY_ENCRYPTED_MEDIA. No problem, it's to be expected when landing a big new feature. (In reply to comment #6) > Comment on attachment 293012 [details] > Patch for landing > > Clearing flags on attachment: 293012 > > Committed r207989: <http://trac.webkit.org/changeset/207989> Good job, it worked. Note that this does hide many crashes. Enrique, you might want to look at the crash logs from the release bot to see what it was doing. They are not all the same. However this bug needs to stay open until the new test expectation is removed: webkit.org/b/164022 media/media-source [ Skip ] It would be better to reference a different bug (preferably a generic "implement MSE" bug) in the expectation. Created attachment 296187 [details]
Patch
Warning: your patch accidentally removes an old changelog entry! (In reply to comment #11) > Warning: your patch accidentally removes an old changelog entry! Hmmm... I have to double-check on top of where I'm applying the branch I use to produce the patch, because there seems to be a discrepancy between how git and svn produce the diffs here. Comment on attachment 296187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296187&action=review > Source/WebCore/ChangeLog:53 > + * platform/graphics/gstreamer/mse/AppendPipeline.h: > + New parameter. This seems to be misaligned and the comment says nothing meaningful. Either remove it or specify it. > Source/WebCore/ChangeLog:60 > + New method. Which > Source/WebCore/ChangeLog:-25993 > - [GStreamer] Drain query support > - https://bugs.webkit.org/show_bug.cgi?id=162872 > - > - Reviewed by Žan Doberšek. > - > - Under some circumstances, GStreamer deadlocks completely during MSE seeks on OpenGL ES > - systems. This is because the video sink still holds samples. Proper DRAIN support > - fixes this issue and is also good for all the platforms, not only OpenGL ES. > - > - * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp: > - (WebCore::MediaPlayerPrivateGStreamerBase::triggerDrain): > - (WebCore::MediaPlayerPrivateGStreamerBase::drainCallback): > - (WebCore::MediaPlayerPrivateGStreamerBase::createVideoSink): > - (WebCore::MediaPlayerPrivateGStreamerBase::createVideoSinkGL): Deleted. > - * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h: > - * platform/graphics/gstreamer/VideoSinkGStreamer.cpp: > - (VideoRenderRequestScheduler::drain): > - (webkitVideoSinkQuery): > - (webkitVideoSinkEvent): > - (webkit_video_sink_class_init): > - (VideoRenderRequestScheduler::stop): Deleted. > - (webkitVideoSinkProposeAllocation): Deleted. > - > -2016-10-26 Enrique Ocaña González <eocanha@igalia.com> > - Cannot remove changelog entries. > Source/WebCore/platform/graphics/MediaPlayer.cpp:239 > +#if USE(GSTREAMER) > + if (Settings::isGStreamerEnabled()) { > +#endif > + PlatformMediaEngineClassName::registerMediaEngine(addMediaEngine); > +#if USE(GSTREAMER) > + } > +#endif I was about to tell you to use a #if #else here but I realized that having just one instruction you can have the flagged if without the {. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:535 > + m_presentationSize = WebCore::FloatSize(); Just a question: just wonder why you decided to switch some other types from float to double and you prefer to keep this one. > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:733 > - GST_ERROR("Unsupported or unknown stream type"); > - ASSERT_NOT_REACHED(); > + GST_ERROR("Unsupported stream type or codec"); Why do you remove the assertion? > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:885 > + gulong* probeId = g_new0(gulong, 1); > + *probeId = gst_pad_add_probe(demuxerSrcPad, GST_PAD_PROBE_TYPE_BUFFER, reinterpret_cast<GstPadProbeCallback>(appendPipelineDemuxerBlackHolePadProbe), nullptr, nullptr); > + g_object_set_data(G_OBJECT(demuxerSrcPad), "blackHoleProbeId", probeId); Maybe we can save this memory allocation with GUINT_TO_POINTER() and GPOINTER_TO_UINT(). > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1001 > + { > + GUniquePtr<gchar> strcaps(gst_caps_to_string(caps.get())); > + GST_DEBUG("Unsupported track codec: %s", strcaps.get()); > + } Please, flag this under GStreamer debug being enabled. Converting caps to string is a non trivial operation we could end up saving if debug is not enabled at compile time. > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:769 > + for (const auto& pattern : supportedCodecs) { > + if (isCodecSupported = !fnmatch(pattern, codecData, 0)) > + break; > + } I think you don't need the { } > Source/WebKit2/UIProcess/API/C/WKPreferencesRef.h:198 > +// Defaults to true. > +WK_EXPORT void WKPreferencesSetGStreamerEnabled(WKPreferencesRef preferencesRef, bool enabled); > +WK_EXPORT bool WKPreferencesGetGStreamerEnabled(WKPreferencesRef preferencesRef); > + Do we need to export this to the C API? Can't it be kept just for the internals? > LayoutTests/media/media-source/media-source-resize.html:51 > + // First resize can be 0x0 (Mac) or 640x480 (GTK+) > + if (video.videoWidth > 0 && video.videoHeight > 0) { > + testExpected('video.videoWidth', 640); > + testExpected('video.videoHeight', 480); > + endTest(); > + } Why do we want to stop the test here instead of waiting for the update, etc? > LayoutTests/platform/gtk/TestExpectations:238 > +webkit.org/b/165394 media/media-source/media-source-seek-detach-crash.html [ Timeout ] I think it is better to Skip the test if it is going to timeout, test run is faster. (In reply to comment #4) > I will submit a patch soon. I am removing these lines from the gtk > expectations, please bear them in mind when reactiving them again. > > ... And I assume that by not adding these expectations again, they work, right? Comment on attachment 296187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296187&action=review >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:535 >> + m_presentationSize = WebCore::FloatSize(); > > Just a question: just wonder why you decided to switch some other types from float to double and you prefer to keep this one. The attributes I changed were related to time management, where lack of precission was causing troubles in the tests. This m_presentationSize attribute is dimensions related, its current type isn't causing any problem with the tests and, what's more important, must be FloatSize to meet the requirements of the MediaSample interface. >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:733 >> + GST_ERROR("Unsupported stream type or codec"); > > Why do you remove the assertion? Because before the "unsupported stream type or codec" condition wasn't reported to the upper layer and we didn't know what to do with it (thus the assertion), but now it's "supported" (we just don't generate any track and let SourceBuffer do its job and trigger an append error[1]. [1] https://github.com/WebKit/webkit/blob/81708ead2798a7c6b32ea99b5678d989219e2118/Source/WebCore/Modules/mediasource/SourceBuffer.cpp#L1012 >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:885 >> + g_object_set_data(G_OBJECT(demuxerSrcPad), "blackHoleProbeId", probeId); > > Maybe we can save this memory allocation with GUINT_TO_POINTER() and GPOINTER_TO_UINT(). No, that can be a source of future problems. gst_pad_add_probe() returns a gulong. If we demote it to a guint to let GUINT_TO_POINTER() fit it in a pointer, we can corrupt its value. We would end up having a hard debugging time trying to find where those random "invalid probe id" asserts are coming from. >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1001 >> + } > > Please, flag this under GStreamer debug being enabled. Converting caps to string is a non trivial operation we could end up saving if debug is not enabled at compile time. I can do that if you really want, but this piece of code is going to be run very rarely (at most 2 or 3 times per played video, that's once per track, and only in situations where we have very bad luck and don't support the codecs). On the other hand, having the info about what's the specific failing codec can be very useful even in release builds targeted to end users. >> Source/WebKit2/UIProcess/API/C/WKPreferencesRef.h:198 >> + > > Do we need to export this to the C API? Can't it be kept just for the internals? Yes, thanks for the hint, keeping it internal is enough. I had just followed the changes originally done for AVFoundationEnabled without really reflecting about if replicating the changes on each layer was really suitable for this case or not. >> LayoutTests/media/media-source/media-source-resize.html:51 >> + } > > Why do we want to stop the test here instead of waiting for the update, etc? The original (Mac) test doesn't wait for the update of the append which triggers the resize either (the second append, containing media data). They just wait for the update of the first (init segment) append because that's a natural step before wanting to start doing the second append, the one really important for them. I've tried to change the test the least possible (actually, just adding the "if" condition). Waiting for an update after the right size is detected would imply deeper changes in the test code and potential regressions on Mac with no additional gain. After all, there are probably better tests than this one to test for the "update" event. (In reply to comment #14) > (In reply to comment #4) > > I will submit a patch soon. I am removing these lines from the gtk > > expectations, please bear them in mind when reactiving them again. > > > > ... > > And I assume that by not adding these expectations again, they work, right? Yes, all the 40 LayoutTests/media/media-source tests pass except the only one I'm skipping (media-source-seek-detach-crash.html): ------------------------- $ NUMBER_OF_PROCESSORS=8 Tools/Scripts/run-webkit-tests --gtk --no-new-test-results --fully-parallel --repeat-each=1 --iterations=1 --skipped=ignore \ media/media-source/media-source-seek-back.html \ media/media-source/media-source-video-playback-quality.html \ media/media-source/media-source-remove-crash.html \ media/media-source/media-source-sequence-timestamps.html \ media/media-source/media-source-remove.html \ media/media-source/media-source-fudge-factor.html \ media/media-source/media-source-append-media-segment-without-init.html \ media/media-source/media-source-addsourcebuffer.html \ media/media-source/media-source-sample-wrong-track-id.html \ media/media-source/media-source-duplicate-seeked.html \ media/media-source/media-source-append-failed.html \ media/media-source/media-source-stalled-holds-sleep-assertion.html \ media/media-source/media-source-canplaythrough.html \ media/media-source/media-source-overlapping-append-buffered.html \ media/media-source/media-source-init-segment-duration.html \ media/media-source/media-source-loadedmetada-with-two-sourcebuffers.html \ media/media-source/media-source-end-of-stream.html \ media/media-source/media-source-track-enabled.html \ media/media-source/media-source-append-buffer.html \ media/media-source/media-source-resize.html \ media/media-source/media-source-seek-complete.html \ media/media-source/media-source-seek-detach-crash.html \ media/media-source/media-source-overlapping-decodetime.html \ media/media-source/media-source-small-gap.html \ media/media-source/media-source-overlapping-append.html \ media/media-source/media-source-duration-after-append.html \ media/media-source/media-source-timeoffset.html \ media/media-source/media-source-delaying-load-event.html \ media/media-source/media-source-tracks.html \ media/media-source/media-source-append-nonsync-sample-after-abort.html \ media/media-source/media-source-play.html \ media/media-source/media-source-multiple-initialization-segments.html \ media/media-source/media-source-closed.html \ media/media-source/media-source-monitor-source-buffers.html \ media/media-source/media-source-fastseek.html \ media/media-source/media-source-end-of-stream-readyState.html \ media/media-source/media-source-end-of-stream-buffered.html \ media/media-source/media-source-stpp-crash.html \ media/media-source/media-source-abort-resets-parser.html \ media/media-source/media-source-append-buffer-with-append-window.html \ fast/history/page-cache-media-source-closed-2.html \ fast/history/page-cache-media-source-closed.html \ fast/history/page-cache-media-source-opened.html \ http/tests/media/media-source [...] Found 44 tests; running 44, skipping 0. Running 44 tests Running 8 WebKitTestRunners in parallel. [44/44] media/media-source/media-source-seek-detach-crash.html failed unexpectedly (test timed out, text diff) Retrying 1 unexpected failure ... Running 1 WebKitTestRunner. [1/1] media/media-source/media-source-seek-detach-crash.html failed unexpectedly (test timed out, text diff) 43 tests ran as expected, 1 didn't: Regressions: Unexpected timeouts (1) media/media-source/media-source-seek-detach-crash.html [ Timeout ] ------------------------- Created attachment 296924 [details]
Patch
Comment on attachment 296924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296924&action=review > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:769 > + for (const auto& pattern : supportedCodecs) { > + if (isCodecSupported = !fnmatch(pattern, codecData, 0)) > + break; > + } I think you don't need the { } (In reply to comment #15) > >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:885 > >> + g_object_set_data(G_OBJECT(demuxerSrcPad), "blackHoleProbeId", probeId); > > > > Maybe we can save this memory allocation with GUINT_TO_POINTER() and GPOINTER_TO_UINT(). > > No, that can be a source of future problems. gst_pad_add_probe() returns a > gulong. If we demote it to a guint to let GUINT_TO_POINTER() fit it in a > pointer, we can corrupt its value. We would end up having a hard debugging > time trying to find where those random "invalid probe id" asserts are coming > from. Maybe it is worth a try to investigate saving it with a guint as temporary buffer. And to be explicit, I acknowledge the rest of your comments. Created attachment 297001 [details]
Patch
(In reply to comment #18) > Comment on attachment 296924 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=296924&action=review > > > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:769 > > + for (const auto& pattern : supportedCodecs) { > > + if (isCodecSupported = !fnmatch(pattern, codecData, 0)) > > + break; > > + } > > I think you don't need the { } Sorry, I was going to answer this before but I forgot. The style checker doesn't like the block without braces. That's why they're there: ERROR: Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:766: Multi line control clauses should use braces. [whitespace/braces] [4] Comment on attachment 297001 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297001&action=review Thanks! Good job! > Source/WTF/wtf/glib/GLibUtilities.h:34 > +#define GPOINTER_TO_ULONG(p) ((gsize) (p)) I guess this meant (gulong) > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1029 > + GST_DEBUG("Not disconnecting demuxer src pad because it wasn't linked"); If this is uncommon, this should be a GST_WARNING. Sorry for not seeing this before. Created attachment 297002 [details]
Patch
Created attachment 297003 [details]
Patch
(In reply to comment #23) > Comment on attachment 297001 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297001&action=review > > Thanks! Good job! > > > Source/WTF/wtf/glib/GLibUtilities.h:34 > > +#define GPOINTER_TO_ULONG(p) ((gsize) (p)) > > I guess this meant (gulong) > > > Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:1029 > > + GST_DEBUG("Not disconnecting demuxer src pad because it wasn't linked"); > > If this is uncommon, this should be a GST_WARNING. > > Sorry for not seeing this before. Both comments are addressed in https://bugs.webkit.org/attachment.cgi?id=297003 This patch should only be committed after the one from https://bugs.webkit.org/show_bug.cgi?id=164116 Comment on attachment 297003 [details] Patch Rejecting attachment 297003 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 297003, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ca3281fe4fafc2384b75ad3cfb989278e9d 4bcf0ec18511bff587454250363acc9ee61bfe98 M Tools Current branch master is up to date. ERROR: Not all changes have been committed into SVN, however the committed ones (if any) seem to be successfully integrated into the working tree. Please see the above messages for details. Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Total errors found: 0 in 1 files Full output: http://webkit-queues.webkit.org/results/2719852 Comment on attachment 297003 [details] Patch Clearing flags on attachment: 297003 Committed r209797: <http://trac.webkit.org/changeset/209797> All reviewed patches have been landed. Closing bug. |