Bug 139248 - [WTF] MediaTime should support round-tripping from and to doubles.
Summary: [WTF] MediaTime should support round-tripping from and to doubles.
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-12-03 18:01 PST by Jer Noble
Modified: 2014-12-05 12:50 PST (History)
9 users (show)

See Also:

Attachments
Patch (14.36 KB, patch)
2014-12-03 18:12 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (479.01 KB, application/zip)
2014-12-03 19:51 PST, Build Bot 		no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (470.84 KB, application/zip)
2014-12-03 20:32 PST, Build Bot 		no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (471.73 KB, application/zip)
2014-12-03 22:34 PST, Build Bot 		no flags Details
Patch (13.94 KB, patch)
2014-12-04 12:09 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch (18.50 KB, patch)
2014-12-04 14:58 PST, Jer Noble 		eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (14.33 KB, patch)
2014-12-05 09:55 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch for landing (18.57 KB, patch)
2014-12-05 10:04 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Patch for landing (18.82 KB, patch)
2014-12-05 11:13 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Show Obsolete (7) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2014-12-03 18:01:01 PST 
[WTF] MediaTime should support round-tripping from and to doubles.
Comment 1 Jer Noble 2014-12-03 18:12:34 PST 
Created attachment 242546 [details]
Patch
Comment 2 Build Bot 2014-12-03 19:51:36 PST 
Comment on attachment 242546 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 3 Build Bot 2014-12-03 19:51:39 PST 
Created attachment 242551 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 4 Build Bot 2014-12-03 20:32:53 PST 
Comment on attachment 242546 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 5 Build Bot 2014-12-03 20:32:56 PST 
Created attachment 242552 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 6 Build Bot 2014-12-03 22:34:07 PST 
Comment on attachment 242546 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 7 Build Bot 2014-12-03 22:34:09 PST 
Created attachment 242553 [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 8 Jer Noble 2014-12-04 12:09:09 PST 
Created attachment 242579 [details]
Patch
Comment 9 Jer Noble 2014-12-04 14:58:59 PST 
Created attachment 242590 [details]
Patch
Comment 10 Eric Carlson 2014-12-05 05:46:10 PST 
Comment on attachment 242590 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/MediaTimeAVFoundation.cpp:73
> +    if (mediaTime.hasDoubleValue())
> +        time = CMTimeMakeWithSeconds(mediaTime.toDouble(), MediaTime::DefaultTimeScale);

This overwrites the flags you just set, can it be done as an early return?
Comment 11 Jer Noble 2014-12-05 08:47:22 PST 
(In reply to comment #10)
> Comment on attachment 242590 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242590&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/MediaTimeAVFoundation.cpp:73
> > +    if (mediaTime.hasDoubleValue())
> > +        time = CMTimeMakeWithSeconds(mediaTime.toDouble(), MediaTime::DefaultTimeScale);
> 
> This overwrites the flags you just set, can it be done as an early return?

Sure, I'll just move this if() statement to the first line. (So that further NaN checks, infinity checks, etc, can still set the correct flags.)
Comment 12 Jer Noble 2014-12-05 09:55:30 PST 
Created attachment 242637 [details]
Patch for landing
Comment 13 Jer Noble 2014-12-05 10:04:10 PST 
Created attachment 242639 [details]
Patch for landing
Comment 14 Jer Noble 2014-12-05 11:13:50 PST 
Created attachment 242645 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2014-12-05 11:29:30 PST 
Attachment 242645 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/MediaTimeAVFoundation.cpp:33:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Jer Noble 2014-12-05 12:50:33 PST 
Committed r176863: <http://trac.webkit.org/changeset/176863>