1388125 - Video elements with URL Object srcs pointing to large video files can hang content process on MediaCache lookups

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Test HTML file for issue STR

STR:

1. Load test html file from this bug in Firefox.
2. Using the file picker, load a fairly large movie file (> 1.5-2GB)
3. Using the controls, select a time somewhere about halfway or greater into the movie.

Expected:

File loads quickly, and skips to time seamlessly

Actual:

File hangs content process while loading, then hangs content process again while seeking. Hang on seek can last for quite a while (30+ seconds).

Tested on:

Firefox 56 beta, 57 nightly, on Windows 10 and Debian Linux Stretch, using .mp4 files of size 1.5gb or higher. Both test machines running on quad core machines with SSD and 32GB ram.

---

According to gecko profiler, most of this time is spent in PLDHashTable lookups in MediaCacheStream::BlockList::GetPrevBlock / MediaCacheStream::BlockList::GetLastBlock. 

Note that this hang does not happen if the video element is set with an src pointing directly to the local file system. This only seems to happen when URL Objects are used.

Comment 1

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

Thank you for the report. Based on your description of the profiler output, my guess would be that a lot of time is spent dealing with blocks (each block represents 32KB in the resource), block informations are arranged in a deque, but their prev/next pointers are stored in a hashtable, which is what you see in the profiler.

I'll see if I can investigate further soon.

(Local file bypass the MediaCache, that's why they don't show similar issues.)

Comment 2

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

Thinking out loud, in case someone else gets to this bug before me:

BlockList's (double-ended queues linking on block id to its prev/next block ids) are stored separately from the blocks themselves, because blocks may belong to more than one list at a time.

Now, in practice there are only three lists, and there's even an `enum BlockClass` that identifies them.
So I'm thinking that instead of having each BlockList using a hashtable (from one block to its prev/next information), each block could directly include such information (prev/next for each of the 3 lists); this would remove the cost of hashtable lookups when traversing lists. Alternatively, these BlockLists could use a simple array; to be costed.

But before trying to optimize that, we may want to first determine if there could be a higher-level optimization that could remove this need to traverse large lists (for large resources); e.g.: maybe blocks for each resource could just be kept in simple arrays, giving us O(1) lookups? -- At the expense of wasting more memory; if that's too much, some kind of sparse array could be the next best thing.

(Note that I'm not *that* familiar with the inner workings of the MediaCache, so the above is my best guess from just looking at the data structures, and their associated functions.)

Comment 3

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

Correction about my comment 2:
My first suggestion above is not applicable: There are 3 BlockList's *per MediaCacheStream*, and a block may be owned by multiple streams, so a block could be part of a more than 3 lists. :-(

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I just ran this in FF57 with a new profile and it's now much worse than when I reported it, I'm able to lock up a tab by jumping around a 500mb video on Windows 10.

Profile when even just trying to load a 1.5gb file from a file input. 46s of jank in content:

https://perfht.ml/2xMUcjv

Is there no way we can bypass the media cache when loading file blobs, if they resolve to a local file?

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Also ni?'ing baku to see if something is feasible from the File/Blob side.

Comment 6

[Andrea Marchesini [:baku]]

What's happening here is that we end up using ChannelMediaResource instead of FileMediaResource. See https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaResource.cpp#1446

The reason why this happens is because:

1. the URL is actually a BlobURL.
2. We are able to retrieve the nsIInputStream using NS_GetStreamForBlobURI. This nsIInputStream is a IPCBlobInputStream because the blob belongs to the parent process.
3. but seekableStream = do_QueryInterface(stream) returns null.

an IPCBlobInputStream is seekable only if an AsyncWait() has been called.
AsyncWait() is used to retrieve the remote stream (in this scenario a nsFileInputStream). A callback is executed when the stream is ready, and from that moment, IPCBlobInputStream will become a proxy from/to the remote stream. Because nsFileInputStream is seekable, also IPCBlobInputStream will be seekable.

But AsyncWait() has not been called yet; a ChannelMediaResource is used; we end up copying all the content of the stream (the 1.5gb) in cache. And yes, this is super slow.

Note that this happens only with blobs created on the parent process, mainly via FilePicker or IDB.

How to fix it? Well, for bug 1377589, I wrote a patch that introduces nsIAsyncFileMetadata. This can be used to retrieve the FileDescriptor out of a IPCBlobInputStream, asynchronously. Maybe we can use this approach to create a similar FileMediaResource. This is going to land soon. I still need to fix a orange failure.

Alternatively, if the stream is not seekable, but it's an Blob URL, we can create a PendingMediaResource that calls AsyncWait(), when the callback is executed, we decide what to use between FileMediaResource and ChannelMediaResource.

Comment 7

[Andrea Marchesini [:baku]]

Note that this was a known issue: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaResource.cpp#1438-1440

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Any chance this could slide into 57, or are things too busy otherwise?

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Kyle Machulis [:qdot] [:kmachulis]  (if a patch has no decent commit message, automatic r-) from comment #8)
> Any chance this could slide into 57, or are things too busy otherwise?

I don't rate this as a super high priority because it is limited to the file picker. However, we can ask Blake to make a priority call within his team.

Blake - do you have someone available to work on this?

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

:baku had mentioned possibly being able to work on it when I talked to him on IRC last week, so I figured I'd see if he could do that before sending it elsewhere or looking at it myself. I'd wait for his reply before assigning.

Comment 11

[Blake Wu [:bwu][:blakewu]]

Great to have baku's help here!

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 1 - BlobImpl->GetSize() instead of ::Available()

In order to fix this bug I did some changes, moved code and so on. There are 4 patches. The last one is the real fix. The previous 3 can land separately, but would be nice to consider them as part of this bug. Up to the reviewer.

The first patch is about using BlobImpl->GetSize() instead of stream::Available. This is important because if the stream is nsIAsyncInputStream, Available() doesn't return the full size of the stream, but only what can be read at the moment. 0 is a valid return value for Available() also when the stream contains data, not ready to be read yet.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 2 - Moving FileMediaResource to separate files

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 3 - Moving ChannelMediaResource to separate files

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 4 - CloneableWithRangeMediaResource

CloneableWithRangeMediaResource is a new MediaResource for streams exposing nsICloneableInputStreamWithRange. These streams (IPCBlobInputStream only at the moment) are not seekable but they can be 'duplicated' with a range.
This operation is fast and it doesn't require any extra read().

More information can be read here: https://hg.mozilla.org/mozilla-central/file/tip/dom/file/ipc/IPCBlobUtils.h#l189

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8908546 [details] [diff] [review]
part 1 - BlobImpl->GetSize() instead of ::Available()

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

next time MozReview please..

review on bugzilla are terrible, as I must use an external editor to get some context.

::: dom/media/MediaResource.cpp
@@ +1402,5 @@
> +      return nullptr;
> +    }
> +
> +    // It's better to read the size from the blob instead of using ::Available,
> +    // becuase, if the stream implements nsIAsyncInputStream interface,

because

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8908548 [details] [diff] [review]
part 2 - Moving FileMediaResource to separate files

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

::: dom/media/FileMediaResource.cpp
@@ +12,5 @@
> +#include "nsNetUtil.h"
> +
> +namespace mozilla {
> +
> +void FileMediaResource::EnsureSizeInitialized()

per coding style:
returnType
classname::function_member(blah)

so void on a separate line.

I understand it was wrong to start with, but now there's no reason for it to be.

If we want this to just be a move.
can you then add another small patch (p3) that put this file to coding style?

thank you

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8908549 [details] [diff] [review]
part 3 - Moving ChannelMediaResource to separate files

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

I would even put those into its own mediaresource directory...

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8908553 [details] [diff] [review]
part 4 - CloneableWithRangeMediaResource

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

r=me with the removal of the use of mutex in CloneableWithRangeMediaReSource

The documentation in MediaResource.h may be amended to reflect that new constraint on MediaResource.

::: dom/media/CloneableWithRangeMediaResource.cpp
@@ +11,5 @@
> +namespace mozilla {
> +
> +namespace {
> +
> +class Reader final : public nsIInputStreamCallback

Can we make the name a bit more unique, one that is less likely to be used elsewhere?
InputStreamReader maybe?

@@ +107,5 @@
> +
> +      lock.Wait();
> +    }
> +
> +    return SyncRead(aBuffer, aSize, aRead);

Can we do this without recursion?

a do/while will do

::: dom/media/CloneableWithRangeMediaResource.h
@@ +102,5 @@
> +private:
> +  void MaybeInitialize();
> +
> +  // This lock protects mStream, mSize, mCurrentPosition and mInitialized.
> +  Mutex mLock;

I think we can do without a mutex here..

The threading model has greatly changed since the MediaResource API got conceived. Time to stop making those no longer relevant assumptions
a MediaResource is only ever read on the same thread (the demuxer taskqueue).

we only need mSize and mCurrentPosition to be atomic. Those values are informational only when accessed outside the demuxer's task queue. We don't really care what they are ... (even think we should remove it)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d2d46e29a15
FileMediaResource should use BlobImpl->Size() instead of stream::Available() to know the size, r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2aef354a6d
Move FileMediaResource to separate files, r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab042a1a2b84
Move ChannelMediaResource to separate files, r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff9ba1e5310
CloneableWithRangeMediaResource for streams implementing nsICloneableInputStreamWithRange, r=jya

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7d2d46e29a15
https://hg.mozilla.org/mozilla-central/rev/ab2aef354a6d
https://hg.mozilla.org/mozilla-central/rev/ab042a1a2b84
https://hg.mozilla.org/mozilla-central/rev/4ff9ba1e5310