Bug 93406 – Cap the number of SourceBuffers that may be added to a MediaSource

• Component: Media
• Status: RESOLVED FIXED

[Anna Cavender]

Make sure that no more SourceBuffer objects are added to a MediaSource than can fit into the sourceBuffers SourceBufferList (internally stored as a Vector).

[Anna Cavender]

Created attachment 157282 [details]
Patch

[Eric Carlson]

Comment on attachment 157282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157282&action=review

I think enough should change here that I am marking it r- for now.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:101
> +    // Ensure a unique id. Until m_nextSourceBufferId wraps around (very unlikely),
> +    // this loop will exit after one check. If m_nextSourceBufferId does wrap,
> +    // this loop may run as many times as the size of m_sourceBuffers, but in most
> +    // cases that should be less than 10. We may want to investigate a more
> +    // efficient approach if many more SourceBuffers are allowed in the future.
> +    size_t oldSourceBufferId = m_nextSourceBufferId;

This is specialized and long enough that is should probably be in its own function.

Actually, it might make sense to make this a function of SourceBufferList.cpp instead of having some of the logic here and some there.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:102
> +    while (m_sourceBuffers->contains(String::number(m_nextSourceBufferId++))) {

This seems wrong. You are searching for m_nextSourceBufferId, but you use (m_nextSourceBufferId+1) for the new buffer ID even if m_nextSourceBufferId doesn't exist in the vector.

> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:89
> +bool SourceBufferList::contains(const String& id) const
> +{
> +    for (size_t i = 0; i < m_list.size(); ++i) {
> +        if (m_list[i]->id() == id)
> +            return true;
> +    }
> +    return false;
> +}

Since you have to implement this, why not pass the ID as a size_t?

[Anna Cavender]

Comment on attachment 157282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157282&action=review

Thanks Eric, revised patch on the way.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:101
>> +    size_t oldSourceBufferId = m_nextSourceBufferId;
> 
> This is specialized and long enough that is should probably be in its own function.
> 
> Actually, it might make sense to make this a function of SourceBufferList.cpp instead of having some of the logic here and some there.

OK, I'll give that a go.

>> Source/WebCore/Modules/mediasource/MediaSource.cpp:102
>> +    while (m_sourceBuffers->contains(String::number(m_nextSourceBufferId++))) {
> 
> This seems wrong. You are searching for m_nextSourceBufferId, but you use (m_nextSourceBufferId+1) for the new buffer ID even if m_nextSourceBufferId doesn't exist in the vector.

Good catch, thanks.

>> Source/WebCore/Modules/mediasource/SourceBufferList.cpp:89
>> +}
> 
> Since you have to implement this, why not pass the ID as a size_t?

Done.

[Anna Cavender]

Created attachment 157325 [details]
revisions based on review

[WebKit Review Bot]

Comment on attachment 157325 [details]
revisions based on review

Rejecting attachment 157325 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
code: 9
s/ChangeLog
Auto-merging LayoutTests/platform/chromium/TestExpectations
Failed to merge in the changes.
Patch failed at 0001 nested fragment parser test slow on Android

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/13453914

[Anna Cavender]

Created attachment 157523 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 157523 [details]
Patch for landing

Rejecting attachment 157523 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Websites/webkit.org/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13473123

[Anna Cavender]

Created attachment 157571 [details]
no diff

Bugger, commit queue ate my patch and lost the r+.

[Anna Cavender]

Committed r125424: <http://trac.webkit.org/changeset/125424>