Bug 134949 - [MSE] http/tests/media/media-source/mediasource-buffered.html is flakey
Summary: [MSE] http/tests/media/media-source/mediasource-buffered.html is flakey
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks: 135118
  Show dependency treegraph
 
Reported: 2014-07-15 16:03 PDT by Jer Noble
Modified: 2014-07-21 09:52 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.74 KB, patch)
2014-07-16 11:28 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2014-07-15 16:03:32 PDT
[MSE] http/tests/media/media-source/mediasource-buffered.html is flakey
Comment 1 Jer Noble 2014-07-16 11:28:14 PDT
Created attachment 235010 [details]
Patch
Comment 2 WebKit Commit Bot 2014-07-16 13:51:40 PDT
Comment on attachment 235010 [details]
Patch

Clearing flags on attachment: 235010

Committed r171151: <http://trac.webkit.org/changeset/171151>
Comment 3 WebKit Commit Bot 2014-07-16 13:51:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Darin Adler 2014-07-19 22:56:04 PDT
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 5 Jer Noble 2014-07-21 09:50:22 PDT
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.)