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

RESOLVED FIXED in Firefox 36, Firefox OS v2.2

Status

()

Core
Audio/Video
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla38
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36+ fixed, firefox37 fixed, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8547779 [details] [diff] [review]
Cache data directly on MP4Stream rather than relying on the media cache. v1
Attachment #8547779 - Flags: review?(roc)
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).
Attachment #8547779 - Flags: review?(roc) → review+
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 6

4 years ago
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.
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/7de37d6b2f4c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
(Assignee)

Comment 9

4 years ago
Ok - Ralph, can you get this into the uplift queue?
Flags: needinfo?(giles)
Yeah, I think we're good here. I haven't seen any further instances on branches where this has landed.
Flags: needinfo?(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
status-firefox36: --- → affected
status-firefox37: --- → fixed
status-firefox38: --- → fixed
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.
Flags: needinfo?(giles)
Attachment #8547779 - Flags: approval-mozilla-beta?
Attachment #8547779 - Flags: approval-mozilla-aurora?
[Tracking Requested - why for this release]: necessary to ship MSE support for youtube.
tracking-firefox36: --- → ?
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/eeb43c655bc3
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
tracking-firefox36: ? → +
Attachment #8547779 - Flags: approval-mozilla-beta?
Attachment #8547779 - Flags: approval-mozilla-beta+
Attachment #8547779 - Flags: approval-mozilla-aurora?
Attachment #8547779 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.