Bug 29041 - HTMLMediaElement buffered attribute should report a list of time range
Summary: HTMLMediaElement buffered attribute should report a list of time range
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-08 10:53 PDT by Hin-Chung Lam
Modified: 2009-09-10 19:43 PDT (History)
2 users (show)

See Also:


Attachments
API change (14.59 KB, patch)
2009-09-08 11:02 PDT, Hin-Chung Lam
eric.carlson: review+
Details | Formatted Diff | Diff
patch to be submitted (16.44 KB, patch)
2009-09-08 17:25 PDT, Hin-Chung Lam
eric.carlson: review+
Details | Formatted Diff | Diff
patch to be submitted (16.49 KB, patch)
2009-09-09 16:40 PDT, Hin-Chung Lam
eric.carlson: review-
Details | Formatted Diff | Diff
patch to be submitted (11.61 KB, patch)
2009-09-09 17:07 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
patch to be submitted (16.48 KB, patch)
2009-09-09 17:10 PDT, Hin-Chung Lam
eric.carlson: review+
eric: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Chung Lam 2009-09-08 10:53:11 PDT
Now MediaPlayer and MediaPlayerPrivateInterface only reports maxTimeBuffered(), so HTMLMediaElement can report at most one time range in the buffered attribute. The interfaces should support multiple time ranges.
Comment 1 Hin-Chung Lam 2009-09-08 11:02:14 PDT
Created attachment 39197 [details]
API change
Comment 2 Eric Carlson 2009-09-08 13:07:32 PDT
Comment on attachment 39197 [details]
API change

> +        * html/HTMLMediaElement.cpp:
> +        (WebCore::HTMLMediaElement::percentLoaded):
> +        (WebCore::HTMLMediaElement::buffered):
> +        * platform/graphics/MediaPlayer.cpp:
> +        (WebCore::NullMediaPlayerPrivate::buffered):
> +        (WebCore::MediaPlayer::buffered):
> +        * platform/graphics/MediaPlayer.h:
> +        * platform/graphics/MediaPlayerPrivate.h:
> +        * platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:
> +        * platform/graphics/gtk/MediaPlayerPrivateGStreamer.h:
> +        * platform/graphics/mac/MediaPlayerPrivateQTKit.h:
> +        * platform/graphics/mac/MediaPlayerPrivateQTKit.mm:
> +        (WebCore::MediaPlayerPrivate::buffered):
> +        * platform/graphics/qt/MediaPlayerPrivatePhonon.cpp:
> +        * platform/graphics/qt/MediaPlayerPrivatePhonon.h:
> +        * platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp:
> +        * platform/graphics/win/MediaPlayerPrivateQuickTimeWin.h:
> +        * platform/graphics/wince/MediaPlayerPrivateWince.h:
> +        * rendering/RenderThemeChromiumMac.mm:
> +        (WebCore::RenderThemeChromiumMac::paintMediaSliderTrack):
> +        * rendering/RenderThemeChromiumSkia.cpp:
> +        * rendering/RenderThemeMac.mm:
> +        (WebCore::RenderThemeMac::paintMediaSliderTrack):

  It would be nice to have a brief comment about what changed in each function.


> +    int length = m_player->buffered()->length();
> +    if (length) {
> +        // FIXME: The end of last range doesn't represent the actual percentage loaded.
> +        ExceptionCode ignoredException;
> +        float end = m_player->buffered()->end(length - 1, ignoredException);
> +        return duration ? end / duration : 0;
> +    }
> +    return 0;


  Please put m_player->buffered() into a local, it can be expensive to calculate the buffered ranges. 

  This code will return the wrong answer as soon as an engine returns a TimeRanges object with more than one entry. You might as well change it now to iterate through all of the ranges to get the total amount buffered.


> diff --git a/WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp b/WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp

> +    RefPtr<TimeRanges> timeRanges = TimeRanges::create();
> +    if (!m_errorOccured && !m_isStreaming && maxTimeLoaded() > 0)
> +        timeRanges->add(0, maxTimeLoaded());
> +    return timeRanges;

  You should store maxTimeLoaded() in a local, it can be expensive to calculate.


> diff --git a/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm b/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm

> +    RefPtr<TimeRanges> timeRanges = TimeRanges::create();
> +    if (maxTimeLoaded() > 0)
> +        timeRanges->add(0, maxTimeLoaded());
> +    return timeRanges;

  Same thing here.


> diff --git a/WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp b/WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp

>  #include "Timer.h"
> +#include "TimeRanges.h"

  Minor style violation here, "TimeRanges.h" sorts before "Timer.h".


> -float MediaPlayerPrivate::maxTimeBuffered() const
> +PassRefPtr<TimeRanges> MediaPlayerPrivate::buffered() const;
>  {
> +    RefPtr<TimeRanges> timeRanges = TimeRanges::create();
>      // rtsp streams are not buffered
> -    return m_isStreaming ? 0 : maxTimeLoaded();
> +    if (!m_isStreaming && maxTimeLoaded() > 0)
> +        timeRanges->add(0, maxTimeLoaded());

  You should store maxTimeLoaded() in a local, it can be expensive to calculate.


> @@ -1935,6 +1935,9 @@ bool RenderThemeChromiumMac::paintMediaSliderTrack(RenderObject* o, const Render
>      float currentTime = 0;
>      float duration = 0;
>      if (MediaPlayer* player = mediaElement->player()) {
> +        RefPtr<TimeRanges> timeRanges = player->buffered();
> +        ExceptionCode ignoredException;
> +        timeLoaded = timeRanges->length() ? timeRanges->end(0, ignoredException) : 0;
>          duration = player->duration();
>          timeLoaded = player->maxTimeBuffered();
>          currentTime = player->currentTime();

  I realize that this isn't new to this patch, but HTMLMediaElement has buffered, duration, and currentTime methods so I would prefer if we used it instead of MediaPlayer.


> @@ -1604,8 +1605,10 @@ bool RenderThemeMac::paintMediaSliderTrack(RenderObject* o, const RenderObject::
>      float currentTime = 0;
>      float duration = 0;
>      if (MediaPlayer* player = mediaElement->player()) {
> +        RefPtr<TimeRanges> timeRanges = player->buffered();
> +        ExceptionCode ignoredException;
> +        timeLoaded = timeRanges->length() ? timeRanges->end(0, ignoredException) : 0;
>          duration = player->duration();
> -        timeLoaded = player->maxTimeBuffered();
>          currentTime = player->currentTime();
>      }

  Same thing here.

  r+ assuming you address the comments.
Comment 3 Hin-Chung Lam 2009-09-08 14:42:47 PDT
OK, will address your concerns before committing it.

(In reply to comment #2)
> (From update of attachment 39197 [details])
> > +        * html/HTMLMediaElement.cpp:
> > +        (WebCore::HTMLMediaElement::percentLoaded):
> > +        (WebCore::HTMLMediaElement::buffered):
> > +        * platform/graphics/MediaPlayer.cpp:
> > +        (WebCore::NullMediaPlayerPrivate::buffered):
> > +        (WebCore::MediaPlayer::buffered):
> > +        * platform/graphics/MediaPlayer.h:
> > +        * platform/graphics/MediaPlayerPrivate.h:
> > +        * platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:
> > +        * platform/graphics/gtk/MediaPlayerPrivateGStreamer.h:
> > +        * platform/graphics/mac/MediaPlayerPrivateQTKit.h:
> > +        * platform/graphics/mac/MediaPlayerPrivateQTKit.mm:
> > +        (WebCore::MediaPlayerPrivate::buffered):
> > +        * platform/graphics/qt/MediaPlayerPrivatePhonon.cpp:
> > +        * platform/graphics/qt/MediaPlayerPrivatePhonon.h:
> > +        * platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp:
> > +        * platform/graphics/win/MediaPlayerPrivateQuickTimeWin.h:
> > +        * platform/graphics/wince/MediaPlayerPrivateWince.h:
> > +        * rendering/RenderThemeChromiumMac.mm:
> > +        (WebCore::RenderThemeChromiumMac::paintMediaSliderTrack):
> > +        * rendering/RenderThemeChromiumSkia.cpp:
> > +        * rendering/RenderThemeMac.mm:
> > +        (WebCore::RenderThemeMac::paintMediaSliderTrack):
> 
>   It would be nice to have a brief comment about what changed in each function.
> 
> 
> > +    int length = m_player->buffered()->length();
> > +    if (length) {
> > +        // FIXME: The end of last range doesn't represent the actual percentage loaded.
> > +        ExceptionCode ignoredException;
> > +        float end = m_player->buffered()->end(length - 1, ignoredException);
> > +        return duration ? end / duration : 0;
> > +    }
> > +    return 0;
> 
> 
>   Please put m_player->buffered() into a local, it can be expensive to
> calculate the buffered ranges. 
> 
>   This code will return the wrong answer as soon as an engine returns a
> TimeRanges object with more than one entry. You might as well change it now to
> iterate through all of the ranges to get the total amount buffered.
> 
> 
> > diff --git a/WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp b/WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp
> 
> > +    RefPtr<TimeRanges> timeRanges = TimeRanges::create();
> > +    if (!m_errorOccured && !m_isStreaming && maxTimeLoaded() > 0)
> > +        timeRanges->add(0, maxTimeLoaded());
> > +    return timeRanges;
> 
>   You should store maxTimeLoaded() in a local, it can be expensive to
> calculate.
> 
> 
> > diff --git a/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm b/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm
> 
> > +    RefPtr<TimeRanges> timeRanges = TimeRanges::create();
> > +    if (maxTimeLoaded() > 0)
> > +        timeRanges->add(0, maxTimeLoaded());
> > +    return timeRanges;
> 
>   Same thing here.
> 
> 
> > diff --git a/WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp b/WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp
> 
> >  #include "Timer.h"
> > +#include "TimeRanges.h"
> 
>   Minor style violation here, "TimeRanges.h" sorts before "Timer.h".
> 
> 
> > -float MediaPlayerPrivate::maxTimeBuffered() const
> > +PassRefPtr<TimeRanges> MediaPlayerPrivate::buffered() const;
> >  {
> > +    RefPtr<TimeRanges> timeRanges = TimeRanges::create();
> >      // rtsp streams are not buffered
> > -    return m_isStreaming ? 0 : maxTimeLoaded();
> > +    if (!m_isStreaming && maxTimeLoaded() > 0)
> > +        timeRanges->add(0, maxTimeLoaded());
> 
>   You should store maxTimeLoaded() in a local, it can be expensive to
> calculate.
> 
> 
> > @@ -1935,6 +1935,9 @@ bool RenderThemeChromiumMac::paintMediaSliderTrack(RenderObject* o, const Render
> >      float currentTime = 0;
> >      float duration = 0;
> >      if (MediaPlayer* player = mediaElement->player()) {
> > +        RefPtr<TimeRanges> timeRanges = player->buffered();
> > +        ExceptionCode ignoredException;
> > +        timeLoaded = timeRanges->length() ? timeRanges->end(0, ignoredException) : 0;
> >          duration = player->duration();
> >          timeLoaded = player->maxTimeBuffered();
> >          currentTime = player->currentTime();
> 
>   I realize that this isn't new to this patch, but HTMLMediaElement has
> buffered, duration, and currentTime methods so I would prefer if we used it
> instead of MediaPlayer.
> 
> 
> > @@ -1604,8 +1605,10 @@ bool RenderThemeMac::paintMediaSliderTrack(RenderObject* o, const RenderObject::
> >      float currentTime = 0;
> >      float duration = 0;
> >      if (MediaPlayer* player = mediaElement->player()) {
> > +        RefPtr<TimeRanges> timeRanges = player->buffered();
> > +        ExceptionCode ignoredException;
> > +        timeLoaded = timeRanges->length() ? timeRanges->end(0, ignoredException) : 0;
> >          duration = player->duration();
> > -        timeLoaded = player->maxTimeBuffered();
> >          currentTime = player->currentTime();
> >      }
> 
>   Same thing here.
> 
>   r+ assuming you address the comments.
Comment 4 Hin-Chung Lam 2009-09-08 17:25:31 PDT
Created attachment 39233 [details]
patch to be submitted
Comment 5 Eric Seidel (no email) 2009-09-09 08:32:52 PDT
Does Hin-Chung have commit-bit, or should this be marked r=?/cq=?
Comment 6 Hin-Chung Lam 2009-09-09 11:08:44 PDT
(In reply to comment #5)
> Does Hin-Chung have commit-bit, or should this be marked r=?/cq=?

I don't have commit bit, I'm having someone else to commit this for me.
Comment 7 Hin-Chung Lam 2009-09-09 11:34:45 PDT
Updated this patch to address Eric's comments. Thanks for the review!
Comment 8 Eric Carlson 2009-09-09 11:48:47 PDT
Comment on attachment 39233 [details]
patch to be submitted


@@ -1287,7 +1287,15 @@ float HTMLMediaElement::percentLoaded() const
     if (!m_player)
         return 0;
     float duration = m_player->duration();
-    return duration ? m_player->maxTimeBuffered() / duration : 0;
+    float buffered = 0;
+    RefPtr<TimeRanges> timeRanges = m_player->buffered();
+    for (unsigned i = 0; i < timeRanges->length(); ++i) {
+        ExceptionCode ignoredException;
+        float start = timeRanges->start(i, ignoredException);
+        float end = timeRanges->end(i, ignoredException);
+        buffered += end - start;
+    }
+    return duration ? buffered / duration : 0;

  Why accumulate the buffered ranges if duration is zero or infinite? Should probably just have an early return instead.

  r=me with this change, no need to re-submit for review.
Comment 9 Hin-Chung Lam 2009-09-09 16:40:40 PDT
Created attachment 39310 [details]
patch to be submitted

Since I'm not a committer, I'm submitting this for review again.
Comment 10 Hin-Chung Lam 2009-09-09 16:42:34 PDT
Submitting this for review again since I'm not a committer.

I did an early return on the condition that duration is 0 or infinity in HTMLMediaElement::percentLoaded().
Comment 11 Eric Carlson 2009-09-09 16:49:21 PDT
Comment on attachment 39310 [details]
patch to be submitted

> +
> +    if (duration == 0 || isinf(duration))
> +        return 0;

"duration == 0" is a style violation, tests for zero/non-zero should be not be done with an equality comparison.

check-webkit-style would have flagged this error.
Comment 12 Hin-Chung Lam 2009-09-09 17:07:34 PDT
Created attachment 39313 [details]
patch to be submitted

Fixed the style violation according Eric's comments. Thanks for looking.
Comment 13 Hin-Chung Lam 2009-09-09 17:10:06 PDT
Created attachment 39314 [details]
patch to be submitted
Comment 14 Eric Carlson 2009-09-09 17:14:58 PDT
Comment on attachment 39314 [details]
patch to be submitted

r=me
Comment 15 WebKit Commit Bot 2009-09-09 17:31:45 PDT
Comment on attachment 39314 [details]
patch to be submitted

Rejecting patch 39314 from commit-queue.

eric.carlson@apple.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
Comment 16 Eric Seidel (no email) 2009-09-09 17:34:46 PDT
Comment on attachment 39314 [details]
patch to be submitted

I have no idea why the commit queue does not think you have commit-bit.  You're clearly in committers.py.  I'll investigate.
Comment 17 Eric Seidel (no email) 2009-09-09 17:38:17 PDT
(In reply to comment #15)
> (From update of attachment 39314 [details])
> Rejecting patch 39314 from commit-queue.
> 
> eric.carlson@apple.com does not have committer permissions according to
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

Oops. bug 29113.

Also I'll move you from the Committer list to the Reviewer list now that you're a reviewer.
Comment 18 WebKit Commit Bot 2009-09-09 17:47:12 PDT
Comment on attachment 39314 [details]
patch to be submitted

Rejecting patch 39314 from commit-queue.

This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment 19 Hin-Chung Lam 2009-09-09 17:49:34 PDT
I'll look into it and see why it failed layout tests
Comment 20 Eric Seidel (no email) 2009-09-09 18:31:20 PDT
Comment on attachment 39314 [details]
patch to be submitted

storage/database-lock-after-reload.html -> failed
I think we have a flakey storage test.  Your change is not related to that at all.
Trying again...
Comment 21 Hin-Chung Lam 2009-09-09 18:42:53 PDT
Can I hold this commit until tomorrow? Since this change would break chromium build, I can get a better coordinations tomorrow.
Comment 22 Eric Seidel (no email) 2009-09-09 18:43:40 PDT
Comment on attachment 39314 [details]
patch to be submitted

You can always cq- your own patches. :)  I'll abort the bot if it's currently processing this one.
Comment 23 Hin-Chung Lam 2009-09-09 18:44:28 PDT
Thanks!
Comment 24 Hin-Chung Lam 2009-09-10 19:43:15 PDT
API now support reporting multiple time ranges, it's up to the media engine to report it. For how QuickTime/GStreamer works their behavior is correct, so mark this as fixed.