[MSE] http/tests/media/media-source/mediasource-buffered.html is flakey
Created attachment 235010 [details] Patch
Comment on attachment 235010 [details] Patch Clearing flags on attachment: 235010 Committed r171151: <http://trac.webkit.org/changeset/171151>
All reviewed patches have been landed. Closing bug.
Comment on attachment 235010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235010&action=review > Source/WebCore/Modules/mediasource/MediaSource.cpp:747 > +void MediaSource::sourceBufferDidChangeAcitveState(SourceBuffer*, bool) Noticed that this function name misspells Active. > Source/WebCore/Modules/mediasource/MediaSource.cpp:874 > + for (auto& sourceBuffer : *m_sourceBuffers) { > + if (sourceBuffer->active()) > + newList.append(sourceBuffer); > + } Regenerating the list every time seems like it will make things O(n^2) when they needn’t be. Is there a way to be lazy about this? Maybe O(n^2) is not a problem? > Source/WebCore/Modules/mediasource/MediaSource.cpp:875 > + m_activeSourceBuffers->swap(newList); Why swap? Can’t we just set it instead? Swapping is extra work. And with move semantics it should be easy to make a set function. > Source/WebCore/Modules/mediasource/SourceBufferList.cpp:78 > + if (!m_list.contains(sourceBuffer)) This O(n^2) algorithm seems like a bad idea, but maybe it’s OK. Worse, if this item is called every time a source buffer changes active state, activating all the source buffers makes this O(n^3). I worry that even with some relatively small numbers this could possibly bite us. > Source/WebCore/Modules/mediasource/SourceBufferList.h:61 > + void swap(Vector<RefPtr<SourceBuffer>>&); As I mentioned above, this should be: void set(Vector<RefPtr<SourceBuffer>>&&);
Comment on attachment 235010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235010&action=review >> Source/WebCore/Modules/mediasource/MediaSource.cpp:874 >> + } > > Regenerating the list every time seems like it will make things O(n^2) when they needn’t be. Is there a way to be lazy about this? Maybe O(n^2) is not a problem? Generally speaking, these entries only have 2 items in them, a video and audio track, and definitely no more than 10. And the list only needs to be regenerated when they become active or inactive (like when a user choses a different audio track). So regardless of the O(), this will be a fast operation. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:875 >> + m_activeSourceBuffers->swap(newList); > > Why swap? Can’t we just set it instead? Swapping is extra work. And with move semantics it should be easy to make a set function. Ah yes! I haven't internalized move semantics yet. >> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:78 >> + if (!m_list.contains(sourceBuffer)) > > This O(n^2) algorithm seems like a bad idea, but maybe it’s OK. > > Worse, if this item is called every time a source buffer changes active state, activating all the source buffers makes this O(n^3). I worry that even with some relatively small numbers this could possibly bite us. The other option here would be to have a SourceBufferList expose two interfaces: the master SourceBufferList, and a list of only the "active" SourceBuffers. This would move all the complexity to the accessing side, rather than the modification side. I'll open up a new bug to look into that alternative. (Which would remove the need for the set() function above.)