164116 2016-10-28 02:32:34 -0700 [GStreamer][MSE] Fix player private selection when MSE is enabled 2016-12-14 02:50:39 -0800 1 1 1 Unclassified WebKit Platform WebKit Local Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 164022 1 eocanha eocanha calvaris commit-queue pnormand oldest_to_newest 1245504 0 eocanha 2016-10-28 02:32:34 -0700 It looks like the player private selection mechanism doesn't rely exclusively on the supportsType() function. Even when supportsType() returns false (a particular player declines to be selected to play a content) MediaPlayer::loadWithNextMediaEngine() can still use nextMediaEngine() and select a completely unappropriate player private for that kind of content. In that case, the expected behaviour (not met by the MSE private player) is to set networkState to MediaPlayer::FormatError. That would force a different player to be used. This misbehaviour was the cause of the massive test crashes reported in #164022, as the MSE player was being selected to play regular videos despite not being properly configured with a MediaSourcePrivateClient (this can't happen for an MSE video). 1245505 1 293131 eocanha 2016-10-28 02:37:26 -0700 Created attachment 293131 Patch 1245510 2 293131 calvaris 2016-10-28 02:52:21 -0700 Comment on attachment 293131 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293131&action=review Anyway, if this fix only the crashes, I would wait for a more complete solution unless the solution for other bugs is incremental. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:275 > + // Properly fail so the global MediaPlayer tries to fallback to the next MediaPlayerPrivate. Make this comment a GST_TRACE. That way we can follow what is going on. > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:129 > + // Properly fail so the global MediaPlayer tries to fallback to the next MediaPlayerPrivate. Ditto > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:136 > + if (UNLIKELY(!initializeGStreamerAndRegisterWebKitMSEElement())) > + return; ASSERT_NOT_REACHED(). Maybe it could be interesting to do the same in other places, but it is not urgent and can be done in other bug. 1245514 3 293131 eocanha 2016-10-28 03:04:16 -0700 Comment on attachment 293131 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293131&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:275 >> + // Properly fail so the global MediaPlayer tries to fallback to the next MediaPlayerPrivate. > > Make this comment a GST_TRACE. That way we can follow what is going on. Are you sure we can use GStreamer logging facilities (potentially) *before* having initialized GStreamer? 1245523 4 pnormand 2016-10-28 03:39:49 -0700 gst_init() needs to be called before using the logging macros. 1245560 5 eocanha 2016-10-28 06:01:55 -0700 (In reply to comment #4) > gst_init() needs to be called before using the logging macros. But in this case the logging is to be done before initializing GStreamer. Is it still worth to initialize GStreamer to log something that is supposed to be part of the normal behaviour of the player? (yield other players when the content isn't appropriate for this one). In the OWR player, for instance, nothing is logged: https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp#L126 1245576 6 pnormand 2016-10-28 06:45:18 -0700 (In reply to comment #5) > (In reply to comment #4) > > gst_init() needs to be called before using the logging macros. > > But in this case the logging is to be done before initializing GStreamer. Is > it still worth to initialize GStreamer to log something that is supposed to > be part of the normal behaviour of the player? (yield other players when the > content isn't appropriate for this one). No. > > In the OWR player, for instance, nothing is logged: > > https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/ > graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp#L126 1246390 7 calvaris 2016-10-31 02:26:38 -0700 (In reply to comment #3) > Are you sure we can use GStreamer logging facilities (potentially) *before* > having initialized GStreamer? Forget it then. 1251401 8 pnormand 2016-11-15 00:18:29 -0800 Does the patch needs an update then? It'd be great to re-enable MSE on the bots ASAP :) 1251886 9 calvaris 2016-11-16 09:14:19 -0800 (In reply to comment #8) > Does the patch needs an update then? According to Quique, this only covers the crashes but there are still a couple of tests that fail so the patch is incomplete. We could land this, but then we would need another bug[s] to track the other failures that Quique reported. > It'd be great to re-enable MSE on the bots ASAP :) Oh yes, we could even make the option public so that you could see that it is enabled when you build it ;) 1256426 10 eocanha 2016-12-05 12:09:52 -0800 The rest of the test fixes are going to be submitted in other bugs. Can this patch have r+ in its current state, even if it's cq+ later? 1259682 11 293131 commit-queue 2016-12-14 02:37:06 -0800 Comment on attachment 293131 Patch Clearing flags on attachment: 293131 Committed r209796: <http://trac.webkit.org/changeset/209796> 1259683 12 commit-queue 2016-12-14 02:37:10 -0800 All reviewed patches have been landed. Closing bug. 293131 2016-10-28 02:37:26 -0700 2016-12-14 02:50:39 -0800 Patch bug-164116-20161028093335.patch text/plain 3010 eocanha U3VidmVyc2lvbiBSZXZpc2lvbjogMjA3OTY3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMWQ4N2I0N2NhNjU5ZjM5 NTY4NTcwMjQ4MzhhMDcxM2RlZTJkNjA5MC4uZmM5MjA3ZDc5NmZmOTBlNTgxMWEyOGI3YWNhZWYx NzMyYTg2MTU5ZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDE2LTEwLTI4ICBFbnJp cXVlIE9jYcOxYSBHb256w6FsZXogIDxlb2NhbmhhQGlnYWxpYS5jb20+CisKKyAgICAgICAgW0dT dHJlYW1lcl1bTVNFXSBGaXggcGxheWVyIHByaXZhdGUgc2VsZWN0aW9uIHdoZW4gTVNFIGlzIGVu YWJsZWQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE2 NDExNgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIENv dmVyZWQgYnkgZXhpc3RpbmcgdGVzdHMuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9n c3RyZWFtZXIvTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyLmNwcDoKKyAgICAgICAgKFdlYkNv cmU6Ok1lZGlhUGxheWVyUHJpdmF0ZUdTdHJlYW1lcjo6bG9hZCk6CisgICAgICAgICogcGxhdGZv cm0vZ3JhcGhpY3MvZ3N0cmVhbWVyL21zZS9NZWRpYVBsYXllclByaXZhdGVHU3RyZWFtZXJNU0Uu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6TWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyTVNFOjps b2FkKToKKwogMjAxNi0xMC0yNyAgWW91ZW5uIEZhYmxldCAgPHlvdWVubkBhcHBsZS5jb20+CiAK ICAgICAgICAgUkVHUkVTU0lPTihyMjA3NzUzLTIwNzc1NSk6IEFTU0VSVElPTiBGQUlMRUQ6IG1f cGFyc2VkU3R5bGVTaGVldENhY2hlLT5pc0luTWVtb3J5Q2FjaGUoKQpkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3N0cmVhbWVyL01lZGlhUGxheWVyUHJpdmF0 ZUdTdHJlYW1lci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9nc3RyZWFt ZXIvTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyLmNwcAppbmRleCBmMDA1NTgwMzdmMzFhYzcz NjE0MmI4NjJiNjEwMzczYmQ5ZjU3MzUxLi5hNWE5MjU5MjgzMTQ0ZjZhZGY1ODlkN2JlNTUyYjZh NzJhY2RmZDgyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9n c3RyZWFtZXIvTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyLmNwcAorKysgYi9Tb3VyY2UvV2Vi Q29yZS9wbGF0Zm9ybS9ncmFwaGljcy9nc3RyZWFtZXIvTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVh bWVyLmNwcApAQCAtMjcyLDcgKzI3Miw5IEBAIHZvaWQgTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVh bWVyOjpsb2FkKGNvbnN0IFN0cmluZyYgdXJsU3RyaW5nKQogI2lmIEVOQUJMRShNRURJQV9TT1VS Q0UpCiB2b2lkIE1lZGlhUGxheWVyUHJpdmF0ZUdTdHJlYW1lcjo6bG9hZChjb25zdCBTdHJpbmcm LCBNZWRpYVNvdXJjZVByaXZhdGVDbGllbnQqKQogewotICAgIG5vdEltcGxlbWVudGVkKCk7Cisg ICAgLy8gUHJvcGVybHkgZmFpbCBzbyB0aGUgZ2xvYmFsIE1lZGlhUGxheWVyIHRyaWVzIHRvIGZh bGxiYWNrIHRvIHRoZSBuZXh0IE1lZGlhUGxheWVyUHJpdmF0ZS4KKyAgICBtX25ldHdvcmtTdGF0 ZSA9IE1lZGlhUGxheWVyOjpGb3JtYXRFcnJvcjsKKyAgICBtX3BsYXllci0+bmV0d29ya1N0YXRl Q2hhbmdlZCgpOwogfQogI2VuZGlmCiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL2dyYXBoaWNzL2dzdHJlYW1lci9tc2UvTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyTVNF LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2dzdHJlYW1lci9tc2UvTWVk aWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyTVNFLmNwcAppbmRleCAwNmQ5NzkxYzRmYzNmMTMxYzhh YTJiZmY4ZTcyNjU5Yjk2MjYzYTMxLi43NmFlZTcyNWY5NjRkY2U2NTFhODdiMjliMjM2MzdjOWIy MmFjZTBmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9nc3Ry ZWFtZXIvbXNlL01lZGlhUGxheWVyUHJpdmF0ZUdTdHJlYW1lck1TRS5jcHAKKysrIGIvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3N0cmVhbWVyL21zZS9NZWRpYVBsYXllclByaXZh dGVHU3RyZWFtZXJNU0UuY3BwCkBAIC0xMjUsMTQgKzEyNSwxNiBAQCBNZWRpYVBsYXllclByaXZh dGVHU3RyZWFtZXJNU0U6On5NZWRpYVBsYXllclByaXZhdGVHU3RyZWFtZXJNU0UoKQogCiB2b2lk IE1lZGlhUGxheWVyUHJpdmF0ZUdTdHJlYW1lck1TRTo6bG9hZChjb25zdCBTdHJpbmcmIHVybFN0 cmluZykKIHsKLSAgICBpZiAoVU5MSUtFTFkoIWluaXRpYWxpemVHU3RyZWFtZXJBbmRSZWdpc3Rl cldlYktpdE1TRUVsZW1lbnQoKSkpCi0gICAgICAgIHJldHVybjsKLQogICAgIGlmICghdXJsU3Ry aW5nLnN0YXJ0c1dpdGgoIm1lZGlhc291cmNlIikpIHsKLSAgICAgICAgR1NUX0VSUk9SKCJVbnN1 cHBvcnRlZCB1cmw6ICVzIiwgdXJsU3RyaW5nLnV0ZjgoKS5kYXRhKCkpOworICAgICAgICAvLyBQ cm9wZXJseSBmYWlsIHNvIHRoZSBnbG9iYWwgTWVkaWFQbGF5ZXIgdHJpZXMgdG8gZmFsbGJhY2sg dG8gdGhlIG5leHQgTWVkaWFQbGF5ZXJQcml2YXRlLgorICAgICAgICBtX25ldHdvcmtTdGF0ZSA9 IE1lZGlhUGxheWVyOjpGb3JtYXRFcnJvcjsKKyAgICAgICAgbV9wbGF5ZXItPm5ldHdvcmtTdGF0 ZUNoYW5nZWQoKTsKICAgICAgICAgcmV0dXJuOwogICAgIH0KIAorICAgIGlmIChVTkxJS0VMWSgh aW5pdGlhbGl6ZUdTdHJlYW1lckFuZFJlZ2lzdGVyV2ViS2l0TVNFRWxlbWVudCgpKSkKKyAgICAg ICAgcmV0dXJuOworCiAgICAgaWYgKCFtX3BsYXliYWNrUGlwZWxpbmUpCiAgICAgICAgIG1fcGxh eWJhY2tQaXBlbGluZSA9IFBsYXliYWNrUGlwZWxpbmU6OmNyZWF0ZSgpOwogCg==