Clean up semantics of blocking/non-blocking reads between source buffers and the MP4 Demuxer

RESOLVED FIXED in Firefox 36

Status

()

P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bobbyholley, Assigned: bobbyholley)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

More pre-reqs for bug 1119456.
This patch refactors things and makes two function changes:
(1) ReadFromCache does not block and properly fails if the data is unavailable.
(2) Read and ReadAt block if an out-param is _not_ provided, rather than the
    reverse. Both karlt and I think this is the appropriate thing to do.
Attachment #8546936 - Flags: review?(cpearce)
If we don't do this, the subsequent changes to DataSourceAdapter will cause
gtest failures in TestMP4Demuxer.
Attachment #8546937 - Flags: review?(cpearce)
Chris, feel free to reassign these as appropriate - I know this isn't really your code, but I'm just preferring you because we're in a hurry and you're around. ;-)
Oh, it looks like anthony is actually around. I'll reassign the demuxer stuff.
Attachment #8546937 - Flags: review?(cpearce) → review?(ajones)
Attachment #8546938 - Flags: review?(cpearce) → review?(ajones)
Comment on attachment 8546936 [details] [diff] [review]
Part 1 - Clean up semantics of SourceBufferResource reading. v1

Review of attachment 8546936 [details] [diff] [review]:
-----------------------------------------------------------------

I don't really know the API here, but it seems odd that ReadAt is stateful and changes mOffset to the read cursor.

::: dom/media/mediasource/SourceBufferResource.cpp
@@ +62,4 @@
>           !mEnded &&
>           mOffset + aCount > static_cast<uint64_t>(GetLength())) {
> +    SBR_DEBUGV("SourceBufferResource(%p)::ReadInternal waiting for data", this);
> +    mMonitor.Wait();

What if we were doing a blocking read, and we blocked here, and then another caller called ReadAt and changed mOffset? Should we be caching mOffset at the start of this function?
Comment on attachment 8546937 [details] [diff] [review]
Part 2 - Fix some bugs in MockMediaResource. v1

Review of attachment 8546937 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/gtest/MockMediaResource.h
@@ +61,5 @@
>    {
> +    uint32_t bytesRead = 0;
> +    nsresult rv = ReadAt(aOffset, aBuffer, aCount, &bytesRead);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    return bytesRead == aCount ? NS_OK : NS_ERROR_FAILURE;

What if the read hits EOF and bytesRead is < aCount? Then we'll report error here won't we?

If the caller isn't supposed to do that, we should assert here that the caller doesn't try to read more bytes than are available.
(In reply to Chris Pearce (:cpearce) from comment #6)
> What if we were doing a blocking read, and we blocked here, and then another
> caller called ReadAt and changed mOffset? Should we be caching mOffset at
> the start of this function?

Yes, that seems like the best we can do given the API constraints.

(In reply to Chris Pearce (:cpearce) from comment #7)
> What if the read hits EOF and bytesRead is < aCount? Then we'll report error
> here won't we?
> 
> If the caller isn't supposed to do that, we should assert here that the
> caller doesn't try to read more bytes than are available.

The API comments suggest that we're supposed to fail in this case: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaResource.h?from=MediaResource.h#361
Comment on attachment 8546937 [details] [diff] [review]
Part 2 - Fix some bugs in MockMediaResource. v1

Review of attachment 8546937 [details] [diff] [review]:
-----------------------------------------------------------------

Right, of course.
Attachment #8546937 - Flags: review?(ajones) → review+
Comment on attachment 8546936 [details] [diff] [review]
Part 1 - Clean up semantics of SourceBufferResource reading. v1

Review of attachment 8546936 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with mOffset cached at the start of SourceBufferResource::ReadInternal, or the problem otherwise fixed.
Attachment #8546936 - Flags: review?(cpearce) → review+
Comment on attachment 8546938 [details] [diff] [review]
Part 3 - Switch DataSourceAdapter to CachedReadAt. v1

Review of attachment 8546938 [details] [diff] [review]:
-----------------------------------------------------------------

I *think* this is OK... We'll find out the hardway if I'm wrong...
Attachment #8546938 - Flags: review?(ajones) → review+
These patches cause test_SplitAppend.html to time out. I'm investigating why, but I need to head out pretty soon.
This only affects WebM MSE, which, as it turns out, doesn't deal well with
non-blocking read (MP4Reader now only operates on cached reads). We should
fix up WebM at some point, but I'm reverting the behavior change for now
because it's pretty much just a distraction.
Attachment #8546995 - Flags: review?(cpearce)
(To be clear, Part 4 just reverts the second behavior change described in comment 1.
Attachment #8546995 - Flags: review?(cpearce) → review+
Comment on attachment 8546938 [details] [diff] [review]
Part 3 - Switch DataSourceAdapter to CachedReadAt. v1

This is moving to bug 1119456.
Attachment #8546938 - Attachment is obsolete: true
Comment on attachment 8546936 [details] [diff] [review]
Part 1 - Clean up semantics of SourceBufferResource reading. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Users likely to see playback stalls with youtube. Less consistent testing.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Risk is low. The first patch is not trivial, but all are localized to MSE code.
[String/UUID change made/needed]: None.

This request applies to all patches in this bug.
Attachment #8546936 - Flags: approval-mozilla-beta?
status-firefox36: --- → affected
status-firefox37: --- → fixed
Comment on attachment 8546995 [details] [diff] [review]
Part 4 - Switch SourceBufferResource::Read{,At} back to blocking. v1

[Triage Comment]
Attachment #8546995 - Flags: approval-mozilla-beta+
Attachment #8546937 - Flags: approval-mozilla-beta+
Attachment #8546936 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.