Bug 55901 – [Qt] Fix the error code for media resource failures when using QtMobility

• Component: Media
• Status: RESOLVED FIXED

[Yi Shen]

According to Html 5 video spec, when the media resource failed to load or that the given URL could not be resolved, the media element should adjust its status as following,

      1. Set the error attribute to a new MediaError object whose code attribute is set to MEDIA_ERR_SRC_NOT_SUPPORTED.
      2. Set the element's networkState attribute to the NETWORK_NO_SOURCE value.

Currently, qMediaPlayer returns QMediaPlayer::InvalidMedia for media resource failures, instead of setting m_networkState to NetworkError, we should use MediaPlayer::FormatError to indicate the resource error.

[Yi Shen]

Created attachment 84977 [details]
proposed fix

[Alexis Menard (darktears)]

Comment on attachment 84977 [details]
proposed fix

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

> Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:535
> +        else if (m_networkState != MediaPlayer::FormatError)

Not sure to get that change...Could you explain me?

[Yi Shen]

(In reply to comment #2)
> (From update of attachment 84977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84977&action=review
> 
> > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:535
> > +        else if (m_networkState != MediaPlayer::FormatError)
> 
> Not sure to get that change...Could you explain me?

That's because Qt Mobility sends out a NetworkError after InvalidMedia (Note, InvalidMedia is not a kind of error for Qt Mobility). I think it makes sense for Qt Mobility, but for MediaPlayerPrivateQt, it should treat this special NetworkError (the one follow the InvalidMedia) as a FormatError.

Maybe following code is more clear about this idea, although it causes an unnecessary assignment.

 -        if (currentError == QMediaPlayer::FormatError)
 +        if (currentError == QMediaPlayer::FormatError || (m_networkState == QMediaPlayer::FormatError && currentError == QMediaPlayer::NetworkError))
             m_networkState = MediaPlayer::FormatError;
         else
             m_networkState = MediaPlayer::NetworkError;

What do you think? Alexis :)

[Alexis Menard (darktears)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 84977 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=84977&action=review
> > 
> > > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:535
> > > +        else if (m_networkState != MediaPlayer::FormatError)
> > 
> > Not sure to get that change...Could you explain me?
> 
> That's because Qt Mobility sends out a NetworkError after InvalidMedia (Note, InvalidMedia is not a kind of error for Qt Mobility). I think it makes sense for Qt Mobility, but for MediaPlayerPrivateQt, it should treat this special NetworkError (the one follow the InvalidMedia) as a FormatError.
> 
> Maybe following code is more clear about this idea, although it causes an unnecessary assignment.
> 
>  -        if (currentError == QMediaPlayer::FormatError)
>  +        if (currentError == QMediaPlayer::FormatError || (m_networkState == QMediaPlayer::FormatError && currentError == QMediaPlayer::NetworkError))
>              m_networkState = MediaPlayer::FormatError;
>          else
>              m_networkState = MediaPlayer::NetworkError;
> 
> What do you think? Alexis :)

Well the assignment is no big deal as soon as it does not send an extra notification. But at least the code is clearer to read

[Alexis Menard (darktears)]

Could you please update the patch with your second suggestion and a little comment on the condition on why we need this extra check, basically what you wrote to me :).

[Yi Shen]

(In reply to comment #5)
> Could you please update the patch with your second suggestion and a little comment on the condition on why we need this extra check, basically what you wrote to me :).

Thanks for reminding me, Alexis :) It has been a while, so I may need to check/test the patch carefully with the latest qt-mobility. I will provide a new patch once I finish the task on my hand. Thanks!

[Laszlo Gombos]

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

[Joel Parks]

needed for BR-7521

[Simon Hausmann]

Comment on attachment 84977 [details]
proposed fix

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

> Source/WebCore/ChangeLog:8
> +        To indicate the media resource failures, set the netwrok state to

netwrok -> network

[Simon Hausmann]

Comment on attachment 84977 [details]
proposed fix

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

Please fix these issues before landing.

>>>> Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:535

>>> 
>>> Not sure to get that change...Could you explain me?
>> 
>> That's because Qt Mobility sends out a NetworkError after InvalidMedia (Note, InvalidMedia is not a kind of error for Qt Mobility). I think it makes sense for Qt Mobility, but for MediaPlayerPrivateQt, it should treat this special NetworkError (the one follow the InvalidMedia) as a FormatError.
>> 
>> Maybe following code is more clear about this idea, although it causes an unnecessary assignment.
>> 
>>  -        if (currentError == QMediaPlayer::FormatError)
>>  +        if (currentError == QMediaPlayer::FormatError || (m_networkState == QMediaPlayer::FormatError && currentError == QMediaPlayer::NetworkError))
>>              m_networkState = MediaPlayer::FormatError;
>>          else
>>              m_networkState = MediaPlayer::NetworkError;
>> 
>> What do you think? Alexis :)
> 
> Well the assignment is no big deal as soon as it does not send an extra notification. But at least the code is clearer to read

I also prefer the more readable code. Let's go for that. The rest looks okay to me :)

[Yi Shen]

Thanks for the review, Simon. I think I need to double check the patch with the QtMobility 1.2 before landing the patch. Will do it tomorrow and put comments here.

(In reply to comment #10)
> (From update of attachment 84977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84977&action=review
> 
> Please fix these issues before landing.
> 
> >>>> Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:535
> 
> >>> 
> >>> Not sure to get that change...Could you explain me?
> >> 
> >> That's because Qt Mobility sends out a NetworkError after InvalidMedia (Note, InvalidMedia is not a kind of error for Qt Mobility). I think it makes sense for Qt Mobility, but for MediaPlayerPrivateQt, it should treat this special NetworkError (the one follow the InvalidMedia) as a FormatError.
> >> 
> >> Maybe following code is more clear about this idea, although it causes an unnecessary assignment.
> >> 
> >>  -        if (currentError == QMediaPlayer::FormatError)
> >>  +        if (currentError == QMediaPlayer::FormatError || (m_networkState == QMediaPlayer::FormatError && currentError == QMediaPlayer::NetworkError))
> >>              m_networkState = MediaPlayer::FormatError;
> >>          else
> >>              m_networkState = MediaPlayer::NetworkError;
> >> 
> >> What do you think? Alexis :)
> > 
> > Well the assignment is no big deal as soon as it does not send an extra notification. But at least the code is clearer to read
> 
> I also prefer the more readable code. Let's go for that. The rest looks okay to me :)

[Yi Shen]

Created attachment 91298 [details]
fix the patch according to the qtmobility change

With the QtMobility 1.2.0, it can send out QMediaPlayer::ResourceError after QMediaPlayer::InvalidMedia for resource issue. So I made a little bit change for it in this new patch.

Also, I noticed that all the media tests are disabled in https://bugs.webkit.org/show_bug.cgi?id=57983, so I just tested the related layouttest(including tests under LayoutTest/media and LayoutTest/fast/media) on my environment.

[Alexis Menard (darktears)]

Comment on attachment 91298 [details]
fix the patch according to the qtmobility change

I'm fine with the change. The only hic is that we will enforce the usage of QtMM 1.2, but I'm also fine with that.

[Simon Hausmann]

Comment on attachment 91298 [details]
fix the patch according to the qtmobility change

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

r=me

> Source/WebCore/ChangeLog:8
> +        To indicate the media resource failures, set the netwrok state to

Please fix the netwrok -> network typo before landing :)

[Yi Shen]

(In reply to comment #14)
> (From update of attachment 91298 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91298&action=review
> 
> r=me
> 
> > Source/WebCore/ChangeLog:8
> > +        To indicate the media resource failures, set the netwrok state to
> 
> Please fix the netwrok -> network typo before landing :)

Thanks for your review, Simon :) I will update/test it ASAP.

[Yi Shen]

Created attachment 96331 [details]
fix typo

[WebKit Review Bot]

Comment on attachment 96331 [details]
fix typo

Clearing flags on attachment: 96331

Committed r88318: <http://trac.webkit.org/changeset/88318>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r88318 cherry-picked into qtwebkit-2.2 with commit f1e20a4 <http://gitorious.org/webkit/qtwebkit/commit/f1e20a4>