WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
93406
Cap the number of SourceBuffers that may be added to a MediaSource
https://bugs.webkit.org/show_bug.cgi?id=93406
Summary
Cap the number of SourceBuffers that may be added to a MediaSource
Anna Cavender
Reported
2012-08-07 16:01:54 PDT
Make sure that no more SourceBuffer objects are added to a MediaSource than can fit into the sourceBuffers SourceBufferList (internally stored as a Vector).
Attachments
Patch
(4.88 KB, patch)
2012-08-08 13:44 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
revisions based on review
(6.47 KB, patch)
2012-08-08 16:16 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
Patch for landing
(6.46 KB, patch)
2012-08-09 13:17 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
no diff
(6.42 KB, patch)
2012-08-09 16:05 PDT
,
Anna Cavender
eric.carlson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Anna Cavender
Comment 1
2012-08-08 13:44:15 PDT
Created
attachment 157282
[details]
Patch
Eric Carlson
Comment 2
2012-08-08 14:02:55 PDT
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 3
2012-08-08 16:10:50 PDT
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
Comment 4
2012-08-08 16:16:19 PDT
Created
attachment 157325
[details]
revisions based on review
WebKit Review Bot
Comment 5
2012-08-08 22:33:58 PDT
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
Comment 6
2012-08-09 13:17:24 PDT
Created
attachment 157523
[details]
Patch for landing
WebKit Review Bot
Comment 7
2012-08-09 15:38:52 PDT
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
Comment 8
2012-08-09 16:05:55 PDT
Created
attachment 157571
[details]
no diff Bugger, commit queue ate my patch and lost the r+.
Anna Cavender
Comment 9
2012-08-13 09:46:15 PDT
Committed
r125424
: <
http://trac.webkit.org/changeset/125424
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug