Add the duration attribute to MediaSource
Created attachment 160845 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 160845 [details] Patch Attachment 160845 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13646126 New failing tests: http/tests/media/media-source/video-media-source-duration-changed.html
Created attachment 160871 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Ah right, this needs a Chromium-side CL to land before the test will pass!
Comment on attachment 160845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160845&action=review > Source/WebCore/Modules/mediasource/MediaSource.cpp:73 > + if (m_readyState == closedKeyword()) > + return std::numeric_limits<float>::quiet_NaN(); > + > + return m_player->duration(); This can be just a ternary if. > Source/WebCore/Modules/mediasource/MediaSource.idl:47 > + attribute double duration > + setter raises (DOMException); Why u break line? > Source/WebCore/html/HTMLMediaElement.cpp:3260 > + if (now > dur) dur->duration. > LayoutTests/http/tests/media/media-source/media-source.js:325 > + if (!this.floatingPointEquals_(expected, videoTag.duration)) > + { Move brace to same line > LayoutTests/http/tests/media/media-source/media-source.js:329 > + { Ditto. > LayoutTests/http/tests/media/media-source/media-source.js:337 > + { Ditto. > LayoutTests/http/tests/media/media-source/media-source.js:342 > + { Ditto. > LayoutTests/http/tests/media/media-source/media-source.js:349 > + { Ditto. > LayoutTests/http/tests/media/media-source/video-media-source-duration-changed.html:127 > + { Ditto.
Comment on attachment 160845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160845&action=review > Source/WebCore/html/HTMLMediaElement.cpp:3261 > + if (now > dur) > + seek(dur, ignoredException); Why is this necessary? > LayoutTests/http/tests/media/media-source/media-source.js:317 > +MediaSourceTest.floatingPointEquals_ = function(expected, actual) { Move brace to new line (take that Dimitri). > LayoutTests/http/tests/media/media-source/video-media-source-duration-changed.html:11 > + function expectExceptionOnSetDuration(value, error) { Ditto. > LayoutTests/http/tests/media/media-source/video-media-source-duration-changed.html:35 > + if (!isNaN(mediaSource.duration)) { > + failTest("Unexpected duration value. Expected NaN got " + mediaSource.duration); > + } Braces are not necessary.
Comment on attachment 160845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160845&action=review >> Source/WebCore/Modules/mediasource/MediaSource.cpp:73 >> + return m_player->duration(); > > This can be just a ternary if. Done. >> Source/WebCore/Modules/mediasource/MediaSource.idl:47 >> + setter raises (DOMException); > > Why u break line? I was just following what Big Bro HTMLMediaElement.idl did :( Changed to one line! >> Source/WebCore/html/HTMLMediaElement.cpp:3260 >> + if (now > dur) > > dur->duration. Can't do that because then you get "float duration = duration()" which doesn't compile. Kept the same because "dur = duration()" is used in other areas of the class as well? Can change to something different if you'd like. >> Source/WebCore/html/HTMLMediaElement.cpp:3261 >> + seek(dur, ignoredException); > > Why is this necessary? Here I'm trying to follow the part of the spec regarding duration change: "If the duration is changed such that the current playback position ends up being greater than the time of the end of the media resource, then the user agent must also seek the to the time of the end of the media resource." (http://dev.w3.org/html5/spec/media-elements.html#durationChange, linked from http://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html#duration-change-algorithm) I wasn't very confident in this part of the change, so let me know if you'd prefer me to do this in a different way. >> LayoutTests/http/tests/media/media-source/media-source.js:317 >> +MediaSourceTest.floatingPointEquals_ = function(expected, actual) { > > Move brace to new line (take that Dimitri). :) done. >> LayoutTests/http/tests/media/media-source/media-source.js:325 >> + { > > Move brace to same line Removed braces instead. >> LayoutTests/http/tests/media/media-source/media-source.js:329 >> + { > > Ditto. Removed braces instead. >> LayoutTests/http/tests/media/media-source/media-source.js:337 >> + { > > Ditto. Removed braches instead. >> LayoutTests/http/tests/media/media-source/media-source.js:342 >> + { > > Ditto. Done. >> LayoutTests/http/tests/media/media-source/media-source.js:349 >> + { > > Ditto. Done. >> LayoutTests/http/tests/media/media-source/video-media-source-duration-changed.html:11 >> + function expectExceptionOnSetDuration(value, error) { > > Ditto. Done. >> LayoutTests/http/tests/media/media-source/video-media-source-duration-changed.html:35 >> + } > > Braces are not necessary. Fixed here and in a few other places where I had braces around a one-liner. >> LayoutTests/http/tests/media/media-source/video-media-source-duration-changed.html:127 >> + { > > Ditto. Done.
Created attachment 161299 [details] Patch
(In reply to comment #8) > (From update of attachment 160845 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160845&action=review > >> Source/WebCore/html/HTMLMediaElement.cpp:3261 > >> + seek(dur, ignoredException); > > > > Why is this necessary? > > Here I'm trying to follow the part of the spec regarding duration change: > "If the duration is changed such that the current playback position ends up being greater than the time of the end of the media resource, then the user agent must also seek the to the time of the end of the media resource." > (http://dev.w3.org/html5/spec/media-elements.html#durationChange, linked from http://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html#duration-change-algorithm) > > I wasn't very confident in this part of the change, so let me know if you'd prefer me to do this in a different way. > I see. This is a good change, but it should be done in a separate patch because it has nothing to do with the rest of these changes.
Created attachment 161336 [details] Patch
> I see. This is a good change, but it should be done in a separate patch because it has nothing to do with the rest of these changes. Good point. Reverted the HTMLMediaElement change and will add to a separate patch!
Comment on attachment 161336 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161336&action=review > Source/WebCore/ChangeLog:16 > + * Modules/mediasource/MediaSource.cpp: > + (WebCore::MediaSource::duration): > + (WebCore): > + (WebCore::MediaSource::setDuration): Nit: I think it is helpful to have comments about what changed in each function in a ChangeLog entry. > Source/WebCore/Modules/mediasource/MediaSource.cpp:71 > + return m_readyState == closedKeyword() ? > + std::numeric_limits<float>::quiet_NaN() : m_player->duration(); Nit: I don't think the line break aids readability here. > LayoutTests/http/tests/media/media-source/media-source.js:142 > + for (var index = 0; index < this.mediaSegments.length; index++) Nit: you use "index" here and "i" elsewhere. > LayoutTests/http/tests/media/media-source/media-source.js:321 > +MediaSourceTest.floatingPointEquals_ = function(expected, actual) > +{ > + var delta = Math.abs(actual - expected); > + return delta < 0.01; > +}; Why does the function name end with an underscore? What makes 0.01 an acceptable delta?
(In reply to comment #7) > > LayoutTests/http/tests/media/media-source/media-source.js:317 > > +MediaSourceTest.floatingPointEquals_ = function(expected, actual) { > > Move brace to new line (take that Dimitri). Nooooooo... (fades away)...ooo.....
Comment on attachment 161336 [details] Patch Attachment 161336 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13684579 New failing tests: http/tests/media/media-source/video-media-source-duration-changed.html
(In reply to comment #15) > (From update of attachment 161336 [details]) > Attachment 161336 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/13684579 > > New failing tests: > http/tests/media/media-source/video-media-source-duration-changed.html This is likely failing because this patch landed first (https://bugs.webkit.org/show_bug.cgi?id=95247). I think you just need to change your "new MediaSource()" to "new WebKitMediaSource()".
Created attachment 162248 [details] Patch
Created attachment 162251 [details] Patch
Comment on attachment 161336 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161336&action=review >> Source/WebCore/ChangeLog:16 >> + (WebCore::MediaSource::setDuration): > > Nit: I think it is helpful to have comments about what changed in each function in a ChangeLog entry. Done. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:71 >> + std::numeric_limits<float>::quiet_NaN() : m_player->duration(); > > Nit: I don't think the line break aids readability here. Line break removed. >> LayoutTests/http/tests/media/media-source/media-source.js:142 >> + for (var index = 0; index < this.mediaSegments.length; index++) > > Nit: you use "index" here and "i" elsewhere. Changed. >> LayoutTests/http/tests/media/media-source/media-source.js:321 >> +}; > > Why does the function name end with an underscore? > > What makes 0.01 an acceptable delta? > Why does the function name end with an underscore? To match the other "private" functions for MediaSourceTest, like runNext_. > What makes 0.01 an acceptable delta? Hmmm, thinking about this again, comparing against an arbitrary delta doesn't make sense. The manifest file presents timestamps rounded to 3 decimal places, but the actual file timestamps are of course unrounded, hence the discrepancy. Makes a lot more sense to just round the timestamps to 3 decimal places. Updated test to do that instead.
Comment on attachment 162251 [details] Patch Clearing flags on attachment: 162251 Committed r127675: <http://trac.webkit.org/changeset/127675>
All reviewed patches have been landed. Closing bug.