Bug 164022 – REGRESSION(r207879-207891): [GStreamer] Introduced many layout test failures and crashes, bots exiting early

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Michael Catanzaro]

Layout test results for r207877:

Failed 2 jsc tests failed 131 failures 68 new passes 14 flakes 0 api tests failed, 0 crashed, 1 timed out, 22 skipped

For r207891, the next test run:

Failed 2 jsc tests failed exiting early after 16 crashes and 34 timeouts. 45946 tests run. 187 failures 35 new passes 1 api tests failed, 0 crashed, 0 timed out, 22 skipped

All of the commits in this range are GStreamer patches. I don't want to slow down your work by rolling out these patches, but I do ask that you please review and update the test expectations to make sure the bot is as happy as it was before this landed.

[Carlos Garcia Campos]

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.

[Enrique Ocaña]

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.

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

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 ]

[Xabier Rodríguez Calvar]

Created attachment 293012 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 293012 [details]
Patch for landing

Clearing flags on attachment: 293012

Committed r207989: <http://trac.webkit.org/changeset/207989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

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.

[Enrique Ocaña]

Created attachment 296187 [details]
Patch

[Michael Catanzaro]

Warning: your patch accidentally removes an old changelog entry!

[Enrique Ocaña]

(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.

[Xabier Rodríguez Calvar]

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  <[e-mail]>
> -

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.

[Xabier Rodríguez Calvar]

(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?

[Enrique Ocaña]

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.

[Enrique Ocaña]

(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 ]

-------------------------

[Enrique Ocaña]

Created attachment 296924 [details]
Patch

[Xabier Rodríguez Calvar]

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 { }

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

And to be explicit, I acknowledge the rest of your comments.

[Enrique Ocaña]

Created attachment 297001 [details]
Patch

[Enrique Ocaña]

(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]

[Xabier Rodríguez Calvar]

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.

[Enrique Ocaña]

Created attachment 297002 [details]
Patch

[Enrique Ocaña]

Created attachment 297003 [details]
Patch

[Enrique Ocaña]

(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

[WebKit Commit Bot]

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

[WebKit Commit Bot]

Comment on attachment 297003 [details]
Patch

Clearing flags on attachment: 297003

Committed r209797: <http://trac.webkit.org/changeset/209797>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.