Note: There are a few cases of duplicates in user autocompletion which are being worked on.

"canplaythrough" doesn't fire with small media cache

RESOLVED FIXED in mozilla15

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Assignee: nobody → cpearce
Blocks: 755609
(Assignee)

Comment 1

5 years ago
Created attachment 626327 [details] [diff] [review]
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.
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-
(Assignee)

Comment 3

5 years ago
Created attachment 627077 [details] [diff] [review]
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).
Attachment #626327 - Attachment is obsolete: true
Attachment #627077 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Summary: "canplaythrough" doesn't with small media cache → "canplaythrough" doesn't fire with small media cache
(Assignee)

Updated

5 years ago
Blocks: 568402
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.
(Assignee)

Comment 5

5 years ago
Created attachment 627091 [details] [diff] [review]
Patch v3
Attachment #627077 - Attachment is obsolete: true
Attachment #627077 - Flags: review?(roc)
Attachment #627091 - Flags: review?(roc)
Attachment #627091 - Attachment is patch: true
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+
I addressed my own comment.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e77a9970de71
bustage fix
https://hg.mozilla.org/integration/mozilla-inbound/rev/0729364c8b30

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/e77a9970de71
https://hg.mozilla.org/mozilla-central/rev/0729364c8b30
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15

Updated

5 years ago
Depends on: 784932
You need to log in before you can comment on or make changes to this bug.