[Webkit-unassigned] [Bug 29041] HTMLMediaElement buffered attribute should report a list of time range
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Sep 8 14:42:47 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=29041
--- Comment #3 from Hin-Chung Lam <hclam at google.com> 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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list