Skip to content

Fix video playback stall at the beginning of some content#1015

Closed
emutavchi wants to merge 2 commits intoWebPlatformForEmbedded:wpe-2.28from
emutavchi:mse-sample-erase-fix
Closed

Fix video playback stall at the beginning of some content#1015
emutavchi wants to merge 2 commits intoWebPlatformForEmbedded:wpe-2.28from
emutavchi:mse-sample-erase-fix

Conversation

@emutavchi
Copy link
Copy Markdown
Collaborator

@emutavchi emutavchi commented Jan 23, 2023

This is reproducible with apps that do transmuxing from TS to MP4 streams. In the reproduction case we can see 2 problems:

  1. AppendPipeline incorrectly extending the second sample, which eventually triggers removal of the first sample (key frame):
  1662:Jan 23 19:47:02 hisense-a6gp WPEFramework[20041]: 0:01:15.314484225 25788 0xaba033c0 TRACE              webkitmse AppendPipeline.cpp:507:appsinkNewSample: append: trackId=V1 PTS={0/\
1000000 = 0} DTS={0/1000000 = 0} DUR={1/1000000 = 0.000001} presentationSize=640x360                                                                                                          
   1664:Jan 23 19:47:02 hisense-a6gp WPEFramework[20041]: 0:01:15.314642390 25788 0xaba033c0 TRACE              webkitmse AppendPipeline.cpp:507:appsinkNewSample: append: trackId=V1 PTS={20\
8544/1000000 = 0.208544} DTS={0/1000000 = 0} DUR={1/1000000 = 0.000001} presentationSize=640x360                                                                                              
   1668:Jan 23 19:47:02 hisense-a6gp WPEFramework[20041]: 0:01:15.315201801 25788 0xaba033c0 DEBUG              webkitmse AppendPipeline.cpp:503:appsinkNewSample: Extending first sample of \
track 'V1' to make it start at PTS=0 buffer: 0x614000, pts 0:00:00.083422222, dts 0:00:00.000000000, dur 0:00:00.041711111, size 19, offset none, offset_end none, flags 0x6000
  1. Even with above fixed, SourceBuffer incorrectly removes the first sample when the duration of the sample is less than 1 ms tolerance:
[WPEWebKit:MediaSource:-] SourceBuffer::sourceBufferPrivateDidReceiveSample(234F0002) {"pts":{"value":0,"numerator":0,"denominator":1000000,"flags":1},"dts":{"value":0,"numerator":0,"denomi\
nator":1000000,"flags":1},"duration":{"value":0.000001,"numerator":1,"denominator":1000000,"flags":1},"flags":1,"presentationSize":{"width":1024,"height":576}}                               
[WPEWebKit:MediaSource:-] SourceBuffer::sourceBufferPrivateDidReceiveSample(234F0002) {"pts":{"value":0.208544,"numerator":208544,"denominator":1000000,"flags":1},"dts":{"value":0,"numerato\
r":0,"denominator":1000000,"flags":1},"duration":{"value":0.000001,"numerator":1,"denominator":1000000,"flags":1},"flags":0,"presentationSize":{"width":1024,"height":576}}                   
[WPEWebKit:MediaSource:-] SourceBuffer::sourceBufferPrivateDidReceiveSample(234F0002) removing sample {"pts":{"value":0,"numerator":0,"denominator":1000000,"flags":1},"dts":{"value":0,"nume\
rator":0,"denominator":1000000,"flags":1},"duration":{"value":0.000001,"numerator":1,"denominator":1000000,"flags":1},"flags":1,"presentationSize":{"width":1024,"height":576}}

This PR backports some of the changes from upstream, and disables PTS extension for non-key frames

@emutavchi emutavchi marked this pull request as ready for review January 24, 2023 16:29
@emutavchi emutavchi requested a review from modeveci January 24, 2023 16:29
@modeveci modeveci requested a review from eocanha January 27, 2023 12:09
@eocanha
Copy link
Copy Markdown
Member

eocanha commented Feb 7, 2023

The PR is good. I'm working to port the !GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_DELTA_UNIT) part upstream and then I'll land everything on wpe-2.28 and wpe-2.38.

@eocanha eocanha added wpe-2.28 Only for PR affecting 2.28 upstream Related to an upstream bug (or should be at some point) labels Feb 7, 2023
@eocanha
Copy link
Copy Markdown
Member

eocanha commented Feb 8, 2023 •
edited
Loading

"Don't extend PTS of non-key frame" patch submitted upstream as https://bugs.webkit.org/show_bug.cgi?id=251868 / WebKit/WebKit#9769 and landed as https://commits.webkit.org/260004@main
The other patch doesn't need upstreaming, as it already comes from https://bugs.webkit.org/show_bug.cgi?id=237078

@webkit-commit-queue webkit-commit-queue mentioned this pull request Feb 8, 2023
Merged
webkit-early-warning-system pushed a commit to eocanha/WebKit that referenced this pull request Feb 8, 2023
@eocanha
[MSE][GStreamer] Don't extend PTS of non-key frame
fa12275 
https://bugs.webkit.org/show_bug.cgi?id=251868

The edit list support patch from
https://bugs.webkit.org/show_bug.cgi?id=231019 was reverted in
https://bugs.webkit.org/show_bug.cgi?id=244428. While that patch isn't
landed again, we're growing the first buffer of an append near the
begining of the stream to make it start from 0. This workaround uses a
tolerance of 0.1s, and can cause problems on frames short enough to fit
more than one time in the tolerance margin (eg: 3 frames at 30fps would
fit, as well as 6 frames at 60fps).

The process of growing the seconde frame that still fit in the tolerance
is triggering the deletion of the first frame, which is usually a sync
one. This sweeps other frames into the deletion and causes problems.

This patch changes the condition to enable frame growing to only apply
on key frames (sync samples).

Original author: Eugene Mutavchi <Ievgen_Mutavchi@comcast.com>
See: WebPlatformForEmbedded/WPEWebKit#1015

Reviewed by Alicia Boya Garcia.

* Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:
(WebCore::AppendPipeline::appsinkNewSample): Don't extend PTS of non-key frame.

Canonical link: https://commits.webkit.org/260004@main
eocanha pushed a commit that referenced this pull request Feb 8, 2023
@emutavchi @eocanha
[MSE][GStreamer] Don't extend PTS of non-key frame
ec7035e 
https://bugs.webkit.org/show_bug.cgi?id=251868

The edit list support patch from
https://bugs.webkit.org/show_bug.cgi?id=231019 was reverted in
https://bugs.webkit.org/show_bug.cgi?id=244428. While that patch isn't
landed again, we're growing the first buffer of an append near the
begining of the stream to make it start from 0. This workaround uses a
tolerance of 0.1s, and can cause problems on frames short enough to fit
more than one time in the tolerance margin (eg: 3 frames at 30fps would
fit, as well as 6 frames at 60fps).

The process of growing the seconde frame that still fit in the tolerance
is triggering the deletion of the first frame, which is usually a sync
one. This sweeps other frames into the deletion and causes problems.

This patch changes the condition to enable frame growing to only apply
on key frames (sync samples).

Original author: Eugene Mutavchi <Ievgen_Mutavchi@comcast.com>
See: #1015

Reviewed by Alicia Boya Garcia.

* Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:
(WebCore::AppendPipeline::appsinkNewSample): Don't extend PTS of non-key frame.

Canonical link: https://commits.webkit.org/260004@main
eocanha added a commit that referenced this pull request Feb 8, 2023
@eocanha
[MSE][GStreamer] Don't extend PTS of non-key frame
3e1e076 
https://bugs.webkit.org/show_bug.cgi?id=251868

The edit list support patch from
https://bugs.webkit.org/show_bug.cgi?id=231019 was reverted in
https://bugs.webkit.org/show_bug.cgi?id=244428. While that patch isn't
landed again, we're growing the first buffer of an append near the
begining of the stream to make it start from 0. This workaround uses a
tolerance of 0.1s, and can cause problems on frames short enough to fit
more than one time in the tolerance margin (eg: 3 frames at 30fps would
fit, as well as 6 frames at 60fps).

The process of growing the seconde frame that still fit in the tolerance
is triggering the deletion of the first frame, which is usually a sync
one. This sweeps other frames into the deletion and causes problems.

This patch changes the condition to enable frame growing to only apply
on key frames (sync samples).

Original author: Eugene Mutavchi <Ievgen_Mutavchi@comcast.com>
See: #1015

Reviewed by Alicia Boya Garcia.

* Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:
(WebCore::AppendPipeline::appsinkNewSample): Don't extend PTS of non-key frame.

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

eocanha commented Feb 8, 2023 •
edited
Loading

I've backported the upstream patch and manually pushed both patches to wpe-2.28 as:

And to wpe-2.38 as:

@eocanha eocanha closed this Feb 8, 2023
aperezdc pushed a commit to WebKit/WebKit that referenced this pull request Feb 12, 2023
@eocanha @aperezdc
Cherry-pick 260004@main (fa12275). https://bugs.webkit.org/show_bug.c…
2db33e3 
…gi?id=251868

    [MSE][GStreamer] Don't extend PTS of non-key frame
    https://bugs.webkit.org/show_bug.cgi?id=251868

    The edit list support patch from
    https://bugs.webkit.org/show_bug.cgi?id=231019 was reverted in
    https://bugs.webkit.org/show_bug.cgi?id=244428. While that patch isn't
    landed again, we're growing the first buffer of an append near the
    begining of the stream to make it start from 0. This workaround uses a
    tolerance of 0.1s, and can cause problems on frames short enough to fit
    more than one time in the tolerance margin (eg: 3 frames at 30fps would
    fit, as well as 6 frames at 60fps).

    The process of growing the seconde frame that still fit in the tolerance
    is triggering the deletion of the first frame, which is usually a sync
    one. This sweeps other frames into the deletion and causes problems.

    This patch changes the condition to enable frame growing to only apply
    on key frames (sync samples).

    Original author: Eugene Mutavchi <Ievgen_Mutavchi@comcast.com>
    See: WebPlatformForEmbedded/WPEWebKit#1015

    Reviewed by Alicia Boya Garcia.

    * Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:
    (WebCore::AppendPipeline::appsinkNewSample): Don't extend PTS of non-key frame.

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

Reviewers

@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.28 Only for PR affecting 2.28

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@emutavchi @eocanha