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

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Blocks 1 bug)

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Flags: needinfo?(roc)
Assignee

Updated

5 years ago
Blocks: 916399
Assignee

Comment 2

5 years ago
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)
Assignee

Comment 4

5 years ago
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)
Assignee

Comment 6

5 years ago
Posted 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+
Assignee

Comment 8

5 years ago
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.
Assignee

Comment 9

5 years ago
1. reset |mDidNotifyDataEnded| on channel recreated.
2. fix some error handling code.
Attachment #8420733 - Attachment is obsolete: true
Attachment #8420748 - Flags: review?(roc)
Assignee

Updated

5 years ago
Blocks: 987106
https://hg.mozilla.org/mozilla-central/rev/5ceb693e9733
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee

Updated

5 years ago
Blocks: 1010023
Assignee

Updated

4 years ago
Duplicate of this bug: 987106
You need to log in before you can comment on or make changes to this bug.