Closed Bug 1140675 Opened 5 years ago Closed 3 years ago

HTMLMediaElement::UpdateReadyStateForData remains stuck at HAVE_METADATA because GetImageContainer()->HasCurrentImage() is false

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: bholley, Assigned: ctai)

References

Details

Attachments

(2 files)

I've been doing some investigation, and I think this is the cause of the timeouts in the three tests we've been seeing (test_eme_stream_capture_blocked.html, test_eme_canvas_blocked.html, and test_bug879717.html). I managed to get some logging out of test_bug879717.html to form my theory, and the eme test hangs seem readyState-related, so I'm guessing it's the same issue.

The problem is that sometimes, for one of the videos in these tests, we get stuck here:

https://hg.mozilla.org/mozilla-central/annotate/7d85ac833cff/dom/html/HTMLMediaElement.cpp#l3407

I can certainly see how this might happen in the current code, because nothing guarantees that we'll invoke UpdateReadyState after the RenderVideoFrame call in MediaDecoderStateMachine::FinishDecodeFirstFrame. However, even when I fix that, the orange still appears, so that can't be the whole story.

These tests are failing frequently enough that I'm going to disable them on win7 opt while we sort this out. I'll also upload my patch that doesn't seem to fully fix the problem.
This doesn't appear to fully fix the problem. Whatever eventual solution we
land, we should also land the logging in this patch.
Tests disabled while we sort this out: https://hg.mozilla.org/integration/mozilla-inbound/rev/826e9c97526b

Flagging edwin to continue his investigation.
Flags: needinfo?(edwin)
Blocks: 1124538
https://hg.mozilla.org/mozilla-central/rev/826e9c97526b
Assignee: nobody → bobbyholley
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Meant to leave this open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Almost certainly another regression from bug 1131638. I'll back that out today until I figure out a better solution.
Will need to uplift I suspect if I uplift other changes...
Flags: needinfo?(cpearce)
(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> Almost certainly another regression from bug 1131638. I'll back that out
> today until I figure out a better solution.

Makes sense. See bug 1123469 comment 37 for the first Windows failure (used to be some occasional b2g desktop only failures) on that bug. Happened a couple of hours after bug 1131638 reached inbound.
Bobby should we uplift this into 37?
Flags: needinfo?(bobbyholley)
(In reply to Sheila Mooney from comment #9)
> Bobby should we uplift this into 37?

Uplift what? We don't have a patch for this bug other than matt's backout in comment 6.

As noted in comment 1, the attached patch didn't fix the problem, which makes sense because it sounds like matt's patch the source of the oranges we were seeing. The attached patch does fix some theoretical races, but I wouldn't uplift it on spec, and may not even land it at all, since I'd rather fix this on a deeper level with some refactoring.
Flags: needinfo?(bobbyholley)
Flags: needinfo?(cpearce)
Bobby, what's the status here? Do you still think fixing something here would improve our orange on EME mochitests?
Flags: needinfo?(bobbyholley)
Spoke about this in the meeting - short answer is that somebody needs to look into the eme test failures, and that the patch in this bug (a theoretical bug that was never confirmed in practice) might be a good place to start. At the very least, the logging in that patch might be useful.
Flags: needinfo?(bobbyholley)
EME test failures have disappeared. Dropping this from the EME blockers.
No longer blocks: EME, 1032660
Component: Audio/Video → Audio/Video: Playback
Pretty sure this bug is now not necessary. The EME mochitests are green now and I'll enable them today.
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → WORKSFORME
Try looks good to me: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e91eaa09aa0c
Enable the test on Windows Opt.
Assignee: bobbyholley → ctai
Comment on attachment 8781367 [details]
Bug 1140675 - Enable test_bug879717.html on windows. .

https://reviewboard.mozilla.org/r/71808/#review69330
Attachment #8781367 - Flags: review?(jwwang) → review+
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Target Milestone: mozilla39 → ---
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → WORKSFORME
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/14974aa2862f
Enable test_bug879717.html on windows. r=jwwang.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.