[webkit-changes] [WebKit/WebKit] d65ff7: [GStreamer] Fix "pipeline and player states are no...

[Alicia Boya García]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: d65ff7e6d420dbf512df6f6da8c0948f9507e423
      https://github.com/WebKit/WebKit/commit/d65ff7e6d420dbf512df6f6da8c0948f9507e423
  Author: Alicia Boya Garcia <aboya at igalia.com>
  Date:   2025-01-14 (Tue, 14 Jan 2025)

  Changed paths:
    A LayoutTests/media/media-source/media-source-muted-scroll-and-seek-crash-expected.txt
    A LayoutTests/media/media-source/media-source-muted-scroll-and-seek-crash.html
    M Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
    M Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h
    M Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp

  Log Message:
  -----------
  [GStreamer] Fix "pipeline and player states are not synchronized" assertion crash
https://bugs.webkit.org/show_bug.cgi?id=285850

Reviewed by Philippe Normand.

WebKit pauses muted videos when scrolling to save resources. However,
when running in Debug, an assertion was failing inside
MediaPlayerPrivateGStreamer::paused() in the GStreamer ports:

> ASSERTION FAILED: pipeline and player states are not synchronized

After some debugging, I found two problems that were causing the
assertion to fail:

(1) MediaPlayerPrivateGStreamerMSE::updateStates() didn't account for
this behavior, which could trigger unexpected state changes. The patch
adds it to the `shouldBePlaying` check.

(2) The assertion checks m_isPipelinePlaying against the actual state of
the pipeline. However, the code setting the pipeline to PAUSED when
scrolling away (see setVisibleInViewport()) didn't update this field.
Normally you use setPipelineState() instead which updates this field.

As drive-by fixes this patch also adds new logs and renames two fields
to have more useful names.

 * m_isVisibleInViewport is negated and renamed m_isPausedByViewport,
   reflecting its actual meaning (`m_isVisibleInViewport` was often true
   while the HTMLMediaElement was not visible in the viewport.

 * m_isVisible is renamed to m_pageIsVisible to both match other code and
   to reflect its actual meaning.

This patch adds back media-source-muted-scroll-and-seek-crash.html from
276798 at main, which can be used to test the crash does not happen.  Note
however the same caveat from back then applies:

> Currently, part of the code to trigger the crash isn't executed
> due to WEBKIT_GST_ALLOW_PLAYBACK_OF_INVISIBLE_VIDEOS=1 being set in the test driver in
> Tools/Scripts/webkitpy/port/glib.py (264017 at main), which needs to be commented manually.
> We should find a way to add a test preference for this code path to be enabled.

* LayoutTests/media/media-source/media-source-muted-scroll-and-seek-crash.html: Added.
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::paused const):
(WebCore::MediaPlayerPrivateGStreamer::changePipelineState):
(WebCore::MediaPlayerPrivateGStreamer::setVisibleInViewport):
(WebCore::MediaPlayerPrivateGStreamer::paint):
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:
* Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
(WebCore::MediaPlayerPrivateGStreamerMSE::updateStates):

Canonical link: https://commits.webkit.org/288906@main



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications