Bug 35328

Created attachment 49358 [details] Initial Patch Patch, as promised.

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.

(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.

(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.

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

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.

(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 :/

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?

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 :/

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.

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

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

(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.

(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.

> It works for me. I should probably be clear here. This is working on OSX.

(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.

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.

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

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

(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?

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.

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.

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

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

=== 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.