Move the MediaCache off of opening its temporary file fd synchronously in the content process

RESOLVED FIXED in Firefox 55

Status

()

Core
Audio/Video: Playback
P1
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: Ehsan, Assigned: cpearce)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

MediaCache::Init() opens mFileCache on the main thread synchronously right now, and we need to make this async in the content process at least (I'm hoping this isn't used in the parent process!)

This is used in two places: the first time gMediaCache is initialized, and when Flush() is called.

Right now, gMediaCache is initialized the first time a media element starts to receive data, or when last-pb-context-exited or cacheservice:empty-cache is dispatched.  The latter two are easy to deal with since they are initiated from the parent process.  Right now I'm planning to send an async request to initialize the cache at the time when gMediaCache gets initialized.  This runs the risk of not getting our cache file fd by the time we start receiving data, so I'd like to check to see whether that risk is acceptable here or not.  Alternative we could for example move this to happen shortly after we start the content process, but opening the file unconditionally seems wasteful.

What do you think, Chris?
Flags: needinfo?(cpearce)
(Assignee)

Comment 1

3 months ago
(In reply to :Ehsan Akhgari from comment #0)
> MediaCache::Init() opens mFileCache on the main thread synchronously right
> now, and we need to make this async in the content process at least (I'm
> hoping this isn't used in the parent process!)

In theory, there's no reason why this couldn't be used from the parent process. So we need to handle that case I think.


> This is used in two places: the first time gMediaCache is initialized, and
> when Flush() is called.
> 
> Right now, gMediaCache is initialized the first time a media element starts
> to receive data, or when last-pb-context-exited or cacheservice:empty-cache
> is dispatched.  The latter two are easy to deal with since they are
> initiated from the parent process.  Right now I'm planning to send an async
> request to initialize the cache at the time when gMediaCache gets
> initialized.  This runs the risk of not getting our cache file fd by the
> time we start receiving data, so I'd like to check to see whether that risk
> is acceptable here or not.  Alternative we could for example move this to
> happen shortly after we start the content process, but opening the file
> unconditionally seems wasteful.
> 
> What do you think, Chris?

I think a simpler solution would be to push down the request for a temporary file handle into the FileBlockCache itself. It already buffers writes and defers then to another thread. So adding another async step before the FileBlockCache::Run() processes the change list that waits until the file descriptor is valid should be pretty easy.
Component: Audio/Video → Audio/Video: Playback
Flags: needinfo?(cpearce)
(Assignee)

Comment 2

3 months ago
Ehsan: I can take this bug if you like. It's up my alley, which makes a nice change for a quantum-flow bug.
Priority: -- → P1
Yes, I would appreciate your help!  I'm landing bug 1346987 on inbound so you can use IPC code from that bug.  :-)
(Maybe we can do something like EncodedBufferCache::AppendBuffer() and reuse much more code)
Assignee: ehsan → cpearce
Whiteboard: [qf:p1]
Comment hidden (mozreview-request)
(Assignee)

Comment 6

3 months ago
Note: the patches here are based on top of bug 1346987, which was backed out.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

3 months ago
mozreview-review
Comment on attachment 8847874 [details]
Bug 1347031 - Move the MediaCache off of opening its temporary file fd synchronously in the content process.

https://reviewboard.mozilla.org/r/120790/#review122808

::: dom/media/FileBlockCache.cpp:141
(Diff revision 3)
>  
>    return NS_OK;
>  }
>  
> +RefPtr<GenericPromise>
> +FileBlockCache::EnsureInitialized()

It will be a lot simpler to do EnsureInitialized() in Init() once and for all.

1. Since Init() is called on the main thread, you don't need to dispatch a task to call ContentChild::AsyncOpenAnonymousTemporaryFile().
2. You don't need locks for synchronization for all code should run after Init() is done.

RefPtr<GenericPromise::Private> mInitPromise;

void EnsureInitialized()
{
  if (XRE_IsParentProcess()) {
    nsresult rv = NS_OpenAnonymousTemporaryFile(&mFD);
    mInitPromise = NS_SUCCEEDED(rv)
                 ? GenericPromise::CreateAndResolve()
                 : GenericPromise::CreateAndReject();
    return;
  }
  mInitPromise = new GenericPromise::Private();
  dom::ContentChild::GetSingleton()->AsyncOpenAnonymousTemporaryFile(
    [self](PRFileDesc* aFD) {
      self->mFD = aFD; // No synchronization since all access to mFD happens after mInitPromise is resolved.
      self->mInitPromise->Resolve(); // Resolve() is thread-safe.
      // Handle aFD is null.
    }
  );
}

void EnsureWriteScheduled()
{
  mInitPromise->Then(
    mAbstractThread, __func__,
    []() { self->Run(); } // self->Run() should only happen mInitPromise is resolved.
    []() {} // Do nothing since this is already handled by EnsureInitialized().
  );
}
Comment hidden (mozreview-request)

Comment 11

3 months ago
mozreview-review
Comment on attachment 8847874 [details]
Bug 1347031 - Move the MediaCache off of opening its temporary file fd synchronously in the content process.

https://reviewboard.mozilla.org/r/120790/#review123244

::: dom/media/FileBlockCache.cpp:62
(Diff revision 4)
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> -    MonitorAutoLock mon(mDataMonitor);
> +  MonitorAutoLock mon(mDataMonitor);
> -    nsresult res = NS_NewNamedThread("FileBlockCache",
> +  nsresult rv;
> +
> +  if (XRE_IsParentProcess()) {

Move this below NS_NewNamedThread(). So SetCacheFile() always sees mIsOpen is true.

::: dom/media/FileBlockCache.cpp:67
(Diff revision 4)
> +  if (XRE_IsParentProcess()) {
> +    rv = NS_OpenAnonymousTemporaryFile(&mFD);
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    mInitPromise->Resolve(true, __func__);

It is a null-deref here. Need to create an object first.
Attachment #8847874 - Flags: review?(jwwang) → review+
(Assignee)

Comment 12

3 months ago
mozreview-review
Comment on attachment 8847874 [details]
Bug 1347031 - Move the MediaCache off of opening its temporary file fd synchronously in the content process.

https://reviewboard.mozilla.org/r/120790/#review123276

::: dom/media/FileBlockCache.cpp:62
(Diff revision 4)
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> -    MonitorAutoLock mon(mDataMonitor);
> +  MonitorAutoLock mon(mDataMonitor);
> -    nsresult res = NS_NewNamedThread("FileBlockCache",
> +  nsresult rv;
> +
> +  if (XRE_IsParentProcess()) {

I deliberately moved it up so that if NS_OpenAnonymousTemporaryFile fails we don't need to worry about shutting down the thread.

AsyncOpenAnonymousTemporaryFile is async, so we can't get a callback to SetCacheFile befofe mIsOpen is initialized.

::: dom/media/FileBlockCache.cpp:67
(Diff revision 4)
> +  if (XRE_IsParentProcess()) {
> +    rv = NS_OpenAnonymousTemporaryFile(&mFD);
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +    mInitPromise->Resolve(true, __func__);

Very true, thanks!
Comment hidden (mozreview-request)

Comment 14

3 months ago
mozreview-review-reply
Comment on attachment 8847874 [details]
Bug 1347031 - Move the MediaCache off of opening its temporary file fd synchronously in the content process.

https://reviewboard.mozilla.org/r/120790/#review123276

> I deliberately moved it up so that if NS_OpenAnonymousTemporaryFile fails we don't need to worry about shutting down the thread.
> 
> AsyncOpenAnonymousTemporaryFile is async, so we can't get a callback to SetCacheFile befofe mIsOpen is initialized.

You still need to worry about the thread shutdown when AsyncOpenAnonymousTemporaryFile fails. The error handling is simpler if open-file is done after creating a thread since you always need to shut down the thread no matter it is in parent or child process. It is less performant though. However, given the chance is low failing to open a file handle, I prefer simplicity over performance.
(Assignee)

Comment 15

3 months ago
mozreview-review-reply
Comment on attachment 8847874 [details]
Bug 1347031 - Move the MediaCache off of opening its temporary file fd synchronously in the content process.

https://reviewboard.mozilla.org/r/120790/#review123276

> You still need to worry about the thread shutdown when AsyncOpenAnonymousTemporaryFile fails. The error handling is simpler if open-file is done after creating a thread since you always need to shut down the thread no matter it is in parent or child process. It is less performant though. However, given the chance is low failing to open a file handle, I prefer simplicity over performance.

Ah yes, I had neglected to check whether the AsyncOpenAnonymousTemporaryFile call itself had failed to send to the parent process. I'll check that and re-arrange as you suggest. Thanks!
Comment hidden (mozreview-request)

Comment 17

3 months ago
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b0d4d072cf0
Move the MediaCache off of opening its temporary file fd synchronously in the content process. r=jwwang

Comment 18

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b0d4d072cf0
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

3 months ago
Depends on: 1349746
You need to log in before you can comment on or make changes to this bug.