[MSE] YouTube videos fail to play
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.
(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!
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
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
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
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
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
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
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
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
Created attachment 225539 [details] Patch
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.
(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.
Committed r164999: <http://trac.webkit.org/changeset/164999>