Closed Bug 1293861 Opened 7 years ago Closed 7 years ago

Intermittent dom/media/test/test_playback.html | bug506094.ogv, seek.yuv, bug498380.ogv, bug504613.ogv, bug523816.ogv checking networkState - got 2, expected 1

Categories

(Core :: Audio/Video: Playback, defect)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: kikuo)

References

Details

(Keywords: intermittent-failure, regression)

Attachments

(2 files)

Notify MediaCache that data is downloaded and cached to the end of resource
1.40 KB, patch
jwwang
: feedback-
Details | Diff | Splinter Review
Bug 1293861 - Remove the unnecessary check for networkState after receiving playback ended event.
58 bytes, text/x-review-board-request
jwwang
: review+
Details 
JW, this issue (along the other bugs) appears to have started since migrating the ogg reader to the OggDemuxer+MediaFormatReader.

Though, something else may have changed as it is same issue with bug 1293857, which has nothing to do with ogg/mp4/webm

Was anything changed lately related to the handling of MediaResource/MediaCache?

I don't see anything in the OggDemuxer that would cause it to access the MediaResource once EOS was reached.
See Also: → 1293860, 1293857, 1293866, 1293884
Flags: needinfo?(jwwang)
Summary: Intermittent dom/media/test/test_playback.html | bug506094.ogv checking networkState - got 2, expected 1 → Intermittent dom/media/test/test_playback.html | bug506094.ogv, seek.yuv, bug498380.ogv, bug504613.ogv, bug523816.ogv checking networkState - got 2, expected 1
Component: Audio/Video → Audio/Video: Playback
bug 1077294 used a more stringent checks against the networkState while receiving HTMLMediaElement's 'ended' event.  Originally, it checked |ok(v.networkState != v.NETWORK_LOADED)| where NETWORK_LOADED no longer exists. ==> useless check. 

So I modified to |is(v.networkState, v.NETWORK_IDLE)|.
I'll take this to see what's happening here. 
Thanks for the report.
Assignee: nobody → kikuo
Flags: needinfo?(kikuo)
Blocks: 1077294
Keywords: regression
Update,

HTMLMediaElement.networkState will transit from LOADING to IDLE after |MediaDecoder::ResourceCallback::NotifyDataEnded| is called.

In failure case, |MediaDecoder::ResourceCallback::NotifyDataEnded| is called after MDSM notifies the event |MediaEventType::PlaybackEnded|.

Though there seems no requirement in spec that HTMLMediaElement's "ended" event should be fired when networkState is IDLE, JW, I'm wondering if we should remove the networkState check in mochitest ?

But I'm going to dig deeper to find out why |MediaDecoder::ResourceCallback::NotifyDataEnded| is invoked so late.  There seems to be something weird happening.
Flags: needinfo?(jwwang)
I would prefer to keep the check because it doesn't make sense for download is not completed when playback reaches the end. Download complete should always happen before playback ends (or even before the last frame is decoded).
Flags: needinfo?(jwwang)
Is it possible for the file to contain extra bytes so you are able to decode the last frame without downloading the whole file?
Flags: needinfo?(jyavenard)
Update,

Resource begins to load after |HttpChannelChild::OnStartRequest| is called and |MediaDecoder::ResourceCallback::NotifyDataEnded| is triggered after |HttpChannelChild::OnStopRequest| is called.

The problem here is when system is busy, sometimes |HttpChannelParent::OnStopRequest| may be called a little-bit late and leads to the result in Comment 9.  I'm not sure if how far should I continue from here.  Later, I'll have a patch to fix it in another way, now I'm still testing.
(In reply to Kilik Kuo [:kikuo] from comment #13)
> The problem here is when system is busy, sometimes
> |HttpChannelParent::OnStopRequest| may be called a little-bit late

So OnStopRequest() is called asynchronously after downloading the whole file and it is possible to read every bytes of the file before OnStopRequest() is called on the main thread?
Hello JW, 

I found |ChannelMediaResource::NotifyLastByteRange()| is defined but never be called. It seems reasonable to have a check and notify MediaCache that the data is downloaded and cached to the end of resource, so that we won't need to care about the timing of invoking |HttpChannelChild::OnStopRequest|.

Or should we just remove the check to networkState ? It seems not to be a requirement by spec.  
But further more, networkState should also be affected mainly by network operation, it should not be affected only by the status of cached data [2].
In this case, I think [2] should be removed too, and we can transit networkState in HTMLMediaElement by checking the status of cached data when |HTMLMediaElement::MediaLoadListener::OnStopRequest| [3] is called.  Maybe another bug ?

What do you think ?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaResource.cpp#937-945
[2] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoder.cpp#270
[3] https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp?q=%2Bfunction%3Amozilla%3A%3Adom%3A%3AHTMLMediaElement%3A%3ADownloadSuspended%28%29&redirect_type=single#460
Attachment #8781888 - Flags: feedback?(jwwang)
Comment on attachment 8781888 [details] [diff] [review]
Notify MediaCache that data is downloaded and cached to the end of resource

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

::: dom/media/MediaResource.cpp
@@ +447,5 @@
>    closure->mResource->mCacheStream.NotifyDataReceived(aCount, aFromSegment,
>                                                        closure->mPrincipal);
>    *aWriteCount = aCount;
> +
> +  if (closure->mResource->IsDataCachedToEndOfResource(closure->mResource->mOffset)) {

It is racy since the client might read the last bytes of the stream off the main thread before this statement has a chance to run. I would suggest to just remove the checks from the mochitest.
Attachment #8781888 - Flags: feedback?(jwwang) → feedback-
Comment on attachment 8781911 [details]
Bug 1293861 - Remove the unnecessary check for networkState after receiving playback ended event.

https://reviewboard.mozilla.org/r/72232/#review69796
Attachment #8781911 - Flags: review?(jwwang) → review+
Comment on attachment 8781911 [details]
Bug 1293861 - Remove the unnecessary check for networkState after receiving playback ended event.

https://reviewboard.mozilla.org/r/72232/#review69796

Thanks for the review.
Keywords: checkin-needed
For future reference, please use commit messages that summarize what the patch is actually doing rather than restating the problem being fixed.
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions
(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)
> For future reference, please use commit messages that summarize what the
> patch is actually doing rather than restating the problem being fixed.
> https://developer.mozilla.org/en-US/docs/Mercurial/
> Using_Mercurial#Commit_Message_Conventions

Thanks for your reminder, I've modified the commit message.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/184313cc7fd8
Intermittent dom/media/test/test_playback.html | bug506094.ogv, seek.yuv, bug498380.ogv, bug504613.ogv, bug523816.ogv checking networkState - got 2, expected 1. r=jwwang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/184313cc7fd8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: needinfo?(jyavenard)
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.