Closed Bug 1001317 Opened 6 years ago Closed 6 years ago

MediaCacheStream doesn't call ChannelMediaResource::CacheClientNotifyDataEnded on 2nd download end

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jwwang, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

In MediaCacheStream::NotifyDataEnded(), |mDidNotifyDataEnded| is set true for the first time download end and call ChannelMediaResource::CacheClientNotifyDataEnded(). However, ChannelMediaResource will close and open another channel for download upon CacheClientSeek(). And MediaCacheStream will not notify its client for the 2nd time download end. This seems to be wrong.

Should we reset |mDidNotifyDataEnded| somewhere when ChannelMediaResource opens another channel as we reset |mChannelEnded| in MediaCache::Update() upon action=SEEK_AND_RESUME.
Flags: needinfo?(roc)
Blocks: 916399
I found sometimes the cloned ChannelMediaResource will open a channel for downloading, that is there are 2 channels (one from the original ChannelMediaResource and the other from the clone) downloading data to the same resource id. Is this a normal case?
Flags: needinfo?(roc)
Yes. That is necessary when two media elements are reading from different parts of the resource and the whole resource cannot fit in cache.
Flags: needinfo?(roc)
Then MediaCacheStream::NotifyDataEnded should only call CacheClientNotifyDataEnded on the client which doesn't have its own channel. Otherwise those ChannelMediaResource which have their own channels will call MediaCacheStream::NotifyDataEnded again when they reach the end of download data.

In a sense, when a ChannelMediaResource open its own channel, it is not regarded as a 'clone' anymore.

However, since the cloned stream shares the same resource id and blocks with the original, and we allow multiple channels downloading data to the same set of resource blocks, the resource blocks should be treated as complete when any of the channel reaches the end.

Which makes more sense?
Flags: needinfo?(roc)
I think our current code is *nearly* correct. It's true that MediaDecoder::NotifyDownloadEnded can be called even while the decoder's own MediaCacheStream is still downloading, but that's OK. It will only be called with a successful aStatus (because MediaCacheStream::NotifyDataEnded makes a failed load disconnect the resource from all other resources). On success, we call UpdatePlaybackRate (which should be safe), UpdateReadyStateForData (also safe), and ResourceLoaded(). It's ResourceLoaded() which is the problem. Really, ResourceLoaded() should be removed, since it doesn't make sense to take special actions just because an HTTP read reached the end of the resource --- it doesn't mean we've really finished loading the video.

Bug 883731 was supposed to fix that, but I never got around to debugging the test failure(s). Maybe you should try that :-).
Flags: needinfo?(roc)
Attached patch 1001317_fix_MediaCache.patch (obsolete) — Splinter Review
1. reset |mDidNotifyDataEnded| on channel recreated.
2. fix some error handling code.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Attachment #8420733 - Flags: feedback?(bechen)
Comment on attachment 8420733 [details] [diff] [review]
1001317_fix_MediaCache.patch

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

::: content/media/MediaCache.cpp
@@ +1874,2 @@
>      }
>    }

Remove the parentheses outside.

@@ +1883,5 @@
> +{
> +  mChannelEnded = false;
> +  mDidNotifyDataEnded = false;
> +}
> +

Please add thread checking such as NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");

Do we need monitor to protect these two variables?
Attachment #8420733 - Flags: feedback?(bechen) → feedback+
Comment on attachment 8420733 [details] [diff] [review]
1001317_fix_MediaCache.patch

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

::: content/media/MediaCache.cpp
@@ +1883,5 @@
> +{
> +  mChannelEnded = false;
> +  mDidNotifyDataEnded = false;
> +}
> +

Yes, we need a monitor. Thanks for catching this.
1. reset |mDidNotifyDataEnded| on channel recreated.
2. fix some error handling code.
Attachment #8420733 - Attachment is obsolete: true
Attachment #8420748 - Flags: review?(roc)
Blocks: 987106
https://hg.mozilla.org/mozilla-central/rev/5ceb693e9733
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Blocks: 1010023
Duplicate of this bug: 987106
You need to log in before you can comment on or make changes to this bug.