Bug 133454 - Update PlatformTimeRanges to use MediaTime rather than doubles for time values.
Summary: Update PlatformTimeRanges to use MediaTime rather than doubles for time values.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-02 11:16 PDT by Jer Noble
Modified: 2014-06-03 14:09 PDT (History)
17 users (show)

See Also:


Attachments
Patch (23.36 KB, patch)
2014-06-02 13:08 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (25.20 KB, patch)
2014-06-02 13:54 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (26.68 KB, patch)
2014-06-02 14:35 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (26.93 KB, patch)
2014-06-02 14:58 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (501.58 KB, application/zip)
2014-06-02 16:29 PDT, Build Bot
no flags Details
Patch (26.84 KB, patch)
2014-06-02 16:43 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (29.05 KB, patch)
2014-06-02 16:46 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (28.98 KB, patch)
2014-06-02 16:57 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (28.96 KB, patch)
2014-06-02 21:02 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (28.97 KB, patch)
2014-06-03 10:16 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2014-06-02 11:16:22 PDT
Update PlatformTimeRanges to use MediaTime rather than doubles for time values.
Comment 1 Jer Noble 2014-06-02 13:08:06 PDT
Created attachment 232395 [details]
Patch
Comment 2 Jer Noble 2014-06-02 13:54:52 PDT
Created attachment 232397 [details]
Patch
Comment 3 Eric Carlson 2014-06-02 14:31:14 PDT
Comment on attachment 232397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232397&action=review

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1404
> +    MediaTime currentTime = MediaTime::createWithDouble(m_source->currentTime());
>      const PlatformTimeRanges& ranges = m_buffered->ranges();
>      if (!ranges.length())
>          return false;

Nit: no need to create "currentTime" if ranges is empty.

> Source/WebCore/platform/graphics/PlatformTimeRanges.h:74
> +        Range(MediaTime start, MediaTime end)

Nit: any reason to not make these "const MediaTime&"?

> Source/WebCore/platform/graphics/PlatformTimeRanges.h:82
> +        inline bool isPointInRange(MediaTime point) const

Ditto.
Comment 4 Jer Noble 2014-06-02 14:35:00 PDT
Created attachment 232399 [details]
Patch
Comment 5 Jer Noble 2014-06-02 14:40:23 PDT
(In reply to comment #3)
> (From update of attachment 232397 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232397&action=review
> 
> > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1404
> > +    MediaTime currentTime = MediaTime::createWithDouble(m_source->currentTime());
> >      const PlatformTimeRanges& ranges = m_buffered->ranges();
> >      if (!ranges.length())
> >          return false;
> 
> Nit: no need to create "currentTime" if ranges is empty.

I'll reverse the order.

> > Source/WebCore/platform/graphics/PlatformTimeRanges.h:74
> > +        Range(MediaTime start, MediaTime end)
> 
> Nit: any reason to not make these "const MediaTime&"?

Nope, no reason.

> > Source/WebCore/platform/graphics/PlatformTimeRanges.h:82
> > +        inline bool isPointInRange(MediaTime point) const
> 
> Ditto.

Ditto.
Comment 6 Jer Noble 2014-06-02 14:58:05 PDT
Created attachment 232400 [details]
Patch
Comment 7 Build Bot 2014-06-02 16:29:54 PDT
Comment on attachment 232400 [details]
Patch

Attachment 232400 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5635521531346944

New failing tests:
media/video-pause-immediately.html
Comment 8 Build Bot 2014-06-02 16:29:59 PDT
Created attachment 232404 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 9 Jer Noble 2014-06-02 16:43:04 PDT
Created attachment 232407 [details]
Patch
Comment 10 Jer Noble 2014-06-02 16:46:24 PDT
Created attachment 232408 [details]
Patch

Updated test to allow for very small deltas between the end buffered time and the end played time.
Comment 11 Jer Noble 2014-06-02 16:57:02 PDT
Created attachment 232409 [details]
Patch
Comment 12 Jer Noble 2014-06-02 21:02:46 PDT
Created attachment 232414 [details]
Patch
Comment 13 Jer Noble 2014-06-03 10:16:06 PDT
Created attachment 232432 [details]
Patch
Comment 14 Jer Noble 2014-06-03 14:09:49 PDT
Committed r169568: <http://trac.webkit.org/changeset/169568>