Closed Bug 1415090 Opened 2 years ago Closed 2 years ago

Move "reopen on error" off the main thread

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(5 files)

http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/dom/media/ChannelMediaResource.cpp#387-396

Calling GetLength() and GetOffset() will need to acquire the cache lock and might block the main thread. We will move the code to the owner thread of MediaCache so we won't block the main thread.
Assignee: nobody → jwwang
Blocks: 1369263
Priority: -- → P3
Attachment #8929325 - Flags: review?(bechen)
Attachment #8929326 - Flags: review?(bechen)
Attachment #8929327 - Flags: review?(bechen)
Attachment #8929328 - Flags: review?(bechen)
Attachment #8929329 - Flags: review?(bechen)
Comment on attachment 8929325 [details]
Bug 1415090. P1 - always move the channel back to the foreground when OnStopRequest() is fired.

https://reviewboard.mozilla.org/r/200642/#review205826
Attachment #8929325 - Flags: review?(bechen) → review+
Comment on attachment 8929326 [details]
Bug 1415090. P2 - move the "reopen on error" code from ChannelMediaResource::OnStopRequest() to MediaCacheStream::NotifyDataEnded().

https://reviewboard.mozilla.org/r/200644/#review205838
Attachment #8929326 - Flags: review?(bechen) → review+
Comment on attachment 8929329 [details]
Bug 1415090. P5 - remove MediaCacheStream::NotifyChannelRecreated().

https://reviewboard.mozilla.org/r/200650/#review205854
Attachment #8929329 - Flags: review?(bechen) → review+
Comment on attachment 8929327 [details]
Bug 1415090. P3 - run MediaCacheStream::NotifyDataEnded() off the main thread.

https://reviewboard.mozilla.org/r/200646/#review205874
Attachment #8929327 - Flags: review?(bechen) → review+
Comment on attachment 8929328 [details]
Bug 1415090. P4 - don't modify mResourceID off the main thread.

https://reviewboard.mozilla.org/r/200648/#review205876

::: dom/media/MediaCache.cpp
(Diff revision 1)
> -  if (NS_FAILED(aStatus)) {
> -    // Disconnect from other streams sharing our resource, since they
> -    // should continue trying to load. Our load might have been deliberately
> -    // canceled and that shouldn't affect other streams.
> -    mResourceID = mMediaCache->AllocateResourceID();
> -  }

Why remove AllocateResourceID()? Is the code useless?
Attachment #8929328 - Flags: review?(bechen) → review+
Comment on attachment 8929328 [details]
Bug 1415090. P4 - don't modify mResourceID off the main thread.

https://reviewboard.mozilla.org/r/200648/#review205876

> Why remove AllocateResourceID()? Is the code useless?

mResourceID can only be modified on the main thread while NotifyDataEndedInternal() runs off the main thread.

As the commit message said, this patch is about safely making a stream whose download ends abnormally
to continue sharing the resource.
Comment on attachment 8929328 [details]
Bug 1415090. P4 - don't modify mResourceID off the main thread.

https://reviewboard.mozilla.org/r/200648/#review205878

Is is possible to merge the MediaCacheStream::mResourceID and MediaCacheStream::mLoadID?
Comment on attachment 8929328 [details]
Bug 1415090. P4 - don't modify mResourceID off the main thread.

https://reviewboard.mozilla.org/r/200648/#review205878

No. They are totally different ideas.

mResourceID: MediaCacheStreams with the same resource id will share the same resource. That is, when one MediaCacheStream commit a block to the cache, this block will be also visible to others streams sharing the same resource id.

mLoadID: Used to identify different channels within a single MediaCacheStream. Since NotifyDataStarted() runs on the main thread while NotifyDataReceived() runs off the main thread, it is possible for NotifyDataStarted() from the new channel to run before NotifyDataReceived() from the old channel. mLoadID is used to check whether the callbacks are from the old channel and should be aborted.
Attachment #8929325 - Flags: review?(gsquelart)
Attachment #8929326 - Flags: review?(gsquelart)
Attachment #8929327 - Flags: review?(gsquelart)
Attachment #8929328 - Flags: review?(gsquelart)
Attachment #8929329 - Flags: review?(gsquelart)
Comment on attachment 8929325 [details]
Bug 1415090. P1 - always move the channel back to the foreground when OnStopRequest() is fired.

https://reviewboard.mozilla.org/r/200642/#review206148
Attachment #8929325 - Flags: review?(gsquelart) → review+
Comment on attachment 8929326 [details]
Bug 1415090. P2 - move the "reopen on error" code from ChannelMediaResource::OnStopRequest() to MediaCacheStream::NotifyDataEnded().

https://reviewboard.mozilla.org/r/200644/#review206150
Attachment #8929326 - Flags: review?(gsquelart) → review+
Comment on attachment 8929327 [details]
Bug 1415090. P3 - run MediaCacheStream::NotifyDataEnded() off the main thread.

https://reviewboard.mozilla.org/r/200646/#review206152
Attachment #8929327 - Flags: review?(gsquelart) → review+
Comment on attachment 8929328 [details]
Bug 1415090. P4 - don't modify mResourceID off the main thread.

https://reviewboard.mozilla.org/r/200648/#review206154
Attachment #8929328 - Flags: review?(gsquelart) → review+
Comment on attachment 8929329 [details]
Bug 1415090. P5 - remove MediaCacheStream::NotifyChannelRecreated().

https://reviewboard.mozilla.org/r/200650/#review206156
Attachment #8929329 - Flags: review?(gsquelart) → review+
Thanks for the reviews!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/846d1bec4f36
P1 - always move the channel back to the foreground when OnStopRequest() is fired. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/94287653dc31
P2 - move the "reopen on error" code from ChannelMediaResource::OnStopRequest() to MediaCacheStream::NotifyDataEnded(). r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/d712d60574b6
P3 - run MediaCacheStream::NotifyDataEnded() off the main thread. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/222597bfd33b
P4 - don't modify mResourceID off the main thread. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/fdb2a41005f2
P5 - remove MediaCacheStream::NotifyChannelRecreated(). r=bechen,gerald
No longer depends on: 1424937
Depends on: 1424937
Depends on: 1450607
Blocks: 1450607
No longer depends on: 1450607
No longer blocks: 1450607
Depends on: 1450607
You need to log in before you can comment on or make changes to this bug.