[GStreamer][MSE] Fix player private selection when MSE is enabled

• Component: Platform
• Status: RESOLVED FIXED

[Enrique Ocaña]

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

[Enrique Ocaña]

Created attachment 293131 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 293131 [details]
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.

[Enrique Ocaña]

Comment on attachment 293131 [details]
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?

[Philippe Normand]

gst_init() needs to be called before using the logging macros.

[Enrique Ocaña]

(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

[Philippe Normand]

(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

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> Are you sure we can use GStreamer logging facilities (potentially) *before*
> having initialized GStreamer?

Forget it then.

[Philippe Normand]

Does the patch needs an update then?

It'd be great to re-enable MSE on the bots ASAP :)

[Xabier Rodríguez Calvar]

(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 ;)

[Enrique Ocaña]

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?

[WebKit Commit Bot]

Comment on attachment 293131 [details]
Patch

Clearing flags on attachment: 293131

Committed r209796: <http://trac.webkit.org/changeset/209796>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.