Today WebMediaPlayer::bytesLoaded() is used only to detect loading progress happening (in HTMLMediaElement::changeNetworkStateFromLoadingToIdle()). Instead, WebMediaPlayer could expose a bool didLoadingProgress(); method that returned whether loading progress happened since the last time this function was called. This would allow simplifying WMP implementations that don't otherwise need to track counts of bytes buffered. (I hear scherkus/eric.carlson/acolwell discussed this f2f; feel free to add color to this bug if there's more to it that I missed)
Yeah the idea was to let ports have more choice in deciding what the definition of "progress" is. There are a few options: 1) Monitor changes in buffered() TimeRanges (would this be sufficient?) 2) Ask ports for a loading percentage (a more abstract version of what we have today) 3) Ask ports a "did you progress since the last time I asked you?" method (what you propose here) While (3) is a bit more complex I believe the added flexibility is warranted here. Monitoring changes in buffered() as well as a load percentage assumes that the underlying port is actually buffering. For example, what would a port report in (1) and (2) if it was an infinite live stream?
bytesLoaded() is also used in HTMLMediaElement::progressEventTimerFired() BTW. Anyway yes, this change proposed by Ami seems like a good approach to me.
Created attachment 145005 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 145005 [details] Patch Attachment 145005 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12866126
Comment on attachment 145005 [details] Patch Attachment 145005 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12861204
Comment on attachment 145005 [details] Patch Attachment 145005 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12868135
Created attachment 145010 [details] Patch
Comment on attachment 145010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145010&action=review > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:374 > + unsigned currentMaxTimeLoaded = maxTimeLoaded(); "unsigned"? Do you really only want to report loading progress once every second? > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:375 > + bool ret = currentMaxTimeLoaded != m_maxTimeLoadedAtLastDidLoadingProgress; "ret"? Be bold, use a few more letters and spell out the name of the variable ;-)
Created attachment 145099 [details] Patch
Comment on attachment 145010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145010&action=review >> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:374 >> + unsigned currentMaxTimeLoaded = maxTimeLoaded(); > > "unsigned"? Do you really only want to report loading progress once every second? Nope; that was a bug. Fixed. >> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:375 >> + bool ret = currentMaxTimeLoaded != m_maxTimeLoadedAtLastDidLoadingProgress; > > "ret"? Be bold, use a few more letters and spell out the name of the variable ;-) On your head be it when the Character Availability Crisis of 2012 hits webkit-dev worldwide!
Comment on attachment 145099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145099&action=review This looks correct to me. Not marking r+ pending review by abarth, dglazkov, fishd, jamesr, or tkent. > Source/WebCore/html/HTMLMediaElement.cpp:1850 > + } else { > + if (timedelta > 3.0 && !m_sentStalledEvent) { Nit: this could be made into an "else if ( ... ) { "
(In reply to comment #11) > (From update of attachment 145010 [details]) > >> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:375 > >> + bool ret = currentMaxTimeLoaded != m_maxTimeLoadedAtLastDidLoadingProgress; > > > > "ret"? Be bold, use a few more letters and spell out the name of the variable ;-) > > On your head be it when the Character Availability Crisis of 2012 hits webkit-dev worldwide! Oh dear, now I am in for it!
Comment on attachment 145099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145099&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:1850 >> + if (timedelta > 3.0 && !m_sentStalledEvent) { > > Nit: this could be made into an "else if ( ... ) { " Done.
Created attachment 145104 [details] Patch
Comment on attachment 145104 [details] Patch Chromium WebKit API changes LGTM.
Comment on attachment 145104 [details] Patch Rejecting attachment 145104 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: webkit-commit-queue/Source/WebKit/chromium/webkit --revision 139747 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 48>At revision 139747. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/12866281
Committed r119125: <http://trac.webkit.org/changeset/119125>
Re-opened since this is blocked by 88007