Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[2.38] Ignore sinks position while seeking #1302

Closed
wants to merge 1 commit into from
Closed

[2.38] Ignore sinks position while seeking #1302

wants to merge 1 commit into from

Conversation

asurdej-comcast
Copy link

@asurdej-comcast asurdej-comcast commented Mar 22, 2024 •
edited
Loading

[2.38] Don't trust sinks position when pipeline is not prerolled as behavor is different across devices.
Use last cached position instead.

After removing MSE data, sinks get flushed and are not able to return valid playback time. Some implementation returns invalid time, 0.00 or even some random value.
This value may be then reported up to HTMLMediaElement that may be confusing to web applications.

This fixes Disney+ video "Restart" button as application may skip currentTime change to 0.00 because video element already reports that position is 0.00:

  1. Video play at position x.xx
  2. pause
  3. Remove video and audio MSE data
  4. video.currentTime is 0.00 so no need to change it (position reported by sink with no preroll)
  5. Append new data and start playback
  6. Position returns back to x.xx

@asurdej-comcast
Ignore sinks position while seeking
df68d46 
Don't trust sinks position when pipeline is not prerolled
as behavor is different across devices.
Use last cached position instead.

After removing MSE data, sinks get flushed and are not able
to return valid playback time. Some implementation returns
invalid time, 0.00 or even some random value.
This value may be then reported up to HTMLMediaElement
that may be confusing to web applications.
@asurdej-comcast asurdej-comcast changed the title Ignore sinks position while seeking [2.38] Ignore sinks position while seeking Mar 22, 2024
@emutavchi emutavchi requested a review from modeveci March 22, 2024 16:29
@modeveci modeveci requested a review from eocanha March 25, 2024 13:28
@eocanha eocanha mentioned this pull request Apr 4, 2024
Merged
@eocanha eocanha added the upstream Related to an upstream bug (or should be at some point) label Apr 5, 2024
@eocanha
Copy link
Member

eocanha commented Apr 5, 2024

Patch submitted upstream for review as https://bugs.webkit.org/show_bug.cgi?id=272167 / WebKit/WebKit#26856

webkit-commit-queue pushed a commit to eocanha/WebKit that referenced this pull request Apr 16, 2024
@eocanha
[GStreamer] Ignore sinks position while seeking
40cfa6a 
https://bugs.webkit.org/show_bug.cgi?id=272167

Reviewed by Alicia Boya Garcia.

After removing MSE data, sinks get flushed and are not able to return
valid playback time. Some implementation return invalid time, 0.00 or
even some random value. This value may be then reported up to
HTMLMediaElement and that may be confusing to web applications.

This commit doesn't trust sinks position when pipeline is not prerolled,
as behavor is different across devices. The last cached position is used
instead.

To reflect better what's actually happening, isPipelineSeeking() has been
renamed as isPipelineWaitingPreroll() and the condition now also includes
pending states higher than paused.

Original author: Andrzej Surdej (https://github.com/asurdej-comcast)

See: WebPlatformForEmbedded/WPEWebKit#1302

* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::isPipelineWaitingPreroll const): Renamed from isPipelineSeeking().
(WebCore::MediaPlayerPrivateGStreamer::play): isPipelineSeeking() now named as isPipelineWaitingPreroll().
(WebCore::MediaPlayerPrivateGStreamer::paused const): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::changePipelineState): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::playbackPosition const): Use GST_CLOCK_TIME_NONE when the pipeline isn't prerolled. This will force the usage of the target seek time (if possible) or the last cached position (in the worst case).
(WebCore::MediaPlayerPrivateGStreamer::isPipelineSeeking const): Deleted. Renamed to isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: isPipelineSeeking() now named as isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
(WebCore::MediaPlayerPrivateGStreamerMSE::updateStates): isPipelineSeeking() now named as isPipelineWaitingPreroll().

Canonical link: https://commits.webkit.org/277541@main
eocanha added a commit that referenced this pull request Apr 16, 2024
@eocanha
[GStreamer] Ignore sinks position while seeking
39bed0d 
https://bugs.webkit.org/show_bug.cgi?id=272167

Reviewed by Alicia Boya Garcia.

After removing MSE data, sinks get flushed and are not able to return
valid playback time. Some implementation return invalid time, 0.00 or
even some random value. This value may be then reported up to
HTMLMediaElement and that may be confusing to web applications.

This commit doesn't trust sinks position when pipeline is not prerolled,
as behavor is different across devices. The last cached position is used
instead.

To reflect better what's actually happening, isPipelineSeeking() has been
renamed as isPipelineWaitingPreroll() and the condition now also includes
pending states higher than paused.

Original author: Andrzej Surdej (https://github.com/asurdej-comcast)

See: #1302

* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::isPipelineWaitingPreroll const): Renamed from isPipelineSeeking().
(WebCore::MediaPlayerPrivateGStreamer::play): isPipelineSeeking() now named as isPipelineWaitingPreroll().
(WebCore::MediaPlayerPrivateGStreamer::paused const): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::changePipelineState): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::playbackPosition const): Use GST_CLOCK_TIME_NONE when the pipeline isn't prerolled. This will force the usage of the target seek time (if possible) or the last cached position (in the worst case).
(WebCore::MediaPlayerPrivateGStreamer::isPipelineSeeking const): Deleted. Renamed to isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: isPipelineSeeking() now named as isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
(WebCore::MediaPlayerPrivateGStreamerMSE::updateStates): isPipelineSeeking() now named as isPipelineWaitingPreroll().

Canonical link: https://commits.webkit.org/277541@main
eocanha pushed a commit that referenced this pull request Apr 16, 2024
@asurdej-comcast @eocanha
[GStreamer] Ignore sinks position while seeking
efc4d3d 
https://bugs.webkit.org/show_bug.cgi?id=272167

Reviewed by Alicia Boya Garcia.

After removing MSE data, sinks get flushed and are not able to return
valid playback time. Some implementation return invalid time, 0.00 or
even some random value. This value may be then reported up to
HTMLMediaElement and that may be confusing to web applications.

This commit doesn't trust sinks position when pipeline is not prerolled,
as behavor is different across devices. The last cached position is used
instead.

To reflect better what's actually happening, isPipelineSeeking() has been
renamed as isPipelineWaitingPreroll() and the condition now also includes
pending states higher than paused.

Original author: Andrzej Surdej (https://github.com/asurdej-comcast)

See: #1302

* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::isPipelineWaitingPreroll const): Renamed from isPipelineSeeking().
(WebCore::MediaPlayerPrivateGStreamer::play): isPipelineSeeking() now named as isPipelineWaitingPreroll().
(WebCore::MediaPlayerPrivateGStreamer::paused const): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::changePipelineState): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::playbackPosition const): Use GST_CLOCK_TIME_NONE when the pipeline isn't prerolled. This will force the usage of the target seek time (if possible) or the last cached position (in the worst case).
(WebCore::MediaPlayerPrivateGStreamer::isPipelineSeeking const): Deleted. Renamed to isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: isPipelineSeeking() now named as isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
(WebCore::MediaPlayerPrivateGStreamerMSE::updateStates): isPipelineSeeking() now named as isPipelineWaitingPreroll().

Canonical link: https://commits.webkit.org/277541@main
eocanha pushed a commit that referenced this pull request Apr 16, 2024
@asurdej-comcast @eocanha
[GStreamer] Ignore sinks position while seeking
bb5f5c2 
https://bugs.webkit.org/show_bug.cgi?id=272167

Reviewed by Alicia Boya Garcia.

After removing MSE data, sinks get flushed and are not able to return
valid playback time. Some implementation return invalid time, 0.00 or
even some random value. This value may be then reported up to
HTMLMediaElement and that may be confusing to web applications.

This commit doesn't trust sinks position when pipeline is not prerolled,
as behavor is different across devices. The last cached position is used
instead.

To reflect better what's actually happening, isPipelineSeeking() has been
renamed as isPipelineWaitingPreroll() and the condition now also includes
pending states higher than paused.

Original author: Andrzej Surdej (https://github.com/asurdej-comcast)

See: #1302

* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::isPipelineWaitingPreroll const): Renamed from isPipelineSeeking().
(WebCore::MediaPlayerPrivateGStreamer::play): isPipelineSeeking() now named as isPipelineWaitingPreroll().
(WebCore::MediaPlayerPrivateGStreamer::paused const): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::changePipelineState): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::playbackPosition const): Use GST_CLOCK_TIME_NONE when the pipeline isn't prerolled. This will force the usage of the target seek time (if possible) or the last cached position (in the worst case).
(WebCore::MediaPlayerPrivateGStreamer::isPipelineSeeking const): Deleted. Renamed to isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: isPipelineSeeking() now named as isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
(WebCore::MediaPlayerPrivateGStreamerMSE::updateStates): isPipelineSeeking() now named as isPipelineWaitingPreroll().

Canonical link: https://commits.webkit.org/277541@main
@eocanha
Copy link
Member

eocanha commented Apr 16, 2024

Landed upstream as eocanha/WebKit@40cfa6a. Backported it to wpe-2.38 as efc4d3d and to the calvaris/wpe-2.42/upstream branch as bb5f5c2. Closing PR.

@eocanha eocanha closed this Apr 16, 2024
calvaris pushed a commit that referenced this pull request May 7, 2024
@asurdej-comcast @calvaris
[GStreamer] Ignore sinks position while seeking
52ff406 
https://bugs.webkit.org/show_bug.cgi?id=272167

Reviewed by Alicia Boya Garcia.

After removing MSE data, sinks get flushed and are not able to return
valid playback time. Some implementation return invalid time, 0.00 or
even some random value. This value may be then reported up to
HTMLMediaElement and that may be confusing to web applications.

This commit doesn't trust sinks position when pipeline is not prerolled,
as behavor is different across devices. The last cached position is used
instead.

To reflect better what's actually happening, isPipelineSeeking() has been
renamed as isPipelineWaitingPreroll() and the condition now also includes
pending states higher than paused.

Original author: Andrzej Surdej (https://github.com/asurdej-comcast)

See: #1302

* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::isPipelineWaitingPreroll const): Renamed from isPipelineSeeking().
(WebCore::MediaPlayerPrivateGStreamer::play): isPipelineSeeking() now named as isPipelineWaitingPreroll().
(WebCore::MediaPlayerPrivateGStreamer::paused const): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::changePipelineState): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::playbackPosition const): Use GST_CLOCK_TIME_NONE when the pipeline isn't prerolled. This will force the usage of the target seek time (if possible) or the last cached position (in the worst case).
(WebCore::MediaPlayerPrivateGStreamer::isPipelineSeeking const): Deleted. Renamed to isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: isPipelineSeeking() now named as isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
(WebCore::MediaPlayerPrivateGStreamerMSE::updateStates): isPipelineSeeking() now named as isPipelineWaitingPreroll().

Canonical link: https://commits.webkit.org/277541@main
calvaris pushed a commit that referenced this pull request Jul 1, 2024
@asurdej-comcast @calvaris
[GStreamer] Ignore sinks position while seeking
32a5b7c 
https://bugs.webkit.org/show_bug.cgi?id=272167

Reviewed by Alicia Boya Garcia.

After removing MSE data, sinks get flushed and are not able to return
valid playback time. Some implementation return invalid time, 0.00 or
even some random value. This value may be then reported up to
HTMLMediaElement and that may be confusing to web applications.

This commit doesn't trust sinks position when pipeline is not prerolled,
as behavor is different across devices. The last cached position is used
instead.

To reflect better what's actually happening, isPipelineSeeking() has been
renamed as isPipelineWaitingPreroll() and the condition now also includes
pending states higher than paused.

Original author: Andrzej Surdej (https://github.com/asurdej-comcast)

See: #1302

* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::isPipelineWaitingPreroll const): Renamed from isPipelineSeeking().
(WebCore::MediaPlayerPrivateGStreamer::play): isPipelineSeeking() now named as isPipelineWaitingPreroll().
(WebCore::MediaPlayerPrivateGStreamer::paused const): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::changePipelineState): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::playbackPosition const): Use GST_CLOCK_TIME_NONE when the pipeline isn't prerolled. This will force the usage of the target seek time (if possible) or the last cached position (in the worst case).
(WebCore::MediaPlayerPrivateGStreamer::isPipelineSeeking const): Deleted. Renamed to isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: isPipelineSeeking() now named as isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
(WebCore::MediaPlayerPrivateGStreamerMSE::updateStates): isPipelineSeeking() now named as isPipelineWaitingPreroll().

Canonical link: https://commits.webkit.org/277541@main
mcatanzaro pushed a commit to WebKit/WebKit that referenced this pull request Jul 30, 2024
@eocanha @mcatanzaro
Cherry-pick 277541@main (40cfa6a). https://bugs.webkit.org/show_bug.c…
4954333 
…gi?id=272167

    [GStreamer] Ignore sinks position while seeking
    https://bugs.webkit.org/show_bug.cgi?id=272167

    Reviewed by Alicia Boya Garcia.

    After removing MSE data, sinks get flushed and are not able to return
    valid playback time. Some implementation return invalid time, 0.00 or
    even some random value. This value may be then reported up to
    HTMLMediaElement and that may be confusing to web applications.

    This commit doesn't trust sinks position when pipeline is not prerolled,
    as behavor is different across devices. The last cached position is used
    instead.

    To reflect better what's actually happening, isPipelineSeeking() has been
    renamed as isPipelineWaitingPreroll() and the condition now also includes
    pending states higher than paused.

    Original author: Andrzej Surdej (https://github.com/asurdej-comcast)

    See: WebPlatformForEmbedded/WPEWebKit#1302

    * Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
    (WebCore::MediaPlayerPrivateGStreamer::isPipelineWaitingPreroll const): Renamed from isPipelineSeeking().
    (WebCore::MediaPlayerPrivateGStreamer::play): isPipelineSeeking() now named as isPipelineWaitingPreroll().
    (WebCore::MediaPlayerPrivateGStreamer::paused const): Ditto.
    (WebCore::MediaPlayerPrivateGStreamer::changePipelineState): Ditto.
    (WebCore::MediaPlayerPrivateGStreamer::playbackPosition const): Use GST_CLOCK_TIME_NONE when the pipeline isn't prerolled. This will force the usage of the target seek time (if possible) or the last cached position (in the worst case).
    (WebCore::MediaPlayerPrivateGStreamer::isPipelineSeeking const): Deleted. Renamed to isPipelineWaitingPreroll().
    * Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: isPipelineSeeking() now named as isPipelineWaitingPreroll().
    * Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
    (WebCore::MediaPlayerPrivateGStreamerMSE::updateStates): isPipelineSeeking() now named as isPipelineWaitingPreroll().

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

Canonical link: https://commits.webkit.org/274313.308@webkitglib/2.44
amol-virnodkar-infosys pushed a commit to LibertyGlobal/WPEWebKit that referenced this pull request Aug 12, 2024
@asurdej-comcast @amol-virnodkar-infosys
[GStreamer] Ignore sinks position while seeking
7bd3885 
https://bugs.webkit.org/show_bug.cgi?id=272167

Reviewed by Alicia Boya Garcia.

After removing MSE data, sinks get flushed and are not able to return
valid playback time. Some implementation return invalid time, 0.00 or
even some random value. This value may be then reported up to
HTMLMediaElement and that may be confusing to web applications.

This commit doesn't trust sinks position when pipeline is not prerolled,
as behavor is different across devices. The last cached position is used
instead.

To reflect better what's actually happening, isPipelineSeeking() has been
renamed as isPipelineWaitingPreroll() and the condition now also includes
pending states higher than paused.

Original author: Andrzej Surdej (https://github.com/asurdej-comcast)

See: WebPlatformForEmbedded#1302

* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::isPipelineWaitingPreroll const): Renamed from isPipelineSeeking().
(WebCore::MediaPlayerPrivateGStreamer::play): isPipelineSeeking() now named as isPipelineWaitingPreroll().
(WebCore::MediaPlayerPrivateGStreamer::paused const): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::changePipelineState): Ditto.
(WebCore::MediaPlayerPrivateGStreamer::playbackPosition const): Use GST_CLOCK_TIME_NONE when the pipeline isn't prerolled. This will force the usage of the target seek time (if possible) or the last cached position (in the worst case).
(WebCore::MediaPlayerPrivateGStreamer::isPipelineSeeking const): Deleted. Renamed to isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: isPipelineSeeking() now named as isPipelineWaitingPreroll().
* Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
(WebCore::MediaPlayerPrivateGStreamerMSE::updateStates): isPipelineSeeking() now named as isPipelineWaitingPreroll().

Canonical link: https://commits.webkit.org/277541@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philn philn Awaiting requested review from philn philn is a code owner

@modeveci modeveci Awaiting requested review from modeveci

@eocanha eocanha Awaiting requested review from eocanha

Assignees
No one assigned
Labels
upstream Related to an upstream bug (or should be at some point) wpe-2.38
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asurdej-comcast @eocanha @modeveci