Bug 86113 – Replace WebMediaPlayer::bytesLoaded() with an explicit didLoadingProgress()

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Ami Fischman]

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)

[Andrew Scherkus]

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?

[Philippe Normand]

bytesLoaded() is also used in HTMLMediaElement::progressEventTimerFired() BTW.

Anyway yes, this change proposed by Ami seems like a good approach to me.

[Ami Fischman]

Created attachment 145005 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Early Warning System Bot]

Comment on attachment 145005 [details]
Patch

Attachment 145005 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12866126

[Early Warning System Bot]

Comment on attachment 145005 [details]
Patch

Attachment 145005 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12861204

[Build Bot]

Comment on attachment 145005 [details]
Patch

Attachment 145005 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12868135

[Ami Fischman]

Created attachment 145010 [details]
Patch

[Eric Carlson]

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 ;-)

[Ami Fischman]

Created attachment 145099 [details]
Patch

[Ami Fischman]

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!

[Eric Carlson]

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 ( ... ) { "

[Eric Carlson]

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

[Ami Fischman]

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.

[Ami Fischman]

Created attachment 145104 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 145104 [details]
Patch

Chromium WebKit API changes LGTM.

[WebKit Review Bot]

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

[Ami Fischman]

Committed r119125: <http://trac.webkit.org/changeset/119125>

[WebKit Review Bot]

Re-opened since this is blocked by 88007