WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
29041
HTMLMediaElement buffered attribute should report a list of time range
https://bugs.webkit.org/show_bug.cgi?id=29041
Summary
HTMLMediaElement buffered attribute should report a list of time range
Hin-Chung Lam
Reported
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.
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Hin-Chung Lam
Comment 1
2009-09-08 11:02:14 PDT
Created
attachment 39197
[details]
API change
Eric Carlson
Comment 2
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.
Hin-Chung Lam
Comment 3
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.
Hin-Chung Lam
Comment 4
2009-09-08 17:25:31 PDT
Created
attachment 39233
[details]
patch to be submitted
Eric Seidel (no email)
Comment 5
2009-09-09 08:32:52 PDT
Does Hin-Chung have commit-bit, or should this be marked r=?/cq=?
Hin-Chung Lam
Comment 6
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.
Hin-Chung Lam
Comment 7
2009-09-09 11:34:45 PDT
Updated this patch to address Eric's comments. Thanks for the review!
Eric Carlson
Comment 8
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.
Hin-Chung Lam
Comment 9
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.
Hin-Chung Lam
Comment 10
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().
Eric Carlson
Comment 11
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.
Hin-Chung Lam
Comment 12
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.
Hin-Chung Lam
Comment 13
2009-09-09 17:10:06 PDT
Created
attachment 39314
[details]
patch to be submitted
Eric Carlson
Comment 14
2009-09-09 17:14:58 PDT
Comment on
attachment 39314
[details]
patch to be submitted r=me
WebKit Commit Bot
Comment 15
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
.
Eric Seidel (no email)
Comment 16
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.
Eric Seidel (no email)
Comment 17
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.
WebKit Commit Bot
Comment 18
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
Hin-Chung Lam
Comment 19
2009-09-09 17:49:34 PDT
I'll look into it and see why it failed layout tests
Eric Seidel (no email)
Comment 20
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...
Hin-Chung Lam
Comment 21
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.
Eric Seidel (no email)
Comment 22
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.
Hin-Chung Lam
Comment 23
2009-09-09 18:44:28 PDT
Thanks!
Hin-Chung Lam
Comment 24
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug