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

RESOLVED FIXED in Firefox 51

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Treeherder Bug Filer, Assigned: kikuo)

Tracking

({intermittent-failure, regression})

Trunk
mozilla51
intermittent-failure, regression
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

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.
Flags: needinfo?(jwwang)

Updated

2 years ago
Duplicate of this bug: 1293857

Updated

2 years ago
Duplicate of this bug: 1293860

Updated

2 years ago
Duplicate of this bug: 1293866

Updated

2 years ago
Duplicate of this bug: 1293884

Updated

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

Comment 7

2 years ago
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

Comment 8

2 years ago
30 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* autoland: 12
* mozilla-inbound: 11
* fx-team: 5
* mozilla-central: 2

Platform breakdown:
* linux64: 24
* linux32: 6

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1293861&startday=2016-08-08&endday=2016-08-14&tree=all
(Assignee)

Comment 9

2 years ago
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)

Comment 12

2 years ago
25 automation job failures were associated with this bug yesterday.

Repository breakdown:
* autoland: 15
* mozilla-inbound: 7
* mozilla-central: 2
* fx-team: 1

Platform breakdown:
* linux64: 19
* linux32: 6

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1293861&startday=2016-08-15&endday=2016-08-15&tree=all
(Assignee)

Comment 13

2 years ago
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.

Comment 14

2 years ago
27 automation job failures were associated with this bug yesterday.

Repository breakdown:
* autoland: 16
* mozilla-inbound: 8
* fx-team: 3

Platform breakdown:
* linux64: 21
* linux32: 6

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1293861&startday=2016-08-16&endday=2016-08-16&tree=all
(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?
(Assignee)

Comment 16

2 years ago
Created attachment 8781888 [details] [diff] [review]
Notify MediaCache that data is downloaded and cached to the end of resource

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 hidden (mozreview-request)

Comment 19

a year ago
mozreview-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
Attachment #8781911 - Flags: review?(jwwang) → review+
(Assignee)

Comment 20

a year ago
mozreview-review-reply
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.
(Assignee)

Updated

a year ago
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
Comment hidden (mozreview-request)
(Assignee)

Comment 24

a year ago
(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.

Comment 25

a year ago
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
23 automation job failures were associated with this bug yesterday.

Repository breakdown:
* autoland: 10
* mozilla-inbound: 8
* fx-team: 3
* mozilla-central: 2

Platform breakdown:
* linux64: 19
* linux32: 4

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1293861&startday=2016-08-17&endday=2016-08-17&tree=all

Comment 27

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/184313cc7fd8
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: needinfo?(jyavenard)
109 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* autoland: 46
* mozilla-inbound: 45
* fx-team: 10
* try: 4
* mozilla-central: 4

Platform breakdown:
* linux64: 91
* linux32: 18

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1293861&startday=2016-08-15&endday=2016-08-21&tree=all
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.