WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
145676
Added getStartDate() support to HTMLMediaElements
https://bugs.webkit.org/show_bug.cgi?id=145676
Summary
Added getStartDate() support to HTMLMediaElements
Matthew Daiter
Reported
2015-06-04 17:21:35 PDT
Allows users to grab original streaming dates from HTMLMediaElements. Useful for grabbing and displaying original airdates.
Attachments
Patch
(9.40 KB, patch)
2015-06-04 17:43 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2
(571.95 KB, application/zip)
2015-06-04 18:23 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews101 for mac-mavericks
(528.06 KB, application/zip)
2015-06-04 18:35 PDT
,
Build Bot
no flags
Details
Patch
(10.59 KB, patch)
2015-06-05 14:19 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Patch
(2.62 KB, patch)
2015-06-05 17:35 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Patch
(13.23 KB, patch)
2015-06-05 17:46 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Patch
(10.88 KB, patch)
2015-06-08 15:09 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews102 for mac-mavericks
(663.40 KB, application/zip)
2015-06-08 15:50 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2
(569.67 KB, application/zip)
2015-06-08 15:59 PDT
,
Build Bot
no flags
Details
Patch
(14.22 KB, patch)
2015-06-08 16:57 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Patch
(14.29 KB, patch)
2015-06-08 17:00 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews103 for mac-mavericks
(552.54 KB, application/zip)
2015-06-08 17:48 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2
(567.92 KB, application/zip)
2015-06-08 17:50 PDT
,
Build Bot
no flags
Details
Patch
(14.71 KB, patch)
2015-06-09 11:05 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews102 for mac-mavericks
(535.61 KB, application/zip)
2015-06-09 11:52 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2
(590.13 KB, application/zip)
2015-06-09 12:23 PDT
,
Build Bot
no flags
Details
New patch for this bug. Should work. Supposedly there were misalignments with the test cases?
(15.37 KB, patch)
2015-06-19 13:28 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Didn't format last patch correctly.
(15.37 KB, patch)
2015-06-19 15:15 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Patch
(15.05 KB, patch)
2015-06-21 01:12 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews103 for mac-mavericks
(557.93 KB, application/zip)
2015-06-21 04:03 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2
(599.11 KB, application/zip)
2015-06-21 04:10 PDT
,
Build Bot
no flags
Details
Patch
(15.52 KB, patch)
2015-06-24 16:42 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2
(692.88 KB, application/zip)
2015-06-24 17:32 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews101 for mac-mavericks
(610.10 KB, application/zip)
2015-06-24 17:53 PDT
,
Build Bot
no flags
Details
Patch
(15.13 KB, patch)
2015-06-26 15:06 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews102 for mac-mavericks
(552.80 KB, application/zip)
2015-06-26 15:46 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2
(693.92 KB, application/zip)
2015-06-26 15:50 PDT
,
Build Bot
no flags
Details
Patch
(14.95 KB, patch)
2015-06-26 15:59 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Patch
(15.62 KB, patch)
2015-06-26 17:08 PDT
,
Matthew Daiter
no flags
Details
Formatted Diff
Diff
Show Obsolete
(24)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2015-06-04 17:24:41 PDT
<
rdar://problem/21252512
>
Matthew Daiter
Comment 2
2015-06-04 17:43:11 PDT
Created
attachment 254320
[details]
Patch
Build Bot
Comment 3
2015-06-04 18:23:52 PDT
Comment on
attachment 254320
[details]
Patch
Attachment 254320
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/5135783639908352
New failing tests: media/video-controller-getStartDate.html
Build Bot
Comment 4
2015-06-04 18:23:56 PDT
Created
attachment 254326
[details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Build Bot
Comment 5
2015-06-04 18:34:58 PDT
Comment on
attachment 254320
[details]
Patch
Attachment 254320
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/5794267221983232
New failing tests: media/video-controller-getStartDate.html
Build Bot
Comment 6
2015-06-04 18:35:01 PDT
Created
attachment 254328
[details]
Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Brent Fulgham
Comment 7
2015-06-04 23:19:03 PDT
Comment on
attachment 254320
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=254320&action=review
This is a great start! I'm not sure we're handling the undefined case right yet, but this looks close!
> Source/WebCore/html/HTMLMediaElement.cpp:876 > + return nanMedia.toDouble();
Does this get handled as NaN? We might need to look at the generated code to see what it's doing.
> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:97 > + virtual MediaTime getStartDate() { return MediaTime::createWithDouble( currentTime() ); }
We don't use spacing around embedded functions like 'currentTime' here.
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:210 > + virtual MediaTime getStartDate();
This should be 'MediaTime getStartDate() override;'
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:726 > + return MediaTime::createWithDouble( date );
No spaces around 'date' here.
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:728 > + double curr_time = CMTimeGetSeconds([m_avPlayerItem currentTime]) * 1000;
Our naming convention is camelCase, and very few truncated words. So, currentTime is better.
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:729 > + printf("Date: %f\nTime: %f\n", date, curr_time);
This should be a LOG call, rather than a bare printf.
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:730 > + return MediaTime::createWithDouble( date - curr_time );
No spaces here.
> LayoutTests/media/video-controller-getStartDate.html:17 > + testExpected("video.getStartDate()", null);
Do we expect null here? Or NaN? I thought the specification said NaN.
> LayoutTests/media/video-controller-getStartDate.html:26 > + <p>Test that getStartDate() returns appropriate NaN or date.</p>
NaN and null are not the same thing! :)
Matthew Daiter
Comment 8
2015-06-05 14:19:15 PDT
Created
attachment 254383
[details]
Patch
Darin Adler
Comment 9
2015-06-05 14:55:49 PDT
Comment on
attachment 254383
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=254383&action=review
> Source/WebCore/ChangeLog:9 > + Test: http/tests/media/hls/video-controller-getStartDate.html
I don’t see the test in this patch.
Matthew Daiter
Comment 10
2015-06-05 17:35:35 PDT
Created
attachment 254400
[details]
Patch
Matthew Daiter
Comment 11
2015-06-05 17:36:37 PDT
(In reply to
comment #9
)
> Comment on
attachment 254383
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=254383&action=review
> > > Source/WebCore/ChangeLog:9 > > + Test: http/tests/media/hls/video-controller-getStartDate.html > > I don’t see the test in this patch.
Just patched it
WebKit Commit Bot
Comment 12
2015-06-05 17:37:39 PDT
Attachment 254400
[details]
did not pass style-queue: ERROR: LayoutTests/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Matthew Daiter
Comment 13
2015-06-05 17:46:48 PDT
Created
attachment 254401
[details]
Patch
Brent Fulgham
Comment 14
2015-06-05 22:40:15 PDT
Comment on
attachment 254401
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=254401&action=review
This looks great! I think many of these could be 'const'. I also would like to see that bindings test we talked about. But I think this is pretty close to done!
> Source/WebCore/ChangeLog:5 > + could inform users about the original date, etc.
This should just be a one-line "title". This paragraph should be placed between the "Test" line and the "*bindings/js/JSDOMBindings.cpp" line.
> Source/WebCore/html/HTMLMediaElement.cpp:878 > +double HTMLMediaElement::getStartDate()
This should be 'const'
> Source/WebCore/html/HTMLMediaElement.h:181 > + virtual double getStartDate();
This should be 'const'
> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:97 > + virtual MediaTime getStartDate() { return MediaTime::createWithDouble(currentTime()); }
This should be 'const'
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:210 > + MediaTime getStartDate() override;
This should be 'const'
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:730 > +
Whoops! Extra space.
Eric Carlson
Comment 15
2015-06-06 16:16:27 PDT
Comment on
attachment 254401
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=254401&action=review
>> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:97 >> + virtual MediaTime getStartDate() { return MediaTime::createWithDouble(currentTime()); } > > This should be 'const'
Shouldn't this return NaN by default?
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:726 > + double date = [[m_avPlayerItem currentDate] timeIntervalSince1970] * 1000; > + if (!date) > + return MediaTime::createWithDouble(date);
Shouldn't this return NaN?
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:729 > + double curr_time = CMTimeGetSeconds([m_avPlayerItem currentTime]) * 1000; > + return MediaTime::createWithDouble(date-curr_time);
Is this right? The startDate is the "current timeline offset", which is defined as: Some video files also have an explicit date and time corresponding to the zero time in the media timeline, known as the timeline offset. Initially, the timeline offset must be set to Not-a-Number (NaN). so why doesn't this just return MediaTime::createWithDouble(date)?
Darin Adler
Comment 16
2015-06-06 22:43:14 PDT
Comment on
attachment 254401
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=254401&action=review
I don’t think there’s enough test coverage here. I’m going to say review- because Eric thinks the "date-curr_time" thing is wrong. If that turns out to be mistaken then we could get it reviewed again.
> Source/WebCore/bindings/js/JSDOMBinding.cpp:121 > +JSC::JSValue jsDateOrNaN(ExecState* exec, double value) > +{ > + if (std::isnan(value)) > + return jsDoubleNumber(value); > + return jsDateOrNull(exec, value); > +}
This seems a little strange to add. Will this really be such a common pattern that we need to support it in the bindings generator? Does WebIDL specifically allow for this now? JSC::JSValue is wrong in this file, I think. Should just be JSValue.
> Source/WebCore/bindings/js/JSDOMBinding.h:321 > +JSC::JSValue jsDateOrNaN(JSC::ExecState*, double);
The two functions below each have a comment. Seems we should add that comment for this too. And if this is going to be the less common idiom, then it should come *after* jsDateOrNull. Since this is a new function, we should probably make it take a JSC::ExecState& instead of a pointer. Easy to accommodate that in the bindings generator by including a "*" and more forward looking. In newer WebCore code we always use references for things like this that can’t be null. The name of this function isn’t quite right, because this function can return a Date object, a NaN, or a null! Although I assume that getStartDate won’t ever be returning a null.
> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3899 > + my $conv = $signature->extendedAttributes->{"TreatReturnedNaNDateAs"};
It’s really ugly to have to add an attribute like this. We try to avoid these as much as possible. I don’t see an obvious alternative but it’s a annoying to be adding this.
> Source/WebCore/html/HTMLMediaElement.cpp:882 > + double timeOption = round(m_player->getStartDate().toDouble()); > + if (!timeOption) > + return std::numeric_limits<double>::quiet_NaN();
I think there should be some kind of comment here explaining why we are translating a zero into a NaN. Is zero not a legal value?
> Source/WebCore/html/HTMLMediaElement.idl:68 > + [TreatReturnedNaNDateAs=NaN] Date getStartDate();
I think this is a really strange type for an attribute and doesn’t seem like something that would be in a new modern standard. Could you please double check that “either a Date object or a number with NaN value” is what we want here?
>>> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:97 >>> + virtual MediaTime getStartDate() { return MediaTime::createWithDouble(currentTime()); } >> >> This should be 'const' > > Shouldn't this return NaN by default?
Is it OK to leave all the other back ends returning sort of “nonsense” values? Returning a wrong start date might be worse than not having the function at all. Maybe we have to make the binding conditional so the function will only appear when it’s going to work. But also, don’t we use multiple back ends in the Mac and/or iOS ports?
>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:726 >> + return MediaTime::createWithDouble(date); > > Shouldn't this return NaN?
I’d like to understand more about when exactly this is zero. We actually get back an object from currentDate that returns 0 when we call timIntervalSince1970 on it?
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:728 > + double curr_time = CMTimeGetSeconds([m_avPlayerItem currentTime]) * 1000;
We never name local variables in this style. It should be currentTime, not cure_time.
>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:729 >> + return MediaTime::createWithDouble(date-curr_time); > > Is this right? The startDate is the "current timeline offset", which is defined as: > > Some video files also have an explicit date and time corresponding to the zero time > in the media timeline, known as the timeline offset. Initially, the timeline offset > must be set to Not-a-Number (NaN). > > so why doesn't this just return MediaTime::createWithDouble(date)?
The formatting here is also incorrect. We put spaces around operators like "-".
Brent Fulgham
Comment 17
2015-06-07 17:27:51 PDT
For reference: The specification defines "getStarteDate()" in <
http://www.w3.org/TR/2014/CR-html5-20140204/embedded-content-0.html
>. "Some video files also have an explicit date and time corresponding to the zero time in the media timeline, known as the timeline offset. Initially, the timeline offset must be set to Not-a-Number (NaN). The getStartDate() method must return a new Date object representing the current timeline offset." So this expects NaN, at least in the pre-playing video state.
Brent Fulgham
Comment 18
2015-06-07 17:29:57 PDT
Comment on
attachment 254401
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=254401&action=review
>> Source/WebCore/bindings/js/JSDOMBinding.cpp:121 >> +} > > This seems a little strange to add. Will this really be such a common pattern that we need to support it in the bindings generator? Does WebIDL specifically allow for this now? > > JSC::JSValue is wrong in this file, I think. Should just be JSValue.
That's my fault, Darin. I told Matt to create this binding because the specification specifically requires NaN return values in certain cases, and our default date bindings always convert those to JavaScript Nulls.
>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3899 >> + my $conv = $signature->extendedAttributes->{"TreatReturnedNaNDateAs"}; > > It’s really ugly to have to add an attribute like this. We try to avoid these as much as possible. I don’t see an obvious alternative but it’s a annoying to be adding this.
We should probably talk to Anders or Sam about a better approach as a future (larger scale) enhancement. This seemed to be in keeping with existing coding style.
>> Source/WebCore/html/HTMLMediaElement.idl:68 >> + [TreatReturnedNaNDateAs=NaN] Date getStartDate(); > > I think this is a really strange type for an attribute and doesn’t seem like something that would be in a new modern standard. Could you please double check that “either a Date object or a number with NaN value” is what we want here?
Perhaps my reading of 4.7.10.6 of the spec is wrong, but I think it states that NaN is needed at least in the case of pre-playback state.
Eric Carlson
Comment 19
2015-06-07 17:44:40 PDT
Comment on
attachment 254401
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=254401&action=review
>> Source/WebCore/html/HTMLMediaElement.cpp:882 >> + return std::numeric_limits<double>::quiet_NaN(); > > I think there should be some kind of comment here explaining why we are translating a zero into a NaN. Is zero not a legal value?
Actually I don't think we should translate to NaN here at all. Lets make it the responsibility of the media engine to return a valid Date (as a double) or NaN.
>>>> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:97 >>>> + virtual MediaTime getStartDate() { return MediaTime::createWithDouble(currentTime()); } >>> >>> This should be 'const' >> >> Shouldn't this return NaN by default? > > Is it OK to leave all the other back ends returning sort of “nonsense” values? Returning a wrong start date might be worse than not having the function at all. Maybe we have to make the binding conditional so the function will only appear when it’s going to work. But also, don’t we use multiple back ends in the Mac and/or iOS ports?
It is more nuanced than that - not all files support startDate so even ports with a backend that supports it will sometimes (usually) return NaN because NaN is a signal that the file doesn't have a startDate. Having said that, making the binding conditional with a build flag seems like a good idea so ports with a backend that doesn't support it won't have the method at all.
Matthew Daiter
Comment 20
2015-06-08 10:00:45 PDT
How should we move forward with this?
Matthew Daiter
Comment 21
2015-06-08 15:09:18 PDT
Created
attachment 254515
[details]
Patch
Matthew Daiter
Comment 22
2015-06-08 15:09:53 PDT
Eric and I just discussed how this should move forward. Updates posted.
Build Bot
Comment 23
2015-06-08 15:50:20 PDT
Comment on
attachment 254515
[details]
Patch
Attachment 254515
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/5113595301986304
New failing tests: http/tests/media/hls/video-controller-getStartDate.html
Build Bot
Comment 24
2015-06-08 15:50:24 PDT
Created
attachment 254516
[details]
Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 25
2015-06-08 15:59:49 PDT
Comment on
attachment 254515
[details]
Patch
Attachment 254515
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/5992349905715200
New failing tests: http/tests/media/hls/video-controller-getStartDate.html
Build Bot
Comment 26
2015-06-08 15:59:52 PDT
Created
attachment 254519
[details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Matthew Daiter
Comment 27
2015-06-08 16:57:57 PDT
Created
attachment 254523
[details]
Patch
Matthew Daiter
Comment 28
2015-06-08 17:00:17 PDT
Created
attachment 254524
[details]
Patch
Darin Adler
Comment 29
2015-06-08 17:25:11 PDT
Comment on
attachment 254524
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=254524&action=review
> Source/WebCore/bindings/js/JSDOMBinding.h:321 > +// Returns a Date instnace for the specified value, or NaN if the date is not a number.
Typo in "instance" here. Confusing wording "if the date is not a number"; should say value. If the value passed is infinity, does this return null? Given that, why doesn’t the comment say that?
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:731 > + return MediaTime::createWithDouble(round(date - currentTime));
Why round this? Why not just leave it fractional?
Build Bot
Comment 30
2015-06-08 17:48:37 PDT
Comment on
attachment 254524
[details]
Patch
Attachment 254524
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/5639861939732480
New failing tests: http/tests/media/hls/video-controller-getStartDate.html
Build Bot
Comment 31
2015-06-08 17:48:41 PDT
Created
attachment 254529
[details]
Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 32
2015-06-08 17:50:24 PDT
Comment on
attachment 254524
[details]
Patch
Attachment 254524
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/4873510925107200
New failing tests: http/tests/media/hls/video-controller-getStartDate.html
Build Bot
Comment 33
2015-06-08 17:50:27 PDT
Created
attachment 254530
[details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Eric Carlson
Comment 34
2015-06-09 08:44:11 PDT
Comment on
attachment 254524
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=254524&action=review
This is really close, but r- for now because the the test will be flakey as written.
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:726 > + // No live streams were made during the epoch (1970). 0 is a default value returned if the system can't find a start time
Minor nit: I would prefer to see a blank line before this comment. Also, "0 is a default value returned if the system can't find a start time" isn't quite correct, something like "AVFoundation returns 0 if the media file doesn't have a start date" would be better.
>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:731 >> + return MediaTime::createWithDouble(round(date - currentTime)); > > Why round this? Why not just leave it fractional?
I agree.
> LayoutTests/http/tests/media/hls/video-controller-getStartDate.html:1 > +<html>
Minor nit: might as well include a doctype: <!DOCTYPE html>
> LayoutTests/http/tests/media/hls/video-controller-getStartDate.html:23 > + setTimeout(function() { > + testExpected("video.getStartDate()", "Wed Nov 03 2010 01:00:00 GMT-0700 (PDT)"); > + endTest(); > + }, 1000);
Running your tests after a timeout is not a good idea - without a sufficiently long timeout you will sometimes fail, but you will usually waste time because the element will be ready before the timer fires. It is better to listen for one of the HTMLMediaElement state change events so your test runs as soon as possible. Something like this (written in the browser and not tested): var canPlayThroughCount = 0; function canPlayThrough() { consoleWrite("EVENT(canplaythrough)"); if (++canPlayThroughCount == 2) testGetStartDate() } function start() { video = document.getElementById("source_without_start_date"); video.src = "../resources/hls/test-vod.m3u8"; video.addEventListener('canplaythrough', canPlayThrough, true); video = document.getElementById('source_with_start_date'); video.src = "../resources/hls/test-vod-date-time.m3u8"; video.addEventListener('canplaythrough', canPlayThrough, true); } function testGetStartDate() { video = document.getElementById("source_without_start_date"); testExpected("isNaN(video.getStartDate())", true ); video = document.getElementById('source_with_start_date'); testExpected("video.getStartDate()", "Wed Nov 03 2010 01:00:00 GMT-0700 (PDT)"); endTest(); }
> LayoutTests/http/tests/media/hls/video-controller-getStartDate.html:30 > + <video id="source_with_start_date" autoplay="autoplay" controls="controls" width="640" height="480"></video> > + <video id="source_without_start_date" autoplay="autoplay" controls="controls" width="640" height="480"></video>
Nit: 'controls' and 'autoplay' are boolean attributes so they don't need values.
Matthew Daiter
Comment 35
2015-06-09 11:05:01 PDT
Created
attachment 254582
[details]
Patch
Build Bot
Comment 36
2015-06-09 11:52:35 PDT
Comment on
attachment 254582
[details]
Patch
Attachment 254582
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/6130118497927168
New failing tests: http/tests/media/hls/video-controller-getStartDate.html
Build Bot
Comment 37
2015-06-09 11:52:40 PDT
Created
attachment 254584
[details]
Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 38
2015-06-09 12:23:21 PDT
Comment on
attachment 254582
[details]
Patch
Attachment 254582
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/5537134911946752
New failing tests: http/tests/media/hls/video-controller-getStartDate.html
Build Bot
Comment 39
2015-06-09 12:23:24 PDT
Created
attachment 254588
[details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Eric Carlson
Comment 40
2015-06-09 13:46:33 PDT
Comment on
attachment 254582
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=254582&action=review
This looks good, thanks for being responsive to all of the feedback! Marking r+, but you will need to fix the build break and test results before landing.
> LayoutTests/http/tests/media/hls/video-controller-getStartDate.html:2 > +<!DOCTYPE html> > + <head>
You need an <html> element here.
Jon Lee
Comment 41
2015-06-12 14:58:04 PDT
rdar://problem/20876076
Matthew Daiter
Comment 42
2015-06-17 00:36:55 PDT
Comment on
attachment 254582
[details]
Patch
>Index: Source/WebCore/ChangeLog >=================================================================== >--- Source/WebCore/ChangeLog (revision 185366) >+++ Source/WebCore/ChangeLog (working copy) >@@ -1,3 +1,47 @@ >+2015-06-09 Matthew Daiter <
mdaiter@apple.com
> >+ >+ Added getStartDate() support to WebKit. Allows HLS streams to define >+ when they had been originally streamed, so that services using them >+ could inform users about the original date, etc. >+
https://bugs.webkit.org/show_bug.cgi?id=145676
>+ <
rdar://problem/20876076
> >+ >+ Test: http/tests/media/hls/video-controller-getStartDate.html >+ >+ * bindings/js/JSDOMBinding.cpp: Needed to make an extra method of >+ certain NaN functions over Null-returning functions >+ (WebCore::valueToStringWithUndefinedOrNullCheck): >+ (WebCore::jsDateOrNaN): >+ (WebCore::jsDateOrNull): >+ * bindings/js/JSDOMBinding.h: >+ * bindings/scripts/CodeGeneratorJS.pm: Needed to create code >+ generation for NaN numbers >+ (NativeToJSValue): >+ * bindings/scripts/IDLAttributes.txt: Added attributes for generating >+ Javascript >+ * html/HTMLMediaElement.cpp: >+ (WebCore::HTMLMediaElement::canPlayType): >+ (WebCore::HTMLMediaElement::getStartDate): >+ (WebCore::HTMLMediaElement::load): >+ * html/HTMLMediaElement.h: >+ * html/HTMLMediaElement.idl: Added defs for MediaElements >+ * platform/graphics/MediaPlayer.cpp: >+ (WebCore::MediaPlayer::currentTime): >+ (WebCore::MediaPlayer::getStartDate): >+ (WebCore::MediaPlayer::seekWithTolerance): >+ * platform/graphics/MediaPlayer.h: >+ * platform/graphics/MediaPlayerPrivate.h: >+ (WebCore::MediaPlayerPrivateInterface::currentTimeDouble): >+ (WebCore::MediaPlayerPrivateInterface::currentMediaTime): >+ (WebCore::MediaPlayerPrivateInterface::getStartDate): >+ (WebCore::MediaPlayerPrivateInterface::seek): >+ (WebCore::MediaPlayerPrivateInterface::seekDouble): >+ * platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h: >+ * platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm: >+ (WebCore::MediaPlayerPrivateAVFoundationObjC::destroyVideoLayer): >+ (WebCore::MediaPlayerPrivateAVFoundationObjC::getStartDate): >+ (WebCore::MediaPlayerPrivateAVFoundationObjC::hasAvailableVideoFrame): >+ > 2015-06-09 Csaba Osztrogonác <
ossy@webkit.org
> > > [cmake] Fix the style issues in cmake project files >Index: Source/WebCore/bindings/js/JSDOMBinding.cpp >=================================================================== >--- Source/WebCore/bindings/js/JSDOMBinding.cpp (revision 185366) >+++ Source/WebCore/bindings/js/JSDOMBinding.cpp (working copy) >@@ -113,6 +113,13 @@ String valueToStringWithUndefinedOrNullC > return value.toString(exec)->value(exec); > } > >+JSValue jsDateOrNaN(ExecState* exec, double value) >+{ >+ if (std::isnan(value)) >+ return jsDoubleNumber(value); >+ return jsDateOrNull(exec, value); >+} >+ > JSValue jsDateOrNull(ExecState* exec, double value) > { > if (!std::isfinite(value)) >Index: Source/WebCore/bindings/js/JSDOMBinding.h >=================================================================== >--- Source/WebCore/bindings/js/JSDOMBinding.h (revision 185366) >+++ Source/WebCore/bindings/js/JSDOMBinding.h (working copy) >@@ -318,6 +318,8 @@ inline uint32_t toUInt32(JSC::ExecState* > WEBCORE_EXPORT int64_t toInt64(JSC::ExecState*, JSC::JSValue, IntegerConversionConfiguration); > WEBCORE_EXPORT uint64_t toUInt64(JSC::ExecState*, JSC::JSValue, IntegerConversionConfiguration); > >+// Returns a Date instnace for the specified value, or NaN if the date is not a number. >+JSC::JSValue jsDateOrNaN(JSC::ExecState*, double); > // Returns a Date instance for the specified value, or null if the value is NaN or infinity. > JSC::JSValue jsDateOrNull(JSC::ExecState*, double); > // NaN if the value can't be converted to a date. >Index: Source/WebCore/bindings/scripts/CodeGeneratorJS.pm >=================================================================== >--- Source/WebCore/bindings/scripts/CodeGeneratorJS.pm (revision 185366) >+++ Source/WebCore/bindings/scripts/CodeGeneratorJS.pm (working copy) >@@ -3896,6 +3896,13 @@ sub NativeToJSValue > > # Need to check Date type before IsPrimitiveType(). > if ($type eq "Date") { >+ my $conv = $signature->extendedAttributes->{"TreatReturnedNaNDateAs"}; >+ if (defined $conv) { >+ return "jsDateOrNull(exec, $value)" if $conv eq "Null"; >+ return "jsDateOrNaN(exec, $value)" if $conv eq "NaN"; >+ >+ die "Unknown value for TreatReturnedNaNDateAs extended attribute"; >+ } > return "jsDateOrNull(exec, $value)"; > } > >Index: Source/WebCore/bindings/scripts/IDLAttributes.txt >=================================================================== >--- Source/WebCore/bindings/scripts/IDLAttributes.txt (revision 185366) >+++ Source/WebCore/bindings/scripts/IDLAttributes.txt (working copy) >@@ -111,6 +111,7 @@ SkipVTableValidation > StrictTypeChecking > SuppressToJSObject > TreatNullAs=NullString >+TreatReturnedNaNDateAs=Null|NaN > TreatReturnedNullStringAs=Null|Undefined > TreatUndefinedAs=NullString > TypedArray=* >Index: Source/WebCore/html/HTMLMediaElement.cpp >=================================================================== >--- Source/WebCore/html/HTMLMediaElement.cpp (revision 185366) >+++ Source/WebCore/html/HTMLMediaElement.cpp (working copy) >@@ -875,6 +875,11 @@ String HTMLMediaElement::canPlayType(con > return canPlay; > } > >+double HTMLMediaElement::getStartDate() const >+{ >+ return m_player->getStartDate().toDouble(); >+} >+ > void HTMLMediaElement::load() > { > Ref<HTMLMediaElement> protect(*this); // loadInternal may result in a 'beforeload' event, which can make arbitrary DOM mutations. >Index: Source/WebCore/html/HTMLMediaElement.h >=================================================================== >--- Source/WebCore/html/HTMLMediaElement.h (revision 185366) >+++ Source/WebCore/html/HTMLMediaElement.h (working copy) >@@ -178,6 +178,7 @@ public: > WEBCORE_EXPORT virtual double currentTime() const override; > virtual void setCurrentTime(double) override; > virtual void setCurrentTime(double, ExceptionCode&); >+ virtual double getStartDate() const; > WEBCORE_EXPORT virtual double duration() const override; > WEBCORE_EXPORT virtual bool paused() const override; > virtual double defaultPlaybackRate() const override; >Index: Source/WebCore/html/HTMLMediaElement.idl >=================================================================== >--- Source/WebCore/html/HTMLMediaElement.idl (revision 185366) >+++ Source/WebCore/html/HTMLMediaElement.idl (working copy) >@@ -65,6 +65,7 @@ > // playback state > [SetterRaisesException] attribute unrestricted double currentTime; > readonly attribute unrestricted double duration; >+ [TreatReturnedNaNDateAs=NaN] Date getStartDate(); > readonly attribute boolean paused; > attribute unrestricted double defaultPlaybackRate; > attribute unrestricted double playbackRate; >Index: Source/WebCore/platform/graphics/MediaPlayer.cpp >=================================================================== >--- Source/WebCore/platform/graphics/MediaPlayer.cpp (revision 185366) >+++ Source/WebCore/platform/graphics/MediaPlayer.cpp (working copy) >@@ -534,6 +534,11 @@ MediaTime MediaPlayer::currentTime() con > return m_private->currentMediaTime(); > } > >+MediaTime MediaPlayer::getStartDate() const >+{ >+ return m_private->getStartDate(); >+} >+ > void MediaPlayer::seekWithTolerance(const MediaTime& time, const MediaTime& negativeTolerance, const MediaTime& positiveTolerance) > { > m_private->seekWithTolerance(time, negativeTolerance, positiveTolerance); >Index: Source/WebCore/platform/graphics/MediaPlayer.h >=================================================================== >--- Source/WebCore/platform/graphics/MediaPlayer.h (revision 185366) >+++ Source/WebCore/platform/graphics/MediaPlayer.h (working copy) >@@ -380,6 +380,8 @@ public: > MediaTime startTime() const; > MediaTime initialTime() const; > >+ MediaTime getStartDate() const; >+ > double rate() const; > void setRate(double); > double requestedRate() const; >Index: Source/WebCore/platform/graphics/MediaPlayerPrivate.h >=================================================================== >--- Source/WebCore/platform/graphics/MediaPlayerPrivate.h (revision 185366) >+++ Source/WebCore/platform/graphics/MediaPlayerPrivate.h (working copy) >@@ -94,6 +94,8 @@ public: > virtual double currentTimeDouble() const { return currentTime(); } > virtual MediaTime currentMediaTime() const { return MediaTime::createWithDouble(currentTimeDouble()); } > >+ virtual MediaTime getStartDate() const { return MediaTime::createWithDouble(std::numeric_limits<double>::quiet_NaN()); } >+ > virtual void seek(float) { } > virtual void seekDouble(double time) { seek(time); } > virtual void seek(const MediaTime& time) { seekDouble(time.toDouble()); } >Index: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h >=================================================================== >--- Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h (revision 185366) >+++ Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h (working copy) >@@ -206,6 +206,8 @@ private: > virtual void updateVideoLayerGravity() override; > > virtual bool hasSingleSecurityOrigin() const; >+ >+ MediaTime getStartDate() const override; > > #if ENABLE(VIDEO_TRACK) > virtual bool requiresTextTrackRepresentation() const override; >Index: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm >=================================================================== >--- Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm (revision 185366) >+++ Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm (working copy) >@@ -719,6 +719,21 @@ void MediaPlayerPrivateAVFoundationObjC: > m_videoLayer = nil; > } > >+MediaTime MediaPlayerPrivateAVFoundationObjC::getStartDate() const >+{ >+ // Date changes as the track's playback position changes. Must subtract currentTime (offset in seconds) from date offset to get date beginning >+ double date = [[m_avPlayerItem currentDate] timeIntervalSince1970] * 1000; >+ >+ // No live streams were made during the epoch (1970). AVFoundation returns 0 if the media file doesn't have a start date >+ if (!date) >+ return MediaTime::invalidTime(); >+ >+ double currentTime = CMTimeGetSeconds([m_avPlayerItem currentTime]) * 1000; >+ >+ // Rounding due to second offset error when subtracting. >+ return MediaTime::createWithDouble(round(date - currentTime)); >+} >+ > bool MediaPlayerPrivateAVFoundationObjC::hasAvailableVideoFrame() const > { > if (currentRenderingMode() == MediaRenderingToLayer) >Index: LayoutTests/ChangeLog >=================================================================== >--- LayoutTests/ChangeLog (revision 185366) >+++ LayoutTests/ChangeLog (working copy) >@@ -1,3 +1,14 @@ >+2015-06-09 Matthew Daiter <
mdaiter@apple.com
> >+ >+ Added tests for getStartDate() >+
https://bugs.webkit.org/show_bug.cgi?id=145676
>+ <
rdar://problem/20876076
> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ * http/tests/media/hls/video-controller-getStartDate.html: Added. >+ * http/tests/media/resources/hls/test-vod-date-time.m3u8: Added. >+ > 2015-06-09 Daniel Bates <
dabates@apple.com
> > > Update iOS TestExpectations files >Index: LayoutTests/http/tests/media/hls/video-controller-getStartDate-expected.txt >=================================================================== >--- LayoutTests/http/tests/media/hls/video-controller-getStartDate-expected.txt (revision 0) >+++ LayoutTests/http/tests/media/hls/video-controller-getStartDate-expected.txt (working copy) >@@ -0,0 +1,7 @@ >+Test that getStartDate() returns appropriate NaN or date. >+ >+EVENT(canplaythrough) >+EVENT(canplaythrough) >+EXPECTED (isNaN(video.getStartDate()) == 'true') OK >+EXPECTED (video.getStartDate() == 'Wed Nov 03 2010 01:00:00 GMT-0700 (PDT)') OK >+END OF TEST >Index: LayoutTests/http/tests/media/hls/video-controller-getStartDate.html >=================================================================== >--- LayoutTests/http/tests/media/hls/video-controller-getStartDate.html (revision 0) >+++ LayoutTests/http/tests/media/hls/video-controller-getStartDate.html (working copy) >@@ -0,0 +1,45 @@ >+<!DOCTYPE html> >+<html> >+ <head> >+ <script src=../../media-resources/media-file.js></script> >+ <script src=../../media-resources/video-test.js></script> >+ <script> >+ var canPlayThroughCount = 0; >+ >+ function canPlayThrough() >+ { >+ consoleWrite("EVENT(canplaythrough)"); >+ if (++canPlayThroughCount == 2) >+ testGetStartDate() >+ } >+ >+ function start() >+ { >+ findMediaElement(); >+ >+ video = document.getElementById("source_without_start_date"); >+ video.src = "../resources/hls/test-vod.m3u8"; >+ video.addEventListener('canplaythrough', canPlayThrough, true); >+ >+ video = document.getElementById('source_with_start_date'); >+ video.src = "../resources/hls/test-vod-date-time.m3u8"; >+ video.addEventListener('canplaythrough', canPlayThrough, true); >+ } >+ >+ function testGetStartDate() >+ { >+ video = document.getElementById("source_without_start_date"); >+ testExpected("isNaN(video.getStartDate())", true ); >+ >+ video = document.getElementById('source_with_start_date'); >+ testExpected("video.getStartDate()", "Wed Nov 03 2010 01:00:00 GMT-0700 (PDT)"); >+ >+ endTest(); >+ } >+ </script> >+ </head> >+ <body onload="start()"> >+ <video id="source_with_start_date" autoplay="autoplay" width="640" height="480"></video> >+ <video id="source_without_start_date" autoplay="autoplay" width="640" height="480"></video> >+ <p>Test that getStartDate() returns appropriate NaN or date.</p> >+ </body> >+</html> >Index: LayoutTests/http/tests/media/resources/hls/test-vod-date-time.m3u8 >=================================================================== >--- LayoutTests/http/tests/media/resources/hls/test-vod-date-time.m3u8 (revision 0) >+++ LayoutTests/http/tests/media/resources/hls/test-vod-date-time.m3u8 (working copy) >@@ -0,0 +1,9 @@ >+#EXTM3U >+#EXT-X-TARGETDURATION:6 >+#EXT-X-PROGRAM-DATE-TIME:2010-11-03T00:00:00-08:00 >+#EXT-X-VERSION:4 >+#EXT-X-MEDIA-SEQUENCE:0 >+#EXT-X-PLAYLIST-TYPE:VOD >+#EXTINF:6.0272, >+test.ts >+#EXT-X-ENDLIST
Matthew Daiter
Comment 43
2015-06-17 00:37:13 PDT
Comment on
attachment 254582
[details]
Patch
>Index: Source/WebCore/ChangeLog >=================================================================== >--- Source/WebCore/ChangeLog (revision 185366) >+++ Source/WebCore/ChangeLog (working copy) >@@ -1,3 +1,47 @@ >+2015-06-09 Matthew Daiter <
mdaiter@apple.com
> >+ >+ Added getStartDate() support to WebKit. Allows HLS streams to define >+ when they had been originally streamed, so that services using them >+ could inform users about the original date, etc. >+
https://bugs.webkit.org/show_bug.cgi?id=145676
>+ <
rdar://problem/20876076
> >+ >+ Test: http/tests/media/hls/video-controller-getStartDate.html >+ >+ * bindings/js/JSDOMBinding.cpp: Needed to make an extra method of >+ certain NaN functions over Null-returning functions >+ (WebCore::valueToStringWithUndefinedOrNullCheck): >+ (WebCore::jsDateOrNaN): >+ (WebCore::jsDateOrNull): >+ * bindings/js/JSDOMBinding.h: >+ * bindings/scripts/CodeGeneratorJS.pm: Needed to create code >+ generation for NaN numbers >+ (NativeToJSValue): >+ * bindings/scripts/IDLAttributes.txt: Added attributes for generating >+ Javascript >+ * html/HTMLMediaElement.cpp: >+ (WebCore::HTMLMediaElement::canPlayType): >+ (WebCore::HTMLMediaElement::getStartDate): >+ (WebCore::HTMLMediaElement::load): >+ * html/HTMLMediaElement.h: >+ * html/HTMLMediaElement.idl: Added defs for MediaElements >+ * platform/graphics/MediaPlayer.cpp: >+ (WebCore::MediaPlayer::currentTime): >+ (WebCore::MediaPlayer::getStartDate): >+ (WebCore::MediaPlayer::seekWithTolerance): >+ * platform/graphics/MediaPlayer.h: >+ * platform/graphics/MediaPlayerPrivate.h: >+ (WebCore::MediaPlayerPrivateInterface::currentTimeDouble): >+ (WebCore::MediaPlayerPrivateInterface::currentMediaTime): >+ (WebCore::MediaPlayerPrivateInterface::getStartDate): >+ (WebCore::MediaPlayerPrivateInterface::seek): >+ (WebCore::MediaPlayerPrivateInterface::seekDouble): >+ * platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h: >+ * platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm: >+ (WebCore::MediaPlayerPrivateAVFoundationObjC::destroyVideoLayer): >+ (WebCore::MediaPlayerPrivateAVFoundationObjC::getStartDate): >+ (WebCore::MediaPlayerPrivateAVFoundationObjC::hasAvailableVideoFrame): >+ > 2015-06-09 Csaba Osztrogonác <
ossy@webkit.org
> > > [cmake] Fix the style issues in cmake project files >Index: Source/WebCore/bindings/js/JSDOMBinding.cpp >=================================================================== >--- Source/WebCore/bindings/js/JSDOMBinding.cpp (revision 185366) >+++ Source/WebCore/bindings/js/JSDOMBinding.cpp (working copy) >@@ -113,6 +113,13 @@ String valueToStringWithUndefinedOrNullC > return value.toString(exec)->value(exec); > } > >+JSValue jsDateOrNaN(ExecState* exec, double value) >+{ >+ if (std::isnan(value)) >+ return jsDoubleNumber(value); >+ return jsDateOrNull(exec, value); >+} >+ > JSValue jsDateOrNull(ExecState* exec, double value) > { > if (!std::isfinite(value)) >Index: Source/WebCore/bindings/js/JSDOMBinding.h >=================================================================== >--- Source/WebCore/bindings/js/JSDOMBinding.h (revision 185366) >+++ Source/WebCore/bindings/js/JSDOMBinding.h (working copy) >@@ -318,6 +318,8 @@ inline uint32_t toUInt32(JSC::ExecState* > WEBCORE_EXPORT int64_t toInt64(JSC::ExecState*, JSC::JSValue, IntegerConversionConfiguration); > WEBCORE_EXPORT uint64_t toUInt64(JSC::ExecState*, JSC::JSValue, IntegerConversionConfiguration); > >+// Returns a Date instnace for the specified value, or NaN if the date is not a number. >+JSC::JSValue jsDateOrNaN(JSC::ExecState*, double); > // Returns a Date instance for the specified value, or null if the value is NaN or infinity. > JSC::JSValue jsDateOrNull(JSC::ExecState*, double); > // NaN if the value can't be converted to a date. >Index: Source/WebCore/bindings/scripts/CodeGeneratorJS.pm >=================================================================== >--- Source/WebCore/bindings/scripts/CodeGeneratorJS.pm (revision 185366) >+++ Source/WebCore/bindings/scripts/CodeGeneratorJS.pm (working copy) >@@ -3896,6 +3896,13 @@ sub NativeToJSValue > > # Need to check Date type before IsPrimitiveType(). > if ($type eq "Date") { >+ my $conv = $signature->extendedAttributes->{"TreatReturnedNaNDateAs"}; >+ if (defined $conv) { >+ return "jsDateOrNull(exec, $value)" if $conv eq "Null"; >+ return "jsDateOrNaN(exec, $value)" if $conv eq "NaN"; >+ >+ die "Unknown value for TreatReturnedNaNDateAs extended attribute"; >+ } > return "jsDateOrNull(exec, $value)"; > } > >Index: Source/WebCore/bindings/scripts/IDLAttributes.txt >=================================================================== >--- Source/WebCore/bindings/scripts/IDLAttributes.txt (revision 185366) >+++ Source/WebCore/bindings/scripts/IDLAttributes.txt (working copy) >@@ -111,6 +111,7 @@ SkipVTableValidation > StrictTypeChecking > SuppressToJSObject > TreatNullAs=NullString >+TreatReturnedNaNDateAs=Null|NaN > TreatReturnedNullStringAs=Null|Undefined > TreatUndefinedAs=NullString > TypedArray=* >Index: Source/WebCore/html/HTMLMediaElement.cpp >=================================================================== >--- Source/WebCore/html/HTMLMediaElement.cpp (revision 185366) >+++ Source/WebCore/html/HTMLMediaElement.cpp (working copy) >@@ -875,6 +875,11 @@ String HTMLMediaElement::canPlayType(con > return canPlay; > } > >+double HTMLMediaElement::getStartDate() const >+{ >+ return m_player->getStartDate().toDouble(); >+} >+ > void HTMLMediaElement::load() > { > Ref<HTMLMediaElement> protect(*this); // loadInternal may result in a 'beforeload' event, which can make arbitrary DOM mutations. >Index: Source/WebCore/html/HTMLMediaElement.h >=================================================================== >--- Source/WebCore/html/HTMLMediaElement.h (revision 185366) >+++ Source/WebCore/html/HTMLMediaElement.h (working copy) >@@ -178,6 +178,7 @@ public: > WEBCORE_EXPORT virtual double currentTime() const override; > virtual void setCurrentTime(double) override; > virtual void setCurrentTime(double, ExceptionCode&); >+ virtual double getStartDate() const; > WEBCORE_EXPORT virtual double duration() const override; > WEBCORE_EXPORT virtual bool paused() const override; > virtual double defaultPlaybackRate() const override; >Index: Source/WebCore/html/HTMLMediaElement.idl >=================================================================== >--- Source/WebCore/html/HTMLMediaElement.idl (revision 185366) >+++ Source/WebCore/html/HTMLMediaElement.idl (working copy) >@@ -65,6 +65,7 @@ > // playback state > [SetterRaisesException] attribute unrestricted double currentTime; > readonly attribute unrestricted double duration; >+ [TreatReturnedNaNDateAs=NaN] Date getStartDate(); > readonly attribute boolean paused; > attribute unrestricted double defaultPlaybackRate; > attribute unrestricted double playbackRate; >Index: Source/WebCore/platform/graphics/MediaPlayer.cpp >=================================================================== >--- Source/WebCore/platform/graphics/MediaPlayer.cpp (revision 185366) >+++ Source/WebCore/platform/graphics/MediaPlayer.cpp (working copy) >@@ -534,6 +534,11 @@ MediaTime MediaPlayer::currentTime() con > return m_private->currentMediaTime(); > } > >+MediaTime MediaPlayer::getStartDate() const >+{ >+ return m_private->getStartDate(); >+} >+ > void MediaPlayer::seekWithTolerance(const MediaTime& time, const MediaTime& negativeTolerance, const MediaTime& positiveTolerance) > { > m_private->seekWithTolerance(time, negativeTolerance, positiveTolerance); >Index: Source/WebCore/platform/graphics/MediaPlayer.h >=================================================================== >--- Source/WebCore/platform/graphics/MediaPlayer.h (revision 185366) >+++ Source/WebCore/platform/graphics/MediaPlayer.h (working copy) >@@ -380,6 +380,8 @@ public: > MediaTime startTime() const; > MediaTime initialTime() const; > >+ MediaTime getStartDate() const; >+ > double rate() const; > void setRate(double); > double requestedRate() const; >Index: Source/WebCore/platform/graphics/MediaPlayerPrivate.h >=================================================================== >--- Source/WebCore/platform/graphics/MediaPlayerPrivate.h (revision 185366) >+++ Source/WebCore/platform/graphics/MediaPlayerPrivate.h (working copy) >@@ -94,6 +94,8 @@ public: > virtual double currentTimeDouble() const { return currentTime(); } > virtual MediaTime currentMediaTime() const { return MediaTime::createWithDouble(currentTimeDouble()); } > >+ virtual MediaTime getStartDate() const { return MediaTime::createWithDouble(std::numeric_limits<double>::quiet_NaN()); } >+ > virtual void seek(float) { } > virtual void seekDouble(double time) { seek(time); } > virtual void seek(const MediaTime& time) { seekDouble(time.toDouble()); } >Index: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h >=================================================================== >--- Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h (revision 185366) >+++ Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h (working copy) >@@ -206,6 +206,8 @@ private: > virtual void updateVideoLayerGravity() override; > > virtual bool hasSingleSecurityOrigin() const; >+ >+ MediaTime getStartDate() const override; > > #if ENABLE(VIDEO_TRACK) > virtual bool requiresTextTrackRepresentation() const override; >Index: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm >=================================================================== >--- Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm (revision 185366) >+++ Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm (working copy) >@@ -719,6 +719,21 @@ void MediaPlayerPrivateAVFoundationObjC: > m_videoLayer = nil; > } > >+MediaTime MediaPlayerPrivateAVFoundationObjC::getStartDate() const >+{ >+ // Date changes as the track's playback position changes. Must subtract currentTime (offset in seconds) from date offset to get date beginning >+ double date = [[m_avPlayerItem currentDate] timeIntervalSince1970] * 1000; >+ >+ // No live streams were made during the epoch (1970). AVFoundation returns 0 if the media file doesn't have a start date >+ if (!date) >+ return MediaTime::invalidTime(); >+ >+ double currentTime = CMTimeGetSeconds([m_avPlayerItem currentTime]) * 1000; >+ >+ // Rounding due to second offset error when subtracting. >+ return MediaTime::createWithDouble(round(date - currentTime)); >+} >+ > bool MediaPlayerPrivateAVFoundationObjC::hasAvailableVideoFrame() const > { > if (currentRenderingMode() == MediaRenderingToLayer) >Index: LayoutTests/ChangeLog >=================================================================== >--- LayoutTests/ChangeLog (revision 185366) >+++ LayoutTests/ChangeLog (working copy) >@@ -1,3 +1,14 @@ >+2015-06-09 Matthew Daiter <
mdaiter@apple.com
> >+ >+ Added tests for getStartDate() >+
https://bugs.webkit.org/show_bug.cgi?id=145676
>+ <
rdar://problem/20876076
> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ * http/tests/media/hls/video-controller-getStartDate.html: Added. >+ * http/tests/media/resources/hls/test-vod-date-time.m3u8: Added. >+ > 2015-06-09 Daniel Bates <
dabates@apple.com
> > > Update iOS TestExpectations files >Index: LayoutTests/http/tests/media/hls/video-controller-getStartDate-expected.txt >=================================================================== >--- LayoutTests/http/tests/media/hls/video-controller-getStartDate-expected.txt (revision 0) >+++ LayoutTests/http/tests/media/hls/video-controller-getStartDate-expected.txt (working copy) >@@ -0,0 +1,7 @@ >+Test that getStartDate() returns appropriate NaN or date. >+ >+EVENT(canplaythrough) >+EVENT(canplaythrough) >+EXPECTED (isNaN(video.getStartDate()) == 'true') OK >+EXPECTED (video.getStartDate() == 'Wed Nov 03 2010 01:00:00 GMT-0700 (PDT)') OK >+END OF TEST >Index: LayoutTests/http/tests/media/hls/video-controller-getStartDate.html >=================================================================== >--- LayoutTests/http/tests/media/hls/video-controller-getStartDate.html (revision 0) >+++ LayoutTests/http/tests/media/hls/video-controller-getStartDate.html (working copy) >@@ -0,0 +1,45 @@ >+<!DOCTYPE html> >+<html> >+ <head> >+ <script src=../../media-resources/media-file.js></script> >+ <script src=../../media-resources/video-test.js></script> >+ <script> >+ var canPlayThroughCount = 0; >+ >+ function canPlayThrough() >+ { >+ consoleWrite("EVENT(canplaythrough)"); >+ if (++canPlayThroughCount == 2) >+ testGetStartDate() >+ } >+ >+ function start() >+ { >+ findMediaElement(); >+ >+ video = document.getElementById("source_without_start_date"); >+ video.src = "../resources/hls/test-vod.m3u8"; >+ video.addEventListener('canplaythrough', canPlayThrough, true); >+ >+ video = document.getElementById('source_with_start_date'); >+ video.src = "../resources/hls/test-vod-date-time.m3u8"; >+ video.addEventListener('canplaythrough', canPlayThrough, true); >+ } >+ >+ function testGetStartDate() >+ { >+ video = document.getElementById("source_without_start_date"); >+ testExpected("isNaN(video.getStartDate())", true ); >+ >+ video = document.getElementById('source_with_start_date'); >+ testExpected("video.getStartDate()", "Wed Nov 03 2010 01:00:00 GMT-0700 (PDT)"); >+ >+ endTest(); >+ } >+ </script> >+ </head> >+ <body onload="start()"> >+ <video id="source_with_start_date" autoplay="autoplay" width="640" height="480"></video> >+ <video id="source_without_start_date" autoplay="autoplay" width="640" height="480"></video> >+ <p>Test that getStartDate() returns appropriate NaN or date.</p> >+ </body> >+</html> >Index: LayoutTests/http/tests/media/resources/hls/test-vod-date-time.m3u8 >=================================================================== >--- LayoutTests/http/tests/media/resources/hls/test-vod-date-time.m3u8 (revision 0) >+++ LayoutTests/http/tests/media/resources/hls/test-vod-date-time.m3u8 (working copy) >@@ -0,0 +1,9 @@ >+#EXTM3U >+#EXT-X-TARGETDURATION:6 >+#EXT-X-PROGRAM-DATE-TIME:2010-11-03T00:00:00-08:00 >+#EXT-X-VERSION:4 >+#EXT-X-MEDIA-SEQUENCE:0 >+#EXT-X-PLAYLIST-TYPE:VOD >+#EXTINF:6.0272, >+test.ts >+#EXT-X-ENDLIST
Eric Carlson
Comment 44
2015-06-18 10:05:39 PDT
(In reply to
comment #43
)
> Comment on
attachment 254582
[details]
> Patch >
It looks like this didn't attach correctly.
Matthew Daiter
Comment 45
2015-06-19 13:28:56 PDT
Created
attachment 255220
[details]
New patch for this bug. Should work. Supposedly there were misalignments with the test cases?
Matthew Daiter
Comment 46
2015-06-19 15:15:33 PDT
Created
attachment 255234
[details]
Didn't format last patch correctly.
Matthew Daiter
Comment 47
2015-06-21 01:12:25 PDT
Created
attachment 255315
[details]
Patch
Build Bot
Comment 48
2015-06-21 04:03:08 PDT
Comment on
attachment 255315
[details]
Patch
Attachment 255315
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/5738834373378048
New failing tests: http/tests/media/hls/video-controller-getStartDate.html
Build Bot
Comment 49
2015-06-21 04:03:14 PDT
Created
attachment 255319
[details]
Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 50
2015-06-21 04:10:34 PDT
Comment on
attachment 255315
[details]
Patch
Attachment 255315
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/4539857745477632
New failing tests: http/tests/media/hls/video-controller-getStartDate.html
Build Bot
Comment 51
2015-06-21 04:10:38 PDT
Created
attachment 255320
[details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Matthew Daiter
Comment 52
2015-06-22 16:46:34 PDT
Looks like test is failing. Will fix test and re-patch.
Brent Fulgham
Comment 53
2015-06-24 14:33:45 PDT
***
Bug 127854
has been marked as a duplicate of this bug. ***
Matthew Daiter
Comment 54
2015-06-24 16:42:41 PDT
Created
attachment 255529
[details]
Patch
Build Bot
Comment 55
2015-06-24 17:32:28 PDT
Comment on
attachment 255529
[details]
Patch
Attachment 255529
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/4539601121181696
New failing tests: http/tests/media/hls/video-controller-getStartDate.html
Build Bot
Comment 56
2015-06-24 17:32:33 PDT
Created
attachment 255533
[details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Build Bot
Comment 57
2015-06-24 17:53:11 PDT
Comment on
attachment 255529
[details]
Patch
Attachment 255529
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/6618119942438912
New failing tests: http/tests/media/hls/video-controller-getStartDate.html
Build Bot
Comment 58
2015-06-24 17:53:15 PDT
Created
attachment 255535
[details]
Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Matthew Daiter
Comment 59
2015-06-26 15:06:32 PDT
Created
attachment 255669
[details]
Patch
Matthew Daiter
Comment 60
2015-06-26 15:07:22 PDT
Supposedly I needed to add a single space to a test. I have added that single space.
Build Bot
Comment 61
2015-06-26 15:46:26 PDT
Comment on
attachment 255669
[details]
Patch
Attachment 255669
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/4578145499873280
New failing tests: http/tests/media/hls/video-controller-getStartDate.html
Build Bot
Comment 62
2015-06-26 15:46:33 PDT
Created
attachment 255670
[details]
Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 63
2015-06-26 15:50:48 PDT
Comment on
attachment 255669
[details]
Patch
Attachment 255669
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/5125682963152896
New failing tests: http/tests/media/hls/video-controller-getStartDate.html
Build Bot
Comment 64
2015-06-26 15:50:52 PDT
Created
attachment 255671
[details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Matthew Daiter
Comment 65
2015-06-26 15:59:19 PDT
Created
attachment 255673
[details]
Patch
Matthew Daiter
Comment 66
2015-06-26 17:08:51 PDT
Created
attachment 255677
[details]
Patch
Michael Catanzaro
Comment 67
2015-06-26 17:42:46 PDT
CCing Carlos for the CodeGeneratorGObject.pm change in this patch (which keeps the GTK port building)
Brent Fulgham
Comment 68
2015-06-26 17:55:23 PDT
Comment on
attachment 255677
[details]
Patch r=me. Finally got it!
WebKit Commit Bot
Comment 69
2015-06-26 18:44:11 PDT
Comment on
attachment 255677
[details]
Patch Clearing flags on attachment: 255677 Committed
r186020
: <
http://trac.webkit.org/changeset/186020
>
WebKit Commit Bot
Comment 70
2015-06-26 18:44:23 PDT
All reviewed patches have been landed. Closing bug.
Pascal Jacquemart
Comment 71
2015-12-11 07:18:00 PST
Hello, It looks very odd to return directly "NaN". I think we are meant to return a valid Date object anyway, even if the timeline offset is unknown. It is valid JavaScript to initialize a Date object with NaN
> new Date(NaN);
Also I think it would fit our existing layout test
> isNaN(new Date(NaN)); // should return true
What do you think?
Eric Carlson
Comment 72
2015-12-11 08:21:34 PST
(In reply to
comment #71
)
> > It looks very odd to return directly "NaN". > > I think we are meant to return a valid Date object anyway, even if the > timeline offset is unknown. >
>
> It is valid JavaScript to initialize a Date object with NaN > > new Date(NaN); > > Also I think it would fit our existing layout test > > isNaN(new Date(NaN)); // should return true > > What do you think?
The spec says [1]: The getStartDate() method must return a new Date object representing the current timeline offset. where "a new Date object" is defined as: When this specification requires a user agent to create a Date object representing a particular time (which could be the special value Not-a-Number) ... so I think you are correct, thanks! I will be happy to review a patch if you have time to prepare one. [1]
http://www.w3.org/TR/html5/embedded-content-0.html#dom-media-getstartdate
[2]
http://www.w3.org/TR/html5/infrastructure.html#create-a-date-object
Pascal Jacquemart
Comment 73
2015-12-11 11:22:51 PST
Ok, I should give a try...
Chris Dumez
Comment 74
2016-09-08 10:31:01 PDT
Comment on
attachment 255677
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=255677&action=review
> Source/WebCore/html/HTMLMediaElement.cpp:887 > + return m_player->getStartDate().toDouble();
Nice null dereference when m_player is null: document.createElement("audio").getStartDate() Fixing via
Bug 161733
.
Chris Dumez
Comment 75
2016-09-08 10:34:58 PDT
Comment on
attachment 255677
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=255677&action=review
> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3937 > + my $conv = $signature->extendedAttributes->{"TreatReturnedNaNDateAs"};
This is not part of Web IDL. When do we ever want to return null? We should not be adding new WebKit-specific IDL extended attributes unless strictly needed.
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