Bug 51407 – A check should be in place to ensure that a media engine error has not occurred after MediaPlayer::play() is called

• Status: CLOSED
• Resolution: WONTFIX
• Component: Media

[Lukas Sydorowski]

Created attachment 77130 [details]
Proposed patch

In the event of a media engine error occurring at the start of media playback (when MediaPlayer::play() is called from HTMLMediaElement::updatePlayState()), a check should be made that an error did not occur immediately after attempting to play media. This ensures that the media element is aware that the media was not able to play in the event that m_error was set from HTMLMediaElement::mediaEngineError().

A proposed patch is attached.

[Darin Adler]

Comment on attachment 77130 [details]
Proposed patch

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

> WebCore/ChangeLog:8
> +        No new tests required.

Why? The WebKit project requires tests for all bug fixes unless there is some reason a test is impossible or impractical. If this has no effect observable in a test, then why would we make the code change?

[Lukas Sydorowski]

(In reply to comment #1)
> (From update of attachment 77130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77130&action=review
> 
> > WebCore/ChangeLog:8
> > +        No new tests required.
> 
> Why? The WebKit project requires tests for all bug fixes unless there is some reason a test is impossible or impractical. If this has no effect observable in a test, then why would we make the code change?

A test would only make sense in testing a media engine failure at this stage of execution, which may very well be platform dependent depending on how media rendering is performed on the particular platform. If there is a way to "fake" a media engine failure in the tests that is NOT platform dependent (such as making a call to mediaEngineError() at this point - Is this actually possible?), then I will write a test for this.

[Alexey Proskuryakov]

Comment on attachment 77130 [details]
Proposed patch

A test that fails on at least one platform without this fix, and passes with it would be enough. If this doesn't happen on any core platforms, perhaps the fix should be in platform layer, not in HTMLMediaElement - making cross-platform WebCore fixes that are not tested by build bots is almost the same as not making them at all.

Marking r- due to lack of tests.

[Eric Carlson]

Comment on attachment 77130 [details]
Proposed patch

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

> WebCore/html/HTMLMediaElement.cpp:2062
> -        startPlaybackProgressTimer();
> -        m_playing = true;
> +        // Ensure that we don't have any errors (especially that the media engine didn't give us an error).
> +        if (!m_error) {
> +            startPlaybackProgressTimer();
> +            m_playing = true;
> +        } else {
> +            // In the event of an error, ensure that the playback rate is 0.
> +            m_player->setRate(0);
> +        }

This assumes that an error triggered by calling m_player->play() will always happen synchronously, eg. that it will happen before the call returns, which is *definitely* not the case on all platforms.

I think a better way to handle this is to ensure that your media engine doesn't signal an error by changing the network state until the next idle (queue a timer, or whatever).

[Lukas Sydorowski]

(In reply to comment #4)
> (From update of attachment 77130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77130&action=review
> 
> > WebCore/html/HTMLMediaElement.cpp:2062
> > -        startPlaybackProgressTimer();
> > -        m_playing = true;
> > +        // Ensure that we don't have any errors (especially that the media engine didn't give us an error).
> > +        if (!m_error) {
> > +            startPlaybackProgressTimer();
> > +            m_playing = true;
> > +        } else {
> > +            // In the event of an error, ensure that the playback rate is 0.
> > +            m_player->setRate(0);
> > +        }
> 
> This assumes that an error triggered by calling m_player->play() will always happen synchronously, eg. that it will happen before the call returns, which is *definitely* not the case on all platforms.

This does not assume that the error setting will *always* happen synchronously, just that it *may* happen synchronously. In the event that the error triggered happens asynchronously instead (as is the assumption on most platforms it seems), then this should result in no change to the program's behavior because m_error will not have been set yet.

So this leads me to the question: Is there a critical reason why we would never want to set the error synchronously here?

[Eric Carlson]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 77130 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=77130&action=review
> > 
> > > WebCore/html/HTMLMediaElement.cpp:2062
> > > -        startPlaybackProgressTimer();
> > > -        m_playing = true;
> > > +        // Ensure that we don't have any errors (especially that the media engine didn't give us an error).
> > > +        if (!m_error) {
> > > +            startPlaybackProgressTimer();
> > > +            m_playing = true;
> > > +        } else {
> > > +            // In the event of an error, ensure that the playback rate is 0.
> > > +            m_player->setRate(0);
> > > +        }
> > 
> > This assumes that an error triggered by calling m_player->play() will always happen synchronously, eg. that it will happen before the call returns, which is *definitely* not the case on all platforms.
> 
> This does not assume that the error setting will *always* happen synchronously, just that it *may* happen synchronously. In the event that the error triggered happens asynchronously instead (as is the assumption on most platforms it seems), then this should result in no change to the program's behavior because m_error will not have been set yet.
> 

I meant that the "fix" assumes the error will be set synchronously. If this behavior change is important, why not also do it when the error is fired asynchronously?

> So this leads me to the question: Is there a critical reason why we would never want to set the error synchronously here?

I am not aware of any reason in this case. The QuickTime based media engines don't end up setting it synchronously because we don't make calls to WebCore directly from framework callbacks so we don't end up recursing back into the framework.

Why is this necessary at all? The media engine signals the error in the first place, why doesn't it know that playback failed?

[Lukas Sydorowski]

I have come to the realization that this patch is no longer needed. Thank you all for your comments and help.

[Lukas Sydorowski]

Closing.