Bug 86310 – [GStreamer] media/media-continues-playing-after-replace-source.html fails

• Component: Media
• Status: RESOLVED FIXED

[Raphael Kubo da Costa (:rakuco)]

<http://trac.webkit.org/changeset/116858> has made the EFL and GTK+ bots fail the media/media-continues-playing-after-replace-source.html and should be fixed.

Layout test results for the bots can be accessed via http://build.webkit.org/results

[Max Feil]

(In reply to comment #0)
> <http://trac.webkit.org/changeset/116858> has made the EFL and GTK+ bots fail the media/media-continues-playing-after-replace-source.html and should be fixed.

This changeset is a result of https://bugs.webkit.org/show_bug.cgi?id=86235. The EWS analysis shows all green, including for efl and gtk. In fact different versions of this test passed both efl and gtk 3 times on separate EWS analyses. So I'm wondering what's going on here. 

The failed test results for EFL are:

  FAIL: Timed out waiting for notifyDone to be called
  Test that media keeps playing when the source element is replaced.

The failed test results for GTK are:

  FAIL: Timed out waiting for notifyDone to be called
  Test that media keeps playing when the source element is replaced.

  EVENT(canplaythrough)
  EXPECTED (testElement.currentTime == '0') OK
  EVENT(playing)
  EXPECTED (testElement.currentTime > '0') OK
  Replacing the media's source element:
  EXPECTED (testElement.currentTime == '0') OK

These seem like actual bugs. So what I propose to do is to add this test to the "skipped" files for these two ports, and then open new bugs to determine why the test fails.

[Max Feil]

(In reply to comment #1)
> These seem like actual bugs.

The EFL case looks like the Layout Test framework is not set up to support any HTML5 media tests. They are all failing. This is just one more in a long list of failures. Is the correct thing to put this test in the Skipped file in that case? Why aren't all the other media tests in the Skipped file?

[Max Feil]

In the GTK case, I'm thinking that it may be best to leave the test failure outstanding, rather than hide it via the Skipped file.

This test has highlighted an area where the GTK port is different than the other major ports, and perhaps should be brought in line instead of ignoring the test.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #1)
> This changeset is a result of https://bugs.webkit.org/show_bug.cgi?id=86235. The EWS analysis shows all green, including for efl and gtk. In fact different versions of this test passed both efl and gtk 3 times on separate EWS analyses. So I'm wondering what's going on here. 

Most of the EWS bots only build the patches, they don't run the layout tests.

(In reply to comment #2)

> The EFL case looks like the Layout Test framework is not set up to support any HTML5 media tests. They are all failing. This is just one more in a long list of failures. Is the correct thing to put this test in the Skipped file in that case? Why aren't all the other media tests in the Skipped file?

They aren't all skipped because they pass :-)

As you said, I think the best option for now is to skip the test on the port to get things green and then investigate.

[Raphael Kubo da Costa (:rakuco)]

I've skipped the test for the EFL port in <http://trac.webkit.org/changeset/116901>, and have filed bug 86322.

[Max Feil]

(In reply to comment #4)
> Most of the EWS bots only build the patches, they don't run the layout tests.

Thanks for that piece of information. I assumed all the EWS bots ran layout tests, as this is an important verification of the sanity of a patch. Can you tell me which bots don't run layout tests of the 7 ports which are currently part of EWS? Also, can you tell me how to actually run a proposed layout test on all major ports to ensure that it passes before I submit a patch for review?

> They aren't all skipped because they pass :-)

I am looking at results in http://build.webkit.org/results/EFL%20Linux%20Release/ and every media test that I look at has the following line in the actual results:

FAIL: Timed out waiting for notifyDone to be called

Am I looking in the wrong place?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #6)
> (In reply to comment #4)
> > Most of the EWS bots only build the patches, they don't run the layout tests.
> 
> Thanks for that piece of information. I assumed all the EWS bots ran layout tests, as this is an important verification of the sanity of a patch. Can you tell me which bots don't run layout tests of the 7 ports which are currently part of EWS?

IIRC, only the chromium bot runs layout tests, all the others only build the patches.

> Also, can you tell me how to actually run a proposed layout test on all major ports to ensure that it passes before I submit a patch for review?

This has been discussed a few times in webkit-dev but I don't think we've reached an agreement yet. Since people are not expected to have all ports built locally and then run all tests, the best we can do for now is to work with the people who do gardening for the ports and make sure the bots look green -- this means the tests need to be skipped until the failures are further investigated, or the commit which introduced them needs to be rolled out.

> > They aren't all skipped because they pass :-)
> 
> I am looking at results in http://build.webkit.org/results/EFL%20Linux%20Release/ and every media test that I look at has the following line in the actual results:
> 
> FAIL: Timed out waiting for notifyDone to be called
> 
> Am I looking in the wrong place?

The Release bot is not in a very good state at the moment -- I recommend taking a look at the Debug bot instead. Anyway, the very fact that some tests are present in the results zip file means it has failed somehow; OTOH, tests which are not there have passed.

[Max Feil]

I looked in http://build.webkit.org/results/ and the results for this test are as follows:
         
Chromium Linux Release (Tests): Test not in results.html, but in tests_run1.txt (i.e. PASS)
Qt Linux Release: Test not in results.html, OR tests_run*.txt (?)
GTK Linux 32-bit Release: time goes to 0 but 2nd source does not play (FAIL)
Lion Leaks: Test not in results.html, but in tests_run7.txt (i.e. PASS)
Lion Debug (Tests): Test not in results.html, but in tests_run3.txt (i.e. PASS)
Lion Debug (WebKit2 Tests): Test not in results.html, but in tests_run3.txt (i.e. PASS)
Windows: I can't find any up to date test results
EFL Linux Debug: time goes to 0 but 2nd source does not play (FAIL)

Both the GTK and EFL ports are skipping this test for now.

[Philippe Normand]

This is not a regression in the sense that this new test simply highlights a previously-hidden bug :)

The issue is that when the media element loads the new <source> element a new MediaPlayerPrivate instance is created and it sets the gst pipeline state to PAUSED instead of PLAYING, so the test forever waits for never-arriving timeupdate events.

One problem I see here is that the MediaPlayerPrivate is not aware that the up-layer previously tried another <source> element. So it's difficult to know when ::load() should set the pipeline to PLAYING instead of simply pausing it. :(

Maybe the MediaPlayerClient could be made aware of a previous URI that it attempted to play, so ::load() could compare the new and old URIs.

I wonder if that could be used by the Blackberry player as well and they could remove their not-so-great ::isElementPaused() which seems to be a layer violation, accessing the HTMLMediaElement API from the private player.

[Raphael Kubo da Costa (:rakuco)]

*** Bug 86322 has been marked as a duplicate of this bug. ***

[Philippe Normand]

I tested this on WebKitGTK WK1 and WK2 (with Martin's patch), I can clearly see that playback is not smooth.

[Philippe Normand]

(In reply to comment #11)
> I tested this on WebKitGTK WK1 and WK2 (with Martin's patch), I can clearly see that playback is not smooth.

wrong bug, I should get some sleep... sorry for the noise!

[Philippe Normand]

Created attachment 147488 [details]
Patch

[Philippe Normand]

Eric, can you please review this patch?

[Eric Carlson]

Comment on attachment 147488 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:948
> +    if (!m_currentSrc.isEmpty())
> +        m_previousSrc = m_currentSrc;

Why is this necessary? It would be good to have a comment here and in the change log.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:312
> +    // Reset network and ready states if the media element previously
> +    // used another location. This allows the media player client to
> +    // potentially emit a timeupdate event while the new location is being
> +    // loaded. States are synchronized again after the pipeline
> +    // pre-rolled.
> +    if (!m_player->mediaPlayerClient()->mediaPlayerPreviousUrl().isEmpty()) {
> +        m_networkState = MediaPlayer::Loading;
> +        m_player->networkStateChanged();
> +        m_readyState = MediaPlayer::HaveNothing;
> +        m_player->readyStateChanged();
> +    }

Why don't you *always* reset the states to Loading and HaveNothing like other ports do?

[Philippe Normand]

Created attachment 155487 [details]
Patch

Thanks Eric for the feedback. Unconditionally resetting the network and ready states is indeed the right and simpler fix.

[Eric Carlson]

Comment on attachment 155487 [details]
Patch

Much simpler, thanks!

[Philippe Normand]

Committed r125045: <http://trac.webkit.org/changeset/125045>

[Csaba Osztrogonác]

(In reply to comment #18)
> Committed r125045: <http://trac.webkit.org/changeset/125045>

It caused a regression on Qt WK2 - https://bugs.webkit.org/show_bug.cgi?id=93658
Could you check it, please?