1120629 - media cache prefilling strategy doesn't work when we hit on the stream before reading an entire cache block

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

In bug 1119456, we came up with the strategy to prefill cache lines with blocking reads outside of the demuxer, and then call back into the demuxer (which does non-blocking reads).

An issue that arose was that media cache stores incomplete cache lines in mPartialBlockBuffer, and discards them if a seek occurs before filling it. roc proposed the workaround of bumping up the priming read to the size of a cache block.

This seemed to work, but as it turns out, misses the case where "rounding up" causes the read to be larger than the actual underlying stream.

It seems like we can either make changes to MediaCache, or just cache the data that we need on MP4Stream for the duration that the stream is pinned. Given that the MediaCache wasn't really designed for this and we're ripping this hack out soon anyway, I think I prefer the latter.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Cache data directly on MP4Stream rather than relying on the media cache. v1

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8547779 [details] [diff] [review]
Cache data directly on MP4Stream rather than relying on the media cache. v1

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

::: dom/media/fmp4/MP4Stream.cpp
@@ +75,5 @@
>  {
> +  // First, check our local cache.
> +  for (size_t i = 0; i < mCache.Length(); ++i) {
> +    if (mCache[i].mOffset == aOffset && mCache[i].mCount >= aCount) {
> +      memcpy(aBuffer, mCache[i].mBuffer, aCount);

This is fine, but should you handle cases where a CachedReadAt can be satisfied (or partially satisfied) by reading from multiple cache entries? Because if you don't, your invariants are more complex (i.e. you're assuming that CachedReadAt calls match the BlockingReadIntoCache calls).

Comment 4

[Bobby Holley (:bholley)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Comment on attachment 8547779 [details] [diff] [review]
> Cache data directly on MP4Stream rather than relying on the media cache. v1
> 
> Review of attachment 8547779 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/fmp4/MP4Stream.cpp
> @@ +75,5 @@
> >  {
> > +  // First, check our local cache.
> > +  for (size_t i = 0; i < mCache.Length(); ++i) {
> > +    if (mCache[i].mOffset == aOffset && mCache[i].mCount >= aCount) {
> > +      memcpy(aBuffer, mCache[i].mBuffer, aCount);
> 
> This is fine, but should you handle cases where a CachedReadAt can be
> satisfied (or partially satisfied) by reading from multiple cache entries?
> Because if you don't, your invariants are more complex (i.e. you're assuming
> that CachedReadAt calls match the BlockingReadIntoCache calls).

I decided not to handle that case, because I think it doesn't matter. We error out of the demuxer when the demuxer tries to read some chunk at a fixed offset while parsing the file. When we retry, it's going to be performing the exact same read (i.e. it doesn't do any internal buffering of its own). So I think the added complexity isn't worth it.

Even if I'm wrong, in the worst case we'll just have a few overlapping entries for a period of time.

Comment 5

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7de37d6b2f4c

Comment 6

[Bobby Holley (:bholley)]

Once we confirm that this fixes the orange, we should uplift this.

Flagging Ryan to confirm the fix as soon as there's enough data to do so. When he does, Ralph can uplift it.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7de37d6b2f4c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Initial indications in bug 1120324 are promising. Let's give it another day to say for sure now that this is merged around to the other trunk branches as well.

Comment 9

[Bobby Holley (:bholley)]

Ok - Ralph, can you get this into the uplift queue?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Yeah, I think we're good here. I haven't seen any further instances on branches where this has landed.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

I pushed this to Aurora to stop the immediate pain from bug 1120324 there. I'll leave it to Ralph to take care of the approval requests and eventual beta uplift.

https://hg.mozilla.org/releases/mozilla-aurora/rev/eeb43c655bc3

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 8547779 [details] [diff] [review]
Cache data directly on MP4Stream rather than relying on the media cache. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Problems with audio/video sync, particularly on Youtube, sites more likely to serve Flash video.
[Describe test coverage new/current, TBPL]: Landed on m-c. Fixes mochitest orange.
[Risks and why]: This changes the way we cache data and affects mp4 playback on Mac and Windows, so there's some risk of regression in other areas. Necessary fix for the MSE feature though.
[String/UUID change made/needed]: None

This request is retroactive for Aurora, and separately for beta, hopefully for the beta2 build.

Comment 13

[Ralph Giles (:rillian)]

[Tracking Requested - why for this release]: necessary to ship MSE support for youtube.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/eeb43c655bc3

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f7bd9ae15c9e