[webkit-changes] [WebKit/WebKit] aedede: [GStreamer] Buffering hysteresis and buffering imp...

Mon Sep 23 06:52:10 PDT 2024

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: aededeb7ddd9e55b53d9eda0be721ab3fd8180e4
      https://github.com/WebKit/WebKit/commit/aededeb7ddd9e55b53d9eda0be721ab3fd8180e4
  Author: Enrique Ocaña González <eocanha at igalia.com>
  Date:   2024-09-23 (Mon, 23 Sep 2024)

  Changed paths:
    M LayoutTests/platform/glib/TestExpectations
    M Source/WebCore/platform/SourcesGStreamer.txt
    M Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp
    M Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h
    M Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
    M Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h
    M Source/WebCore/platform/gstreamer/GStreamerHolePunchQuirkBcmNexus.h
    M Source/WebCore/platform/gstreamer/GStreamerHolePunchQuirkFake.h
    M Source/WebCore/platform/gstreamer/GStreamerHolePunchQuirkRialto.h
    M Source/WebCore/platform/gstreamer/GStreamerHolePunchQuirkWesteros.h
    M Source/WebCore/platform/gstreamer/GStreamerQuirkAmLogic.h
    M Source/WebCore/platform/gstreamer/GStreamerQuirkBcmNexus.h
    M Source/WebCore/platform/gstreamer/GStreamerQuirkBroadcom.h
    A Source/WebCore/platform/gstreamer/GStreamerQuirkBroadcomBase.cpp
    A Source/WebCore/platform/gstreamer/GStreamerQuirkBroadcomBase.h
    M Source/WebCore/platform/gstreamer/GStreamerQuirkRealtek.h
    M Source/WebCore/platform/gstreamer/GStreamerQuirkRialto.h
    M Source/WebCore/platform/gstreamer/GStreamerQuirkWesteros.h
    M Source/WebCore/platform/gstreamer/GStreamerQuirks.cpp
    M Source/WebCore/platform/gstreamer/GStreamerQuirks.h

  Log Message:
  -----------
  [GStreamer] Buffering hysteresis and buffering improvements for Broadcom
https://bugs.webkit.org/show_bug.cgi?id=275683

Reviewed by Xabier Rodriguez-Calvar.

The usage of a fixed buffering level to evaluate the need for buffering
results in a too unstable behaviour, with quick changes between states
that may create stuttering.

This commit adds hysteresis to the buffering level, with low and high
watermark levels that trigger "buffering needed" (when below the low
watermark), "buffering completed" (when above high watermark) and "same
state as before" when the level is between both marks.

In addition to that, the pipeline is automatically paused on a seek when
working on stream mode (using GstQueue2 for partial in-memory buffering
instead of using GstDownload for full on-disk download of the media).
This is because, when seeking, rebuffering must occur from scratch to
get the data for the new target position, and buffering level starts
from 0% in that case, which means that buffering is needed and the
pipeline must be paused. Both the m_isBuffering flag and
m_bufferingPercentage are immediately forced to true and 0%, respectively.
The previous values (see explanation below) are also set to the same
values by using the new resetHistory parameter in updateBufferingStatus().
This makes sure that the "end of buffering" condition can be properly
detected.

The buffering status and buffering level computation has been
centralized in updateBufferingStatus(). Both the status and the level
now store the last value, so it's possible to check if there's been any
difference on them and to decide here on the hysteresis status
evolution.

updateStates() is where the consequences of buffering changes happen.
This method now relies in the status and level updates explained in the
previous paragraph. In the specific case of an async change, the current
values for m_isBuffering and m_bufferingPercentage are ignored (previous
ones are used). This is to be able to detect significant changes in the
hysteresis (watermark level crossing) after the async change has
completed. Otherwise, we would be detecting those crossings in a moment
where nothing can be done (pipeline state changes are ignored can can't
be enforced during async state changes).

Some extra improvements have been added to adapt to the special
circumstances of Broadcom devices, where the PlayPump Nexus component
can store a lot of data which is hidden to the vanilla buffering
algorithm. Now that data is had into account to compute the realistic
buffering level. Also, more agile low/high watermarks of 20% and 80% are
used for buffering in the case of playbin2 in stream buffering mode on
Broadcom.

20-80% levels for low/high watermark are now used on all devices
(Broadcom and not Broadcom), since usage tests proved that this makes
the buffering more agile also on Raspberry Pi, for instance.

All these changes are compatible with the Quirks framework. A new
needsBufferingPercentageCorrection() method has been added to enable the
extra buffering logic (which corrects the queue2 buffering level with the
buffer levels of PlayPump and multiqueue) on the platforms that use Nexus.
All the code to implement that logic has been hidden in the
GStreamerQuirkBroadcomBase class, from which both GStreamerQuirkBcmNexus
and GStreamerQuirkBroadcom inherit.

That code needs some elements that in principle would be very coupled with
MediaPlayerPrivateGStreamer, such as vidfilter, queue2, multiqueue and a
MovingAverage to smooth buffering levels. To avoid that coupling, they
are stored in an opaque GStreamerQuirkState, stored by the player private
but implemented by a GStreamerQuirkBroadcomBaseState subclass that only
GStreamerQuirkBroadcomBase knows how to interpret (by casting after
ensuring that the casting is legal).

There's also the problem that multiple competing quirks can be installing
at the same time, but all this buffering percentage correction code must
only be run by one (only one!) of them. To allow the theoretical coexistence
of multiple quirk state, each quirk instance has one associated state, and
the player private maintains that correspondence using a HashMap indexed by
quirk. To ensure that only one quirk is run at each time, the family of methods
in GStreamerQuirksManager only forward the call to the first configured
GStreamerQuirk subclass that declares the need to apply corrections. Any other
configured subclass is ignored.

Finally, some drive-by improvements are present in this commit. For instance,
converting some const char* to ASCIILiteral here and there.

* Source/WebCore/platform/SourcesGStreamer.txt: Added GStreamerQuirkBroadcomBase.
* Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:
(WebCore::gstStructureGetArray): Added.
* Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:
(WebCore::gstStructureGetArray): Method to get a GstArray GstValue stored into a GstStructure. Returns a Vector of the specified type, which includes basic types and "const GstStructure*".
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::play): Make code compatible with InitiallyPaused.
(WebCore::MediaPlayerPrivateGStreamer::doSeek): Pause pipeline and reset buffering level to 0% in case we're operating in stream mode.
(WebCore::MediaPlayerPrivateGStreamer::queryBufferingPercentage): Get the buffering percentage from the most relevant element and delegate to the quirks.
(WebCore::MediaPlayerPrivateGStreamer::fillTimerFired): Clarify that this method is only used in on-disk buffering. Rely on queryBufferingPercentage() now, instead of manually getting the value from here.
(WebCore::MediaPlayerPrivateGStreamer::handleMessage): Delegate to the quirks to store/delete references to specific elements in the quirks state.
(WebCore::MediaPlayerPrivateGStreamer::processBufferingStats): Delegate to the quierks to apply buffering percentage corrections on platforms that need them.
(WebCore::MediaPlayerPrivateGStreamer::updateBufferingStatus): Update the values of m_wasBuffering, m_isBuffering, m_previousBufferingPercentage, m_bufferingPercentage and m_didDownloadFinish. The first 4 attributes should *ONLY* be modified from this method, in order to guarantee coherency. The only *EXCEPTION* to this rule is the delay which can happen in updateStates(). The new resetHistory parameter sets the value both in the previous value family of attributes and in the current value family of variables. Also, restart the fillTimer if download has restarted (it was never restarted before this patch!)
(WebCore::MediaPlayerPrivateGStreamer::updateStates): Assume that m_wasBuffering, etc. have been precomputed by updateBufferingStatus() before updateStates() is called. In the case of an async state change, delay m_isBuffering and m_bufferingPercentage "to the past" (m_wasBuffering and m_previousBufferingPercentage), so that any change on them can be detected on the next iteration.
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: Added new m_wasBuffering and m_previousBufferingPercentage attributes to store previous values of m_isBuffering and m_bufferingPercentage and ease detection of changes. Added quirks state.
(WebCore::MediaPlayerPrivateGStreamer::shouldDownload): Needed by quirks.
(WebCore::MediaPlayerPrivateGStreamer::setQuirkState): Configure a GStreamerQuirkState for a specific GStreamerQuirk on the player private (each quirk has its own state, and in theory there can be many quirk-state stored).
(WebCore::MediaPlayerPrivateGStreamer::quirkState): Get the quirks state associated to a specific quirk from the player private, or nullptr if there's no configured state for that quirk.
* Source/WebCore/platform/gstreamer/GStreamerHolePunchQuirkBcmNexus.h: identifier() is now const ASCIILiteral.
* Source/WebCore/platform/gstreamer/GStreamerHolePunchQuirkFake.h: Ditto.
* Source/WebCore/platform/gstreamer/GStreamerHolePunchQuirkRialto.h: Ditto.
* Source/WebCore/platform/gstreamer/GStreamerHolePunchQuirkWesteros.h: Ditto.
* Source/WebCore/platform/gstreamer/GStreamerQuirkAmLogic.h: Ditto.
* Source/WebCore/platform/gstreamer/GStreamerQuirkBcmNexus.h: Ditto. Inherit from base class.
* Source/WebCore/platform/gstreamer/GStreamerQuirkBroadcom.h: Ditto. Inherit from base class.
* Source/WebCore/platform/gstreamer/GStreamerQuirkBroadcomBase.cpp: Added. Base class for GStreamerQuirkBcmNexus and GStreamerQuirkBroadcom.
(WebCore::GStreamerQuirkBroadcomBase::GStreamerQuirkBroadcomBase): Constructor.
(WebCore::GStreamerQuirkBroadcomBase::queryBufferingPercentage const): Get the buffering percentage from Queue2 if present and the player shouldn't download (streaming mode). Returns the name of the element finally used to query the percentage to, or nullptr if none was queried.
(WebCore::GStreamerQuirkBroadcomBase::correctBufferingPercentage const): On systems using PlayPump (eg: Broadcom Nexus), correct the buffering percentage with the data stored in multiqueue and PlayPump in order to make the buffering percentage more realistic. Uses the MovingAverage. Queries vidfilter and audfilter if needed, so it also works in audio or video-only pipelines.
(WebCore::GStreamerQuirkBroadcomBase::resetBufferingPercentage const): Sets the percentage to a given value in the MovingAverage.
(WebCore::GStreamerQuirkBroadcomBase::setupBufferingPercentageCorrection const): Gets/deletes references to vidfilter, multiqueue and queue2 in the quirks state.
(WebCore::GStreamerQuirkBroadcomBase::ensureState const): Retrieves GStreamerQuirkState associated to this quirk, creating one and storing it on MediaPlayerPrivateGStreamer if not already present.
* Source/WebCore/platform/gstreamer/GStreamerQuirkBroadcomBase.h: Added.
(WebCore::GStreamerQuirkBroadcomBase::needsBufferingPercentageCorrection const): True for this class and derived ones.
(WebCore::GStreamerQuirkBroadcomBase::MovingAverage::MovingAverage): Initialize an array of "length" number of elements, asserting if length is too big that the values would create overflow when computing the average.
(WebCore::GStreamerQuirkBroadcomBase::MovingAverage::reset): Reset all the values to a given value (usually zero).
(WebCore::GStreamerQuirkBroadcomBase::MovingAverage::accumulate): Insert a new value in the array and recompute the moving average.
* Source/WebCore/platform/gstreamer/GStreamerQuirkRealtek.h: identifier() is now const ASCIILiteral.
* Source/WebCore/platform/gstreamer/GStreamerQuirkRialto.h: Ditto.
* Source/WebCore/platform/gstreamer/GStreamerQuirkWesteros.h: Ditto.
* Source/WebCore/platform/gstreamer/GStreamerQuirks.cpp: Added new methods.
(WebCore::GStreamerQuirksManager::GStreamerQuirksManager): Adapt to ASCIILiteral quirk identifier.
(WebCore::GStreamerQuirksManager::createAudioSink): Ditto.
(WebCore::GStreamerQuirksManager::createWebAudioSink): Ditto.
(WebCore::GStreamerQuirksManager::createHolePunchVideoSink): Ditto.
(WebCore::GStreamerQuirksManager::isHardwareAccelerated const): Ditto.
(WebCore::GStreamerQuirksManager::audioVideoDecoderFactoryListType const): Ditto.
(WebCore::GStreamerQuirksManager::getAdditionalPlaybinFlags const): Ditto.
(WebCore::GStreamerQuirksManager::needsBufferingPercentageCorrection const): True if any configured quirk needs it. The first configured quirk that needs it will be the one responsible to fulfill all the requests for this family of methods. The other ones will be ignored, even if they also need percentage correction. This is to avoid more than one implementation doing the corrections, as we don't want them to accumulate and over-correct.
(WebCore::GStreamerQuirksManager::queryBufferingPercentage const): Forward query request to the first configured quirk needing correction.
(WebCore::GStreamerQuirksManager::correctBufferingPercentage const): Forward correction request to the first configured quirk needing correction.
(WebCore::GStreamerQuirksManager::resetBufferingPercentage const): Ditto, but for reset.
(WebCore::GStreamerQuirksManager::setupBufferingPercentageCorrection const): Ditto, but for setup.
* Source/WebCore/platform/gstreamer/GStreamerQuirks.h:
(WebCore::GStreamerQuirkBase::GStreamerQuirkState::GStreamerQuirkState): Constructor.
(WebCore::GStreamerQuirk::needsBufferingPercentageCorrection const): Default empty implementation.
(WebCore::GStreamerQuirk::queryBufferingPercentage const): Ditto.
(WebCore::GStreamerQuirk::correctBufferingPercentage const): Ditto.
(WebCore::GStreamerQuirk::resetBufferingPercentage const): Ditto.
(WebCore::GStreamerQuirk::setupBufferingPercentageCorrection const): Ditto.
* LayoutTests/platform/glib/TestExpectations: Reported new failing test as https://bugs.webkit.org/show_bug.cgi?id=280069, but IMHO it's not a regression. The test was passing before by sheer luck.

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

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