Bug 185725 - [GStreamer] Don't set the ReadyState to HaveNothing when an error occurs in the playback pipeline
Summary: [GStreamer] Don't set the ReadyState to HaveNothing when an error occurs in t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 185723
Blocks: 185593
  Show dependency treegraph
 
Reported: 2018-05-17 06:52 PDT by Yacine Bandou
Modified: 2018-05-22 04:40 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.68 KB, patch)
2018-05-17 10:58 PDT, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (3.25 KB, patch)
2018-05-19 18:50 PDT, Yacine Bandou
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.84 MB, application/zip)
2018-05-21 21:10 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yacine Bandou 2018-05-17 06:52:24 PDT
This patch fixes the crash of some WPT encrypted-media tests like: clearkey-mp4-playback-temporary-multikey-sequential.https.html or clearkey-mp4-playback-temporary-clear-encrypted.https.html.

These crashes should be fixed in 185242 but unfortunately it isn't the case. See bug 185242 for more detail.
	
Here is the root cause of the crash:

1.When an error occurs in playback pipeline (no decipher key for example), we receive an error message in MediaPlayerPrivateGStreamer::handleMessage -> MediaPlayerPrivateGStreamer::loadingFailed (MediaPlayer::FormatError).

2.The function loadingFailed -> HTMLMediaElement::mediaPlayerNetworkStateChanged (MediaPlayer::FormatError) -> setNetworkState -> mediaLoadingFailed -> noneSupported go to the point 3
                            |
                             -> HTMLMediaElement::mediaPlayerReadyStateChanged(MediaPlayer::HaveNothing) -> setReadyState -> updatePlayState() go to the point 5

3.nonSupported -> detachMediaSource -> MediaSource::detachFromElement -> removeSourceBuffer -> MediaSourceGStreamer::removeSourceBuffer -> .. ->PlaybackPipeline::removeSourceBuffer

4.PlaybackPipeline::removeSourceBuffer -> webKitMediaSrcFreeStream . 

5.HTMLMediaElement::updatePlayState -> potentiallyPlaying -> stoppedDueToErrors ( This function returns false because (m_readyState >= HAVE_METADATA && m_error) is false, m_readyState equal to HaveNothing )

6.HTMLMediaElement::updatePlayState -> MediaPlayer::play -> MediaPlayerPrivateGStreamer::play -> changePipelineState (set the pipeline to playing state)

7.WebkitMediaSourceGStreamer sends "source-setup" signal when its state change from Ready to Paused, the signal catched in MediaPlayerPrivateGStreamer::sourceSetupCallback

8.MediaPlayerPrivateGStreamer::sourceSetupCallback -> MediaPlayerPrivateGStreamerMSE::sourceSetup -> MediaSourceGStreamer::open -> MediaSource::setPrivateAndOpen (crash in this function because the mediaSource is detached from mediaElement in the point 3, thus the variable  m_mediaElement is null).

In the point 5 the function "HTMLMediaElement::stoppedDueToErrors" returns false, because m_readyState equal to HaveNothing and it is not upper than HAVE_METADATA.

In order to avoid trying again to play the same URI after an error, we don't set the ReadyState to HaveNothing when an error occurs in the pipeline, we should simply set NetworkState to error.
Comment 1 Yacine Bandou 2018-05-17 10:58:18 PDT
Created attachment 340602 [details]
Patch
Comment 2 Philippe Normand 2018-05-18 04:44:45 PDT
Comment on attachment 340602 [details]
Patch

I see no MSE-specific changes in this patch, confusing. Can't you simply override loadingFailed() in the MSE player?
Comment 3 Yacine Bandou 2018-05-18 07:35:04 PDT
(In reply to Philippe Normand from comment #2)
> Comment on attachment 340602 [details]
> Patch
> 
> I see no MSE-specific changes in this patch, confusing. Can't you simply
> override loadingFailed() in the MSE player?

I think even in the no-mse (regular playback), we have not to set the ReadyState to HaveNothing when an error occurs in playback because it's not true, we have enough data, just an error occurs in playing state.

see W3C spec: https://dev.w3.org/html5/pf-summary/video.html#dom-media-readystate
<<< HAVE_NOTHING (numeric value 0):
No information regarding the media resource is available. No data for the current playback position is available. Media elements whose networkState attribute is NETWORK_EMPTY are always in the HAVE_NOTHING state. >>>

In AVFoundation port, they explicitly set the ReadyState to HaveNothing only when they start a new load.

But I agree, I should put these explanations in changeLog and change the title of the bug and the patch.
Comment 4 Philippe Normand 2018-05-18 08:01:06 PDT
Then if this method has only one call site there is no need for a method. Please also check this change doesn't break any test.
Comment 5 Yacine Bandou 2018-05-19 18:50:06 PDT
Created attachment 340790 [details]
Patch
Comment 6 Philippe Normand 2018-05-21 04:21:27 PDT
Did you check no test was affected by this patch?
Comment 7 Yacine Bandou 2018-05-21 04:35:53 PDT
(In reply to Philippe Normand from comment #6)
> Did you check no test was affected by this patch?

Yes, I tested all the media tests with WPE port. I have not seen any regression.
Comment 8 Philippe Normand 2018-05-21 04:53:36 PDT
According to the wpe/TestExpectations, all media tests are skipped. Please check with WebKitGTK.
Comment 9 Yacine Bandou 2018-05-21 05:28:01 PDT
(In reply to Philippe Normand from comment #8)
> According to the wpe/TestExpectations, all media tests are skipped. Please
> check with WebKitGTK.

I agree that most test are skipped but not all, we have 
67 tests  in LayoutTests/media/ 
53 tests  in LayoutTests/imported/w3c/web-platform-tests/encrypted-media/

I am preparing an new env for GTK in order to do the tests.
Comment 10 Yacine Bandou 2018-05-21 09:07:15 PDT
(In reply to Yacine Bandou from comment #9)
> (In reply to Philippe Normand from comment #8)
> > According to the wpe/TestExpectations, all media tests are skipped. Please
> > check with WebKitGTK.
> 
> I agree that most test are skipped but not all, we have 
> 67 tests  in LayoutTests/media/ 
> 53 tests  in LayoutTests/imported/w3c/web-platform-tests/encrypted-media/
> 
> I am preparing an new env for GTK in order to do the tests.

I have passed all these tests with GTK port, I have not seen a regression:

730 tests in LayoutTests/media
59  tests in LayoutTests/imported/w3c/web-platform-tests/media-source/
Comment 11 Philippe Normand 2018-05-21 09:09:54 PDT
Cool. Please check http/tests/media too :)
Comment 12 Yacine Bandou 2018-05-21 09:18:28 PDT
(In reply to Philippe Normand from comment #11)
> Cool. Please check http/tests/media too :)

All 58 tests ran as expected
Comment 13 Build Bot 2018-05-21 21:10:12 PDT
Comment on attachment 340790 [details]
Patch

Attachment 340790 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7760761

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html
Comment 14 Build Bot 2018-05-21 21:10:23 PDT
Created attachment 340950 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 15 Xabier Rodríguez Calvar 2018-05-21 23:56:16 PDT
(In reply to Yacine Bandou from comment #10)
> (In reply to Yacine Bandou from comment #9)
> > (In reply to Philippe Normand from comment #8)
> > > According to the wpe/TestExpectations, all media tests are skipped. Please
> > > check with WebKitGTK.
> > 
> > I agree that most test are skipped but not all, we have 
> > 67 tests  in LayoutTests/media/ 
> > 53 tests  in LayoutTests/imported/w3c/web-platform-tests/encrypted-media/
> > 
> > I am preparing an new env for GTK in order to do the tests.
> 
> I have passed all these tests with GTK port, I have not seen a regression:
> 
> 730 tests in LayoutTests/media
> 59  tests in LayoutTests/imported/w3c/web-platform-tests/media-source/

Please ensure MSE/EME are enabled in runtime.
Comment 16 WebKit Commit Bot 2018-05-22 04:40:18 PDT
Comment on attachment 340790 [details]
Patch

Clearing flags on attachment: 340790

Committed r232061: <https://trac.webkit.org/changeset/232061>
Comment 17 WebKit Commit Bot 2018-05-22 04:40:20 PDT
All reviewed patches have been landed.  Closing bug.