Closed Bug 1108960 Opened 5 years ago Closed 5 years ago

HTMLMediaElement doesn't kick off autoplay when download suspended

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file, 1 obsolete file)

We expect |mDownloadSuspendedByCache| [1] to be true when download reaches the end so the readyState can switch to HAVE_ENOUGH_DATA and kick off autoplay. However, since ChannelMediaResource::IsSuspendedByCache() checks if all streams for the resource are suspended, it will return false when there is still one stream downloading while other streams reach end of download.

Can we switch to HAVE_ENOUGH_DATA when download reaches the end without any error instead of checking IsSuspendedByCache? It seems to be a good idea to reduce dependency on the underlying implementation (the cache mechanism should be internal to a MediaResource).

[1] http://hg.mozilla.org/mozilla-central/file/f1f48ccb2d4e/dom/html/HTMLMediaElement.cpp#l3104
Flags: needinfo?(roc)
Btw, I think we should check if download is suspended (network status == NETWORK_IDLE) without any error since the cache size might be too small to hold the whole file.
(In reply to JW Wang [:jwwang] from comment #0)
> We expect |mDownloadSuspendedByCache| [1] to be true when download reaches
> the end so the readyState can switch to HAVE_ENOUGH_DATA and kick off
> autoplay. However, since ChannelMediaResource::IsSuspendedByCache() checks
> if all streams for the resource are suspended, it will return false when
> there is still one stream downloading while other streams reach end of
> download.

I'm a little confused about what the situation is here. Which ranges are buffered when this happens? I don't see how an autoplay element could download the entire resource but have another stream still reading from the network.
Flags: needinfo?(roc)
I will add more logs to show the buffer ranges.

The problem comes from test_preload_actions.html where we have timeouts in test15 [1] intermittently. Here are the events we got from test15:

16:34:30 INFO - 302 INFO test15-12: got loadstart
16:34:30 INFO - 303 INFO test15-12: got loadedmetadata
16:34:30 INFO - 304 INFO test15-12: got loadeddata
16:34:30 INFO - 305 INFO test15-12: got canplay
16:34:30 INFO - 306 INFO test15-12: got suspend

Since 'autoplay' is true, there is no  way to suspend download after 1st frame loaded [2]. So the 'suspend' event comes from download end. Since we've reached the end of download, we should have |mDownloadSuspendedByCache == true| and readyState will switch to HAVE_ENOUGH_DATA. However, we didn't see 'canplaythrough' in the logs. Therefore, I assume mDownloadSuspendedByCache is false due to the problem stated in comment 0.

Btw, all tests in test_preload_actions.html play the same file and we have multiple streams for the same resource which is the condition described in comment 0.

[1] http://hg.mozilla.org/mozilla-central/file/47f0671e2c65/dom/media/test/test_preload_actions.html#l490
[2] http://hg.mozilla.org/mozilla-central/file/47f0671e2c65/dom/html/HTMLMediaElement.cpp#l2935
[3] http://hg.mozilla.org/mozilla-central/file/f1f48ccb2d4e/dom/html/HTMLMediaElement.cpp#l3104
Flags: needinfo?(roc)
Let me add more logs before I can confirm that.
Flags: needinfo?(roc)
Here is the buffer range when 'suspend' is received:

73 INFO test15-12: got suspend
74 INFO test15-12: ranges=2
75 INFO test15-12: start[0]=0, end[0]=1.699983
76 INFO test15-12: start[1]=3.733296, end[1]=3.99996

I come to realize that download is suspended not because the download reaches the end but because it is suspended by the cache. And ChannelMediaResource::IsSuspendedByCache() returns false for there are still other streams downloading for the same resource. Therefore, autoplay doesn't kick off for the media element whose download is suspended.

I wonder why ChannelMediaResource::IsSuspendedByCache() needs to consider all streams for the same resource instead of the one gets suspended?
Flags: needinfo?(roc)
Because in general multiple channels can share a single stream. E.g. if several autoplay elements for the same resource are created at the same time, we don't want to create multiple streams all reading the same data, but we don't want to autoplay some of them immediately just because they don't have their own stream.

It's still not totally clear to me what's going on here. As long as there is at least one stream still reading, it seems to me we should eventually fire canplaythrough --- because eventually that stream is going to be suspended or read all the data or the cache becomes full.
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> It's still not totally clear to me what's going on here. As long as there is
> at least one stream still reading, it seems to me we should eventually fire
> canplaythrough --- because eventually that stream is going to be suspended
> or read all the data or the cache becomes full.

Below is a simplified case of test_preload_actions.html where all tests read the same file.

Suppose we have 2 media elements A and B, both reading the same file, say seek.webm. Now we have A's ChannelMediaResource::CacheClientSuspend is called by MediaCache::Update to suspend the download of A while B's download is still continuing. ChannelMediaResource::IsSuspendedByCache() returns false for A for B is still downloading. By the time B's ChannelMediaResource reads all data or cache becomes full, ChannelMediaResource::IsSuspendedByCache() will return true for B. However, A is not notified about this and A's autoplay will never kick off.

It then appears to me that when cache suspend/resume status changes, all streams for the same resource should be notified instead of the one gets suspended/resumed.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Because in general multiple channels can share a single stream. E.g. if
> several autoplay elements for the same resource are created at the same
> time, we don't want to create multiple streams all reading the same data,

IIRC, AddMediaElementToURITable() is not called until HTMLMediaElement::FinishDecoderSetup. If we have several media elements reading the same file at the same, we will fail to look up the table and MediaCache will allocate multiple resource ids for the same file. There is no resource sharing/cloning for that case.
Flags: needinfo?(roc)
(In reply to JW Wang [:jwwang] from comment #7)
> Suppose we have 2 media elements A and B, both reading the same file, say
> seek.webm. Now we have A's ChannelMediaResource::CacheClientSuspend is
> called by MediaCache::Update to suspend the download of A while B's download
> is still continuing. ChannelMediaResource::IsSuspendedByCache() returns
> false for A for B is still downloading. By the time B's ChannelMediaResource
> reads all data or cache becomes full,
> ChannelMediaResource::IsSuspendedByCache() will return true for B. However,
> A is not notified about this and A's autoplay will never kick off.
> 
> It then appears to me that when cache suspend/resume status changes, all
> streams for the same resource should be notified instead of the one gets
> suspended/resumed.

Yes, that sounds right.

(In reply to JW Wang [:jwwang] from comment #8)
> IIRC, AddMediaElementToURITable() is not called until
> HTMLMediaElement::FinishDecoderSetup. If we have several media elements
> reading the same file at the same, we will fail to look up the table and
> MediaCache will allocate multiple resource ids for the same file. There is
> no resource sharing/cloning for that case.

Good point. That seems like a bug but it may not matter much in practice.
Flags: needinfo?(roc)
Summary: HTMLMediaElement doesn't kick off autoplay when download ended → HTMLMediaElement doesn't kick off autoplay when download suspended
MediaCacheStream::AreAllStreamsForResourceSuspended depends on all streams for the resourceID.

We should notify all steams for the resource ID about cache suspended status changes when any stream get closed/suspended/resumed.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Attachment #8538986 - Flags: review?(roc)
Comment on attachment 8538986 [details] [diff] [review]
1108960_notify_cache_suspend_change.patch

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

::: dom/media/MediaResource.h
@@ +537,5 @@
>    // received data for the same resource.
>    void CacheClientNotifyDataEnded(nsresult aStatus);
>    // Notify that the principal for the cached resource changed.
>    void CacheClientNotifyPrincipalChanged();
> +  // Notify the decoder that the cache suspended status changes.

"changed", not "changes".
Attachment #8538986 - Flags: review?(roc) → review+
Address nits.

Try: https://tbpl.mozilla.org/?tree=Try&rev=c86475871264
Most green on media tests. I guess those oranges not mine...
Attachment #8538986 - Attachment is obsolete: true
Attachment #8539060 - Flags: review+
Blocks: 886188
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cf81114dbc71
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Blocks: 1388612
You need to log in before you can comment on or make changes to this bug.