Bug 29041

Created attachment 39197 [details] API change

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.

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.

Created attachment 39233 [details] patch to be submitted

Does Hin-Chung have commit-bit, or should this be marked r=?/cq=?

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

Updated this patch to address Eric's comments. Thanks for the review!

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.

Created attachment 39310 [details] patch to be submitted Since I'm not a committer, I'm submitting this for review again.

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 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.

Created attachment 39313 [details] patch to be submitted Fixed the style violation according Eric's comments. Thanks for looking.

Created attachment 39314 [details] patch to be submitted

Comment on attachment 39314 [details] patch to be submitted r=me

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 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.

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

I'll look into it and see why it failed layout tests

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...

Can I hold this commit until tomorrow? Since this change would break chromium build, I can get a better coordinations tomorrow.

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.

Thanks!

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.