1374180 - Blocks may not be correctly marked as used when MediaResourceIndex::ReadAt fills, and reads from, its own cache

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Bug 1368837 (Add lockless cache to MediaResourceIndex) introduced a subtle change in the way MediaResourceIndex::ReadAt() interacts with the MediaCache:
In MediaResourceIndex::CacheOrReadAt, we attempt to mResource->ReadFromCache (instead of ReadAt), which in the case of a MediaCache-backed resource, means the MediaCache will *not* mark the corresponding block as used, which may have some impact on what will happen to that block, and when to download more data from that stream.

So I think we will need to have a way to notify the MediaCache when that cached data is being read, hopefully without re-introducing too much of the locking that bug 1368837 was designed to remove.

The simplest way (to be vetted) might be to just mark that block as used when it's first cached, so we don't need to get back to the MediaCache again.
I think marking it as read early is not a problem; in the worst case it may trigger the download of more data from the stream; but overall the rate of usage will remain similar.

Another option would be to notify the MediaCache when the cached data is actually read; and try to only send one notification per MediaCache block, to avoid going back to using one lock per read.

I will investigate further first, to try and understand the exact impact of bug 1368837, and then pick a solution...

(Suggestions welcome.)

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1374180 - MediaResourceIndex won't use ReadFromCache, to keep MediaCache more consistent -

Bug 1368837 (Add lockless cache to MediaResourceIndex) used
MediaResource::ReadFromCache when trying to fill its local cache. But this
meant that these reads would not count as "real reads" for the MediaCache,
which could lead to unexpected states, e.g. "Played block after the current
stream position?"

To solve this, this patch reverts to using normal ReadAt() calls, which let
the MediaCache update itself properly.

While still trying to read more than what is requested (to hopefully fulfill
future reads from the local cache), we don't want to be reading so much that
we block waiting for data past the caller-expected point.
To do this, MediaResourceIndex::UncachedRangedReadAt() is called to try and
read as much as could fill the local cache, but if a single
MediaResource::ReadAt() ends before we can fill the local cache, but *after*
the minimum caller-requested size, we stop trying to read, as such extra read
is at risk of blocking (assuming that the caller knows not to read too far.)

Review commit: https://reviewboard.mozilla.org/r/151144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151144/

Comment 2

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

Comment on attachment 8879774 [details]
Bug 1374180 - MediaResourceIndex won't use ReadFromCache, to keep MediaCache more consistent -

https://reviewboard.mozilla.org/r/151144/#review156502

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b51657a3495d
MediaResourceIndex won't use ReadFromCache, to keep MediaCache more consistent - r=cpearce

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b51657a3495d