Closed Bug 461287 Opened 16 years ago Closed 16 years ago

nsHttpStreamStrategy::Seek() incorrectly increments read cursor

Categories

(Core :: Audio/Video, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(1 file, 1 obsolete file)

When you seek an nsMediaStream nsHttpStreamStrategy::Seek(), and the seek-to position is less than SEEK_VS_READ_THRESHOLD bytes ahead of the current read cursor, the stream reads and discards data to advance the read cursor, rather than starting a new byte-range HTTP request. However, the nsIInputStream::Read() on nsHttpStreamStrategy::mPipeInput [ http://mxr.mozilla.org/mozilla-central/source/content/media/video/src/nsMediaStream.cpp#597 ] does not guarantee to read all requested bytes in one go, it only reads all available bytes. But the read cursor (mPosition) is advanced by the number of bytes expected to be read, not the actual bytes read [ http://mxr.mozilla.org/mozilla-central/source/content/media/video/src/nsMediaStream.cpp#599 ]. This can result in the seek operation completing without the read cursor actually being advanced to the new seek-to-position.
This fixes the bug for me - it shows up for on one particular file on only one of my web servers when using the DirectShow backend.
Attachment #344428 - Flags: superreview?(roc)
Attachment #344428 - Flags: review?(chris.double)
Same as v1, but Chris D pointed out we could go into an infinite loop if we're at EOF when we do the read on the input stream, so I've added a check for EOF to break out of the loop and return failure.
Attachment #344428 - Attachment is obsolete: true
Attachment #344430 - Flags: superreview?(roc)
Attachment #344430 - Flags: review?(chris.double)
Attachment #344428 - Flags: superreview?(roc)
Attachment #344428 - Flags: review?(chris.double)
Attachment #344430 - Flags: review?(chris.double) → review+
Attachment #344430 - Flags: superreview?(roc) → superreview+
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
+        NS_ENSURE_TRUE(bytes != 0, NS_ERROR_FAILURE); // Tried to read past EOF.

should you really warn in this case? EOF does not seem that unusual to me, but I don't know this code so maybe I'm wrong.
(In reply to comment #4)
> +        NS_ENSURE_TRUE(bytes != 0, NS_ERROR_FAILURE); // Tried to read past
> EOF.
> 
> should you really warn in this case? EOF does not seem that unusual to me, but
> I don't know this code so maybe I'm wrong.

I don't think the backends should try to seek past EOF, and that loop should terminate with rv=NS_OK when reading up to EOF. 

The seeking spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#dom-media-seek says:

"If the (possibly now changed) new playback position is not in one of the ranges given in the seekable attribute, then the user agent must raise an INDEX_SIZE_ERR exception (if the seek was in response to a DOM method call or setting of a DOM attribute), and abort these steps."

I don't see us checking that anywhere, we should probably check in nsHTMLMediaElement::SetCurrentTime() (if we know the duration). That should probably be a separate bug. The spec also says that the current time should be capped at max and min, but I don't see those defined.
I think the plan is, once we have implemented duration, we will implement the seekable attribute so that if the duration is available you can seek anywhere between 0 and the duration (and if the duration is not available (yet), you can seek only to 0). Then we'd add the check that seeks fall within the seekable range.
You need to log in before you can comment on or make changes to this bug.