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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Bobby Holley (:bholley)]

More pre-reqs for bug 1119456.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Clean up semantics of SourceBufferResource reading. v1

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.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Fix some bugs in MockMediaResource. v1

If we don't do this, the subsequent changes to DataSourceAdapter will cause
gtest failures in TestMP4Demuxer.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Switch DataSourceAdapter to CachedReadAt. v1

Comment 4

[Bobby Holley (:bholley)]

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. ;-)

Comment 5

[Bobby Holley (:bholley)]

Oh, it looks like anthony is actually around. I'll reassign the demuxer stuff.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 8

[Bobby Holley (:bholley)]

(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 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

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...

Comment 12

[Bobby Holley (:bholley)]

These patches cause test_SplitAppend.html to time out. I'm investigating why, but I need to head out pretty soon.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 4 - Switch SourceBufferResource::Read{,At} back to blocking. v1

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.

Comment 14

[Bobby Holley (:bholley)]

(To be clear, Part 4 just reverts the second behavior change described in comment 1.

Comment 15

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3bc63fd1ba15

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8546938 [details] [diff] [review]
Part 3 - Switch DataSourceAdapter to CachedReadAt. v1

This is moving to bug 1119456.

Comment 17

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fb3619b2bfbe

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8ce96306c2d3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/677f21c77f7e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9de247dc3161

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ce96306c2d3
https://hg.mozilla.org/mozilla-central/rev/677f21c77f7e
https://hg.mozilla.org/mozilla-central/rev/9de247dc3161

Comment 19

[Ralph Giles (:rillian)]

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.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8546995 [details] [diff] [review]
Part 4 - Switch SourceBufferResource::Read{,At} back to blocking. v1

[Triage Comment]

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/60f6890d84cf
https://hg.mozilla.org/releases/mozilla-beta/rev/e5cc2f8f3f7e
https://hg.mozilla.org/releases/mozilla-beta/rev/423cb20b5f43