Bug 1411504. P7 - don't change mChannelOffset in MediaCache::Update().
This is a fix to P3.
Since seek is performed asynchronously by CacheClientSeek(), it is possible
for OnStopRequest() to come before Seek(). Changing mChannelOffset will
cause MediaCacheStream::NotifyDataEnded() to update mStreamLength incorrectly.
mChannelOffset should only be changed in response to channel activities such
as NotifyDataStarted() and NotifyDataReceived().
However, if MediaCache::Update() calls CacheClientSeek() without updating
mChannelOffset, next Update() might make a wrong decision and call CacheClientSeek()
again which is bad. So we add a member mSeekTarget to track if there is a pending
seek on which the stream reading decisions will be made.
MozReview-Commit-ID: VWP0vdlEYM
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -1313,47 +1313,53 @@ MediaCache::Update()
actions.AppendElement(StreamAction{});
MediaCacheStream* stream = mStreams[i];
if (stream->mClosed) {
LOG("Stream %p closed", stream);
continue;
}
+ // We make decisions based on mSeekTarget when there is a pending seek.
+ // Otherwise we will keep issuing seek requests until mChannelOffset
+ // is changed by NotifyDataStarted() which is bad.
+ int64_t channelOffset =
+ stream->mSeekTarget != -1 ? stream->mSeekTarget : stream->mChannelOffset;
+
// Figure out where we should be reading from. It's the first
// uncached byte after the current mStreamOffset.
int64_t dataOffset = stream->GetCachedDataEndInternal(stream->mStreamOffset);
MOZ_ASSERT(dataOffset >= 0);
// Compute where we'd actually seek to to read at readOffset
int64_t desiredOffset = dataOffset;
if (stream->mIsTransportSeekable) {
- if (desiredOffset > stream->mChannelOffset &&
- desiredOffset <= stream->mChannelOffset + SEEK_VS_READ_THRESHOLD) {
+ if (desiredOffset > channelOffset &&
+ desiredOffset <= channelOffset + SEEK_VS_READ_THRESHOLD) {
// Assume it's more efficient to just keep reading up to the
// desired position instead of trying to seek
- desiredOffset = stream->mChannelOffset;
+ desiredOffset = channelOffset;
}
} else {
// We can't seek directly to the desired offset...
- if (stream->mChannelOffset > desiredOffset) {
+ if (channelOffset > desiredOffset) {
// Reading forward won't get us anywhere, we need to go backwards.
// Seek back to 0 (the client will reopen the stream) and then
// read forward.
NS_WARNING("Can't seek backwards, so seeking to 0");
desiredOffset = 0;
// Flush cached blocks out, since if this is a live stream
// the cached data may be completely different next time we
// read it. We have to assume that live streams don't
// advertise themselves as being seekable...
ReleaseStreamBlocks(stream);
} else {
// otherwise reading forward is looking good, so just stay where we
// are and don't trigger a channel seek!
- desiredOffset = stream->mChannelOffset;
+ desiredOffset = channelOffset;
}
}
// Figure out if we should be reading data now or not. It's amazing
// how complex this is, but each decision is simple enough.
bool enableReading;
if (stream->mStreamLength >= 0 && dataOffset >= stream->mStreamLength) {
// We want data at the end of the stream, where there's nothing to
@@ -1361,18 +1367,18 @@ MediaCache::Update()
// might create a new channel and seek unnecessarily (and incorrectly,
// since HTTP doesn't allow seeking to the actual EOF), and we don't want
// to suspend if we're not suspended and already reading at the end of
// the stream, since there just might be more data than the server
// advertised with Content-Length, and we may as well keep reading.
// But we don't want to seek to the end of the stream if we're not
// already there.
LOG("Stream %p at end of stream", stream);
- enableReading = !stream->mCacheSuspended &&
- stream->mStreamLength == stream->mChannelOffset;
+ enableReading =
+ !stream->mCacheSuspended && stream->mStreamLength == channelOffset;
} else if (desiredOffset < stream->mStreamOffset) {
// We're reading to try to catch up to where the current stream
// reader wants to be. Better not stop.
LOG("Stream %p catching up", stream);
enableReading = true;
} else if (desiredOffset < stream->mStreamOffset + BLOCK_SIZE) {
// The stream reader is waiting for us, or nearly so. Better feed it.
LOG("Stream %p feeding reader", stream);
@@ -1420,46 +1426,44 @@ MediaCache::Update()
}
}
if (enableReading) {
for (uint32_t j = 0; j < i; ++j) {
MediaCacheStream* other = mStreams[j];
if (other->mResourceID == stream->mResourceID && !other->mClosed &&
!other->mClient->IsSuspended() &&
- OffsetToBlockIndexUnchecked(other->mChannelOffset) ==
+ OffsetToBlockIndexUnchecked(other->mSeekTarget != -1
+ ? other->mSeekTarget
+ : other->mChannelOffset) ==
OffsetToBlockIndexUnchecked(desiredOffset)) {
// This block is already going to be read by the other stream.
// So don't try to read it from this stream as well.
enableReading = false;
LOG("Stream %p waiting on same block (%" PRId32 ") from stream %p",
stream,
OffsetToBlockIndexUnchecked(desiredOffset),
other);
break;
}
}
}
- if (stream->mChannelOffset != desiredOffset && enableReading) {
+ if (channelOffset != desiredOffset && enableReading) {
// We need to seek now.
NS_ASSERTION(stream->mIsTransportSeekable || desiredOffset == 0,
"Trying to seek in a non-seekable stream!");
// Round seek offset down to the start of the block. This is essential
// because we don't want to think we have part of a block already
// in mPartialBlockBuffer.
- stream->mChannelOffset =
+ stream->mSeekTarget =
OffsetToBlockIndexUnchecked(desiredOffset) * BLOCK_SIZE;
actions[i].mTag = StreamAction::SEEK;
actions[i].mResume = stream->mCacheSuspended;
- actions[i].mSeekTarget = stream->mChannelOffset;
- // mChannelOffset is updated to a new position. We don't want data from
- // the old channel to be written to the wrong position. 0 is a sentinel
- // value which will not match any ID passed to NotifyDataReceived().
- stream->mLoadID = 0;
+ actions[i].mSeekTarget = stream->mSeekTarget;
} else if (enableReading && stream->mCacheSuspended) {
actions[i].mTag = StreamAction::RESUME;
} else if (!enableReading && !stream->mCacheSuspended) {
actions[i].mTag = StreamAction::SUSPEND;
}
}
#ifdef DEBUG
mInUpdate = false;
@@ -1915,33 +1919,37 @@ MediaCacheStream::NotifyDataStarted(uint
int64_t aOffset,
bool aSeekable)
{
NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
MOZ_ASSERT(aLoadID > 0);
LOG("Stream %p DataStarted: %" PRId64 " aLoadID=%u", this, aOffset, aLoadID);
ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
- NS_WARNING_ASSERTION(aOffset == mChannelOffset,
+ NS_WARNING_ASSERTION(aOffset == mSeekTarget || aOffset == mChannelOffset,
"Server is giving us unexpected offset");
MOZ_ASSERT(aOffset >= 0);
mChannelOffset = aOffset;
if (mStreamLength >= 0) {
// If we started reading at a certain offset, then for sure
// the stream is at least that long.
mStreamLength = std::max(mStreamLength, mChannelOffset);
}
mLoadID = aLoadID;
MOZ_ASSERT(aOffset == 0 || aSeekable,
"channel offset must be zero when we become non-seekable");
mIsTransportSeekable = aSeekable;
// Queue an Update since we may change our strategy for dealing
// with this stream
mMediaCache->QueueUpdate();
+
+ // Reset mSeekTarget since the seek is completed so MediaCache::Update() will
+ // make decisions based on mChannelOffset instead of mSeekTarget.
+ mSeekTarget = -1;
}
void
MediaCacheStream::UpdatePrincipal(nsIPrincipal* aPrincipal)
{
MOZ_ASSERT(NS_IsMainThread());
MediaCache::ResourceStreamIterator iter(mMediaCache, mResourceID);
while (MediaCacheStream* stream = iter.Next()) {
--- a/dom/media/MediaCache.h
+++ b/dom/media/MediaCache.h
@@ -496,16 +496,18 @@ private:
nsresult mNotifyDataEndedStatus;
// The last reported read mode
ReadMode mCurrentMode;
// True if some data in mPartialBlockBuffer has been read as metadata
bool mMetadataInPartialBlockBuffer;
// The load ID of the current channel. Used to check whether the data is
// coming from an old channel and should be discarded.
uint32_t mLoadID = 0;
+ // The seek target initiated by MediaCache. -1 if no seek is going on.
+ int64_t mSeekTarget = -1;
bool mThrottleReadahead = false;
// Data received for the block containing mChannelOffset. Data needs
// to wait here so we can write back a complete block. The first
// mChannelOffset%BLOCK_SIZE bytes have been filled in with good data,
// the rest are garbage.
// Heap allocate this buffer since the exact power-of-2 will cause allocation