1415090 - Move "reopen on error" off the main thread

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1415090. P1 - always move the channel back to the foreground when OnStopRequest() is fired.

It is a good practice to make the call flow simplier. It also makes
the changes in the following patches easier.

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

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1415090. P2 - move the "reopen on error" code from ChannelMediaResource::OnStopRequest() to MediaCacheStream::NotifyDataEnded().

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

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1415090. P3 - run MediaCacheStream::NotifyDataEnded() off the main thread.

Since NotifyDataEnded() run its code asynchronously, it is possible that a new
channel is created and NotifyDataStarted() is called before NotifyDataEndedInternal()
has a chance to run. We check the load ID to exit the function if necessary.

We also need to fix data races when running NotifyDataEndedInternal() off the
main thread in next patches.

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

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1415090. P4 - don't modify mResourceID off the main thread.

There are some works to do when we allow a stream whose download ends abnormally
to continue sharing the resource:

1. Abort Read() when download error happens. We might still have a chance to
   get all the data successfully. However, it doesn't really matter since
   the stream data is incomplete and we will encounter decode errors sooner
   or later.
2. Update() needs to check mChannelEnded since an ended stream will not
   download data needed by other streams.

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

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1415090. P5 - remove MediaCacheStream::NotifyChannelRecreated().

NotifyDataEnded() runs off the main thread which might set mChannelEnded
wrongly after NotifyChannelRecreated reset it on the main thread.

We should reset the flags in NotifyDataStarted() which indicate a new load has begun.

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

Comment 6

[Benjamin Chen [: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

Comment 7

[Benjamin Chen [:bechen]]

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

Comment 8

[Benjamin Chen [:bechen]]

Comment on attachment 8929329 [details]
Bug 1415090. P5 - remove MediaCacheStream::NotifyChannelRecreated().

https://reviewboard.mozilla.org/r/200650/#review205854

Comment 9

[Benjamin Chen [:bechen]]

Comment on attachment 8929327 [details]
Bug 1415090. P3 - run MediaCacheStream::NotifyDataEnded() off the main thread.

https://reviewboard.mozilla.org/r/200646/#review205874

Comment 10

[Benjamin Chen [:bechen]]

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?

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

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 12

[Benjamin Chen [:bechen]]

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 13

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 14

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

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

Comment 15

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

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

Comment 16

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

Comment on attachment 8929327 [details]
Bug 1415090. P3 - run MediaCacheStream::NotifyDataEnded() off the main thread.

https://reviewboard.mozilla.org/r/200646/#review206152

Comment 17

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

Comment on attachment 8929328 [details]
Bug 1415090. P4 - don't modify mResourceID off the main thread.

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

Comment 18

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

Comment on attachment 8929329 [details]
Bug 1415090. P5 - remove MediaCacheStream::NotifyChannelRecreated().

https://reviewboard.mozilla.org/r/200650/#review206156

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the reviews!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/846d1bec4f36
https://hg.mozilla.org/mozilla-central/rev/94287653dc31
https://hg.mozilla.org/mozilla-central/rev/d712d60574b6
https://hg.mozilla.org/mozilla-central/rev/222597bfd33b
https://hg.mozilla.org/mozilla-central/rev/fdb2a41005f2