Bug 35328 – [Qt] Various Fixes to MediaPlayerPrivateQt

• Component: Media
• Status: RESOLVED INVALID

[Nick Young]

A few minor bugs can be addressed in the current implementation of MediaPlayerPrivateQt

1) In updateStates(), assume a player with an "unknown" media status can be played and has loaded.
This allows us to remove an ugly hack in bytesLoaded() - which is not a reliable test anyway.
This also allows us to remove mutable from m_networkState and m_readyState.

In load(), we call updateStates() - because if the QMediaPlayer has just been constructed then there won't necessarily be a mediaStatusChanged signal.

2) In load(), Get the current volume and mute status from the MediaPlayer rather than being evil and pulling it from the MediaElement.
Currently we are breaking through the MediaPlayerClient interface as a work around for a bug.
Instead, we solve the bug. Blocked by Bug 35327.

Patch Forthcoming.

[Nick Young]

Created attachment 49358 [details]
Initial Patch

Patch, as promised.

[Eric Carlson]

Comment on attachment 49358 [details]
Initial Patch

 
> +        // Grab the client media element
> +        HTMLMediaElement* element = static_cast<HTMLMediaElement*>(m_player->mediaPlayerClient());
> +
>          // Grab the current document
>          Document* document = element->document();
>          if (!document)

Instead of using the element to get to the frame and document, you can use MediaPlayer's frameView().


>      QLatin1String bytesLoadedKey("bytes-loaded");
>      if (m_mediaPlayer->availableExtendedMetaData().contains(bytesLoadedKey))
>          return m_mediaPlayer->extendedMetaData(bytesLoadedKey).toInt();
>  
> -    return percentage;
> +    return m_mediaPlayer->bufferStatus();

It looks like bufferStatus() return the percent loaded. Shouldn't you multiply this by the resource size here to get bytes loaded?

r=me with these changes.

[Nick Young]

(In reply to comment #2)
> Instead of using the element to get to the frame and document, you can use
> MediaPlayer's frameView().

That's what I used to have, it doesn't work.

This is dependent on a renderer being created for the video element before we hit load(). This is not always the case.

Check out this example: http://people.freedesktop.org/~company/stuff/video-demo.html

In this example, a renderer is NEVER created for these videos because they are never added to the document. The demo still works as expected, but MediaPlayer::frameView() will always return NULL.

Perhaps there is room for MediaPlayerClient::mediaPlayerOwningDocument(), or something like that?

> It looks like bufferStatus() return the percent loaded. Shouldn't you multiply
> this by the resource size here to get bytes loaded?

Sure. I'm wasn't too worried about this method because we don't actually use it anywhere, but for the sake of implementing the interface properly, I'll do it.

[Eric Carlson]

(In reply to comment #3)
> (In reply to comment #2)
> > Instead of using the element to get to the frame and document, you can use
> > MediaPlayer's frameView().
> 
> That's what I used to have, it doesn't work.
> 
> This is dependent on a renderer being created for the video element before we
> hit load(). This is not always the case.
> 
> Check out this example:
> http://people.freedesktop.org/~company/stuff/video-demo.html
> 
> In this example, a renderer is NEVER created for these videos because they are
> never added to the document. The demo still works as expected, but
> MediaPlayer::frameView() will always return NULL.
> 
> Perhaps there is room for MediaPlayerClient::mediaPlayerOwningDocument(), or
> something like that?
> 
Good point. mediaPlayerOwningDocument sounds like a fine way to go.

[Nick Young]

> Good point. mediaPlayerOwningDocument sounds like a fine way to go.

Bug 35374

[Nick Young]

Created attachment 49466 [details]
Updated Patch

This patch fixes the issues raised by Eric.

This patch also expands the scope of this Bug to contain two more issues:

1) A bug introduced by http://trac.webkit.org/changeset/55153 - which has some very broken MIME logic.
2) A bug discovered during painting.

This bug is blocked by Bug 35374, as we are utilising the new interface.
I'll come back and set r? and cq? on this patch once the patch in that bug has been landed by the cq.

[Tor Arne Vestbø]

(In reply to comment #6)
> 1) A bug introduced by http://trac.webkit.org/changeset/55153 - which has some
> very broken MIME logic.

Wow, what the hell was I smoking!? Sorry :/

[Tor Arne Vestbø]

Comment on attachment 49466 [details]
Updated Patch

> +        if (mime.startsWith("audio/") || mime.startsWith("video/") || mime == "application/octet-stream")

The initial problem this tried to fix was that the media backend was suddenly claiming to being able to handle for example text files, so if you opened file:///tmp/foo.txt it would create a mock html document with a media element with the src set to the txt file. Not sure of the interaction here, but would this change potentially introduce the same bug for application/octet-stream, so instead of getting download notifications for all you exe files you would suddenly start opening them as media elements?

[Tor Arne Vestbø]

Comment on attachment 49466 [details]
Updated Patch


> -    unsigned percentage = m_mediaPlayer->bufferStatus();
> -
> -    if (percentage == 100) {
> -        if (m_networkState != MediaPlayer::Idle) {
> -            m_networkState = MediaPlayer::Idle;
> -            m_player->networkStateChanged();
> -        }
> -        if (m_readyState != MediaPlayer::HaveEnoughData) {
> -            m_readyState = MediaPlayer::HaveEnoughData;
> -            m_player->readyStateChanged();
> -        }
> -    }
> -
>      QLatin1String bytesLoadedKey("bytes-loaded");
>      if (m_mediaPlayer->availableExtendedMetaData().contains(bytesLoadedKey))
>          return m_mediaPlayer->extendedMetaData(bytesLoadedKey).toInt();
>  
> -    return percentage;
> +    return (m_mediaPlayer->bufferStatus() * totalBytes()) / 100;
>  }
>  
>  unsigned MediaPlayerPrivate::totalBytes() const
> @@ -468,16 +451,19 @@ void MediaPlayerPrivate::updateStates()
>              m_networkState = MediaPlayer::FormatError;
>          else
>              m_networkState = MediaPlayer::NetworkError;
> -    } else if (currentStatus == QMediaPlayer::UnknownMediaStatus
> -               || currentStatus == QMediaPlayer::NoMedia) {
> +    } else if (currentStatus == QMediaPlayer::NoMedia) {
>          m_networkState = MediaPlayer::Idle;
>          m_readyState = MediaPlayer::HaveNothing;
> +    } else if (currentStatus == QMediaPlayer::UnknownMediaStatus) {
> +        // If the status is unknown, it's quite likely we are able to play
> +        m_networkState = MediaPlayer::Idle;
> +        m_readyState = MediaPlayer::HaveEnoughData;

Doing this makes loading the Transformers trailer go from instantaneous to 5-10 seconds before the first loadedmetadata, and even then it does not seem to load fully to be able to play. 

Presumably because we have the buffer filled to 100%, which with the old code would dump us into Idle and HaveEnoughData, but now we don't update our states anymore on bytesLoaded() (a good thing per se).

I tried doing updateStates() on the bufferStateChange() signal, which I would assume would be the right fix, but that didn't change anything as we don't seem to receive this signal :/

[Tor Arne Vestbø]

Btw, referring to this test page:

http://chaos.troll.no/~tavestbo/webkit/mediaelement/

I've update the states and attributes to match the IDL files.

[Tor Arne Vestbø]

Created attachment 49494 [details]
Before patch, going directly into idle/havedata

[Tor Arne Vestbø]

Created attachment 49495 [details]
After patch, loading takes a long time

[Eric Carlson]

(In reply to comment #6)
> Created an attachment (id=49466) [details]
> Updated Patch
> 
> This patch fixes the issues raised by Eric.
> 
> This patch also expands the scope of this Bug to contain two more issues:
> 
> 1) A bug introduced by http://trac.webkit.org/changeset/55153 - which has some
> very broken MIME logic.
> 2) A bug discovered during painting.
> 
I would prefer that these be split into separate bugs/patches.

[Nick Young]

(In reply to comment #9)

> Doing this makes loading the Transformers trailer go from instantaneous to 5-10
> seconds before the first loadedmetadata, and even then it does not seem to load
> fully to be able to play.

It works for me. Also if you look at the code, I never set the MediaPlayer in to the HAVE_METADATA state, and I can not see anywhere in WebKit where HAVE_METADATA would be returned instead of the media player's state. This seems to be inconsistent with the data in your second attachment.

> Presumably because we have the buffer filled to 100%, which with the old code
> would dump us into Idle and HaveEnoughData, but now we don't update our states
> anymore on bytesLoaded() (a good thing per se).

As I understand it, bytesLoaded() should never be called. It is not used by webkit currently, it is only provided to have a complete interface.

> I tried doing updateStates() on the bufferStateChange() signal, which I would
> assume would be the right fix, but that didn't change anything as we don't seem
> to receive this signal :/

bufferStateChange() is only emitted when the media status is either Buffering or Stalled.

[Nick Young]

> It works for me.

I should probably be clear here. This is working on OSX.

[Nick Young]

(In reply to comment #8)
> The initial problem this tried to fix was that the media backend was suddenly
> claiming to being able to handle for example text files, so if you opened
> file:///tmp/foo.txt it would create a mock html document with a media element
> with the src set to the txt file. Not sure of the interaction here, but would
> this change potentially introduce the same bug for application/octet-stream, so
> instead of getting download notifications for all you exe files you would
> suddenly start opening them as media elements?

OK, I will concede this point. None of the other backends will claim to support application/octet-stream, so we probably shouldn't either.

[Nick Young]

Created attachment 49552 [details]
Updated Patch

As requested by eric, this patch only contains the issues originally raised in this bug.
All other issues have been resolved as previously mentioned.

[Nick Young]

(In reply to comment #13)
> I would prefer that these be split into separate bugs/patches.

Bug 35412 and Bug 35413.

[Tor Arne Vestbø]

Created attachment 49576 [details]
After patch, loading takes a long time (v2)

[Tor Arne Vestbø]

(In reply to comment #14)
> (In reply to comment #9)
> 
> > Doing this makes loading the Transformers trailer go from instantaneous to 5-10
> > seconds before the first loadedmetadata, and even then it does not seem to load
> > fully to be able to play.
> 
> It works for me. Also if you look at the code, I never set the MediaPlayer in
> to the HAVE_METADATA state, and I can not see anywhere in WebKit where
> HAVE_METADATA would be returned instead of the media player's state. This seems
> to be inconsistent with the data in your second attachment.

Sorry, my bad, we don't go to HAVE_METADATA but to HAVE_ENOUGH_DATA, see
updated screenshot.

The loading is still very slow though (15 seconds). We go to HAVE_ENOUGH_DATA
as a result of the QMediaPlayer::mediaStatusChanged() signal when the status
changes to QMediaPlayer::LoadedMedia, but we never end up in the
UnknownMediaStatus which would be the code-path to replace the old code from
bytesLoaded().

> > Presumably because we have the buffer filled to 100%, which with the old code
> > would dump us into Idle and HaveEnoughData, but now we don't update our states
> > anymore on bytesLoaded() (a good thing per se).
> 
> As I understand it, bytesLoaded() should never be called. It is not used by
> webkit currently, it is only provided to have a complete interface.

MediaPlayerPrivate::bytesLoaded() is called from
HTMLMediaElement::progressEventTimerFired() through MediaPlayer::bytesLoaded().

In the 15 second timeframe above when waiting for mediaStatusChanged(), we've
received numerous calls to bytesLoaded(), where the call to
m_mediaPlayer->bufferStatus() returns 100, so as far as the QtMultimedia
backend is concerned we've buffered 100%, but we never receive any signals
where we can react to that by setting HAVE_ENOUGH_DATA (see below).

> > I tried doing updateStates() on the bufferStateChange() signal, which I would
> > assume would be the right fix, but that didn't change anything as we don't seem
> > to receive this signal :/
> 
> bufferStateChange() is only emitted when the media status is either Buffering
> or Stalled.

Okey, not even at 100%? Do we get some other signal when the buffer is filled
to 100%, for example a media status change? Any idea why we're not getting the
UnknownMediaStatus change?

[Tor Arne Vestbø]

Comment on attachment 49552 [details]
Updated Patch

Makes sense to split this one up too, 3 patches with their own changelog

 - volume fix
 - use mediaPlayerOwningDocument
 - bytesLoaded hack -> UnknownMediaStatus with mutables removed

Lets us land the first two while we investigate the third.

[WebKit Commit Bot]

Comment on attachment 49358 [details]
Initial Patch

Cleared Eric Carlson's review+ from obsolete attachment 49358 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should only be used for bugs/features in the
    public QtWebKit API layer, not to signify that the bug is specific to
    the Qt port of WebKit

      http://trac.webkit.org/wiki/QtWebKitBugs#Component

  - Add the keyword 'Qt' to signal that it's a Qt-related bug

      http://trac.webkit.org/wiki/QtWebKitBugs#Keywords

[Alexis Menard (darktears)]

This bug is outdated and the patches two. Could it be re-investigated?

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.