Allows users to grab original streaming dates from HTMLMediaElements. Useful for grabbing and displaying original airdates.
<rdar://problem/21252512>
Created attachment 254320 [details] Patch
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
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
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
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
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! :)
Created attachment 254383 [details] Patch
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.
Created attachment 254400 [details] Patch
(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
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.
Created attachment 254401 [details] Patch
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.
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)?
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 "-".
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.
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.
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.
How should we move forward with this?
Created attachment 254515 [details] Patch
Eric and I just discussed how this should move forward. Updates posted.
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
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
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
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
Created attachment 254523 [details] Patch
Created attachment 254524 [details] Patch
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?
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
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
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
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
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.
Created attachment 254582 [details] Patch
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
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
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
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
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.
rdar://problem/20876076
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
(In reply to comment #43) > Comment on attachment 254582 [details] > Patch > It looks like this didn't attach correctly.
Created attachment 255220 [details] New patch for this bug. Should work. Supposedly there were misalignments with the test cases?
Created attachment 255234 [details] Didn't format last patch correctly.
Created attachment 255315 [details] Patch
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
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
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
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
Looks like test is failing. Will fix test and re-patch.
*** Bug 127854 has been marked as a duplicate of this bug. ***
Created attachment 255529 [details] Patch
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
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
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
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
Created attachment 255669 [details] Patch
Supposedly I needed to add a single space to a test. I have added that single space.
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
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
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
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
Created attachment 255673 [details] Patch
Created attachment 255677 [details] Patch
CCing Carlos for the CodeGeneratorGObject.pm change in this patch (which keeps the GTK port building)
Comment on attachment 255677 [details] Patch r=me. Finally got it!
Comment on attachment 255677 [details] Patch Clearing flags on attachment: 255677 Committed r186020: <http://trac.webkit.org/changeset/186020>
All reviewed patches have been landed. Closing bug.
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?
(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
Ok, I should give a try...
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.
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.