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

Status: RESOLVED WORKSFORME

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

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Try to prevent HTMLMediaElement readyState from being stale. v1

This doesn't appear to fully fix the problem. Whatever eventual solution we
land, we should also land the logging in this patch.

Comment 2

[Bobby Holley (:bholley)]

Tests disabled while we sort this out: https://hg.mozilla.org/integration/mozilla-inbound/rev/826e9c97526b

Flagging edwin to continue his investigation.

Comment 3

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/826e9c97526b

Comment 4

[Bobby Holley (:bholley)]

Meant to leave this open.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

We're bailing out here: https://dxr.mozilla.org/mozilla-central/source/dom/media/VideoFrameContainer.cpp#36

Need to figure out why. aImage isn't null.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Almost certainly another regression from bug 1131638. I'll back that out today until I figure out a better solution.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Will need to uplift I suspect if I uplift other changes...

Comment 8

[Andreas Pehrson [:pehrsons]]

(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.

Comment 9

[Sheila Mooney]

Bobby should we uplift this into 37?

Comment 10

[Bobby Holley (:bholley)]

(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.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Bobby, what's the status here? Do you still think fixing something here would improve our orange on EME mochitests?

Comment 12

[Bobby Holley (:bholley)]

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.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

EME test failures have disappeared. Dropping this from the EME blockers.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pretty sure this bug is now not necessary. The EME mochitests are green now and I'll enable them today.

Comment 15

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1140675 - Enable test_bug879717.html on windows. .

Review commit: https://reviewboard.mozilla.org/r/71808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71808/

Comment 16

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Try looks good to me: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e91eaa09aa0c
Enable the test on Windows Opt.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8781367 [details]
Bug 1140675 - Enable test_bug879717.html on windows. .

https://reviewboard.mozilla.org/r/71808/#review69330

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14974aa2862f
Enable test_bug879717.html on windows. r=jwwang.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/14974aa2862f