Closed Bug 1371882 Opened 7 years ago Closed 7 years ago

When a media resource is known to be small, bypass the shared MediaCache and keep the whole resource in memory

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
55 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

Details

Attachments

(27 files)

Bug 1371882 - MediaCacheFlusher allows for multiple MediaCache's -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - MediaCache::ResourceStreamIterator is given the MediaCache to work with -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - static MediaCache::GetMediaCache to get file-backed MediaCache -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - MediaCache::constructor/destructor/Init() don't need to be public -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - MediaCacheStream accesses its MediaCache through a member pointer -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - MediaCacheStream::mInitialized is redundant, mMediaCache is non-null after initialization -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - Removed unnecessary `gMediaCache->` from MediaCache member functions -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - Delay MediaCache destruction if update queued -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - Make gMediaCache private inside MediaCache, to avoid misuse -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - MediaCacheStream::Init forwards the known content length to the MediaCache factory -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - Move MEDIACACHESTREAM_NOTIFIED_LENGTH telemetry collection to MediaCacheStream::Init -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - If content length <= 'media.memory_cache_max_size', use a discrete memory-backed MediaCache -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - Remove FileBlockCache's dependency on Runnable -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - Virtualize FileBlockCache's API into MediaBlockCacheBase -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - Implement MemoryBlockCache -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - MEMORYBLOCKCACHE_ERRORS telemetry to catch unexpected errors without crashing -
59 bytes, text/x-review-board-request
cpearce
: review+
francois
: review+
Details
Bug 1371882 - MediaCache uses MemoryBlockCache when content length is known -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - Avoid MemoryBlockCache when combined sizes > 'media.memory_caches_combined_limit_...' -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - Rename MediaCache::mFileCache to mBlockCache -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - Remove MediaBlockCacheBase::Close() -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - MediaCache is now ref-counted -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - MediaBlockCacheBase::Init may be called again to re-initialize cache -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - MediaCache::Flush reinitializes block cache instead of recreating it -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - Let GetMediaCache decide which block cache to use -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - MemoryBlockCache is responsible for tracking the combined size of all its buffers -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - MemoryBlockCache claims extra buffer capacity if any -
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1371882 - Remove unneeded MediaCache::mContentLength -
59 bytes, text/x-review-board-request
cpearce
: review+
Details 
Based on the following data:
- bug 1366929: MEDIACACHE_WATERMARK_KB telemetry, showing >90% of MediaCache instances don't go over 8MB.
- bug 1369538: MEDIACACHESTREAM_LENGTH_KB telemetry, showing >90% of MediaCacheStream instances don't go over 8MB.
Chris suggested that when we start downloading "small-enough" resources, we could just bypass the shared MediaCache, and keep the whole resource in memory. So the resource itself will have zero file IOs; and it won't impact the shared MediaCache that will still take care of bigger resources.

Bug 1371205 will soon be collecting telemetry about typical initial content lengths (MEDIACACHESTREAM_NOTIFIED_LENGTH), which should help us pick a good starting default size under which we will use discrete memory-backed MediaCaches.
Comment on attachment 8877004 [details]
Bug 1371882 - MEMORYBLOCKCACHE_ERRORS telemetry to catch unexpected errors without crashing -

https://reviewboard.mozilla.org/r/148336/#review153058

datareview+
Attachment #8877004 - Flags: review?(francois) → review+
Looking at early telemetry results from bug 1371205 [1], an 8MB maximum for in-memory MediaCaches would handle 90% of streams for which we know the content length.
Going to 16MB would only add 2-3% of streams.
And 50% of streams are less than 256KB anyway, and 74% are less than 1MB, so it means in most cases we won't use that much memory for these smallish resources.

So I think we can keep the 8MB limit for in-memory MediaCache, to start with.

[1] https://telemetry.mozilla.org/new-pipeline/dist.html#measure=MEDIACACHESTREAM_NOTIFIED_LENGTH
Comment on attachment 8876989 [details]
Bug 1371882 - MediaCacheFlusher allows for multiple MediaCache's -

https://reviewboard.mozilla.org/r/148306/#review153114

::: dom/media/MediaCache.cpp:171
(Diff revision 1)
>  
>    // Main thread only. Creates the backing cache file. If this fails,
>    // then the cache is still in a semi-valid state; mFD will be null,
>    // so all I/O on the cache file will fail.
>    nsresult Init();
>    // Shut down the global cache if it's no longer needed. We shut down

s/global cache/cache/

Given that there will soon be more than one cache.

::: dom/media/MediaCache.cpp:664
(Diff revision 1)
>  
>  void
>  MediaCache::CloseStreamsForPrivateBrowsing()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> -  if (!gMediaCache) {
> +  for (auto& s : mStreams) {

Prefer types over auto.
Attachment #8876989 - Flags: review?(cpearce) → review+
Comment on attachment 8876990 [details]
Bug 1371882 - MediaCache::ResourceStreamIterator is given the MediaCache to work with -

https://reviewboard.mozilla.org/r/148308/#review153124

::: dom/media/MediaCache.cpp:265
(Diff revision 1)
>     * have a given resource ID and are not closed.
>     * Can be used on the main thread or while holding the media cache lock.
>     */
>    class ResourceStreamIterator {
>    public:
> -    explicit ResourceStreamIterator(int64_t aResourceID) :
> +    ResourceStreamIterator(MediaCache& aMediaCache, int64_t aResourceID)

Everywhere you create one of these, you pass \*this or \*gMediaCache to the constructor. So why not just have this function accept a pointer instead of a reference?

::: dom/media/MediaCache.cpp:266
(Diff revision 1)
>     * Can be used on the main thread or while holding the media cache lock.
>     */
>    class ResourceStreamIterator {
>    public:
> -    explicit ResourceStreamIterator(int64_t aResourceID) :
> -      mResourceID(aResourceID), mNext(0) {}
> +    ResourceStreamIterator(MediaCache& aMediaCache, int64_t aResourceID)
> +      : mMediaCache(aMediaCache), mResourceID(aResourceID), mNext(0) {}

Shouldn't you have line breaks before the ',' characters, so that each initializer is on its own line? I'd have through clang-format would have done that for you.
Attachment #8876990 - Flags: review?(cpearce) → review+
Comment on attachment 8876991 [details]
Bug 1371882 - static MediaCache::GetMediaCache to get file-backed MediaCache -

https://reviewboard.mozilla.org/r/148310/#review153126
Attachment #8876991 - Flags: review?(cpearce) → review+
Comment on attachment 8876992 [details]
Bug 1371882 - MediaCache::constructor/destructor/Init() don't need to be public -

https://reviewboard.mozilla.org/r/148312/#review153138
Attachment #8876992 - Flags: review?(cpearce) → review+
Comment on attachment 8876993 [details]
Bug 1371882 - MediaCacheStream accesses its MediaCache through a member pointer -

https://reviewboard.mozilla.org/r/148314/#review153144
Attachment #8876993 - Flags: review?(cpearce) → review+
Comment on attachment 8876994 [details]
Bug 1371882 - MediaCacheStream::mInitialized is redundant, mMediaCache is non-null after initialization -

https://reviewboard.mozilla.org/r/148316/#review153154
Attachment #8876994 - Flags: review?(cpearce) → review+
Comment on attachment 8876995 [details]
Bug 1371882 - Removed unnecessary `gMediaCache->` from MediaCache member functions -

https://reviewboard.mozilla.org/r/148318/#review153156
Attachment #8876995 - Flags: review?(cpearce) → review+
Comment on attachment 8876996 [details]
Bug 1371882 - Delay MediaCache destruction if update queued -

https://reviewboard.mozilla.org/r/148320/#review153158

MediaCache really needs to be refcounted. Without that, this is adding a foot gun. r- until that happens.

::: dom/media/MediaCache.cpp:1474
(Diff revision 1)
>  {
>  public:
> -  UpdateEvent() : Runnable("MediaCache::UpdateEvent") {}
> +  explicit UpdateEvent(MediaCache& aMediaCache)
> +    : Runnable("MediaCache::UpdateEvent")
> +    , mMediaCache(aMediaCache)
> +  {

This seems a little risky, as the type system doesn't help you ensure that the reference to the media cache is still valid here; the previous code would null check gMediaCache before derefing it.

Basically, references should be used for things that can never be null/free'd, but the type system can't guarantee that here, due to how your shutdown works here.

For safety, MediaCache needs to be made refcounted. That can be one in a follow up patch in this bug.
Attachment #8876996 - Flags: review?(cpearce) → review-
Comment on attachment 8876997 [details]
Bug 1371882 - Make gMediaCache private inside MediaCache, to avoid misuse -

https://reviewboard.mozilla.org/r/148322/#review153196

::: dom/media/MediaCache.cpp:417
(Diff revision 1)
>  #endif
>    // A list of resource IDs to notify about the change in suspended status.
>    nsTArray<int64_t> mSuspendedStatusToNotify;
>  };
>  
> +/* static */ MediaCache* MediaCache::gMediaCache;

= nullptr;
Attachment #8876997 - Flags: review?(cpearce) → review+
Comment on attachment 8876998 [details]
Bug 1371882 - MediaCacheStream::Init forwards the known content length to the MediaCache factory -

https://reviewboard.mozilla.org/r/148324/#review153198

::: dom/media/MediaCache.cpp:2563
(Diff revision 2)
>    if (mMediaCache) {
>      return NS_OK;
>    }
>  
> -  nsresult rv = Init();
> -  if (NS_FAILED(rv))
> +  if (aOriginal->mMediaCache) {
> +    // Use the same MediaCache as our clone (important if it's not the shared

In-memory MediaCache objects can be shared too. So this comment is somewhat ambiguous.

It's probably clearer to call the MediaCache that is backed by FileBlockCache something other than "the shared MediaCache". Maybe "the disk-backed MediaCache" or something like that?

::: dom/media/MediaResource.cpp:534
(Diff revision 2)
>  
> -  nsresult rv = mCacheStream.Init();
> +  int64_t cl = -1;
> +  if (mChannel) {
> +    nsCOMPtr<nsIHttpChannel> hc = do_QueryInterface(mChannel);
> +    if (hc) {
> +      if (NS_FAILED(hc->GetContentLength(&cl)) && cl != -1) {

Does GetContentLength really modify its outparam if it has failed? I would have thought you could rely on your -1 initializer.
Attachment #8876998 - Flags: review?(cpearce) → review+
Comment on attachment 8876999 [details]
Bug 1371882 - Move MEDIACACHESTREAM_NOTIFIED_LENGTH telemetry collection to MediaCacheStream::Init -

https://reviewboard.mozilla.org/r/148326/#review153200
Attachment #8876999 - Flags: review?(cpearce) → review+
Comment on attachment 8877000 [details]
Bug 1371882 - If content length <= 'media.memory_cache_max_size', use a discrete memory-backed MediaCache -

https://reviewboard.mozilla.org/r/148328/#review153202

::: dom/media/MediaCache.cpp:737
(Diff revision 2)
>  MediaCache::GetMediaCache(int64_t aContentLength)
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
> +  if (aContentLength > 0 &&
> +      aContentLength <=
> +        int64_t(MediaPrefs::MediaUnsharedCacheMaxSize()) * 1024) {

s/Unshared/Memory/

So it's clear that this pref controls how memory allocation works.

::: dom/media/MediaPrefs.h:90
(Diff revision 2)
>    };
>  
>    // This is where DECL_MEDIA_PREF for each of the preferences should go.
>  
>    // Cache sizes.
> +  DECL_MEDIA_PREF("media.unshared_cache_max_size",            MediaUnsharedCacheMaxSize, uint32_t, 8192);

How about:

media.memory_cache_max_stream_length
Attachment #8877000 - Flags: review?(cpearce) → review+
Comment on attachment 8877001 [details]
Bug 1371882 - Remove FileBlockCache's dependency on Runnable -

https://reviewboard.mozilla.org/r/148330/#review153204
Attachment #8877001 - Flags: review?(cpearce) → review+
Comment on attachment 8877002 [details]
Bug 1371882 - Virtualize FileBlockCache's API into MediaBlockCacheBase -

https://reviewboard.mozilla.org/r/148332/#review153206
Attachment #8877002 - Flags: review?(cpearce) → review+
Comment on attachment 8877003 [details]
Bug 1371882 - Implement MemoryBlockCache -

https://reviewboard.mozilla.org/r/148334/#review153208
Attachment #8877003 - Flags: review?(cpearce) → review+
Comment on attachment 8877004 [details]
Bug 1371882 - MEMORYBLOCKCACHE_ERRORS telemetry to catch unexpected errors without crashing -

https://reviewboard.mozilla.org/r/148336/#review153210
Attachment #8877004 - Flags: review?(cpearce) → review+
Comment on attachment 8877003 [details]
Bug 1371882 - Implement MemoryBlockCache -

https://reviewboard.mozilla.org/r/148334/#review153218

::: dom/media/MemoryBlockCache.cpp:64
(Diff revision 2)
> +  MutexAutoLock lock(mMutex);
> +
> +  size_t offset = BlockIndexToOffset(aBlockIndex);
> +  if (offset + aData1.Length() + aData2.Length() > mBuffer.Length()) {
> +    // Closed, or trying to write over the expected limit.
> +    return NS_ERROR_FAILURE;

Actually, I think you should grow the cache in this case, rather than erroring.

My time working on web browsers has taught me to never trust metadata. The content-length HTTP header could easily be wrong, either by incompetence, malice, or due to some use case we don't envisage. 

So I think we need to be forgiving here in what we accept, otherwise Firefox will appear broken.

We should grow the cache here, but we should have a safety valve in place so we don't grow too much (so we handle the malice case); say use the check for whether the combined sizes of all caches is above some threshold from the last patch here, and grow if we're under that threshold.
Attachment #8877003 - Flags: review+ → review-
Comment on attachment 8877005 [details]
Bug 1371882 - MediaCache uses MemoryBlockCache when content length is known -

https://reviewboard.mozilla.org/r/148338/#review153212

::: dom/media/MediaCache.cpp:397
(Diff revision 2)
>  
>    // There is at most one shared media cache.
>    static MediaCache* gMediaCache;
>  
> +  // Expected content length if known initially (this is a non-global, unshared,
> +  // MediaCache), otherwise -1 (global MediaCache).

I think you should explicitly state in the comment that this length corresponds to the HTTP Content-Length header.

::: dom/media/MediaCache.cpp:671
(Diff revision 2)
> +  if (mContentLength <= 0) {
> +    // The global MediaCache uses a file-backed storage for its resource blocks.
> -  mFileCache = new FileBlockCache();
> +    mFileCache = new FileBlockCache();
> +  } else {
> +    // Non-global MediaCaches keep the whole resource in memory.
> +    mFileCache = new MemoryBlockCache(mContentLength);

Perhaps mFileCache isn't the best name for this variable now. Perhaps mBlockCache?
Attachment #8877005 - Flags: review?(cpearce) → review+
Comment on attachment 8877006 [details]
Bug 1371882 - Avoid MemoryBlockCache when combined sizes > 'media.memory_caches_combined_limit_...' -

https://reviewboard.mozilla.org/r/148340/#review153220

::: dom/media/MediaCache.cpp:750
(Diff revision 2)
>      return;
>    }
>  
> +  if (mContentLength > 0) {
> +    // This is an unshared MediaCache, update the combined memory usage.
> +    gMediaUnsharedCachesCombinedSize -= mContentLength;

You're maintaining a copy of state here. Which means you need to worry about the state getting out of sync with reality. You'd be better to "let the world be its own model" here, and just recalculate the combined size every time. This is especially important once we allow the MemoryBlockCache's to grow.

::: modules/libpref/init/all.js:335
(Diff revision 2)
>  pref("media.unshared_cache_max_size", 8192);
> +// Don't create more discrete MediaCaches if their combined size would pass
> +// this limit (in kilobytes).
> +#if defined(_WIN64) || defined(__LP64__)
> +// 64-bit -> Allow up to 512MB of memory in unshared MediaCaches.
> +pref("media.unshared_caches_combined_limit", 524288);

Is __LP64__ defined on Android? I'm uncomfortable growing the in-memory cache to 512MB on Android.
Attachment #8877006 - Flags: review?(cpearce) → review-
Comment on attachment 8876996 [details]
Bug 1371882 - Delay MediaCache destruction if update queued -

https://reviewboard.mozilla.org/r/148320/#review153250

::: dom/media/MediaCache.cpp:1474
(Diff revision 1)
>  {
>  public:
> -  UpdateEvent() : Runnable("MediaCache::UpdateEvent") {}
> +  explicit UpdateEvent(MediaCache& aMediaCache)
> +    : Runnable("MediaCache::UpdateEvent")
> +    , mMediaCache(aMediaCache)
> +  {

We can change |bool mUpdateQueued| to |nsCOMPtr<nsICancelableRunnable> mPendingUpdate|.

1. !!mPendingUpdate to test if there is a queued update.
2. mPendingUpdate.Cancel() to cancel the queued update.
Comment on attachment 8876997 [details]
Bug 1371882 - Make gMediaCache private inside MediaCache, to avoid misuse -

https://reviewboard.mozilla.org/r/148322/#review153254

::: dom/media/MediaCache.cpp:417
(Diff revision 1)
>  #endif
>    // A list of resource IDs to notify about the change in suspended status.
>    nsTArray<int64_t> mSuspendedStatusToNotify;
>  };
>  
> +/* static */ MediaCache* MediaCache::gMediaCache;

Static globals are zero-initialized by default.
Comment on attachment 8876998 [details]
Bug 1371882 - MediaCacheStream::Init forwards the known content length to the MediaCache factory -

https://reviewboard.mozilla.org/r/148324/#review153256

::: dom/media/MediaCache.cpp:2562
(Diff revision 2)
>  
>    if (mMediaCache) {
>      return NS_OK;
>    }
>  
> -  nsresult rv = Init();
> +  if (aOriginal->mMediaCache) {

|aOriginal->mMediaCache == nullptr| means the original failed to init, right?

How can we clone an object which failed to init?

::: dom/media/MediaResource.cpp:534
(Diff revision 2)
>  
> -  nsresult rv = mCacheStream.Init();
> +  int64_t cl = -1;
> +  if (mChannel) {
> +    nsCOMPtr<nsIHttpChannel> hc = do_QueryInterface(mChannel);
> +    if (hc) {
> +      if (NS_FAILED(hc->GetContentLength(&cl)) && cl != -1) {

The general API design is not to modify the outparam upon failure. But better safe than sorry.

The code can be simply:
if (NS_FAILED(...)) {
  cl = -1;
}

We want cl to be -1 when the call fails.
Comment on attachment 8876999 [details]
Bug 1371882 - Move MEDIACACHESTREAM_NOTIFIED_LENGTH telemetry collection to MediaCacheStream::Init -

https://reviewboard.mozilla.org/r/148326/#review153268

::: dom/media/MediaCache.cpp:2531
(Diff revision 2)
>    if (mMediaCache) {
>      return NS_OK;
>    }
>  
>    if (aContentLength > 0) {
> +    uint32_t length = uint32_t(std::min(aContentLength, int64_t(UINT32_MAX)));

See bug 1370177 comment 3. The content length might be wrong when OnStartRequest() is called.

This will result in wrongly using a memory cache for a large file. We should add probes to see if mStreamLength can change drastically.
Comment on attachment 8876999 [details]
Bug 1371882 - Move MEDIACACHESTREAM_NOTIFIED_LENGTH telemetry collection to MediaCacheStream::Init -

https://reviewboard.mozilla.org/r/148326/#review153268

> See bug 1370177 comment 3. The content length might be wrong when OnStartRequest() is called.
> 
> This will result in wrongly using a memory cache for a large file. We should add probes to see if mStreamLength can change drastically.

What causes the stream length to be wrong during OnStartRequest()?
Comment on attachment 8877001 [details]
Bug 1371882 - Remove FileBlockCache's dependency on Runnable -

https://reviewboard.mozilla.org/r/148330/#review153316

::: dom/media/FileBlockCache.cpp:59
(Diff revision 2)
>          // task to service the request.
> -        mThread->Dispatch(this, NS_DISPATCH_NORMAL);
> +        nsCOMPtr<nsIRunnable> event = mozilla::NewRunnableMethod(
> +          "FileBlockCache::SetCacheFile -> PerformBlockIOs",
> +          this,
> +          &FileBlockCache::PerformBlockIOs);
> +        mThread->Dispatch(event, NS_DISPATCH_NORMAL);

s/event/event.forget()/.

::: dom/media/FileBlockCache.cpp:228
(Diff revision 2)
>    }
> -  mThread->Dispatch(this, NS_DISPATCH_NORMAL);
> +  nsCOMPtr<nsIRunnable> event = mozilla::NewRunnableMethod(
> +    "FileBlockCache::EnsureWriteScheduled -> PerformBlockIOs",
> +    this,
> +    &FileBlockCache::PerformBlockIOs);
> +  mThread->Dispatch(event, NS_DISPATCH_NORMAL);

Ditto.
Comment on attachment 8876989 [details]
Bug 1371882 - MediaCacheFlusher allows for multiple MediaCache's -

https://reviewboard.mozilla.org/r/148306/#review153114

> s/global cache/cache/
> 
> Given that there will soon be more than one cache.

Already taken care of in "Delay MediaCache destruction" patch.

> Prefer types over auto.

It was like that before, but happy to change for clarity.
Comment on attachment 8876990 [details]
Bug 1371882 - MediaCache::ResourceStreamIterator is given the MediaCache to work with -

https://reviewboard.mozilla.org/r/148308/#review153124

> Everywhere you create one of these, you pass \*this or \*gMediaCache to the constructor. So why not just have this function accept a pointer instead of a reference?

I prefer references where possible as they imply a non-null pointer; but ok to change in this case.

> Shouldn't you have line breaks before the ',' characters, so that each initializer is on its own line? I'd have through clang-format would have done that for you.

I must have forgotten to run clang-format on that patch... I need to semi-automate this!
Comment on attachment 8876996 [details]
Bug 1371882 - Delay MediaCache destruction if update queued -

https://reviewboard.mozilla.org/r/148320/#review153158

> This seems a little risky, as the type system doesn't help you ensure that the reference to the media cache is still valid here; the previous code would null check gMediaCache before derefing it.
> 
> Basically, references should be used for things that can never be null/free'd, but the type system can't guarantee that here, due to how your shutdown works here.
> 
> For safety, MediaCache needs to be made refcounted. That can be one in a follow up patch in this bug.

Will work on a follow-up with ref-counting...
Comment on attachment 8876997 [details]
Bug 1371882 - Make gMediaCache private inside MediaCache, to avoid misuse -

https://reviewboard.mozilla.org/r/148322/#review153196

> = nullptr;

Initialized to nullptr by non-local static initialization.
I'll add a comment.
Comment on attachment 8876998 [details]
Bug 1371882 - MediaCacheStream::Init forwards the known content length to the MediaCache factory -

https://reviewboard.mozilla.org/r/148324/#review153256

> |aOriginal->mMediaCache == nullptr| means the original failed to init, right?
> 
> How can we clone an object which failed to init?

Yeah, it seems impossible; I'll remove the test and else-clause.

> The general API design is not to modify the outparam upon failure. But better safe than sorry.
> 
> The code can be simply:
> if (NS_FAILED(...)) {
>   cl = -1;
> }
> 
> We want cl to be -1 when the call fails.

I was trying to save a write... Old habit :-)
Comment on attachment 8876999 [details]
Bug 1371882 - Move MEDIACACHESTREAM_NOTIFIED_LENGTH telemetry collection to MediaCacheStream::Init -

https://reviewboard.mozilla.org/r/148326/#review153268

> What causes the stream length to be wrong during OnStartRequest()?

I've got telemetry to catch such issues, and I will allow the MemoryBlockCache to grow larger than first expected.
Comment on attachment 8877003 [details]
Bug 1371882 - Implement MemoryBlockCache -

https://reviewboard.mozilla.org/r/148334/#review153218

> Actually, I think you should grow the cache in this case, rather than erroring.
> 
> My time working on web browsers has taught me to never trust metadata. The content-length HTTP header could easily be wrong, either by incompetence, malice, or due to some use case we don't envisage. 
> 
> So I think we need to be forgiving here in what we accept, otherwise Firefox will appear broken.
> 
> We should grow the cache here, but we should have a safety valve in place so we don't grow too much (so we handle the malice case); say use the check for whether the combined sizes of all caches is above some threshold from the last patch here, and grow if we're under that threshold.

Ok I will grow the buffer here -- when writing only, as reading should never fail since the MediaCache should keep track of what's in the MemoryBlockCache.
I will add the safety valve in a later patch.
Comment on attachment 8877005 [details]
Bug 1371882 - MediaCache uses MemoryBlockCache when content length is known -

https://reviewboard.mozilla.org/r/148338/#review153212

> Perhaps mFileCache isn't the best name for this variable now. Perhaps mBlockCache?

Will do, in a separate patch.
Comment on attachment 8877006 [details]
Bug 1371882 - Avoid MemoryBlockCache when combined sizes > 'media.memory_caches_combined_limit_...' -

https://reviewboard.mozilla.org/r/148340/#review153220

> You're maintaining a copy of state here. Which means you need to worry about the state getting out of sync with reality. You'd be better to "let the world be its own model" here, and just recalculate the combined size every time. This is especially important once we allow the MemoryBlockCache's to grow.

I'm worried about concurrent access, either we'll need more locking, or we'll risk reading changing values (but that wouldn't be too bad, as we're interested in an estimate).

(And this reminds me that I should probably update MediaCacheStream::SizeOfExcludingThis. I'll need to ensure we don't double-count shared MediaCache objects.)

This will be ... interesting.

> Is __LP64__ defined on Android? I'm uncomfortable growing the in-memory cache to 512MB on Android.

__LP64__ is #defined by gcc&clang when `long int`s and pointers use 64 bits, so it could very well be defined if we build for 64-bit Android platforms.
Should I just remove Android? Or instead only allow some 64-bit platforms?
Comment on attachment 8876996 [details]
Bug 1371882 - Delay MediaCache destruction if update queued -

https://reviewboard.mozilla.org/r/148320/#review153250

> We can change |bool mUpdateQueued| to |nsCOMPtr<nsICancelableRunnable> mPendingUpdate|.
> 
> 1. !!mPendingUpdate to test if there is a queued update.
> 2. mPendingUpdate.Cancel() to cancel the queued update.

The update is not cancelable anymore, it keeps a reference on the MediaCache (to ensure it's alive) and runs to completion.
With no streams left, it should be quick enough; but we can change that later on if it appears to be an issue.

A note about mozreview: I only see line 1474 with a lonely `{` in mozreview. You can select multiple lines (for better context) by dragging the [+] icon across from the first to the last line that you want to highlight.
Comment on attachment 8877970 [details]
Bug 1371882 - Rename MediaCache::mFileCache to mBlockCache -

https://reviewboard.mozilla.org/r/149374/#review154188
Attachment #8877970 - Flags: review?(cpearce) → review+
Comment on attachment 8876996 [details]
Bug 1371882 - Delay MediaCache destruction if update queued -

https://reviewboard.mozilla.org/r/148320/#review154192
Attachment #8876996 - Flags: review?(cpearce) → review+
Comment on attachment 8877006 [details]
Bug 1371882 - Avoid MemoryBlockCache when combined sizes > 'media.memory_caches_combined_limit_...' -

https://reviewboard.mozilla.org/r/148340/#review154198
Attachment #8877006 - Flags: review?(cpearce) → review+
Comment on attachment 8877003 [details]
Bug 1371882 - Implement MemoryBlockCache -

https://reviewboard.mozilla.org/r/148334/#review154200
Attachment #8877003 - Flags: review?(cpearce) → review+
Comment on attachment 8877971 [details]
Bug 1371882 - Remove MediaBlockCacheBase::Close() -

https://reviewboard.mozilla.org/r/149376/#review154204
Attachment #8877971 - Flags: review?(cpearce) → review+
Comment on attachment 8877972 [details]
Bug 1371882 - MediaCache is now ref-counted -

https://reviewboard.mozilla.org/r/149378/#review154208

::: dom/media/MediaCache.cpp:404
(Diff revision 1)
> -  // If there is no queued update, destroy the MediaCache immediately.
> -  // Otherwise when the update is processed, it will destroy the MediaCache.
> -  void ShutdownAndDestroyThis();
> -
>    // There is at most one file-backed media cache.
> +  // It was owned by all MediaCacheStreams that use it.

s/was/is/

right?
Attachment #8877972 - Flags: review?(cpearce) → review+
Comment on attachment 8877973 [details]
Bug 1371882 - MediaBlockCacheBase::Init may be called again to re-initialize cache -

https://reviewboard.mozilla.org/r/149380/#review154210
Attachment #8877973 - Flags: review?(cpearce) → review+
Comment on attachment 8877974 [details]
Bug 1371882 - MediaCache::Flush reinitializes block cache instead of recreating it -

https://reviewboard.mozilla.org/r/149382/#review154212
Attachment #8877974 - Flags: review?(cpearce) → review+
Comment on attachment 8877975 [details]
Bug 1371882 - Let GetMediaCache decide which block cache to use -

https://reviewboard.mozilla.org/r/149384/#review154214
Attachment #8877975 - Flags: review?(cpearce) → review+
Comment on attachment 8877976 [details]
Bug 1371882 - MemoryBlockCache is responsible for tracking the combined size of all its buffers -

https://reviewboard.mozilla.org/r/149386/#review154240
Attachment #8877976 - Flags: review?(cpearce) → review+
Comment on attachment 8877977 [details]
Bug 1371882 - MemoryBlockCache claims extra buffer capacity if any -

https://reviewboard.mozilla.org/r/149388/#review154242
Attachment #8877977 - Flags: review?(cpearce) → review+
Comment on attachment 8877978 [details]
Bug 1371882 - Remove unneeded MediaCache::mContentLength -

https://reviewboard.mozilla.org/r/149390/#review154244
Attachment #8877978 - Flags: review?(cpearce) → review+
Chris: Thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you thank you. :-)
And Francois for the datareview.
And thank you JW for the extra pair of eyes.

I'll do a last rebase and ship it soon...
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d050ab0f879b
MediaCacheFlusher allows for multiple MediaCache's - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/66550240c5fe
MediaCache::ResourceStreamIterator is given the MediaCache to work with - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/fd40bd8e4a6a
static MediaCache::GetMediaCache to get file-backed MediaCache - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/b0c55ca939f6
MediaCache::constructor/destructor/Init() don't need to be public - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/b3878ad0681f
MediaCacheStream accesses its MediaCache through a member pointer - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/c2f53f29d78c
MediaCacheStream::mInitialized is redundant, mMediaCache is non-null after initialization - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/995108c4c705
Removed unnecessary `gMediaCache->` from MediaCache member functions - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/c361945c67e5
Delay MediaCache destruction if update queued - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/d25c4891077b
Make gMediaCache private inside MediaCache, to avoid misuse - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/200b97bc755b
MediaCacheStream::Init forwards the known content length to the MediaCache factory - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/b417b74fd954
Move MEDIACACHESTREAM_NOTIFIED_LENGTH telemetry collection to MediaCacheStream::Init - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/0b219d49822a
If content length <= 'media.memory_cache_max_size', use a discrete memory-backed MediaCache - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/45cf5d142515
Remove FileBlockCache's dependency on Runnable - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/deb5f329e7c4
Virtualize FileBlockCache's API into MediaBlockCacheBase - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/3d948ab0fa87
Rename MediaCache::mFileCache to mBlockCache - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/782b764e00e7
Implement MemoryBlockCache - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/77af7cf044e9
MEMORYBLOCKCACHE_ERRORS telemetry to catch unexpected errors without crashing - r=cpearce,francois
https://hg.mozilla.org/integration/autoland/rev/da3bfbe8e711
MediaCache uses MemoryBlockCache when content length is known - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/fe82587013c0
Avoid MemoryBlockCache when combined sizes > 'media.memory_caches_combined_limit_...' - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/221b43eb5511
Remove MediaBlockCacheBase::Close() - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/559ca369351e
MediaCache is now ref-counted - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/c6dbc8096414
MediaBlockCacheBase::Init may be called again to re-initialize cache - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/cbde6680b663
MediaCache::Flush reinitializes block cache instead of recreating it - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/8fb5e5de6e08
Let GetMediaCache decide which block cache to use - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/7a8e9be84be8
MemoryBlockCache is responsible for tracking the combined size of all its buffers - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/ce7161a44afd
MemoryBlockCache claims extra buffer capacity if any - r=cpearce
https://hg.mozilla.org/integration/autoland/rev/c07481b91462
Remove unneeded MediaCache::mContentLength - r=cpearce
https://hg.mozilla.org/mozilla-central/rev/d050ab0f879b
https://hg.mozilla.org/mozilla-central/rev/66550240c5fe
https://hg.mozilla.org/mozilla-central/rev/fd40bd8e4a6a
https://hg.mozilla.org/mozilla-central/rev/b0c55ca939f6
https://hg.mozilla.org/mozilla-central/rev/b3878ad0681f
https://hg.mozilla.org/mozilla-central/rev/c2f53f29d78c
https://hg.mozilla.org/mozilla-central/rev/995108c4c705
https://hg.mozilla.org/mozilla-central/rev/c361945c67e5
https://hg.mozilla.org/mozilla-central/rev/d25c4891077b
https://hg.mozilla.org/mozilla-central/rev/200b97bc755b
https://hg.mozilla.org/mozilla-central/rev/b417b74fd954
https://hg.mozilla.org/mozilla-central/rev/0b219d49822a
https://hg.mozilla.org/mozilla-central/rev/45cf5d142515
https://hg.mozilla.org/mozilla-central/rev/deb5f329e7c4
https://hg.mozilla.org/mozilla-central/rev/3d948ab0fa87
https://hg.mozilla.org/mozilla-central/rev/782b764e00e7
https://hg.mozilla.org/mozilla-central/rev/77af7cf044e9
https://hg.mozilla.org/mozilla-central/rev/da3bfbe8e711
https://hg.mozilla.org/mozilla-central/rev/fe82587013c0
https://hg.mozilla.org/mozilla-central/rev/221b43eb5511
https://hg.mozilla.org/mozilla-central/rev/559ca369351e
https://hg.mozilla.org/mozilla-central/rev/c6dbc8096414
https://hg.mozilla.org/mozilla-central/rev/cbde6680b663
https://hg.mozilla.org/mozilla-central/rev/8fb5e5de6e08
https://hg.mozilla.org/mozilla-central/rev/7a8e9be84be8
https://hg.mozilla.org/mozilla-central/rev/ce7161a44afd
https://hg.mozilla.org/mozilla-central/rev/c07481b91462
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
See Also: → 1400166
Depends on: 1412442
See Also: → 1466569
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: