Bug 90819

Created attachment 152320 [details] Patch for discussion/reference only - not intended for review/commit

Comment on attachment 152320 [details] Patch for discussion/reference only - not intended for review/commit I have a few questions inline. Also, what is the best way handle the webkit- prefix for the new objects? View in context: https://bugs.webkit.org/attachment.cgi?id=152320&action=review > Source/WebCore/Modules/mediasource/MediaSource.h:100 > + MediaPlayer* m_player; // <-- should this be a RefPtr? Should we go ahead and make MediaPlayer RefCounted so that this can be a RefPtr (and also make HTMLMediaElement's m_player a RefPtr instead of an OwnPtr)? > Source/WebCore/WebCore.xcodeproj/project.pbxproj:13971 > + B1EF1A3515A3A1F4001682A6 /* MediaSource.cpp */, I have never figured out how to properly add new files to xcode project files and I'm sure this is not the right way. Pointers please! > Source/WebCore/html/HTMLMediaElement.cpp:4287 > + m_mediaSource->setMediaPlayer(m_player.get()); This seems a little suspect. Other ideas how to handle this case? > Source/WebCore/html/HTMLMediaElement.h:599 > + RefPtr<MediaSource> m_mediaSource; Should this be OwnPtr? > LayoutTests/http/tests/media/media-source/media-source.js:280 > + if (videoTag.networkState != HTMLMediaElement.NETWORK_LOADING && This check was added to ensure that the video element is in an appropriate state when the webkitsoureopen event is fired. Because webkitsourceopen and loadstart are now on two different objects, we can't guarantee the order with which they will be fired. So, I've removed waitForEvent(loadstart) from all of the media-source LayoutTests and added this additional check here.

Created attachment 152322 [details] Updating to ToT

Comment on attachment 152322 [details] Updating to ToT View in context: https://bugs.webkit.org/attachment.cgi?id=152322&action=review Here are my initial comments on the C++ changes. Overall I think this is looking pretty good. I'll review LayoutTest changes next week. > Source/WebCore/Modules/mediasource/MediaSource.cpp:61 > + return SourceBufferList::create(scriptExecutionContext()).get(); Why create a new SourceBufferList in this case instead of just returning m_sourceBuffers? Say someone wants to attach event handlers to the list before the source is opened. I think if they try that they'll never get the events when the source is opened and new SourceBuffers are added. > Source/WebCore/Modules/mediasource/MediaSource.cpp:115 > + if (!m_sourceBuffers->remove(buffer)) { You should probably add a clear() method on SourceBuffer to clear the reference to this MediaSource so that it can't be used once it is removed. > Source/WebCore/Modules/mediasource/MediaSource.idl:45 > + // readonly attribute SourceBufferList activeSourceBuffers; For now we could assume that if the MediaPlayer accepts the new source it is considered active. Once we have the ability to select tracks, we could add the infrastructure to signal when SourceBuffers should be added/removed from this list. > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:44 > + , m_type(type) It doesn't look like you use m_type anywhere so I think it can be removed. > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:79 > + createAndFireEvent(eventNames().webkitremovesourcebufferEvent); This won't cause JavaScript to run while in this loop right? > Source/WebCore/html/HTMLMediaElement.cpp:149 > static const char* mediaSourceURLProtocol = "x-media-source"; I think this patch should also rip out the old code. I don't think there is a good reason to keep both versions of the API in place at the same time. > Source/WebCore/html/HTMLMediaElement.cpp:937 > + m_mediaSource = adoptRef(MediaSourceRegistry::registry().lookupMediaSource(url.string())); If m_mediaSource is already set, will this cause a closed event to fire on the old object? At a minimum I think you'll need to have the old object release its reference to MediaPlayer so the old source can't mess with playback.

Comment on attachment 152322 [details] Updating to ToT Thanks for the speedy feedback!! View in context: https://bugs.webkit.org/attachment.cgi?id=152322&action=review >> Source/WebCore/Modules/mediasource/MediaSource.cpp:61 >> + return SourceBufferList::create(scriptExecutionContext()).get(); > > Why create a new SourceBufferList in this case instead of just returning m_sourceBuffers? Say someone wants to attach event handlers to the list before the source is opened. I think if they try that they'll never get the events when the source is opened and new SourceBuffers are added. Good call. Fixed. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:115 >> + if (!m_sourceBuffers->remove(buffer)) { > > You should probably add a clear() method on SourceBuffer to clear the reference to this MediaSource so that it can't be used once it is removed. Very good point. I originally thought the RefPtr would take care of that but forgot that JS could have a handle on the SourceBuffer object. Thanks! I put the call to clear in SourceBufferList::remove() >> Source/WebCore/Modules/mediasource/MediaSource.idl:45 >> + // readonly attribute SourceBufferList activeSourceBuffers; > > For now we could assume that if the MediaPlayer accepts the new source it is considered active. Once we have the ability to select tracks, we could add the infrastructure to signal when SourceBuffers should be added/removed from this list. OK. I'll make activeSourceBuffers just return sourceBuffers for now. >> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:44 >> + , m_type(type) > > It doesn't look like you use m_type anywhere so I think it can be removed. Done. >> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:79 >> + createAndFireEvent(eventNames().webkitremovesourcebufferEvent); > > This won't cause JavaScript to run while in this loop right? I'm not sure what you mean. The events should be dispatched one by one during the loop. >> Source/WebCore/html/HTMLMediaElement.cpp:149 >> static const char* mediaSourceURLProtocol = "x-media-source"; > > I think this patch should also rip out the old code. I don't think there is a good reason to keep both versions of the API in place at the same time. OK. Done. >> Source/WebCore/html/HTMLMediaElement.cpp:937 >> + m_mediaSource = adoptRef(MediaSourceRegistry::registry().lookupMediaSource(url.string())); > > If m_mediaSource is already set, will this cause a closed event to fire on the old object? At a minimum I think you'll need to have the old object release its reference to MediaPlayer so the old source can't mess with playback. Good thinking. Added a check here and set an already existing m_mediaSource to "closed", which will release its reference to the MediaPlayer.

Created attachment 152776 [details] revised with acolwell suggestions

Comment on attachment 152776 [details] revised with acolwell suggestions Attachment 152776 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13279185

Comment on attachment 152776 [details] revised with acolwell suggestions Attachment 152776 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13275207

Comment on attachment 152776 [details] revised with acolwell suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=152776&action=review This is looking pretty good to me. eric.carlson probably needs to start chiming in at this point. > Source/WebCore/Modules/mediasource/MediaSource.cpp:66 > + return m_sourceBuffers.get(); Eventhough these lists will contain the same objects right now, I think we should still have different list objects so that you can't add an event handler to sourceBuffers() and remove it from activeSourceBuffers(). I believe that would be possible with this code. > Source/WebCore/Modules/mediasource/MediaSource.cpp:96 > + RefPtr<SourceBuffer> buffer = SourceBuffer::create(String::number(++m_nextSourceBufferId)); Could this be moved into the MediaPlayer::Ok case and the MediaSource be passed to SourceBuffer::create() so we don't need setMediaSource()? > Source/WebCore/Modules/mediasource/MediaSource.cpp:119 > + I think you need a return 0 here. > Source/WebCore/Modules/mediasource/MediaSource.idl:57 > + attribute EventListener onwebkitsourceopen; This and the 2 below should be removed. onwebkitsourceXXX attributes were removed from the spec. I think the DEFINE_ATTRIBUTE_EVENT_LISTENER() can be removed from the .h file as well. > Source/WebCore/Modules/mediasource/SourceBufferList.h:60 > + DEFINE_ATTRIBUTE_EVENT_LISTENER(webkitaddsourcebuffer); I believe these can be removed since the onxxx attributes were removed from the spec. > Source/WebCore/Modules/mediasource/SourceBufferList.idl:43 > + attribute EventListener onwebkitaddsourcebuffer; please remove. > Source/WebCore/html/HTMLMediaElement.cpp:933 > + mediaLoadingFailed(MediaPlayer::FormatError); I don't think this is quite right. It is possible to have blob: URLs that are not MediaSource related. WebRTC local camera preview is one example. I believe the correct thing to do is just drop this else block. You might want to test with other types of blob: URLs just to make sure. > LayoutTests/http/tests/media/media-source/media-source.js:3 > +var sourceBuffer = null; I think sourceBuffer should be a member of SegmentHelper and all references converted to this.sourceBuffer. I also think you should be able to remove this.SourceID now since the API doesn't use source IDs. > LayoutTests/http/tests/media/media-source/media-source.js:128 > this.videoTag.webkitSourceAddId(this.SourceID, this.segmentInfo.type); Can this addSourceId() method go away? > LayoutTests/http/tests/media/media-source/media-source.js:148 > + if (!sourceBuffer.append) { I think this should be !this.sourceBuffer since this check is just here to make sure that the MediaSource API is present. sourceBuffer should be null if that was the case. > LayoutTests/http/tests/media/media-source/video-media-source-abort.html:20 > + sourceBuffer.append(partialInitSegment); I think calls like this should be converted to segmentHelper.sourceBuffer.append(). same goes for abort()

Comment on attachment 152776 [details] revised with acolwell suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=152776&action=review In my humble opinion, there are a little nits. Please take a look them. > Source/WebCore/ChangeLog:3 > + Convert Media Source to new object-oriented API Bug title is different from this bug. > Source/WebCore/GNUmakefile.list.am:1173 > + Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp \ Nit : wrong indentation. > Source/WebCore/Modules/mediasource/MediaSource.cpp:236 > + if (!m_player->sourceAppend(id, data->data(), data->length())) { Nit : wrong indentation. > Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp:2 > + * Copyright (C) 2011 Google Inc. All rights reserved. 2011 -> 2012 ? > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:35 > +#include "SourceBuffer.h" It seems header file of implementation file is just placed below #include "config.h" generally. > Source/WebCore/Modules/mediasource/SourceBuffer.h:67 > + SourceBuffer(const String& id); Don't you need to use *explicit* keyword as well ? > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:35 > +#include "SourceBufferList.h" ditto. > Source/WebCore/Modules/mediasource/SourceBufferList.h:35 > + Nit : There is an unneeded line. > Source/WebCore/dom/EventTarget.h:60 > + class MediaSource; Nit : wrong alphabetical order.

Comment on attachment 152776 [details] revised with acolwell suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=152776&action=review > Source/WebCore/ChangeLog:22 > + New Objects: > + * Modules/mediasource/MediaSource.cpp: Added. I don't think it is necessary to list the method names of newly added files. > Source/WebCore/Modules/mediasource/MediaSource.cpp:65 > + // TODO(): support track selection We usually use "FIXME:". I think it is also worth filing a bug and listing the number in the comment. > Source/WebCore/Modules/mediasource/MediaSource.cpp:72 > + // 1. If type is null or an empty then throw an INVALID_ACCESS_ERR exception and > + // abort these steps. If this is a comment from the spec, please make that clear by including a link to the appropriate section of the spec. Here and throughout the rest of the patch. > Source/WebCore/Modules/mediasource/MediaSource.cpp:149 > + // 4. Remove track information from audioTracks, videoTracks, and textTracks for all tracks associated with sourceBuffer and fire a simple event named change on the modified lists. > + > + // 5. If sourceBuffer is in activeSourceBuffers, then remove it from that list and fire a removesourcebuffer event on that object. Please include FIXMEs with bug numbers for unimplemented steps. > Source/WebCore/Modules/mediasource/MediaSource.cpp:157 > +void MediaSource::setState(const String& state) Why "setState" instead of "setReadyState"? > Source/WebCore/Modules/mediasource/MediaSource.cpp:164 > + String oldState = m_readyState; > + m_readyState = state; > + > + if (m_readyState == oldState) > + return; Nit: why update m_readyState if it does not change? > Source/WebCore/Modules/mediasource/MediaSource.cpp:169 > + dispatchEvent(Event::create(eventNames().webkitsourcecloseEvent, false, true)); Does the spec really call for synchronous event dispatch? If so, you are going to have to add a bunch of code to guard against bad things happening when an event listener does something causes reentrancy. > Source/WebCore/Modules/mediasource/MediaSource.idl:50 > + //enum State { "closed", "open", "ended" }; > + //readonly attribute State readyState; Why are these commented out? > Source/WebCore/Modules/mediasource/MediaSource.idl:53 > + // enum EndOfStreamError { "network", "decode" }; Ditto. > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:81 > + for (size_t i = 0; i < m_list.size(); ++i) > + createAndFireEvent(eventNames().webkitremovesourcebufferEvent); > + m_list.clear(); Is there any reason to not call In SourceBufferList::remove? If so, it seems like the behavior here should at least be equivalent to what it does: remove the buffer from the list, call buffer->clear(), and then fire the event. Currently this will fire the 'removesourcebuffer' events while the buffers are still in the list. > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:89 > + EventTarget::dispatchEvent(event); Synchronous dispatch? > Source/WebCore/html/DOMURL.cpp:68 > + // Since WebWorkers cannot obtain MediaSource objects, we should be on the main thread. > + ASSERT(isMainThread()); Shouldn't this be the first thing checked? > Source/WebCore/html/HTMLMediaElement.cpp:922 > + // If this is a media source URL, fetch the media source from the blob url. This comment just describes what the code does. It should be removed.

Comment on attachment 152776 [details] revised with acolwell suggestions Thanks everyone for the feedback! I'll upload a patch to address your suggestions, just for your reference, but I'll be splitting this patch into 3 smaller ones coming up shortly. View in context: https://bugs.webkit.org/attachment.cgi?id=152776&action=review >> Source/WebCore/ChangeLog:3 >> + Convert Media Source to new object-oriented API > > Bug title is different from this bug. Done. >> Source/WebCore/ChangeLog:22 >> + * Modules/mediasource/MediaSource.cpp: Added. > > I don't think it is necessary to list the method names of newly added files. Done. >> Source/WebCore/GNUmakefile.list.am:1173 >> + Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp \ > > Nit : wrong indentation. Done. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:65 >> + // TODO(): support track selection > > We usually use "FIXME:". I think it is also worth filing a bug and listing the number in the comment. Done. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:66 >> + return m_sourceBuffers.get(); > > Eventhough these lists will contain the same objects right now, I think we should still have different list objects so that you can't add an event handler to sourceBuffers() and remove it from activeSourceBuffers(). I believe that would be possible with this code. Done. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:72 >> + // abort these steps. > > If this is a comment from the spec, please make that clear by including a link to the appropriate section of the spec. Here and throughout the rest of the patch. Done. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:96 >> + RefPtr<SourceBuffer> buffer = SourceBuffer::create(String::number(++m_nextSourceBufferId)); > > Could this be moved into the MediaPlayer::Ok case and the MediaSource be passed to SourceBuffer::create() so we don't need setMediaSource()? Sure. Done. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:119 >> + > > I think you need a return 0 here. Done. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:149 >> + // 5. If sourceBuffer is in activeSourceBuffers, then remove it from that list and fire a removesourcebuffer event on that object. > > Please include FIXMEs with bug numbers for unimplemented steps. Done. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:157 >> +void MediaSource::setState(const String& state) > > Why "setState" instead of "setReadyState"? Done. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:164 >> + return; > > Nit: why update m_readyState if it does not change? Done. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:169 >> + dispatchEvent(Event::create(eventNames().webkitsourcecloseEvent, false, true)); > > Does the spec really call for synchronous event dispatch? If so, you are going to have to add a bunch of code to guard against bad things happening when an event listener does something causes reentrancy. No, that was a mistake. I will change all dispatching to be non-cancelable. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:236 >> + if (!m_player->sourceAppend(id, data->data(), data->length())) { > > Nit : wrong indentation. Done. >> Source/WebCore/Modules/mediasource/MediaSource.idl:50 >> + //readonly attribute State readyState; > > Why are these commented out? I just wanted to document what is in the spec even though it won't appear in the IDL. I'll remove them. >> Source/WebCore/Modules/mediasource/MediaSource.idl:53 >> + // enum EndOfStreamError { "network", "decode" }; > > Ditto. Ditto. >> Source/WebCore/Modules/mediasource/MediaSource.idl:57 >> + attribute EventListener onwebkitsourceopen; > > This and the 2 below should be removed. onwebkitsourceXXX attributes were removed from the spec. I think the DEFINE_ATTRIBUTE_EVENT_LISTENER() can be removed from the .h file as well. Oops thanks. Done. >> Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp:2 >> + * Copyright (C) 2011 Google Inc. All rights reserved. > > 2011 -> 2012 ? Done. >> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:35 >> +#include "SourceBuffer.h" > > It seems header file of implementation file is just placed below #include "config.h" generally. Done. >> Source/WebCore/Modules/mediasource/SourceBuffer.h:67 >> + SourceBuffer(const String& id); > > Don't you need to use *explicit* keyword as well ? Yes, but I've added a another parameter to the constructor, so not needed anymore. >> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:35 >> +#include "SourceBufferList.h" > > ditto. done. >> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:81 >> + m_list.clear(); > > Is there any reason to not call In SourceBufferList::remove? If so, it seems like the behavior here should at least be equivalent to what it does: remove the buffer from the list, call buffer->clear(), and then fire the event. Currently this will fire the 'removesourcebuffer' events while the buffers are still in the list. Good catch, calling remove instead. >> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:89 >> + EventTarget::dispatchEvent(event); > > Synchronous dispatch? I think I've addressed this by making the event non-cancelable. Is that correct? >> Source/WebCore/Modules/mediasource/SourceBufferList.h:35 >> + > > Nit : There is an unneeded line. Done. >> Source/WebCore/Modules/mediasource/SourceBufferList.h:60 >> + DEFINE_ATTRIBUTE_EVENT_LISTENER(webkitaddsourcebuffer); > > I believe these can be removed since the onxxx attributes were removed from the spec. Done. >> Source/WebCore/Modules/mediasource/SourceBufferList.idl:43 >> + attribute EventListener onwebkitaddsourcebuffer; > > please remove. Done. >> Source/WebCore/dom/EventTarget.h:60 >> + class MediaSource; > > Nit : wrong alphabetical order. Done. >> Source/WebCore/html/DOMURL.cpp:68 >> + ASSERT(isMainThread()); > > Shouldn't this be the first thing checked? Sure. Done. >> Source/WebCore/html/HTMLMediaElement.cpp:922 >> + // If this is a media source URL, fetch the media source from the blob url. > > This comment just describes what the code does. It should be removed. Gone. >> Source/WebCore/html/HTMLMediaElement.cpp:933 >> + mediaLoadingFailed(MediaPlayer::FormatError); > > I don't think this is quite right. It is possible to have blob: URLs that are not MediaSource related. WebRTC local camera preview is one example. I believe the correct thing to do is just drop this else block. You might want to test with other types of blob: URLs just to make sure. Good point, thanks. >> LayoutTests/http/tests/media/media-source/media-source.js:3 >> +var sourceBuffer = null; > > I think sourceBuffer should be a member of SegmentHelper and all references converted to this.sourceBuffer. I also think you should be able to remove this.SourceID now since the API doesn't use source IDs. Done. >> LayoutTests/http/tests/media/media-source/media-source.js:128 >> this.videoTag.webkitSourceAddId(this.SourceID, this.segmentInfo.type); > > Can this addSourceId() method go away? Yep. Gone. >> LayoutTests/http/tests/media/media-source/media-source.js:148 >> + if (!sourceBuffer.append) { > > I think this should be !this.sourceBuffer since this check is just here to make sure that the MediaSource API is present. sourceBuffer should be null if that was the case. Done. >> LayoutTests/http/tests/media/media-source/video-media-source-abort.html:20 >> + sourceBuffer.append(partialInitSegment); > > I think calls like this should be converted to segmentHelper.sourceBuffer.append(). same goes for abort() Done.

Created attachment 153313 [details] update based on feedback

Comment on attachment 153313 [details] update based on feedback Attachment 153313 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13277940

Comment on attachment 153313 [details] update based on feedback Attachment 153313 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13280945 New failing tests: http/tests/media/media-source/video-media-source-objects.html

Created attachment 153335 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 153345 [details] win build files and layout test expectations

Comment on attachment 153345 [details] win build files and layout test expectations Attachment 153345 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13284990

Comment on attachment 153345 [details] win build files and layout test expectations Attachment 153345 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13272986 New failing tests: http/tests/messaging/cross-domain-message-event-dispatch.html

Created attachment 153366 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 153368 [details] another try at windows build files

Comment on attachment 153368 [details] another try at windows build files Attachment 153368 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13304108

Created attachment 153536 [details] fix typo in vcproj file - using bots to check windows build

Created attachment 153550 [details] testing windows build lets see if this one applies