1415461 - Consolidate the life cycle of the channel managed by ChannelSuspendAgent

Status: RESOLVED FIXED

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

Description

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

http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/dom/media/ChannelMediaResource.cpp#1042,1056

We should suspend/resume the channel after OnStartRequest() and before OnStopRequest() is fired.

It doesn't make sense to suspend/resume a channel after it is closed. Likewise, it doesn't make much sense either to suspend/resume a channel before OnStartRequest() is received.

Comment 1

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

Attachment: Bug 1415461 - consolidate the life cycle of the channel managed by ChannelSuspendAgent.

We want to suspend/resume a channel only after OnStartRequest()
and before OnStopRequest().

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

Comment 2

[Benjamin Chen [:bechen]]

Comment on attachment 8927118 [details]
Bug 1415461 - consolidate the life cycle of the channel managed by ChannelSuspendAgent.

https://reviewboard.mozilla.org/r/198340/#review203582

So this patch actually change the behavior of "suspend comes before OnStartRequest". Do we have testcases? Or should we add some assert?

Comment 3

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

Comment on attachment 8927118 [details]
Bug 1415461 - consolidate the life cycle of the channel managed by ChannelSuspendAgent.

https://reviewboard.mozilla.org/r/198340/#review203582

No, it is just a case we want to avoid as stated in comment 0. Consider the following case:
suspend -> OnChannelRedirect -> OnStartRequest

Without this change, we would suspend the old channel without resuming it again.

Comment 4

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

Comment on attachment 8927118 [details]
Bug 1415461 - consolidate the life cycle of the channel managed by ChannelSuspendAgent.

https://reviewboard.mozilla.org/r/198340/#review203646

::: dom/media/ChannelMediaResource.cpp:1076
(Diff revision 1)
> -  if (!mIsChannelSuspended && IsSuspended()) {
> +  MOZ_ASSERT(aChannel);
> +  MOZ_ASSERT(!mChannel, "The previous channel not closed.");
> +  MOZ_ASSERT(!mIsChannelSuspended);
> +
> +  mChannel = aChannel;
> +  // Ensure the suspend status of the channel match our suspend count.

'match' -> 'matches'

::: dom/media/ChannelMediaResource.cpp:1092
(Diff revision 1)
> -void
> -ChannelSuspendAgent::NotifyChannelClosing()
> -{
> -  MOZ_ASSERT(NS_IsMainThread());
> -  MOZ_ASSERT(mChannel);
>    // Before close the channel, it need to be resumed to make sure its internal

Since you're modifying nearby code: 'close' -> 'closing', 'need' -> 'needs'

Comment 5

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

Thanks for the reviews!

Comment 6

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

Comment on attachment 8927118 [details]
Bug 1415461 - consolidate the life cycle of the channel managed by ChannelSuspendAgent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198340/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/056e41a8b982
consolidate the life cycle of the channel managed by ChannelSuspendAgent. r=bechen,gerald

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/056e41a8b982