Bug 219822

Summary: REGRESSION(r270612) [GStreamer][MSE] Some play/seek related failures
Product: WebKit Reporter: Lauro Moura <lmoura>
Component: MediaAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: aboya, bugs-noreply, calvaris, cgarcia, eocanha, ews-watchlist, gustavo, menard, peng.liu6, pnormand, vjaquez, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=219565
https://bugs.webkit.org/show_bug.cgi?id=220055
Attachments:
Description Flags
Patch none

Description Lauro Moura 2020-12-11 21:43:12 PST
After the fix of bug219565, there are still some failures playing/seeking, likely related to r270612.

List of tests failing locally (to be gardened together with the fix of bug219565):

# Failures

imported/w3c/web-platform-tests/media-source/mediasource-buffered-seek.html [ Failure ]
imported/w3c/web-platform-tests/media-source/mediasource-changetype-play-negative.html [ Failure ]
imported/w3c/web-platform-tests/media-source/mediasource-h264-play-starved.html [ Failure ]
imported/w3c/web-platform-tests/media-source/mediasource-liveseekable.html [ Failure ]
imported/w3c/web-platform-tests/media-source/mediasource-play-then-seek-back.html [ Failure ]
imported/w3c/web-platform-tests/media-source/mediasource-redundant-seek.html [ Failure ]
imported/w3c/web-platform-tests/media-source/mediasource-seek-during-pending-seek.html [ Failure ]
imported/w3c/web-platform-tests/media-source/mediasource-seekable.html [ Failure ]
imported/w3c/web-platform-tests/media-source/mediasource-sequencemode-append-buffer.html [ Failure ]
media/media-source/media-source-canplaythrough-event.html [ Failure ]
media/media-source/media-source-stalled-holds-sleep-assertion.html [ Failure ]
media/media-source/media-source-unnecessary-seek-seeked.html [ Failure ]

# Timeouts
imported/w3c/web-platform-tests/media-source/mediasource-video-is-visible.html [ Timeout ]
media/encrypted-media/clearKey/clearKey-cenc-audio-playback-mse.html [ Timeout ]
media/encrypted-media/clearKey/clearKey-cenc-video-playback-mse.html [ Timeout ]
media/media-source/media-source-paint-to-canvas.html [ Timeout ]
media/media-source/media-source-seek-redundant-append.html [ Timeout ]
media/media-source/media-source-seek-twice.html [ Timeout ]
Comment 1 Philippe Normand 2020-12-14 04:15:20 PST
Adding MSE experts in CC...

I had a quick look, the append pipeline sink receives no sample and that triggers a typefind error down the line during seeks...
Comment 2 Philippe Normand 2020-12-21 04:37:34 PST
The biggest issue is that allSamplesInTrackEnqueued() is now called too early and signals EOS even before the pipeline had a chance to preroll. This regression was introduced in r270612.

I'm preparing a patch...
Comment 3 Philippe Normand 2020-12-21 04:40:33 PST
Another issue is that the SourceBufferPrivateGStreamer doesn't call appendComplete(), so the parent class doesn't correctly synchronize the TrackBuffer buffered ranges.
Comment 4 Alicia Boya García 2020-12-21 04:53:37 PST
(In reply to Philippe Normand from comment #2)
> The biggest issue is that allSamplesInTrackEnqueued() is now called too
> early and signals EOS even before the pipeline had a chance to preroll. This
> regression was introduced in r270612.
> 
> I'm preparing a patch...

I'm also working on this.
Comment 5 Philippe Normand 2020-12-22 04:04:13 PST
Created attachment 416660 [details]
Patch
Comment 6 Xabier Rodríguez Calvar 2020-12-22 05:07:09 PST
Comment on attachment 416660 [details]
Patch

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

r+ conditional to what Alicia thinks of this

> Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:196
> +    SourceBufferPrivate::appendCompleted(true, m_mediaSource ? m_mediaSource->isEnded() : true);

Is it necessary to prepend the superclass name? Method is protected and should be available in the subclass, right?

> Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:201
> +    SourceBufferPrivate::appendCompleted(false, m_mediaSource ? m_mediaSource->isEnded() : true);

Ditto.
Comment 7 Alicia Boya García 2020-12-22 10:03:50 PST
Comment on attachment 416660 [details]
Patch

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

LGTM

>> Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:196
>> +    SourceBufferPrivate::appendCompleted(true, m_mediaSource ? m_mediaSource->isEnded() : true);
> 
> Is it necessary to prepend the superclass name? Method is protected and should be available in the subclass, right?

It's not necessary, but I appreciate it, since it shows explicitly that we're jumping to the multiplatform code. The refactor mushed together SourceBufferPrivateGStreamer and SourceBufferPrivate and has since made noticing this difference harder.
Comment 8 EWS 2020-12-22 10:28:31 PST
Committed r271056: <https://trac.webkit.org/changeset/271056>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416660 [details].
Comment 9 Radar WebKit Bug Importer 2020-12-22 10:29:17 PST
<rdar://problem/72592069>