Bug 129525 – [MSE] YouTube videos fail to play

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[MSE] YouTube videos fail to play

[Jer Noble]

Created attachment 225516 [details]
Patch

Uploading work-in-progress patch. The view controller presentViewController:animated:completion: method is non-cancellable, so we should probably look into adding a delegate which can exit full screen only once the animation into full screen completes.

[Jer Noble]

(In reply to comment #1)
> Created an attachment (id=225516) [details]
> Work in progress
> 
> Uploading work-in-progress patch. The view controller presentViewController:animated:completion: method is non-cancellable, so we should probably look into adding a delegate which can exit full screen only once the animation into full screen completes.

Ignore this comment. I ran `!webkit-patch` from the wrong  window.  The patch is good though!

[Build Bot]

Comment on attachment 225516 [details]
Patch

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

New failing tests:
media/media-source/media-source-monitor-source-buffers.html
media/media-source/media-source-addsourcebuffer.html

[Build Bot]

Created attachment 225521 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 225516 [details]
Patch

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

New failing tests:
media/media-source/media-source-monitor-source-buffers.html
media/media-source/media-source-addsourcebuffer.html

[Build Bot]

Created attachment 225522 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 225516 [details]
Patch

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

New failing tests:
media/media-source/media-source-monitor-source-buffers.html
media/media-source/media-source-addsourcebuffer.html

[Build Bot]

Created attachment 225529 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 225516 [details]
Patch

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

New failing tests:
media/media-source/media-source-monitor-source-buffers.html
media/media-source/media-source-addsourcebuffer.html

[Build Bot]

Created attachment 225533 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Jer Noble]

Created attachment 225539 [details]
Patch

[Darin Adler]

Comment on attachment 225539 [details]
Patch

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

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1185
> +    if (!m_bufferedSinceLastMonitor)
> +        return;

Why do this after computing the values above? I suggest moving this line of code up to the top of the function.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1203
> +    double nearest = m_buffered->nearest(m_source->currentTime());
> +    double currentTime = m_source->currentTime();

Lets evaluate this in the reverse order so we can use currentTime to compute nearest. Seems more logical what way too. In fact, I probably would not put nearest into a local variable at all.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1204
> +    return abs(nearest - currentTime) <= CurrentTimeFudgeFactor;

This is wrong. Calling abs here converts the result to an integer. We want to call fabs. This also means this code wasn’t working before; instead of 1/24 second our threshold was more like 1 second. Is there some way we can test to make sure it is really working?

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1214
> +    return m_buffered->end(found, IGNORE_EXCEPTION) - currentTime > CurrentTimeFudgeFactor;

Isn’t ASSERT_NO_EXCEPTION what we want, rather than IGNORE_EXCEPTION? And further, Antti has been encouraging us not to use the public DOM functions in internal WebKit code in this way, so it would be nice to avoid it entirely.

> Source/WebCore/Modules/mediasource/SourceBuffer.h:88
> +    bool hasCurrentTime();
> +    bool hasFutureTime();
> +    bool canPlayThrough();

Why aren’t any of these functions const? I guess canPlayThrough has some serious side effects. But the other two don’t.

[Jer Noble]

(In reply to comment #12)
> (From update of attachment 225539 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225539&action=review
> 
> > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1185
> > +    if (!m_bufferedSinceLastMonitor)
> > +        return;
> 
> Why do this after computing the values above? I suggest moving this line of code up to the top of the function.

Done.

> > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1203
> > +    double nearest = m_buffered->nearest(m_source->currentTime());
> > +    double currentTime = m_source->currentTime();
> 
> Lets evaluate this in the reverse order so we can use currentTime to compute nearest. Seems more logical what way too. In fact, I probably would not put nearest into a local variable at all.

Ok, changed.

> > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1204
> > +    return abs(nearest - currentTime) <= CurrentTimeFudgeFactor;
> 
> This is wrong. Calling abs here converts the result to an integer. We want to call fabs. This also means this code wasn’t working before; instead of 1/24 second our threshold was more like 1 second. Is there some way we can test to make sure it is really working?

Whoops.  Yeah, I think this should be testable.  I'll add this.

> > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1214
> > +    return m_buffered->end(found, IGNORE_EXCEPTION) - currentTime > CurrentTimeFudgeFactor;
> 
> Isn’t ASSERT_NO_EXCEPTION what we want, rather than IGNORE_EXCEPTION? And further, Antti has been encouraging us not to use the public DOM functions in internal WebKit code in this way, so it would be nice to avoid it entirely.

Eric just made a change where TimeRanges are just wrappers around PlatformTimeRanges.  I'll use those methods.

> > Source/WebCore/Modules/mediasource/SourceBuffer.h:88
> > +    bool hasCurrentTime();
> > +    bool hasFutureTime();
> > +    bool canPlayThrough();
> 
> Why aren’t any of these functions const? I guess canPlayThrough has some serious side effects. But the other two don’t.

Sure, I'll make the has* methods const.

[Jer Noble]

Committed r164999: <http://trac.webkit.org/changeset/164999>