Bug 134949 – [MSE] http/tests/media/media-source/mediasource-buffered.html is flakey

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[MSE] http/tests/media/media-source/mediasource-buffered.html is flakey

[Jer Noble]

Created attachment 235010 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 235010 [details]
Patch

Clearing flags on attachment: 235010

Committed r171151: <http://trac.webkit.org/changeset/171151>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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>>&&);

[Jer Noble]

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