Add the duration attribute to MediaSource

• Component: New Bugs
• Status: RESOLVED FIXED

[Victoria Kirst]

Add the duration attribute to MediaSource

[Victoria Kirst]

Created attachment 160845 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Victoria Kirst]

Ah right, this needs a Chromium-side CL to land before the test will pass!

[Dimitri Glazkov (Google)]

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.

[Eric Carlson]

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.

[Victoria Kirst]

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.

[Victoria Kirst]

Created attachment 161299 [details]
Patch

[Eric Carlson]

(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.

[Victoria Kirst]

Created attachment 161336 [details]
Patch

[Victoria Kirst]

> 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!

[Eric Carlson]

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?

[Dimitri Glazkov (Google)]

(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.....

[WebKit Review Bot]

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

[Aaron Colwell]

(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()".

[Victoria Kirst]

Created attachment 162248 [details]
Patch

[Victoria Kirst]

Created attachment 162251 [details]
Patch

[Victoria Kirst]

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.

[WebKit Review Bot]

Comment on attachment 162251 [details]
Patch

Clearing flags on attachment: 162251

Committed r127675: <http://trac.webkit.org/changeset/127675>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.