Allow SourceBufferIterator users to specify how many bytes to advance

RESOLVED FIXED in Firefox 50



2 years ago
2 years ago


(Reporter: seth, Assigned: seth)


Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)



(1 attachment, 1 obsolete attachment)



2 years ago
This is the last piece of the puzzle before we can implement yielding in bug 1284031: we need the ability to tell a SourceBufferIterator "only advance by this many bytes", and we need it to work efficiently.

In this case, we'll keep it efficient by having the SourceBufferIterator continue to grab as much data as it can from its owning SourceBuffer. It will then allow users to advance a few bytes at a time, if they so desire, through the data it has already grabbed. Only when it reaches the end of the data it has available does it need to talk to the owning SourceBuffer again to get more data. Since interacting with the SourceBuffer involves taking a lock, this greatly reduces the overhead required for advancing in many very small steps, which is what we want to support.

We want tests for this, obviously. They're included in the patch in bug 1286161, which adds a GTest suite for SourceBuffer (there wasn't one up to now).

Comment 1

2 years ago
Created attachment 8770045 [details] [diff] [review]
Allow SourceBufferIterator users to specify how many bytes to advance.

Here's the patch. Basically the SourceBufferIterator now keeps track of the
amount of data it actually got from the SourceBuffer most recently
(mAvailableLength) as well as the amount that the caller wanted to read on the
last call to Advance() (mNextReadLength). Calls to Advance() don't have to
interact with the owning SourceBuffer and take the lock as long as
|mAvailableLength| isn't exhausted.

SourceBufferIterator's code could be a little cleaner; it's a bit weird how
SourceBufferIterator calls into SourceBuffer which calls back into
SourceBufferIterator to call methods like SetReady(). But this bug probably
isn't the place to fix that, so I've stuck with that approach here.
Attachment #8770045 - Flags: review?(edwin)

Comment 2

2 years ago
Created attachment 8770703 [details] [diff] [review]
Allow SourceBufferIterator users to specify how many bytes to advance.

Thanks for the review!

This updated patch contains a small fix that makes sure that we don't read a new
chunk from the SourceBuffer if a zero-byte read was requested, even if we're at
the end of the previous chunk. I've updated the SourceBuffer GTest suite to
include a test for this.


2 years ago
Attachment #8770045 - Attachment is obsolete: true

Comment 3

2 years ago
Pushed by
Allow SourceBufferIterator users to specify how many bytes to advance. r=edwin

Comment 4

2 years ago
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.