Bug 236884 - [GStreamer] Switch media player to playbin3
Summary: [GStreamer] Switch media player to playbin3
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
Depends on: 237326
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-19 09:05 PST by Philippe Normand
Modified: 2024-03-05 00:51 PST (History)
4 users (show)

See Also:


Attachments
Patch (5.54 KB, patch)
2022-02-19 09:12 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (5.54 KB, patch)
2022-02-19 09:15 PST, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2022-02-19 09:05:13 PST
MSE and Mediastream require playbin3. Regular playback uses playbin3 if GStreamer >= 1.20 is detected, unless the WEBKIT_GST_USE_PLAYBIN2 environment variable is set to 1.
Comment 1 Philippe Normand 2022-02-19 09:12:25 PST
Created attachment 452642 [details]
Patch
Comment 2 Philippe Normand 2022-02-19 09:15:58 PST
Created attachment 452643 [details]
Patch
Comment 3 EWS 2022-02-22 11:35:57 PST
Committed r290325 (247647@main): <https://commits.webkit.org/247647@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452643 [details].
Comment 4 Radar WebKit Bug Importer 2022-02-22 11:36:19 PST
<rdar://problem/89307315>
Comment 5 Philippe Normand 2022-02-27 09:46:10 PST
Comment on attachment 452643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452643&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2671
> +    auto usePlaybin2 = usePlaybin2Override ? parseInteger<unsigned>(usePlaybin2Override) : 0;

Clearly I didn't test this properly, nice auto footgun. This is actually an optional<unsigned>.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2672
> +    if ((usePlaybin2 || !webkitGstCheckVersion(1, 20, 0)) && !isMediaSource() && !url.protocolIs("mediastream"))

so the usePlaybin2 test here always returns true. I'll follow-up in a new patch, there are layout test updates needed.
Comment 6 WebKit Commit Bot 2022-03-01 09:03:14 PST
Re-opened since this is blocked by bug 237326
Comment 7 Philippe Normand 2022-09-04 09:51:51 PDT
Pull request: https://github.com/WebKit/WebKit/pull/4007
Comment 8 Philippe Normand 2022-11-10 08:35:01 PST
(In reply to Philippe Normand from comment #7)
> Pull request: https://github.com/WebKit/WebKit/pull/4007

I've found a couple more playbin3 issues which make me think this is not production-ready yet, at least until 1.22:

- https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1419 which was closed actually. Download buffering is not runtime toggle-able in playbin3
- https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1562 URI redirects are not supported yet
- https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1418 some tests failing due to this one, though I think it's a wpt-specific corner case...

So I think we shouldn't flip the switch yet, it's too early.
Comment 9 Philippe Normand 2023-05-29 04:22:06 PDT
(In reply to Philippe Normand from comment #8)
> (In reply to Philippe Normand from comment #7)
> > Pull request: https://github.com/WebKit/WebKit/pull/4007
> 
> I've found a couple more playbin3 issues which make me think this is not
> production-ready yet, at least until 1.22:
> 
> - https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1419 which was
> closed actually. Download buffering is not runtime toggle-able in playbin3

Nothing we can do about this.

> - https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1562 URI
> redirects are not supported yet

Fixed upstream (scheduled to ship in 1.24).

> - https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1418 some
> tests failing due to this one, though I think it's a wpt-specific corner
> case...
> 

MR fixing this: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4725

> So I think we shouldn't flip the switch yet, it's too early.

Playbin3 support for decoder/sinks is going to be another requirement.
Comment 10 Philippe Normand 2024-03-04 03:43:59 PST
AFAICT the last big blocker is https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4391

playbin3 in 1.24 is no longer advertised as experimental.
Comment 11 Philippe Normand 2024-03-04 09:58:58 PST
Pull request: https://github.com/WebKit/WebKit/pull/25429
Comment 12 Xabier Rodríguez Calvar 2024-03-04 22:32:41 PST
(In reply to Philippe Normand from comment #10)
> AFAICT the last big blocker is
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4391
> 
> playbin3 in 1.24 is no longer advertised as experimental.

That PR is not landing. I witnessed Edward's review in person and he is not accepting that. So we'll have to live with the current custom solutions for the corresponding platforms.
Comment 13 Philippe Normand 2024-03-05 00:51:53 PST
(In reply to Xabier Rodríguez Calvar from comment #12)
> (In reply to Philippe Normand from comment #10)
> > AFAICT the last big blocker is
> > https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4391
> > 
> > playbin3 in 1.24 is no longer advertised as experimental.
> 
> That PR is not landing. I witnessed Edward's review in person and he is not
> accepting that. So we'll have to live with the current custom solutions for
> the corresponding platforms.

We could provide a playbin2 quirk for those :)