755533 - "canplaythrough" doesn't fire with small media cache

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

When we have a small media cache, we sometimes (all the time?) don't fire the canplaythrough event when loading video.

Steps to reproduce:
1. Open about:config, set media.cache_size to 128 (so 128kB cache).
2. Open http://pearce.org.nz/video/events-webm.html in Firefox.
3. Observe that canplaythrough is never logged as firing.

Expected result:
We should fire canplaythrough when we suspend downloading the resource.

This is occurring in desktop Firefox as on B2G.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: Ensure we fire canplaythrough if the media's channel is suspended before metadata is loaded.

The problem is that the media cache is filling up and suspending the load, and so canplaythrough can't fire. Note that we actually don't fire canplaythrough when the download is suspended due to media cache being full, we only fire it when the entire resource is loaded into the cache.

So add a flag nsHTMLMediaElement::mDownloadSuspended to detect download suspended, and advance the readyState to HAVE_ENOUGH_DATA once we've past metadata loaded (we can fill the cache before the load has loaded metadata).

This means we'll fire canplaythrough whenever the download is suspended, even if we can't actually "play through".

Also rename mLoadIsSuspended to mSuspendedForPreloadNone to help make it clearer how these two flags differ.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 626327 [details] [diff] [review]
Patch: Ensure we fire canplaythrough if the media's channel is suspended before metadata is loaded.

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +2783,5 @@
>      mNetworkState = nsIDOMHTMLMediaElement::NETWORK_IDLE;
>      AddRemoveSelfReference();
>      DispatchAsyncEvent(NS_LITERAL_STRING("suspend"));
>    }
> +  mDownloadSuspended = true;

Seems to me this can happen when we suspend the download by calling mDecoder->Suspend() from FirstFrameLoaded and NotifyOwnerDocumentActivityChanged. We don't want to trigger HAVE_ENOUGH_DATA in those cases!

I think mDownloadSuspended needs to be mDownloadSuspendedByCache and we should set it from nsBuiltinDecoder::NotifySuspendedStatusChanged.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

Review comments addressed.

I had to only move to readyState HAVE_ENOUGH_DATA when the download is suspended by cache if !mDecoder->IsEnded(), otherwise when we'll switch to HAVE_ENOUGH_DATA when we really should only be in HAVE_CURRENT_DATA (test_play_twice was failing without this).

We also switch to HAVE_METADATA in SeekingStarted(). Without this we can remain in HAVE_ENOUGH_DATA throughout a seek operation, and so we won't fire "canplaythrough" after a seek if we suspended the download before the seek (test_bug493187 was failing without this).

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 627077 [details] [diff] [review]
Patch v2

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +2826,5 @@
> +    // transition, we will never fire the "canplaythrough" event if the
> +    // media cache is too small, and scripts are bound to fail. Don't force
> +    // this transition if the decoder is in ended state; the readyState
> +    // should remain at HAVE_CURRENT_DATA in this case.
> +    ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA);

We shouldn't transition from HAVE_METADATA to HAVE_ENOUGH_DATA. We should only do this if our state is HAVE_CURRENT_DATA or greater, so we should check that here.

That means in FirstFrameLoaded we may need to switch to HAVE_ENOUGH_DATA if mDownloadSuspendedByCache.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 627091 [details] [diff] [review]
Patch v3

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

r+ with that.

::: content/media/nsBuiltinDecoder.cpp
@@ +685,5 @@
> +      // If this is an autoplay element, we need to kick off its autoplaying
> +      // now so we consume data and hopefully free up cache space.
> +      mElement->NotifyAutoplayDataReady();
> +    }
> +    mElement->NotifySuspendedByCache(suspended);

I think we need to call UpdateReadyStateForData() here.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I addressed my own comment.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e77a9970de71

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

bustage fix
https://hg.mozilla.org/integration/mozilla-inbound/rev/0729364c8b30

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e77a9970de71
https://hg.mozilla.org/mozilla-central/rev/0729364c8b30