Closed Bug 755533 Opened 10 years ago Closed 10 years ago

"canplaythrough" doesn't fire with small media cache


(Core :: Audio/Video, defect)

Not set





(Reporter: cpearce, Assigned: cpearce)





(1 file, 2 obsolete files)

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 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.
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.
Attachment #626327 - Flags: review?(roc)
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.
Attachment #626327 - Flags: review?(roc) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
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).
Attachment #626327 - Attachment is obsolete: true
Attachment #627077 - Flags: review?(roc)
Summary: "canplaythrough" doesn't with small media cache → "canplaythrough" doesn't fire with small media cache
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.
Attached patch Patch v3Splinter Review
Attachment #627077 - Attachment is obsolete: true
Attachment #627077 - Flags: review?(roc)
Attachment #627091 - Flags: review?(roc)
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.
Attachment #627091 - Flags: review?(roc) → review+
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Depends on: 784932
You need to log in before you can comment on or make changes to this bug.