1108960 - HTMLMediaElement doesn't kick off autoplay when download suspended

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

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

Comment 1

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

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.

Comment 2

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

(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.

Comment 3

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

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

Comment 4

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

Let me add more logs before I can confirm that.

Comment 5

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

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?

Comment 6

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

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.

Comment 7

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

(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.

Comment 8

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

(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.

Comment 9

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

(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.

Comment 10

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

Attachment: 1108960_notify_cache_suspend_change.patch

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.

Comment 11

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

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".

Comment 12

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

Attachment: 1108960_notify_cache_suspend_change-v2.patch

Address nits.

Try: https://tbpl.mozilla.org/?tree=Try&rev=c86475871264
Most green on media tests. I guess those oranges not mine...

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf81114dbc71

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf81114dbc71